fix(agent): stop a denied tool looping past the repeated-failure halt - #866
fix(agent): stop a denied tool looping past the repeated-failure halt#866Vasanthdev2004 wants to merge 2 commits into
Conversation
The repeated-failure guard keys its streak on the first 80 characters of the error text. A permission denial reads "Error: Permission denied for <tool>: <reason>", and reason names the path or command that was refused, so the text differs on every call while describing the same unchanging refusal. Each call therefore rebuilt the record at count 1 and toolFailureStopAt was never reached. Not hypothetical. A headless run made 384 denied calls over 26 minutes under a halt set to 6, produced no files, and reported nothing. #702 already hit this shape once and fixed it by making one error message id-invariant; that works per message and needs every future message to remember. Denials now key on their DenialCategory instead, which is a small closed enum the loop already sets on the result, so the class is fixed rather than one instance of it. Adds a second, content-blind counter beside the streak. The signature-keyed one cannot by construction see a tool that fails with a genuinely different error every time, and that is still a tool that is not working. It counts consecutive failures regardless of the error and is cleared only by a success of that same tool, so changing how a tool fails is not progress and neither is some other tool succeeding. It stops at 12 rather than 6 on purpose: a model iterating on a tricky edit legitimately fails a few times with different errors while converging, which is the same reasoning that moved toolFailureStopAt from 4 to 6. Two counters, tripping on either, is what both of the agent CLIs I compared against arrived at independently after hitting this bug — a tight bound on identical failures ORed with a looser bound that no amount of varying the error can reset. Every guard is mutation-checked. Reverting the denial re-key fails TestPermissionDenialStreakSurvivesVaryingReasonText; deleting the content-blind bound, or letting a signature change reset it, each fail TestToolFailingWithDifferentErrorsEveryTimeStillStops and TestSuccessResetsBothFailureCounters. One existing test call site gains the new parameter.
WalkthroughThe tool loop now passes denial categories to guardrails. Guardrails track repeated identical failures and consecutive failures with varying errors. Tests cover denial categorization, counter resets, per-tool streak isolation, end-to-end halting, and stop messages. ChangesTool failure guardrails
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ToolExecutionLoop
participant guardState
participant FailureRecord
ToolExecutionLoop->>guardState: Pass tool result and DenialCategory
guardState->>FailureRecord: Update signature and any-error counters
guardState-->>ToolExecutionLoop: Return halt outcome at either threshold
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/agent/guardrails_test.go (1)
262-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the same-signature counter reset.
Both loops use a new error string on every call. Therefore,
countstays at1and this test only proves theanyErrorCountreset. Add repeated identical failures before and after the success, then assert that the sixth post-success failure stops the tool.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/guardrails_test.go` around lines 262 - 283, Update TestSuccessResetsBothFailureCounters to use the same failure signature repeatedly in both loops, rather than generating distinct error strings. Ensure the pre-success sequence establishes both counters, then verify that after the success the sixth identical post-success failure stops the tool, proving the signature-specific count reset as well as the any-error reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agent/loop.go`:
- Around line 742-743: In internal/agent/loop.go at lines 742-743, update the
failure flag passed to observeToolResult to include cases where
toolResult.DenialReason is non-empty, so that policy denials are tracked as
failures. In internal/agent/guardrails.go at lines 510-512, preserve the
category-based counting logic for denials but prevent InjectHint from being
called when a denial is present, since schema hints should not encourage
retrying blocked behavior. In internal/agent/guardrails_test.go at lines
217-234, add a new Run-level regression test that submits repeated categorized
denials and asserts that the run terminates at the toolFailureStopAt limit
rather than continuing until the turn limit.
---
Nitpick comments:
In `@internal/agent/guardrails_test.go`:
- Around line 262-283: Update TestSuccessResetsBothFailureCounters to use the
same failure signature repeatedly in both loops, rather than generating distinct
error strings. Ensure the pre-success sequence establishes both counters, then
verify that after the success the sixth identical post-success failure stops the
tool, proving the signature-specific count reset as well as the any-error reset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2eb0a201-6787-4a37-9f48-63bf467db61d
📒 Files selected for processing (3)
internal/agent/guardrails.gointernal/agent/guardrails_test.gointernal/agent/loop.go
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
@jatmn @anandh8x @gnanam1990 — one guard change, but it lands differently for each of you, so here's the short version of why I'm tagging all three. The repeated-failure halt has never been able to fire on a permission denial. It keys the streak on the first 80 characters of the error text, and a denial message embeds the path or command that was refused — so the text is different every call while the refusal is identical. The record rebuilt at 1 each time and a halt set to 6 was simply unreachable. I hit it for real: 384 denied calls, 26 minutes, no files, no error. @jatmn — the part worth your scepticism is the second counter, not the re-key. It's content-blind, so nothing about the error text can reset it, and it stops at 12 rather than 6. I chose the looser bound because a model iterating on a tricky edit legitimately fails several times with different errors while converging, and cutting those runs short would be a worse bug than the one I'm fixing. That's the same argument that moved @anandh8x — this touches the agent loop, one line at the @gnanam1990 — most relevant to #829. Zeromaxing raises the turn budget 80 → 480 and says so in the banner, which means it multiplies this exact failure by six: a run that would have burned 80 turns going nowhere now burns 480. The 384-call run I measured was under zeromaxing. This fix is upstream of your PR, so #829 gets it for free, but it's worth knowing the posture was amplifying a real unbounded loop rather than just a slow one. This generalises #702 rather than replacing it. That fix made one error message id-invariant so its streak could count; this keys denials on Six tests, and every guard mutation-checked — reverting the re-key, deleting the content-blind bound, or letting a signature change reset it each turn tests red. |
There was a problem hiding this comment.
Two blocking issues remain on the latest commit:
-
internal/agent/loop.go:742-743 still passes only isRetriableToolError(toolResult) as the guard failure flag. Categorized denials intentionally return false there, so observeToolResult takes its success branch and deletes the record before it can key on DenialReason. I reproduced this against the exact head: six varying DenialPermissionDenied results never stop. Please count retriableFailure OR a non-empty toolResult.DenialReason, while keeping schema-hint injection disabled for denials, and add a Run-level regression so the production call path, not only the guard helper, is covered.
-
internal/agent/guardrails.go:529-530 returns the signature-specific record.count even when the new content-blind anyErrorCount is what trips the stop. With twelve distinct errors, count is 1, so loop.go:753 reports that the tool failed 1 time with the same error. Return enough outcome information to produce the correct count and a truthful generic or differentiated stop message; cover the rendered final answer.
The focused tests added by the PR pass and focused vet is clean, but they do not exercise either integration behavior above.
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.
|
@anandh8x both fixed in 1. Denials now count. The flag is split rather than widened. 2. The count is truthful. 3. The Run-level regression you asked for: I checked it catches your bug rather than assuming. Reverting the flag split: It loops past the bound and dies on the no-output guard 13 turns later — and the helper-level
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agent/guardrails_test.go`:
- Around line 310-312: The alwaysPromptingTool type is declared twice at package
scope in the test file, which causes a Go redeclaration error. Locate the second
alwaysPromptingTool declaration elsewhere in the file and remove it, preserving
the one shown in the diff that includes the explanatory comment about its
purpose in the Run-level test.
In `@internal/agent/loop.go`:
- Around line 743-749: Update toolResultFromPrePermissionReject to set
ToolResult.DenialReason when converting a pre-permission rejection, mapping the
rejection error type or message to the appropriate DenialCategory such as
DenialFiltered or DenialPermissionDenied. Preserve the existing output and
non-retriable behavior while ensuring categorized pre-permission denials are
counted by the observeToolResult countedFailure logic.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e568f8b9-8d71-42a2-82e8-ad5992a3d842
📒 Files selected for processing (3)
internal/agent/guardrails.gointernal/agent/guardrails_test.gointernal/agent/loop.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/agent/guardrails.go
| // 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 } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate alwaysPromptingTool declaration.
alwaysPromptingTool is declared twice at package scope. Go rejects the test package with a redeclaration error. Keep one declaration so the regression tests compile.
Proposed fix
type alwaysPromptingTool struct{ ran int }
-type alwaysPromptingTool struct{ ran int }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agent/guardrails_test.go` around lines 310 - 312, The
alwaysPromptingTool type is declared twice at package scope in the test file,
which causes a Go redeclaration error. Locate the second alwaysPromptingTool
declaration elsewhere in the file and remove it, preserving the one shown in the
diff that includes the explanatory comment about its purpose in the Run-level
test.
| // 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) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inventory policy-denial representations and category assignment.
rg -n --type go -C 6 \
'DenialReason|permission_action|Permission(Action|Decision)Deny|Permission denied for |Permission required for |Sandbox block|Sandbox approval required for |is not enabled for this run' \
internal/agent || true
# Inspect ToolResult construction sites that can reach the agent loop.
rg -n --type go -C 8 'ToolResult\s*\{' internal/agent || trueRepository: Gitlawb/zero
Length of output: 50369
🏁 Script executed:
# Find all sites constructing ToolResult with error status
rg -n --type go 'return ToolResult\{' internal/agent/loop.go | head -20
# Find executeToolCall and check what it delegates to
ast-grep outline internal/agent/loop.go --view expanded | grep -A 5 "executeToolCall"
# Check for tool results coming from tools.Tool execution
rg -n --type go 'toolResult|tools\.Result' internal/agent/loop.go | grep -E '(toolResult\s*:=|tools\.Result)' | head -20Repository: Gitlawb/zero
Length of output: 4172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all ToolResult{} constructions in executeToolCall to identify denial producers
rg -n --type go -B 2 -A 8 'func executeToolCall' internal/agent/loop.go | head -30
# Check for tool.Execute calls and their result handling
rg -n --type go -A 3 'tool\.Execute|\.Execute(' internal/agent/loop.go | head -30
# Identify all denial-producing paths and check if they set DenialReason
echo "=== Checking for ToolResult errors without DenialReason field ==="
rg -n --type go 'ToolResult\{[^}]*Status:\s*tools\.StatusError[^}]*Output:' internal/agent/loop.goRepository: Gitlawb/zero
Length of output: 766
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where tool.Execute is called and how results are handled
rg -n --type go 'tool\.Execute\(' internal/agent/loop.go -A 5
# Find all ToolResult constructions and check if DenialReason is set
echo "=== ToolResult constructions from lines 1080-1150 (parseArgs through tool-not-found) ==="
sed -n '1076,1150p' internal/agent/loop.go | grep -n 'ToolResult\|DenialReason'
# Check executeToolCall main path - tool execution
echo ""
echo "=== Tool execution path in executeToolCall ==="
sed -n '1150,1300p' internal/agent/loop.go | head -80Repository: Gitlawb/zero
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Trace the exact flow: where tool.Execute result flows to ToolResult
# Look for toolResultFromTools or similar conversions
rg -n --type go 'toolResultFrom|convertToolResult|toolToResult' internal/agent/loop.go
# Look at the main path after tool.Execute call
echo "=== Searching around line 1270-1400 for tool.Execute result handling ==="
sed -n '1270,1400p' internal/agent/loop.go
# Check where tools.Result gets converted to ToolResult
rg -n --type go 'ToolResult.*Status.*tools\.' internal/agent/loop.go | head -20Repository: Gitlawb/zero
Length of output: 6665
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Continue from result := registry.RunWithOptions to see how it flows to ToolResult
echo "=== Looking for tools.Result to ToolResult conversion after registry.RunWithOptions ==="
sed -n '1350,1450p' internal/agent/loop.go
# Also look at toolResultFromPrePermissionReject to understand the pattern
echo ""
echo "=== toolResultFromPrePermissionReject pattern ==="
sed -n '1717,1775p' internal/agent/loop.go
# Find where tools.Result becomes ToolResult in the main path
echo ""
echo "=== End of executeToolCall - how is tools.Result converted ==="
sed -n '1400,1445p' internal/agent/loop.goRepository: Gitlawb/zero
Length of output: 9493
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# The key issue: main ToolResult conversion at 1417-1438 does NOT set DenialReason from Meta
# Need to find if tools.Result can carry permission_action without DenialReason
# Search tools package for where permission_action meta is set
echo "=== Searching for permission_action meta sources in internal/agent ==="
rg -n --type go '"permission_action"' internal/agent/ | grep -v test | grep -v '\.go-'
echo ""
echo "=== Checking for PrePermissionRejecter and how it's used ==="
rg -n --type go 'PrePermissionRejecter|RejectBeforePermission' internal/agent/loop.go -B 2 -A 8
echo ""
echo "=== Checking the actual tools.Registry/tool.Execute path ==="
# The tools are in a different package - check what meta they can return
rg -n --type go 'permission_action' . --glob '*/tools/*' 2>/dev/null | head -20Repository: Gitlawb/zero
Length of output: 1138
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what deniedPermissionResult and canceledPermissionResult actually set
echo "=== deniedPermissionResult (lines 2445-2463) ==="
sed -n '2445,2463p' internal/agent/loop.go
echo ""
echo "=== canceledPermissionResult (lines 2465-2489) ==="
sed -n '2465,2489p' internal/agent/loop.go
echo ""
echo "=== Check if any tool error flow sets DenialReason before line 1417 ==="
# Look for early returns that set DenialReason
sed -n '1076,1420p' internal/agent/loop.go | grep -B 3 'DenialReason:'
echo ""
echo "=== Verify: does toolResultFromPrePermissionReject set DenialReason? ==="
sed -n '1717,1753p' internal/agent/loop.go | grep 'DenialReason'Repository: Gitlawb/zero
Length of output: 2108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# The critical question: does any tool execution path (registry.RunWithOptions)
# return a tools.Result with permission_action meta without being converted to ToolResult with DenialReason?
# Let's check all early returns that set permission_action meta
echo "=== All locations that set permission_action meta ==="
rg -n --type go '"permission_action"' internal/agent/loop.go
echo ""
echo "=== Check toolResultFromPrePermissionReject more carefully ==="
# This path doesn't set DenialReason. PrePermissionRejecter tools return tools.Result
# What Meta can they have?
rg -n --type go 'PrePermissionRejecter' . --glob '*/tools/*' -A 3 2>/dev/null | head -30
echo ""
echo "=== Search in tools package for permission-related metadata ==="
fd . --type f --name '*.go' | grep tools | head -10Repository: Gitlawb/zero
Length of output: 2956
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# The critical path: toolResultFromPrePermissionReject doesn't set DenialReason
# Check if PrePermissionRejecter tools set permission_action in their Meta
echo "=== Search for PrePermissionRejecter implementations and their Meta ==="
fd '\.go$' internal/tools | xargs grep -l 'RejectBeforePermission' | head -5
echo ""
echo "=== Check WebFetchTool for permission_action meta ==="
rg -n --type go 'RejectBeforePermission|permission_action' internal/tools/web_fetch.go -B 3 -A 8
echo ""
echo "=== Check LocalDesktopTerminalTool ==="
rg -n --type go 'RejectBeforePermission|permission_action' internal/tools/local_desktop_terminal.go -B 3 -A 8 | head -40Repository: Gitlawb/zero
Length of output: 1219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find observeToolResult implementation to understand the vulnerability
echo "=== guardState.observeToolResult implementation ==="
rg -n --type go -A 25 'func.*observeToolResult' internal/agent/guardrails.go | head -50
echo ""
echo "=== Check what happens when countedFailure=false ==="
sed -n '200,350p' internal/agent/guardrails.go | grep -B 10 -A 10 'failed'Repository: Gitlawb/zero
Length of output: 1494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Trace the full vulnerability: toolResultFromPrePermissionReject path
echo "=== toolResultFromPrePermissionReject does NOT set DenialReason ==="
sed -n '1717,1753p' internal/agent/loop.go
echo ""
echo "=== But PrePermissionRejecter can return errors with permission_action? ==="
# Check if any PrePermissionRejecter tool sets permission_action in Meta
rg -n --type go 'RejectBeforePermission' internal/tools/ -A 15 | grep -E 'Meta|permission_action|Result\{' | head -40
echo ""
echo "=== Verify loop test case 577 shows the vulnerability ==="
sed -n '577,590p' internal/agent/loop_test.go
echo ""
echo "=== Trace loop.go line 1116: toolResultFromPrePermissionReject call ==="
sed -n '1113,1120p' internal/agent/loop.goRepository: Gitlawb/zero
Length of output: 4079
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# The smoking gun: does toolResultFromPrePermissionReject lose DenialReason?
# And can a PrePermissionRejecter return Meta["permission_action"]?
echo "=== Check if any PrePermissionRejecter implementation sets permission_action meta ==="
rg -n --type go 'RejectBeforePermission' internal/tools/ -A 20 | grep -B 5 'permission_action'
echo ""
echo "=== Check web_fetch.go RejectBeforePermission more carefully ==="
sed -n '159,200p' internal/tools/web_fetch.go
echo ""
echo "=== Now verify the actual gap: can a PrePermissionRejecter denial avoid DenialReason? ==="
# The issue is: even if PrePermissionRejecter tools don't set permission_action,
# the tools.Result they return could have Meta with permission_action
# But more importantly: toolResultFromPrePermissionReject NEVER sets DenialReason
# So a PrePermissionRejecter error won't be counted as a failure if isRetriableToolError
# checks Meta["permission_action"] instead of DenialReason
echo "=== isRetriableToolError logic (lines 1891-1912) ==="
sed -n '1891,1912p' internal/agent/loop.goRepository: Gitlawb/zero
Length of output: 236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's verify the exact vulnerability by examining toolResultFromPrePermissionReject
echo "=== toolResultFromPrePermissionReject - does it ever set DenialReason? ==="
sed -n '1717,1755p' internal/agent/loop.go | cat -n
echo ""
echo "=== Compare with deniedPermissionResult which DOES set DenialReason ==="
sed -n '2445,2463p' internal/agent/loop.go | cat -n
echo ""
echo "=== The vulnerability: PrePermissionRejecter errors don't get DenialReason ==="
echo "Line 1116 returns toolResultFromPrePermissionReject which has NO DenialReason field"
echo "So when isRetriableToolError runs (lines 1897-1901), it checks:"
sed -n '1897,1901p' internal/agent/loop.go
echo ""
echo "=== If a PrePermissionRejecter returns error text matching line 1905 fallback ==="
sed -n '1904,1908p' internal/agent/loop.goRepository: Gitlawb/zero
Length of output: 3460
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Now check: does any PrePermissionRejecter actually return a denied tool error?
echo "=== PrePermissionRejecter implementations returning errors ==="
rg -n --type go 'RejectBeforePermission' internal/tools/web_fetch.go -A 30 | head -60
echo ""
echo "=== Summary of the vulnerability ==="
echo "1. toolResultFromPrePermissionReject (line 1116 call) never sets DenialReason"
echo "2. If it returns StatusError with text matching 'Permission denied for'"
echo "3. isRetriableToolError returns false (line 1905 match)"
echo "4. countedFailure = false || DenialReason!=DenialNone = false"
echo "5. observeToolResult(false) deletes record, streak resets"
echo ""
echo "=== Confirm: line 748 countedFailure logic ==="
sed -n '742,749p' internal/agent/loop.goRepository: Gitlawb/zero
Length of output: 2834
Set DenialReason in toolResultFromPrePermissionReject to categorize denials correctly.
The toolResultFromPrePermissionReject function (line 1717) converts a tool's pre-permission error to a ToolResult but does not set DenialReason. When a PrePermissionRejecter tool returns an error message that matches denial text patterns (e.g., "Permission denied for"), isRetriableToolError correctly classifies it as non-retriable, but line 748's countedFailure calculation becomes false because both retriableFailure and DenialReason != DenialNone are false. This allows repeated pre-permission denials to reset the failure streak and continue looping until the turn limit.
Map the pre-permission error output to an appropriate DenialCategory (such as DenialFiltered or DenialPermissionDenied based on the error type) and set it in the returned ToolResult. Alternatively, include pre-permission rejection patterns in the countedFailure condition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agent/loop.go` around lines 743 - 749, Update
toolResultFromPrePermissionReject to set ToolResult.DenialReason when converting
a pre-permission rejection, mapping the rejection error type or message to the
appropriate DenialCategory such as DenialFiltered or DenialPermissionDenied.
Preserve the existing output and non-retriable behavior while ensuring
categorized pre-permission denials are counted by the observeToolResult
countedFailure logic.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: Approve
Verified empirically on the branch (checked out, built).
What I checked
- Gut-the-fix: disabling the category keying at
guardrails.go:524turnsTestRunStopsARepeatedlyDeniedToolAtTheFailureBoundred — a 10-denial run no longer halts at 6; it loops until the no-output guard trips at turn 13. The tests exercise the fix, not just the shape. - Not a leaky deny-list — this is the important part.
observeToolResultkeeps a content-blindanyErrorCountbackstop (guardrails.go:541,548,toolFailureAnyErrorStopAt = 12) incremented on every failure regardless of signature. So a denial that isn't categorized (DenialNone), or any non-denial error whose prose varies, still halts.DenialCategorydoesn't need to be exhaustive, which is what makes this hold up where #702's per-message id-invariance couldn't. Good call superseding that approach with a structural one. - Reports the counter that tripped (
Varied+anyErrorCount,:553), so a tool that failed 12 different ways isn't described as "failed once". - hintable/failed split (
:505-509): a categorized denial counts toward the streak but gets no schema hint — a policy refusal isn't a call-shape problem. Correct. - Clean scope:
guardrails.go, its test, and the one call site inloop.go.
Well shaped. The two-tier bound is the right design.
4 participants
The repeated-failure guard keys its streak on the first 80 characters of the error text. A permission denial reads
Error: Permission denied for <tool>: <reason>, andreasonnames the path or command that was refused — so the text differs on every call while describing the same unchanging refusal. Each call rebuilt the record atcount: 1, andtoolFailureStopAt = 6was never reached.I hit this for real, not in theory. A headless run made 384 denied calls over 26 minutes, produced zero files, and reported nothing. The guard was working exactly as written the whole time.
#702 already hit this shape once — the unknown-session error leaked its session id into the signature — and fixed it by making that one message id-invariant. That works, but it's per-message and depends on every future error remembering to be invariant. Denials now key on
DenialCategoryinstead, a small closed enum the loop already sets on the result. That fixes the class rather than one instance.The second counter
The signature-keyed streak cannot, by construction, see a tool that fails with a genuinely different error every time — and that is still a tool that isn't working. So there's now a content-blind counter beside it: consecutive failures of that tool regardless of error, cleared only by a success of that same tool. Changing how a tool fails isn't progress, and neither is some other tool succeeding while this one is refused.
It stops at 12, not 6, deliberately. A model iterating on a tricky edit legitimately fails a few times with different errors while converging — the same reasoning that moved
toolFailureStopAtfrom 4 to 6. Cutting that short would be a worse bug than the one being fixed.Two counters tripping on either is also where both of the agent CLIs I compared against landed independently, after hitting this same bug: a tight bound on identical failures OR'd with a looser one that no amount of varying the error text can reset. Convergent design, not my taste.
Verification
Six tests, and every guard mutation-checked:
TestPermissionDenialStreakSurvivesVaryingReasonText,TestAnotherToolSucceedingDoesNotClearAFailingToolsStreakTestToolFailingWithDifferentErrorsEveryTimeStillStops,TestSuccessResetsBothFailureCountersTestSuccessResetsBothFailureCountersis the regression guard that makes the new bound safe to add — it drives the tool to one below the bound, succeeds once, and requires a full fresh count afterwards rather than a resumed one.Behaviour when nothing is looping is unchanged:
toolFailureStopAtandtoolFailureHintAtkeep their values and their existing semantics.One existing test call site gains the new parameter.
internal/agentgreen,gofmtandvetclean.Summary by CodeRabbit