diff --git a/internal/agent/guardrails.go b/internal/agent/guardrails.go index ea179b459..a6d157bff 100644 --- a/internal/agent/guardrails.go +++ b/internal/agent/guardrails.go @@ -55,6 +55,22 @@ const ( // error, so this only affects true same-error loops. toolFailureStopAt = 6 + // toolFailureAnyErrorStopAt halts a tool that keeps failing with DIFFERENT + // errors. The streak above cannot see that case by construction: a changed + // signature rebuilds the record at 1, so a tool whose error text varies every + // call never reaches toolFailureStopAt however long it loops. + // + // That is not hypothetical. A headless run made 384 denied calls over 26 + // minutes without tripping a halt set to 6, because each denial named a + // different path. Keying denials on their category (see observeToolResult) + // fixes that case; this bound covers every error with no category to key on. + // + // Deliberately well above toolFailureStopAt. A model iterating on a genuinely + // tricky edit legitimately fails several times with different errors while it + // converges, and this must not cut those runs short — the same reasoning that + // moved toolFailureStopAt from 4 to 6. Both counters reset on success. + toolFailureAnyErrorStopAt = 12 + // maxContinueNudges bounds how many times the headless completion gate // (Options.RequireCompletionSignal) re-prompts a model that stopped without a // tool call while work clearly remained. Once spent, the run finalizes as @@ -327,12 +343,22 @@ type toolFailureRecord struct { count int errSig string hintShown bool + // anyErrorCount counts consecutive failures of this tool REGARDLESS of the + // error, and is cleared only by a success. count above restarts whenever the + // signature changes, which is exactly what a varying error message defeats; + // this one cannot be reset by changing the text. + anyErrorCount int } type toolFailureOutcome struct { InjectHint bool Stop bool Count int + // Varied reports that the stop came from the content-blind bound, i.e. Count + // failures with DIFFERENT errors rather than the same one repeated. The final + // answer says so, because "failed 12 times with the same error" would be a + // plainly false description of a tool that failed 12 different ways. + Varied bool } // errorSignature normalizes a tool error to a short, comparable signature so @@ -358,9 +384,13 @@ func toolFailureHint(toolName, schemaJSON, errOutput string) string { // toolFailureStopAnswer is the final answer when the repeated-failure guard halts // a run. -func toolFailureStopAnswer(toolName string, count int) string { +func toolFailureStopAnswer(toolName string, count int, varied bool) string { + cause := " times in a row with the same error, " + if varied { + cause = " times in a row, each with a different error, " + } return "Agent stopped: the `" + toolName + "` tool failed " + strconv.Itoa(count) + - " times in a row with the same error, so I halted instead of looping further. " + + cause + "so I halted instead of looping further. " + "Please check the request or adjust the tool arguments." } @@ -472,28 +502,59 @@ func newGuardState() *guardState { // observeToolResult tracks repeated identical failures of a tool. A successful // result clears that tool's failure streak. Returns whether to inject a one-shot // corrective hint and/or stop the run. -func (state *guardState) observeToolResult(name string, failed bool, output string) toolFailureOutcome { +// hintable is separate from failed on purpose. A categorized denial MUST count +// toward the streaks — that is the whole point of keying on the category — but a +// schema hint is the wrong response to a policy refusal: the call shape is fine, +// the answer was no. Collapsing the two is what made the earlier version of this +// fix a no-op, since the caller could only pass a flag that excluded denials. +func (state *guardState) observeToolResult(name string, failed bool, hintable bool, output string, denial DenialCategory) toolFailureOutcome { if state.toolFailures == nil { state.toolFailures = map[string]*toolFailureRecord{} } if !failed { - delete(state.toolFailures, name) // success resets the streak + delete(state.toolFailures, name) // success resets both counters return toolFailureOutcome{} } + // A denial keys on its CATEGORY, not its prose. The message embeds the path + // or command that was refused, so it differs on every call while describing + // the same unchanging refusal — which rebuilt the record at 1 each time and + // let a denied tool loop indefinitely under a halt set to 6. The category is + // a small closed enum the loop already sets on the result. sig := errorSignature(output) + if denial != DenialNone { + sig = "denial:" + string(denial) + } record := state.toolFailures[name] - if record == nil || record.errSig != sig { - record = &toolFailureRecord{count: 1, errSig: sig} + if record == nil { + record = &toolFailureRecord{errSig: sig} state.toolFailures[name] = record + } + if record.errSig != sig { + // A different error restarts the same-error streak but NOT the + // content-blind one: changing how a tool fails is not progress. + record.count = 1 + record.errSig = sig + record.hintShown = false } else { record.count++ } + record.anyErrorCount++ + outcome := toolFailureOutcome{Count: record.count} - if record.count >= toolFailureStopAt { + switch { + case record.count >= toolFailureStopAt: + outcome.Stop = true + return outcome + case record.anyErrorCount >= toolFailureAnyErrorStopAt: + // Report the counter that actually tripped. record.count is the + // same-signature streak and is often 1 here, which would describe a tool + // that failed a dozen different ways as having failed once. outcome.Stop = true + outcome.Count = record.anyErrorCount + outcome.Varied = true return outcome } - if record.count >= toolFailureHintAt && !record.hintShown { + if hintable && record.count >= toolFailureHintAt && !record.hintShown { record.hintShown = true outcome.InjectHint = true } diff --git a/internal/agent/guardrails_test.go b/internal/agent/guardrails_test.go index fc8079f13..859275120 100644 --- a/internal/agent/guardrails_test.go +++ b/internal/agent/guardrails_test.go @@ -2,6 +2,8 @@ package agent import ( "context" + "path/filepath" + "strconv" "strings" "testing" @@ -189,7 +191,7 @@ func TestUnknownExecSessionProbingTripsFailureHalt(t *testing.T) { var state guardState var stoppedAt int for i := 1; i <= toolFailureStopAt; i++ { - out := state.observeToolResult(tools.WriteStdinToolName, true, tools.UnknownExecSessionError(i)) + out := state.observeToolResult(tools.WriteStdinToolName, true, true, tools.UnknownExecSessionError(i), "") if out.Stop { stoppedAt = i break @@ -200,6 +202,223 @@ func TestUnknownExecSessionProbingTripsFailureHalt(t *testing.T) { } } +// A permission denial repeats forever when the streak is keyed on the error +// TEXT, because the denial message embeds the path or command that varies per +// call. +// +// Observed, not theorised: a headless run made 384 denied calls over 26 minutes +// without tripping a halt that stops at 6. Every denial carried the same typed +// category and a different reason string, so errorSignature differed each time +// and the record was rebuilt at count 1 on every call. +// +// TestUnknownExecSessionErrorSignatureIsIDInvariant fixed one message this way. +// Keying on the category fixes the class, without needing every future denial +// message to remember to be invariant. +func TestPermissionDenialStreakSurvivesVaryingReasonText(t *testing.T) { + var state guardState + stoppedAt := 0 + for i := 1; i <= toolFailureStopAt; i++ { + // The shape the loop actually produces: same tool, same category, a + // different path every time. + output := "Error: Permission denied for write_file: cannot write " + + filepath.Join("C:", "ws", "pkg", "file"+strconv.Itoa(i)+".go") + out := state.observeToolResult("write_file", true, true, output, DenialPermissionDenied) + if out.Stop { + stoppedAt = i + break + } + } + if stoppedAt != toolFailureStopAt { + t.Fatalf("denials with varying reason text stopped at %d, want %d", stoppedAt, toolFailureStopAt) + } +} + +// The content-blind bound. A tool failing over and over with genuinely +// DIFFERENT errors and no denial category is still a tool that is not working, +// and the same-signature streak can never see it. +// +// Bounded well above toolFailureStopAt on purpose: a model iterating on a +// tricky edit legitimately fails a few times with different errors while it +// converges, and cutting that short is the regression +// TestSuccessResetsBothFailureCounters guards. +func TestToolFailingWithDifferentErrorsEveryTimeStillStops(t *testing.T) { + var state guardState + stoppedAt := 0 + for i := 1; i <= toolFailureAnyErrorStopAt; i++ { + out := state.observeToolResult("bash", true, true, "distinct failure "+strconv.Itoa(i), "") + if out.Stop { + stoppedAt = i + break + } + } + if stoppedAt != toolFailureAnyErrorStopAt { + t.Fatalf("a tool failing with a new error each call stopped at %d, want %d", stoppedAt, toolFailureAnyErrorStopAt) + } +} + +// Neither counter may outlive a success, or a long run that fails occasionally +// and recovers would eventually halt for no reason. This is the property that +// keeps the content-blind bound safe to add. +func TestSuccessResetsBothFailureCounters(t *testing.T) { + var state guardState + for i := 1; i < toolFailureAnyErrorStopAt; i++ { + if out := state.observeToolResult("bash", true, true, "distinct failure "+strconv.Itoa(i), ""); out.Stop { + t.Fatalf("stopped at %d before the success that should reset it", i) + } + } + state.observeToolResult("bash", false, false, "ok", "") + + // Same again from zero. Reaching the bound a second time proves the counter + // restarted rather than merely paused. + stoppedAt := 0 + for i := 1; i <= toolFailureAnyErrorStopAt; i++ { + if out := state.observeToolResult("bash", true, true, "later failure "+strconv.Itoa(i), ""); out.Stop { + stoppedAt = i + break + } + } + if stoppedAt != toolFailureAnyErrorStopAt { + t.Fatalf("after a success the tool stopped at %d, want a full fresh %d", stoppedAt, toolFailureAnyErrorStopAt) + } +} + +// Records are keyed per tool, so ANOTHER tool succeeding in between must not +// clear the failing tool's streak. +// +// This is the realistic shape of the run that motivated the fix: the model kept +// making progress elsewhere — reading files, updating its plan — while one tool +// was refused over and over. A reset keyed on "something succeeded" rather than +// "this tool succeeded" would make the halt unreachable in exactly the runs that +// need it. +func TestAnotherToolSucceedingDoesNotClearAFailingToolsStreak(t *testing.T) { + var state guardState + stoppedAt := 0 + for i := 1; i <= toolFailureStopAt; i++ { + state.observeToolResult("read_file", false, false, "ok", "") + out := state.observeToolResult("write_file", true, false, + "Error: Permission denied for write_file: "+strconv.Itoa(i), DenialPermissionDenied) + if out.Stop { + stoppedAt = i + break + } + } + if stoppedAt != toolFailureStopAt { + t.Fatalf("denials interleaved with another tool's successes stopped at %d, want %d", stoppedAt, toolFailureStopAt) + } +} + +// alwaysPromptingTool is never allowed to run: it exists so a Run-level test can +// drive real permission denials through the loop. +type alwaysPromptingTool struct{ ran int } + +func (tool *alwaysPromptingTool) Name() string { return "bash" } +func (tool *alwaysPromptingTool) Description() string { return "test shell tool" } +func (tool *alwaysPromptingTool) Parameters() tools.Schema { + return tools.Schema{ + Type: "object", + Properties: map[string]tools.PropertySchema{"command": {Type: "string"}}, + Required: []string{"command"}, + AdditionalProperties: false, + } +} +func (tool *alwaysPromptingTool) Safety() tools.Safety { + return tools.Safety{SideEffect: tools.SideEffectShell, Permission: tools.PermissionPrompt, Reason: "runs shell commands"} +} +func (tool *alwaysPromptingTool) Run(context.Context, map[string]any) tools.Result { + tool.ran++ + return tools.Result{Status: tools.StatusOK, Output: "should never run"} +} + +// The regression for the gap this whole change existed to close, driven through +// Run rather than through the guard helper. +// +// The first version of this fix was a no-op in production and every one of its +// unit tests passed, because they called observeToolResult directly with +// failed=true. The loop passes isRetriableToolError, which returns false for a +// categorized denial, so the guard took its success branch and deleted the +// record before it could key on the category. A denied tool still looped to the +// turn limit. +// +// This drives the real path: a tool that always prompts, an approver that always +// denies, and a different command each turn so the denial REASON — and therefore +// the error text — varies exactly as it does in a real run. +func TestRunStopsARepeatedlyDeniedToolAtTheFailureBound(t *testing.T) { + tool := &alwaysPromptingTool{} + registry := tools.NewRegistry() + registry.Register(tool) + + // Comfortably more turns than the bound, so reaching the bound is what stops + // the run rather than exhausting the provider or MaxTurns. + turns := make([][]zeroruntime.StreamEvent, 0, toolFailureStopAt+4) + for i := range toolFailureStopAt + 4 { + turns = append(turns, toolTurn("call-"+strconv.Itoa(i), "bash", + `{"command":"touch /etc/file`+strconv.Itoa(i)+`"}`)) + } + provider := &mockProvider{turns: turns} + + denials := 0 + result, err := Run(context.Background(), "do the thing", provider, Options{ + Registry: registry, + PermissionMode: PermissionModeAsk, + MaxTurns: len(turns) + 5, + OnPermissionRequest: func(_ context.Context, request PermissionRequest) (PermissionDecision, error) { + denials++ + // The varying half. In production this is the refused path or command; + // here it is the command, which lands in the denial message the guard + // used to key on. + return PermissionDecision{ + Action: PermissionDecisionDeny, + Reason: "refused " + request.ToolName + " call " + strconv.Itoa(denials), + }, nil + }, + }) + if err != nil { + t.Fatal(err) + } + + if denials != toolFailureStopAt { + t.Errorf("the run made %d denied calls, want it halted at %d", denials, toolFailureStopAt) + } + if tool.ran != 0 { + t.Errorf("the denied tool executed %d times; the denial must precede execution", tool.ran) + } + want := toolFailureStopAnswer("bash", toolFailureStopAt, false) + if result.FinalAnswer != want { + t.Errorf("final answer =\n %q\nwant\n %q", result.FinalAnswer, want) + } +} + +// The stop message must describe the bound that actually tripped. When the +// content-blind counter is what halts the run, the same-signature streak is +// usually 1, and reporting that would tell the user a tool failed once after it +// failed a dozen different ways. +func TestVariedFailureStopAnswerReportsTheRightCounter(t *testing.T) { + var state guardState + var outcome toolFailureOutcome + for i := 1; i <= toolFailureAnyErrorStopAt; i++ { + outcome = state.observeToolResult("bash", true, true, "distinct failure "+strconv.Itoa(i), "") + if outcome.Stop { + break + } + } + if !outcome.Stop { + t.Fatal("never stopped") + } + if !outcome.Varied { + t.Error("Varied = false for a stop driven by the content-blind counter") + } + if outcome.Count != toolFailureAnyErrorStopAt { + t.Errorf("Count = %d, want the counter that tripped (%d)", outcome.Count, toolFailureAnyErrorStopAt) + } + answer := toolFailureStopAnswer("bash", outcome.Count, outcome.Varied) + if !strings.Contains(answer, "each with a different error") { + t.Errorf("stop answer describes the wrong cause: %q", answer) + } + if strings.Contains(answer, "with the same error") { + t.Errorf("stop answer claims a same-error loop after distinct failures: %q", answer) + } +} + func TestGuardStateResetsToolOnlyStreakOnEmptyNonToolTurn(t *testing.T) { var state guardState toolOnly := zeroruntime.CollectedStream{ diff --git a/internal/agent/loop.go b/internal/agent/loop.go index 05691295a..5e4245350 100644 --- a/internal/agent/loop.go +++ b/internal/agent/loop.go @@ -740,7 +740,13 @@ func Run(ctx context.Context, prompt string, provider Provider, options Options) // aren't fixed by reformatting the call, so a "match this schema" hint // would misdirect the model toward JSON shape or blocked behavior. retriableFailure := isRetriableToolError(toolResult) - outcome := guards.observeToolResult(call.Name, retriableFailure, toolResult.Output) + // A categorized denial is NOT retriable — retrying it verbatim is + // pointless — but it is still a failure the streaks must count, or a + // refused tool loops until the turn limit. Passing retriableFailure for + // both is what let that happen: observeToolResult took its success + // branch and deleted the record before it could key on the category. + countedFailure := retriableFailure || toolResult.DenialReason != DenialNone + outcome := guards.observeToolResult(call.Name, countedFailure, retriableFailure, toolResult.Output, toolResult.DenialReason) posture.observeToolOutcome(outcome, toolResult) if outcome.Stop { // The assistant message advertised EVERY collected tool call, but @@ -750,7 +756,7 @@ func Run(ctx context.Context, prompt string, provider Provider, options Options) // messages stay valid for a strict provider replay (Anthropic // rejects a tool_use with no answering tool_result). messages = appendAbortedToolResults(messages, collected.ToolCalls[index+1:]) - result.FinalAnswer = toolFailureStopAnswer(call.Name, outcome.Count) + result.FinalAnswer = toolFailureStopAnswer(call.Name, outcome.Count, outcome.Varied) result.Messages = copyMessages(messages) return result, nil }