diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 3f78445e8..ec4c0686c 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -159,6 +159,7 @@ type Agent struct { editAbandon *editAbandonState // edit abandonment detection (PASTE/LLMCompiler-inspired attention-shift tracking) toolCallBudget *toolCallBudget // per-session tool invocation limit (action-level guardrail) commandCache *commandCache // deterministic build/test command result caching + effectLedger *effectLedgerState // side-effect ledger: duplicate-effect awareness on retries (LangEffect/RAC-inspired) postEditVerify postEditVerifyState // tracks source-code edits to inject periodic verification hints planner *planState // agent-side auto task decomposition (Devin/Claude Code-inspired) todoStaleness *todoStalenessState // mid-run stale todo detection (plan abandonment awareness) @@ -356,6 +357,7 @@ func NewAgent(p provider.Provider, tools *tool.Registry, systemPrompt string, ma editAbandon: newEditAbandonState(), toolCallBudget: newToolCallBudget(), commandCache: newCommandCache(), + effectLedger: newEffectLedger(), errorClassifier: NewErrorClassifier(), planner: newPlanState(), todoStaleness: newTodoStalenessState(), @@ -3204,6 +3206,15 @@ func (a *Agent) RunStreamWithContent(ctx context.Context, content []provider.Con // Cache deterministic command results (build, test, lint, etc.) // for reuse when the same command is called again without file changes. a.storeCommandResult(tc.Name, tc.Arguments, result) + // Effect ledger (LangEffect/RAC-inspired): record failed/uncertain + // shell executions and, when the identical command is retried after + // such an attempt, annotate the retry result so the model verifies + // external state instead of trusting an outcome that may duplicate + // side effects. Runs after storeCommandResult: only raw results are + // cached (annotation text must never be cached). + if hint := a.recordEffectAttempt(tc.Name, tc.Arguments, result); hint != "" { + result.Content += hint + } } // Secret redaction (#1195): mask secret values in external-content // tool results BEFORE any recorder, cache annotation, context append, diff --git a/internal/agent/effect_ledger.go b/internal/agent/effect_ledger.go new file mode 100644 index 000000000..dbd0d822d --- /dev/null +++ b/internal/agent/effect_ledger.go @@ -0,0 +1,247 @@ +package agent + +// effect_ledger.go -- Side-Effect Duplicate Guard (Effect Ledger). +// +// This is a tool-execution-layer mechanism (sibling of commandCache), NOT a +// code-quality detector: it records what actually executed and annotates tool +// results at the execution path. +// +// Research basis: +// - Uber LangEffect (2025): every side-effecting agent action is registered +// in an effect log; retries after an uncertain outcome risk duplicating +// the external effect (LIFO compensation on failure is the full saga +// treatment — this implements the awareness layer of that design). +// - RAC: Robust Agent Compensation (arXiv:2605.03409): log-based recovery +// for agent frameworks; the log of executed effects is the prerequisite +// for any compensation. +// - "Agent Idempotency" production patterns (2026): agents retry failed +// tool calls, and a retry of a side-effecting command after timeout or +// mid-flight error can duplicate the external effect (git push twice, +// npm publish twice, duplicate API writes) because the first attempt's +// outcome is UNKNOWN — a timeout kill says nothing about whether the +// work already landed. +// +// Gap in this codebase: commandCache only serves deterministic build/test +// commands and never caches failures (#1717). Nothing else records shell +// command outcomes, so when the agent retries the identical side-effecting +// command after a timeout/error, the retry result is presented as +// authoritative with no hint that an earlier attempt may have (partially) +// landed. The model then draws conclusions from incomplete state. +// +// Design: +// - record() stores the outcome of every actually-executed run_command +// that ended in failure or an UNCERTAIN state (timeout / kill / cancel). +// Successful executions are not recorded (successful non-cacheable +// re-runs are usually intentional; verification re-runs are already +// covered by redundantReverify). +// - priorHint() returns an annotation when the identical +// (command, working_dir) has a failed/uncertain prior attempt inside +// the retry window. The caller appends it to the retry's tool result, +// so the model is told to verify external state before treating the +// retry output as authoritative. +// - Never-executed shapes (permission denial, gate block, invalid input, +// shell resolution failure) are excluded: they leave no external effect. +// +// Protocol safety: the annotation is appended to the tool result content +// itself (same pattern as the [cached — ...] annotation in command_cache.go); +// no message is inserted between tool_calls and tool_results. + +import ( + "encoding/json" + "fmt" + "strings" + "sync" + "time" + + "github.com/topcheer/ggcode/internal/tool" +) + +// effectOutcome classifies how a side-effecting execution ended. +type effectOutcome int + +const ( + effectFailed effectOutcome = iota // clean failure; effect most likely did not land + effectUncertain // timeout/kill/cancel; effect state UNKNOWN (may have partially landed) +) + +type effectRecord struct { + command string + workDir string + at time.Time + outcome effectOutcome +} + +var ( + // maxEffectRecords bounds the ledger (ring eviction of the oldest). + maxEffectRecords = 64 + // maxEffectWarnings caps annotations per Agent to avoid nagging in a + // tight retry loop. + maxEffectWarnings = 5 + // effectRetryWindow bounds how old a prior attempt may be to still + // trigger an annotation — older failures are usually deliberate re-runs + // (e.g. re-deploying much later), not blind retries. + effectRetryWindow = 30 * time.Minute +) + +// neverExecutedMarkers are error-result shapes produced BEFORE the command +// ever ran. They leave no external effect and must not enter the ledger. +var neverExecutedMarkers = []string{ + "Permission denied for tool", // policy/user denial (agent_tool.go) + "invalid input:", // argument parse failure + "blocked", // command gate block + "failed to resolve shell", // environment failure + "command job manager not available", + "failed to start command job", // never spawned +} + +// uncertainMarkers identify outcomes where the process ran but its final +// state is unknown — the dangerous retry case. +var uncertainMarkers = []string{ + "timed out after", // job-manager timeout (command_jobs.go) + "context deadline exceeded", // direct-path timeout + "signal: killed", // OOM/kill mid-flight + "context canceled", // user cancel mid-flight +} + +type effectLedgerState struct { + mu sync.Mutex + records []effectRecord + warned int + // now is swappable for tests. + now func() time.Time +} + +func newEffectLedger() *effectLedgerState { + return &effectLedgerState{now: time.Now} +} + +// classifyEffectOutcome maps a tool result to a ledger outcome. +// The second return value is false when the command never executed +// (denial/parse/gate) or succeeded — neither enters the ledger. +func classifyEffectOutcome(res tool.Result) (effectOutcome, bool) { + if !res.IsError { + return 0, false + } + c := res.Content + for _, m := range neverExecutedMarkers { + if strings.Contains(c, m) { + return 0, false + } + } + for _, m := range uncertainMarkers { + if strings.Contains(c, m) { + return effectUncertain, true + } + } + return effectFailed, true +} + +// effectKey normalizes a (command, workDir) identity. Leading activity +// comments are stripped the same way command_cache does (#1530) so the +// mandated '# description' line does not defeat matching. +func effectKey(command, workDir string) string { + return strings.TrimSpace(workDir) + "\x00" + strings.TrimSpace(stripLeadingShellComment(command)) +} + +// priorHint returns an annotation if an identical command has a +// failed/uncertain prior attempt inside the retry window. +func (e *effectLedgerState) priorHint(command, workDir string) string { + if e == nil || command == "" { + return "" + } + key := effectKey(command, workDir) + e.mu.Lock() + defer e.mu.Unlock() + now := e.now() + for i := len(e.records) - 1; i >= 0; i-- { + r := e.records[i] + if effectKey(r.command, r.workDir) != key { + continue + } + if now.Sub(r.at) > effectRetryWindow { + return "" + } + if e.warned >= maxEffectWarnings { + return "" + } + e.warned++ + return effectHintText(r, now) + } + return "" +} + +func effectHintText(r effectRecord, now time.Time) string { + state := "failed" + why := "" + if r.outcome == effectUncertain { + state = "timed out / was interrupted" + why = " A timed-out or interrupted command may have PARTIALLY landed" + } + return fmt.Sprintf( + "\n[Effect Ledger] This exact command already ran earlier in this session — the most recent prior attempt (%s ago) %s.%s "+ + "Do not treat this run's output as proof about the earlier attempt. Verify the actual external state before "+ + "re-attempting or drawing conclusions (e.g. git log/status for pushes, check the published/deployed artifact, "+ + "re-read files the command may have touched).\n", + now.Sub(r.at).Round(time.Second), state, why) +} + +// record stores the outcome of an executed run_command attempt (failures and +// uncertain outcomes only; successes are intentionally not recorded). +func (e *effectLedgerState) record(command, workDir string, outcome effectOutcome) { + if e == nil || command == "" { + return + } + e.mu.Lock() + defer e.mu.Unlock() + e.records = append(e.records, effectRecord{ + command: strings.TrimSpace(stripLeadingShellComment(command)), + workDir: strings.TrimSpace(workDir), + at: e.now(), + outcome: outcome, + }) + if len(e.records) > maxEffectRecords { + // Drop the oldest half to amortize the slice copy. + e.records = append([]effectRecord(nil), e.records[len(e.records)/2:]...) + } +} + +// recordEffectAttempt is the agent-loop entry point: called after a real +// run_command execution. It first checks for a prior failed/uncertain +// attempt (BEFORE recording the current one, so an attempt never annotates +// itself), then records the current outcome. The returned annotation, if +// non-empty, must be appended to the tool result content. +func (a *Agent) recordEffectAttempt(name string, args []byte, res tool.Result) string { + if name != "run_command" { + return "" + } + var parsed struct { + Command string `json:"command"` + WorkingDir string `json:"working_dir"` + } + if err := json.Unmarshal(args, &parsed); err != nil || parsed.Command == "" { + return "" + } + ledger := a.effectLedger + if ledger == nil { + return "" + } + outcome, executed := classifyEffectOutcome(res) + // Denials/parse failures never executed: nothing to record, and a prior + // failure hint is still legitimate (the model is re-formulating a call + // for a command that previously failed to run) — but the annotation is + // most useful on actual executions, so only hint when executed. + hint := "" + if executed { + hint = ledger.priorHint(parsed.Command, parsed.WorkingDir) + ledger.record(parsed.Command, parsed.WorkingDir, outcome) + } + return hint +} + +// resetEffectLedgerForTests clears ledger state between tests. +func (e *effectLedgerState) resetForTests() { + e.mu.Lock() + defer e.mu.Unlock() + e.records = nil + e.warned = 0 +} diff --git a/internal/agent/effect_ledger_test.go b/internal/agent/effect_ledger_test.go new file mode 100644 index 000000000..4a46bab67 --- /dev/null +++ b/internal/agent/effect_ledger_test.go @@ -0,0 +1,160 @@ +package agent + +// Tests for the effect ledger (side-effect duplicate guard). +// +// The ledger records failed/uncertain shell executions and annotates retries +// of the identical command so the model verifies external state instead of +// treating the retry output as authoritative. + +import ( + "encoding/json" + "strings" + "testing" + "time" + + "github.com/topcheer/ggcode/internal/tool" +) + +func mkCmdArgs(command, workDir string) []byte { + b, _ := json.Marshal(struct { + Command string `json:"command"` + WorkingDir string `json:"working_dir"` + }{Command: command, WorkingDir: workDir}) + return b +} + +func TestClassifyEffectOutcome(t *testing.T) { + cases := []struct { + name string + res tool.Result + wantRec bool + wantOutc effectOutcome + }{ + {"success not recorded", tool.Result{Content: "ok"}, false, 0}, + {"job timeout uncertain", tool.Result{IsError: true, Content: "command timed out after 30m0s"}, true, effectUncertain}, + {"ctx deadline uncertain", tool.Result{IsError: true, Content: "Command failed: context deadline exceeded"}, true, effectUncertain}, + {"killed uncertain", tool.Result{IsError: true, Content: "Command failed: signal: killed"}, true, effectUncertain}, + {"canceled uncertain", tool.Result{IsError: true, Content: "Command failed: context canceled"}, true, effectUncertain}, + {"plain failure", tool.Result{IsError: true, Content: "STDERR:\nerror: failed to push some refs\nCommand failed: exit status 1"}, true, effectFailed}, + {"permission denial excluded", tool.Result{IsError: true, Content: "Permission denied for tool \"run_command\". User rejected the request."}, false, 0}, + {"invalid input excluded", tool.Result{IsError: true, Content: "invalid input: unexpected end of JSON input"}, false, 0}, + {"gate block excluded", tool.Result{IsError: true, Content: "Command blocked by policy"}, false, 0}, + {"shell resolve excluded", tool.Result{IsError: true, Content: "failed to resolve shell: no such file"}, false, 0}, + } + for _, tc := range cases { + outcome, rec := classifyEffectOutcome(tc.res) + if rec != tc.wantRec || (rec && outcome != tc.wantOutc) { + t.Errorf("%s: got (rec=%v outcome=%v), want (rec=%v outcome=%v)", + tc.name, rec, outcome, tc.wantRec, tc.wantOutc) + } + } +} + +func TestEffectLedgerRetryHint(t *testing.T) { + e := newEffectLedger() + + // No prior attempt -> no hint. + if h := e.priorHint("git push origin main", "/repo"); h != "" { + t.Fatalf("no prior attempt: got hint %q", h) + } + + // A failed attempt for a different command must not leak. + e.record("make test", "/repo", effectFailed) + if h := e.priorHint("git push origin main", "/repo"); h != "" { + t.Fatalf("different command: got hint %q", h) + } + + e.record("git push origin main", "/repo", effectUncertain) + h := e.priorHint("git push origin main", "/repo") + if h == "" { + t.Fatal("retry after uncertain attempt: want hint, got none") + } + if !strings.Contains(h, "Effect Ledger") || !strings.Contains(h, "timed out") { + t.Errorf("hint text incomplete: %q", h) + } + + // Different workDir is a different effect key. + if h := e.priorHint("git push origin main", "/other"); h != "" { + t.Fatalf("different workDir: got hint %q", h) + } + + // Successes are never recorded, so they never produce hints. + e.record("make lint", "/repo", effectFailed) + _ = e.priorHint("make lint", "/repo") // consumes nothing; records are not deleted +} + +func TestEffectLedgerWindow(t *testing.T) { + e := newEffectLedger() + base := time.Now() + e.now = func() time.Time { return base } + e.record("npm publish", "/pkg", effectFailed) + + e.now = func() time.Time { return base.Add(effectRetryWindow - time.Minute) } + if h := e.priorHint("npm publish", "/pkg"); h == "" { + t.Error("inside window: want hint") + } + + e.now = func() time.Time { return base.Add(effectRetryWindow + time.Minute) } + if h := e.priorHint("npm publish", "/pkg"); h != "" { + t.Errorf("outside window: want no hint, got %q", h) + } +} + +func TestEffectLedgerWarningCap(t *testing.T) { + orig := maxEffectWarnings + maxEffectWarnings = 2 + defer func() { maxEffectWarnings = orig }() + + e := newEffectLedger() + e.record("terraform apply", "/infra", effectUncertain) + for i := 0; i < 3; i++ { + h := e.priorHint("terraform apply", "/infra") + if i < 2 && h == "" { + t.Fatalf("warning %d: want hint", i) + } + if i == 2 && h != "" { + t.Fatalf("warning %d: want capped empty, got %q", i, h) + } + } +} + +func TestRecordEffectAttemptSelfNotHinted(t *testing.T) { + a := &Agent{effectLedger: newEffectLedger()} + args := mkCmdArgs("git push origin main", "/repo") + + // First attempt (times out): no hint, but recorded. + hint := a.recordEffectAttempt("run_command", args, + tool.Result{IsError: true, Content: "command timed out after 30m0s"}) + if hint != "" { + t.Fatalf("first attempt must not hint itself, got %q", hint) + } + + // Identical retry: hint present even though this attempt also failed. + hint = a.recordEffectAttempt("run_command", args, + tool.Result{IsError: true, Content: "Command failed: exit status 1"}) + if hint == "" || !strings.Contains(hint, "Effect Ledger") { + t.Fatalf("retry: want Effect Ledger hint, got %q", hint) + } +} + +func TestRecordEffectAttemptIgnoresOtherTools(t *testing.T) { + a := &Agent{effectLedger: newEffectLedger()} + if h := a.recordEffectAttempt("edit_file", []byte(`{}`), tool.Result{IsError: true, Content: "timed out after 1s"}); h != "" { + t.Fatalf("non-run_command must be ignored, got %q", h) + } + if h := a.recordEffectAttempt("run_command", []byte(`not json`), tool.Result{IsError: true, Content: "x"}); h != "" { + t.Fatalf("unparseable args must be ignored, got %q", h) + } +} + +func TestEffectKeyStripsLeadingComment(t *testing.T) { + a := &Agent{effectLedger: newEffectLedger()} + // The mandated '# activity' comment line (#1530) must not defeat matching. + a.recordEffectAttempt("run_command", mkCmdArgs("# pushing changes\ngit push origin main", "/repo"), + tool.Result{IsError: true, Content: "Command failed: context deadline exceeded"}) + hint := a.recordEffectAttempt("run_command", mkCmdArgs("git push origin main", "/repo"), + tool.Result{IsError: true, Content: "Command failed: exit status 1"}) + if hint == "" { + t.Fatal("comment-stripped retry: want hint") + } +}