mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-09 13:31:26 -10:00
Vasanth T
b32f7ce4d6
* fix(agent): stop a denied tool looping past the repeated-failure halt 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. * 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. * fix(agent): count uncategorized policy refusals in the halt guard Reported by jatmn, and he is right that the guard missed the paths it most needed to cover. countedFailure asked `DenialReason != DenialNone`, but a category is only attached where a TYPED denial is built. A headless run leaves OnPermissionRequest nil, so the loop never reaches that branch and the registry returns a bare `Error: Permission required ...` with no category. A sandbox preflight denial on a non-shell tool loses its SandboxDecision converting to ToolResult and arrives as an uncategorized `Sandbox block`. isRetriableToolError rejects both, so both operands were false, observeToolResult took its success branch, and the record the guard accumulates was cleared. The same refused call could then repeat to MaxTurns, which is the loop this PR exists to stop. The text patterns for those outcomes already existed, enumerated inside isRetriableToolError. They simply were not reachable from the counting question. They are now a shared isPolicyRefusal predicate that both callers use, so the two questions cannot drift apart again, which is how they diverged in the first place. Also, denials no longer feed the execution-profile failure-streak trigger. That trigger restores the displaced turn budget and reasoning effort on the theory that a tool is struggling and needs room. A policy refusal is not a struggling tool, it is an answer, and spending the one-shot escalation on one contradicts the trigger's documented retriable-failure contract. Denials still count for the halt; they just no longer buy more budget. On coverage, honestly: the new tests pin the PREDICATE, including both uncategorized shapes, and I verified by mutation that removing the text branch fails them. They do NOT pin the wiring. Mutating countedFailure leaves them green, which is the same unit-versus-call-path gap that produced the original defect here. A Run-level test through the headless path is what would close it and this commit does not add one. * fix(agent): stop the halt answer claiming a pattern the counters do not track jatmn's P3. The final answer overclaimed in both directions. The content-blind bound said "each with a different error". anyErrorCount only establishes that the tool failed consecutively without a success. Five A, five B and two C reaches 12 without any signature repeating six times, and three of those errors were shared, so "each different" is false. It now says "varying errors", which is what reaching 12 without tripping the signature bound actually proves: no signature repeated six times in a row. The signature bound said "with the same error", which is false the other way for a denial streak. A denial keys on its CATEGORY precisely because the prose embeds the path or command refused and therefore differs on every call. That streak now reports as refused rather than as one repeated error, carried on a Refused flag derived from the signature prefix. The one claim that IS justified is kept: an error-signature streak really did repeat the same signature, so that wording stands. The existing denial test asserted the old "same error" phrasing, so it was describing the very defect this fixes; it now expects the refusal wording. Tests: the three wordings against their counters, plus the mixed-signature regression jatmn asked for, driven through the real counter rather than asserted about the strings in isolation, so it proves the 5/5/2 run trips the content-blind bound and is not described as all-different. * fix(agent): do not classify successful tool output as a policy refusal isPolicyRefusal decides on denial category, then permission metadata, then output text. None of those questions is meaningful about a call the tool completed, and the last one is answered by content the model does not control. isRetriableToolError gated on StatusError before calling in, so the boundary held while that was the only caller. Extracting the helper and calling it from the counting path dropped the gate: an allowed bash printing "Sandbox block", or a read_file returning a document that quotes it, set policyRefusal, made countedFailure true, and recorded a failure against the tool's signature. Six such successes tripped the same-signature stop and ended a healthy run with "the `bash` tool failed 6 times in a row with the same error". The gate belongs in the classifier rather than at each caller, because the next caller will forget it too. Covered by a direct StatusOK classifier case over every signal the helper reads, and by an end-to-end run of ten successful greps whose output quotes the phrase. Both fail against the ungated helper: the run halts at 6 with the refusal answer above. * test(agent): drive the uncategorized refusal paths through Run The categorized denial was never the gap. The gap is a refusal arriving with DenialReason empty, because a category is attached only where a typed denial is built: a headless run leaves OnPermissionRequest nil and the registry gate returns a bare "Permission required for ...", and a sandbox preflight denial on a non-shell tool loses its SandboxDecision converting to ToolResult and arrives as a bare "Sandbox block". Testing that through the helper proves nothing. The first version of this fix passed every helper test while being a no-op in production, because the loop asked a different question than the tests did. Both cases here go through Run and pin what the loop does with the classification: halt at the bound, never execute the tool, and withhold the profile's one-shot failure escalation. Each half of the wiring falsifies the tests on its own. Dropping policyRefusal from countedFailure lets the headless refusal run 13 turns instead of halting at 6. Dropping the empty-outcome branch for the posture controller reports posture_escalations = 1 instead of 0. * fix(agent): classify a policy refusal from provenance, never from tool output isPolicyRefusal fell back to matching phrases in the model-visible output, and output is tool-controlled. bash preserves arbitrary stdout and stderr on a StatusError for any nonzero exit, so an allowed command running `printf 'Sandbox block\n' >&2; exit 1` had actually executed, carried no denial category, and was still classified as refused. read_file returning a document that quotes one of the phrases did the same, which is the likelier way a real session hits it. The loop then withheld the retry hint and the profile failure-streak recovery and accumulated the executed failure toward the refusal halt, so a later stop told the user a tool had been refused when it had run. The registry already had the structured facts and dropped them on the floor. Every path that returns BEFORE the tool runs now carries one marker naming which gate refused: sandbox deny, sandbox approval required, permission required, permission denied. That includes the two cases that were previously uncategorized, the headless prompt refusal and the sandbox preflight denial on a non-shell tool, neither of which builds a typed DenialReason. isPolicyRefusal reads DenialReason, permission metadata and that marker, and nothing else. markStructuredSandboxDenial already stated this rule for the sandbox adapter, "Classification is never inferred from stdout or stderr"; this carries the same guarantee across the remaining gates. Coverage runs in both directions. The existing refusal fixtures now carry the provenance their production paths attach rather than relying on their text, and there is a Run-level regression where an allowed tool that ran, failed, and printed each recognized phrase still receives the retry hint. Reverting the classifier to substrings fails all three tests, including every phrase of the Run-level one. * fix(tools): give capture_artifact's configuration refusals the same provenance as the gates capture_artifact rejects in RejectBeforePermission, which the registry returns straight back before any of the gates that attach provenance. Its valid-but-unavailable calls therefore reached the classifier with no denial category, no permission metadata and no refusal marker, so they were read as ordinary retriable failures: the model got the schema hint telling it to fix arguments that were already valid, and the call could consume the profile failure-streak escalation, for a tool that never executed and that no argument change can enable. PolicyRefusalToolNotEnabled existed for exactly this and I never wired it. The missing-artifact-directory and disabled-driver branches carry it now. The malformed-argument branch deliberately stays an ordinary error. That one IS fixable by trying again differently, which is what the hint is for, so marking every early rejection would trade one wrong answer for another. Both directions are covered. Checked the rest of the class rather than only the reported tool: web_fetch, browser_launch, browser_connect, browser_open, desktop_windows, desktop_snapshot and terminal_session all reject on arguments alone, which is correctly retriable. capture_artifact was the only one refusing on configuration. Also rebased onto current main rather than carrying the two merge commits, per the same requirement raised on #886. * fix(agent): give a marker-only refusal the same identity the guard keys on The registry marks its pre-execution refusals in metadata, and isPolicyRefusal read that marker while observeToolResult keyed on DenialReason, which those paths leave empty. The guard fell back to errorSignature(output), so two refusals of the same category with different wording looked like two different failures and the streak restarted at 1 on every call. A model alternating capture_artifact's browser_screenshot and browser_pdf against a disabled driver is refused identically each time, and never tripped the six-call refusal halt. Only the generic twelve-error fallback stopped the run, reporting varied errors rather than a repeated refusal. The category is derived once now, at the boundary where a tools.Result becomes a ToolResult, and both the classification and the streak read that one value. It had to go in twice, because a RejectBeforePermission refusal takes its own constructor, and that is the route capture_artifact actually takes. Deriving it at the producers instead would have left the same gap for the next path that returns before the gates. One behaviour change worth stating plainly rather than burying. A headless prompt refusal is marked too, so it now carries a category and the stop answer says the tool was refused rather than that it failed with the same error. The bound is unchanged, and the new wording is the accurate one: the tool never ran. The test that pinned the old wording is updated, along with the comment that explained why it was uncategorized. * fix(agent): type the refusal identity and carry the halt to terminal status Three defects with one shape: provenance was encoded in a string, recovered by inspecting that string, and then not carried far enough. The same-identity streak keyed on one string namespace holding both a normalized error signature and a synthetic "denial:<category>" key, with provenance recovered afterwards by testing for that prefix. Trusted provenance and untrusted content in one namespace is a namespace the untrusted side can write into: a command printing exactly "denial:permission_denied" and exiting non-zero acquired the identity of a real permission refusal and the run reported it as refused although it executed every time. The identity is now a (kind, key) pair, so no output can spell a refusal. The content-blind bound reported only that the failures varied, and it fires precisely when no identity repeated, so the identity present at the end says nothing about the eleven before it. Alternating two refusal categories reached twelve without either reaching six and the answer described a tool that never ran as having failed. The record now carries the aggregate, and the guard returns a typed cause instead of two overlapping booleans, with the mixed case named rather than left to whichever field a switch tested first. The halt returned straight out of the tool loop, so it never crossed the completion gate the max-turns paths go through. Under RequireCompletionSignal zero exec treats only Incomplete as exit 4, so a task denied six times came back as a successful automation result having done none of the work. Also closes two coverage gaps that made the markers untestable: reverting either the sandbox deny marker in registry.go or the disabled-driver refusal in local_capture.go left both suites green. The sandbox case now runs a real engine evaluation through Run and asserts the body was skipped; the capture case configures an artifact root with one driver enabled so it reaches the disabled-driver branch instead of returning at the missing-directory one, with a sibling case pinning that a malformed argument stays retriable. * fix(agent): close out advertised tool calls by what actually happened Parallel read-ahead broke the assumption the aborted placeholders were written under. executeParallelReadBatch runs an entire eligible run of read calls before the loop consumes any of them, so "not consumed yet" stopped meaning "not executed". Every terminal branch closed out the calls after the current index as aborted, and a sibling that had already run was recorded that way: its real result discarded, and its callbacks, trace counter, task observation, loaded tools and images lost with it. Where the sibling is a successful read, execution may already have committed file-observation credit for content the model never receives, so the authorization state disagreed with the transcript. Each remaining call is now put in the state that is true of it. A completed one is finalized exactly once with the same bookkeeping the main path performs; an unstarted one still gets a placeholder so every tool_use keeps its answering tool_result. The guard is deliberately not consulted for a drained sibling: it cannot reverse a decision already made, it is only owed an honest record. All three early returns go through one helper rather than repeating the assumption, so the next stop condition inherits the fix instead of the bug. * fix(agent): authenticate refusal provenance and separate completion from abort Two things this branch left keyed on something that could disagree with the fact it stands for. A tool that ran and failed could claim the registry refused it before it ran. IsPolicyRefusalResult trusted a metadata key, and Registry.RunWithOptions forwards an executed result and its Meta unchanged, so a tool could set it by mistake, by copying metadata forward from something it called, or on purpose. The loop then withheld the retry hint, suppressed the failure-streak recovery, counted the call in refusal accounting, and could tell the user a tool was refused when it had executed. That is the output-text trust problem one layer down. The execution boundary now strips the marker, so no value survives running, recognized or invented. Pre-execution refusals are untouched, including RejectBeforePermission, which decides before any of this. precomputedResultFor treated any batch entry carrying an abort error as unstarted. Producing a result and asking the run to stop are different facts, and executeToolCall's cancelled-permission path returns both: an earlier sibling reaching a terminal branch would discard the real cancellation result and write an aborted placeholder over it. The batch now records what it ran, where that is known, and the placeholder is reserved for entries that produced nothing. The terminal decision is unchanged; draining only makes the record honest. * test(agent): pin a drained read-ahead sibling's image through the guard halt The halt drains read-ahead siblings that already ran and records them as completed, images included. Images now reach the transcript through the turn's deferred builder, and nothing checked that a drained sibling's image still went through it: dropping it left every test green. The probe tool returns an image from the sibling and the run must carry exactly one image message. * chore: follow the Twigpine module path main moved the module to github.com/Twigpine/zero in #1105. Rename this branch's imports to match, including the two in internal/workspaceindex that main itself still carries until #1153 lands. * chore(agent): drop two unused zero-value constants failureKindNone and toolFailureCauseNone were never referenced, so the pinned unused check flagged them. The zero values stay reserved with a blank identifier. * fix(agent): report mixed guard halts as mixed and pair drained siblings - toolFailureIncompleteReason gains the mixed branch, so a headless run halted by the content-blind bound over failures and refusals says the tool failed or was refused, as the stop answer does. - closeOutRemaining sets ToolCallProviderID on a drained sibling's tool result, matching the consumption path, so a strict replay can pair it. - executedToolResult strips permission_action as well as the refusal marker. The loop reads a denied permission action as a refusal, so an executed failure carrying one was counted toward the refusal halt. The key is a tools constant now, and the loop uses it too. - The parallel read-ahead comment says what differs from sequential execution: a halt can leave up to seven sibling reads already run. Reported by @jatmn.