From 22abdc3a7a34f73610dcabe370f331ba3eee9381 Mon Sep 17 00:00:00 2001 From: opencode Date: Tue, 16 Jun 2026 22:39:25 +0200 Subject: [PATCH] feat(agentloop): Self-Reflection-Turn vor Completion (issue #152) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What ships: - cmd/sin-code/internal/agentloop/loop.go: - New Reflection + Reflector types (per-proposal self-critique) - New Loop.Reflector field - State: reflectedThisProposal (resets when worker makes tool calls) - Logic: runs Reflector exactly once per completion proposal, BEFORE the stop-gate. If issues non-empty, injects them as a user message and continues (not a stop-gate reject — worker self-fixes). - cmd/sin-code/internal/ledger/store.go: new EntryType TypeReflection. - cmd/sin-code/internal/hooks/hooks.go: new event hooks.ReflectIssues. - cmd/sin-code/internal/agentloop/loop_reflect_test.go: 3 tests (disabled, forces-another-turn, no-issues-proceeds). Acceptance criteria (from #152): - [x] Reflector runs once per proposal (not infinite self-doubt). - [x] Non-empty Issues force another turn via user message. - [x] Reflector=nil preserves legacy behavior. - [x] go test ./cmd/sin-code/internal/agentloop/... all green. Hard mandates honored: - M2: no new deps. - M3: reflection is post-verify, pre-stop-gate; never short-circuits the verify-gate. - M7: 3/3 tests pass under go test -race -count=1. Refs: OpenSIN-Code/SIN-Code#152 --- cmd/sin-code/internal/agentloop/loop.go | 56 +++++++++ .../internal/agentloop/loop_reflect_test.go | 113 ++++++++++++++++++ cmd/sin-code/internal/hooks/hooks.go | 3 + cmd/sin-code/internal/ledger/store.go | 3 + 4 files changed, 175 insertions(+) create mode 100644 cmd/sin-code/internal/agentloop/loop_reflect_test.go diff --git a/cmd/sin-code/internal/agentloop/loop.go b/cmd/sin-code/internal/agentloop/loop.go index 0064df78..7a56c112 100644 --- a/cmd/sin-code/internal/agentloop/loop.go +++ b/cmd/sin-code/internal/agentloop/loop.go @@ -78,6 +78,20 @@ type StopDecision struct { // A nil StopGate preserves the legacy behavior exactly. type StopGate func(ctx context.Context, snap StopSnapshot) StopDecision +// Reflection is the worker's self-critique of a proposed completion. Issues +// non-empty means the agent found problems in its own work and should fix +// them before the stop-gate is consulted. +type Reflection struct { + Issues []string + Notes string +} + +// Reflector performs a self-critique pass on a proposed completion. Returning +// a Reflection with non-empty Issues forces one more work turn. A nil +// Reflector disables the reflection step (legacy behavior). +// Issue #152. +type Reflector func(ctx context.Context, snap StopSnapshot) Reflection + type Loop struct { Gate *verify.Gate LocalTool LocalToolFunc @@ -119,6 +133,13 @@ type Loop struct { // crosses this fraction of MaxTokens (e.g. 0.8). Useful for alerting. BudgetWarnRatio float64 + // Reflector, if set, runs a self-critique pass right BEFORE the stop-gate. + // If it returns issues, the loop injects them and continues working — a + // cheap quality lift that reduces stop-gate rejections. Runs at most once + // per proposed completion to avoid infinite self-doubt loops. + // Issue #152. + Reflector Reflector + // AllowContinuation switches the maxTurns outcome from a hard error to a // checkpointed, resumable Result (Continuation=true). Daemons set this so // a long task is re-enqueued and resumed rather than abandoned; one-shot @@ -283,6 +304,7 @@ func (l *Loop) Run(ctx context.Context, sess *session.Session, prompt string) (* stallCount := 0 totalTokens := 0 // issue #151: cumulative tokens across the run warnedBudget := false // fires hooks.BudgetWarn once per run + reflectedThisProposal := false toolsSeen := map[string]bool{} var toolsUsed []string @@ -386,6 +408,37 @@ func (l *Loop) Run(ctx context.Context, sess *session.Session, prompt string) (* }) l.record(ctx, ledger.TypeVerifyPass, map[string]any{"mode": string(res.Mode)}, "verification passed ("+string(res.Mode)+")") + // Self-reflection: one cheap self-critique pass before the + // independent stop-gate. Reset the flag whenever the worker did + // real work (tool calls) in between, so each fresh proposal gets + // exactly one reflection. Issue #152. + if l.Reflector != nil && !reflectedThisProposal { + reflectedThisProposal = true + ref := l.Reflector(ctx, StopSnapshot{ + Prompt: prompt, FinalOutput: resp.Text, Turns: turn + 1, + ToolsUsed: toolsUsed, VerifyPassed: res.Passed, SessionID: sess.ID, + }) + if len(ref.Issues) > 0 { + l.fire(ctx, hooks.ReflectIssues, "", map[string]any{"issues": ref.Issues}) + l.record(ctx, ledger.TypeReflection, + map[string]any{"issues": ref.Issues}, + "self-reflection found issues; continuing") + var b strings.Builder + b.WriteString("SELF-REVIEW found issues to fix before completing:\n") + for i, is := range ref.Issues { + fmt.Fprintf(&b, " %d. %s\n", i+1, is) + } + if strings.TrimSpace(ref.Notes) != "" { + b.WriteString("Notes: " + ref.Notes + "\n") + } + msgs = append(msgs, session.Message{Role: "user", Content: b.String()}) + if err := sess.SaveHistory(msgs); err != nil { + return nil, err + } + continue + } + } + // Stop-gate: completion authority is decoupled from the worker. // The verify-gate passing is necessary but not sufficient — an // independent evaluator confirms the goal contract is satisfied @@ -483,6 +536,9 @@ func (l *Loop) Run(ctx context.Context, sess *session.Session, prompt string) (* } for _, tc := range resp.ToolCalls { + // Real work happened in this turn — reset the reflection + // flag so a fresh proposal can be re-evaluated. + reflectedThisProposal = false if !toolsSeen[tc.Name] { toolsSeen[tc.Name] = true toolsUsed = append(toolsUsed, tc.Name) diff --git a/cmd/sin-code/internal/agentloop/loop_reflect_test.go b/cmd/sin-code/internal/agentloop/loop_reflect_test.go new file mode 100644 index 00000000..c6ff0d09 --- /dev/null +++ b/cmd/sin-code/internal/agentloop/loop_reflect_test.go @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: MIT +// Purpose: tests for issue #152 — self-reflection pass before the +// stop-gate. When the Reflector returns issues, the loop continues +// working instead of consulting the gate. +package agentloop + +import ( + "context" + "testing" + + "github.com/OpenSIN-Code/SIN-Code/cmd/sin-code/internal/session" +) + +// Reflector=nil preserves legacy behavior (no reflection). +func TestReflector_DisabledWhenNil(t *testing.T) { + s := setupSession(t) + gateCalls := 0 + loop := &Loop{ + Gate: passGate(), + // Reflector: nil (default) + StopGate: func(ctx context.Context, snap StopSnapshot) StopDecision { + gateCalls++ + return StopDecision{Complete: true} + }, + Completion: func(ctx context.Context, msgs []session.Message, tools []ToolSpec) (*Completion, error) { + return &Completion{Text: "ok", Raw: session.Message{Role: "assistant", Content: "ok"}}, nil + }, + } + _, err := loop.Run(context.Background(), s, "x") + if err != nil { + t.Fatal(err) + } + if gateCalls != 1 { + t.Errorf("expected 1 stop-gate call, got %d", gateCalls) + } +} + +// Reflector returns issues -> loop injects them and forces another turn. +func TestReflector_ForcesAnotherTurn(t *testing.T) { + s := setupSession(t) + reflections := 0 + completionCalls := 0 + gateCalls := 0 + loop := &Loop{ + Gate: passGate(), + Reflector: func(ctx context.Context, snap StopSnapshot) Reflection { + reflections++ + // Return issues once, then accept. The Reflector runs at + // most once per proposal — the second time the worker + // proposes completion, reflectedThisProposal is true and + // the Reflector is skipped. + if reflections == 1 { + return Reflection{Issues: []string{"missing test"}, Notes: "add a unit test"} + } + return Reflection{} + }, + StopGate: func(ctx context.Context, snap StopSnapshot) StopDecision { + gateCalls++ + return StopDecision{Complete: true} + }, + Completion: func(ctx context.Context, msgs []session.Message, tools []ToolSpec) (*Completion, error) { + completionCalls++ + return &Completion{Text: "done", Raw: session.Message{Role: "assistant", Content: "done"}}, nil + }, + } + _, err := loop.Run(context.Background(), s, "x") + if err != nil { + t.Fatal(err) + } + // Reflector runs ONCE (returns issues, the loop continues). + // On the second turn, reflectedThisProposal is still true, so + // the Reflector is skipped — straight to stop-gate. + if reflections != 1 { + t.Errorf("expected 1 reflection call (one per proposal), got %d", reflections) + } + if completionCalls != 2 { + t.Errorf("expected 2 completion calls (one per turn), got %d", completionCalls) + } + if gateCalls != 1 { + t.Errorf("expected 1 stop-gate call (after second completion), got %d", gateCalls) + } +} + +// Reflector with no issues on first call -> straight to stop-gate. +func TestReflector_NoIssuesProceeds(t *testing.T) { + s := setupSession(t) + reflections := 0 + gateCalls := 0 + loop := &Loop{ + Gate: passGate(), + Reflector: func(ctx context.Context, snap StopSnapshot) Reflection { + reflections++ + return Reflection{} // no issues + }, + StopGate: func(ctx context.Context, snap StopSnapshot) StopDecision { + gateCalls++ + return StopDecision{Complete: true} + }, + Completion: func(ctx context.Context, msgs []session.Message, tools []ToolSpec) (*Completion, error) { + return &Completion{Text: "ok", Raw: session.Message{Role: "assistant", Content: "ok"}}, nil + }, + } + _, err := loop.Run(context.Background(), s, "x") + if err != nil { + t.Fatal(err) + } + if reflections != 1 { + t.Errorf("expected 1 reflection call, got %d", reflections) + } + if gateCalls != 1 { + t.Errorf("expected 1 stop-gate call, got %d", gateCalls) + } +} diff --git a/cmd/sin-code/internal/hooks/hooks.go b/cmd/sin-code/internal/hooks/hooks.go index 0c56b68d..8198345c 100644 --- a/cmd/sin-code/internal/hooks/hooks.go +++ b/cmd/sin-code/internal/hooks/hooks.go @@ -56,6 +56,9 @@ const ( // Token budget lifecycle (issue #151). BudgetWarn = "budget.warn" BudgetExhausted = "budget.exhausted" + // ReflectIssues fires when the self-reflection pass finds problems the + // worker must fix before completion is evaluated. + ReflectIssues = "reflect.issues" AgentSpawn = "agent.spawn" AgentComplete = "agent.complete" diff --git a/cmd/sin-code/internal/ledger/store.go b/cmd/sin-code/internal/ledger/store.go index 57972d0a..584a17b6 100644 --- a/cmd/sin-code/internal/ledger/store.go +++ b/cmd/sin-code/internal/ledger/store.go @@ -40,6 +40,9 @@ const ( // TypeTokenBudgetExhausted is recorded when cumulative token usage exceeds // MaxTokens and the run stops spending (issue #151). TypeTokenBudgetExhausted EntryType = "token_budget_exhausted" + // TypeReflection records a self-critique pass that found issues and forced + // another work turn before stop-gate evaluation. + TypeReflection EntryType = "reflection" ) // Entry is one row in the ledger.