fix(agent): count denials as failures and report the bound that tripped

Addresses both blocking findings from @anandh8x's review. He was right on
both, and the first was fatal: the previous commit was a no-op in production.

loop.go passed isRetriableToolError as the guard's `failed` flag, and that
returns false for any categorized denial (a policy refusal is deliberately not
retriable). observeToolResult therefore took its success branch and DELETED the
record before it could key on DenialReason, so a denied tool still looped to the
turn limit. The re-key was correct and unreachable.

The flag is now split. `failed` counts a denial toward the streaks; `hintable`
stays retriable-only, because a schema hint is the wrong response to a refusal —
the call shape is fine, the answer was no. Collapsing the two is what made a
caller unable to express "count this but do not coach the model about it".

Second finding: outcome.Count returned the signature-keyed record.count even
when the content-blind counter was what tripped the stop. With twelve distinct
errors that count is 1, so the final answer told the user a tool "failed 1 time
in a row with the same error". The outcome now carries the counter that actually
fired plus a Varied flag, and the stop answer says "each with a different error"
in that case.

Every earlier test passed while the production path was broken, because they
called observeToolResult directly with failed=true. So the important addition
here is TestRunStopsARepeatedlyDeniedToolAtTheFailureBound, which drives Run
itself: a tool that always prompts, an approver that always denies, and a
different command per turn so the denial reason varies as it does in a real run.

Verified by reverting the fix: the run makes 10 denied calls instead of halting
at 6 and dies on the no-output guard 13 turns later, while the helper-level test
stays green — which is precisely why this shipped in the first place.

Author: Vasanthdev2004

internal/agent/guardrails.go

@@ -354,6 +354,11 @@ 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
@@ -379,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."
 }
 
@@ -493,7 +502,12 @@ 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, denial DenialCategory) 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{}
 	}
@@ -507,7 +521,7 @@ func (state *guardState) observeToolResult(name string, failed bool, output stri
 	// 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 != "" {
+	if denial != DenialNone {
 		sig = "denial:" + string(denial)
 	}
 	record := state.toolFailures[name]
@@ -527,11 +541,20 @@ func (state *guardState) observeToolResult(name string, failed bool, output stri
 	record.anyErrorCount++
 
 	outcome := toolFailureOutcome{Count: record.count}
-	if record.count >= toolFailureStopAt || record.anyErrorCount >= toolFailureAnyErrorStopAt {
+	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
 	}

internal/agent/guardrails_test.go

@@ -191,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
@@ -222,7 +222,7 @@ func TestPermissionDenialStreakSurvivesVaryingReasonText(t *testing.T) {
 		// 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, output, DenialPermissionDenied)
+		out := state.observeToolResult("write_file", true, true, output, DenialPermissionDenied)
 		if out.Stop {
 			stoppedAt = i
 			break
@@ -245,7 +245,7 @@ func TestToolFailingWithDifferentErrorsEveryTimeStillStops(t *testing.T) {
 	var state guardState
 	stoppedAt := 0
 	for i := 1; i <= toolFailureAnyErrorStopAt; i++ {
-		out := state.observeToolResult("bash", true, "distinct failure "+strconv.Itoa(i), "")
+		out := state.observeToolResult("bash", true, true, "distinct failure "+strconv.Itoa(i), "")
 		if out.Stop {
 			stoppedAt = i
 			break
@@ -262,17 +262,17 @@ func TestToolFailingWithDifferentErrorsEveryTimeStillStops(t *testing.T) {
 func TestSuccessResetsBothFailureCounters(t *testing.T) {
 	var state guardState
 	for i := 1; i < toolFailureAnyErrorStopAt; i++ {
-		if out := state.observeToolResult("bash", true, "distinct failure "+strconv.Itoa(i), ""); out.Stop {
+		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, "ok", "")
+	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, "later failure "+strconv.Itoa(i), ""); out.Stop {
+		if out := state.observeToolResult("bash", true, true, "later failure "+strconv.Itoa(i), ""); out.Stop {
 			stoppedAt = i
 			break
 		}
@@ -294,8 +294,8 @@ func TestAnotherToolSucceedingDoesNotClearAFailingToolsStreak(t *testing.T) {
 	var state guardState
 	stoppedAt := 0
 	for i := 1; i <= toolFailureStopAt; i++ {
-		state.observeToolResult("read_file", false, "ok", "")
-		out := state.observeToolResult("write_file", true,
+		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
@@ -307,6 +307,118 @@ func TestAnotherToolSucceedingDoesNotClearAFailingToolsStreak(t *testing.T) {
 	}
 }
 
+// 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{

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, toolResult.DenialReason)
+			// 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
 			}