diff --git a/internal/agent/capture_artifact_refusal_test.go b/internal/agent/capture_artifact_refusal_test.go new file mode 100644 index 000000000..190a84025 --- /dev/null +++ b/internal/agent/capture_artifact_refusal_test.go @@ -0,0 +1,143 @@ +package agent + +import ( + "context" + "strconv" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/zeroruntime" +) + +// A CONFIGURATION REFUSAL IS NOT A RETRIABLE FAILURE, EVEN WHEN IT ARRIVES +// EARLY. +// +// 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, and were read as +// ordinary retriable failures: the model got the schema hint and the call could +// consume the profile failure-streak escalation, for a tool that never executed +// and that no argument change can enable. +func TestDisabledCaptureArtifactIsAPolicyRefusalNotARetriableFailure(t *testing.T) { + // No artifacts directory and no enabled driver: valid arguments, unavailable + // tool. This is the shape an operator produces by configuration alone. + registry := tools.NewRegistry() + for _, tool := range tools.NewLocalControlArtifactTools(tools.LocalControlArtifactOptions{}) { + registry.Register(tool) + } + + result := registry.RunWithOptions(context.Background(), "capture_artifact", map[string]any{ + "action": "browser_screenshot", + "name": "shot", + }, tools.RunOptions{PermissionGranted: true}) + + if result.Status != tools.StatusError { + t.Fatalf("SETUP INVALID: expected the disabled tool to refuse, got %s: %s", result.Status, result.Output) + } + if !tools.IsPolicyRefusalResult(result) { + t.Fatalf("the refusal carries no provenance, so nothing downstream can tell it from a failed command: %#v", result.Meta) + } + + // Through the conversion the loop performs, which is where the marker has to + // survive to be worth anything. + converted := ToolResult{ + Status: result.Status, + Output: result.Output, + Meta: result.Meta, + } + if !isPolicyRefusal(converted) { + t.Error("the marker did not survive into the agent-facing result") + } + if isRetriableToolError(converted) { + t.Error("a tool that never executed, and that no argument change can enable, was marked retriable: the model gets a schema hint telling it to fix arguments that were already valid") + } +} + +// The malformed-argument branch must stay retriable. That one IS fixable by +// trying again differently, which is what the hint exists for, so marking every +// early rejection would trade one wrong answer for another. +func TestMalformedCaptureArtifactArgumentsStayRetriable(t *testing.T) { + registry := tools.NewRegistry() + for _, tool := range tools.NewLocalControlArtifactTools(tools.LocalControlArtifactOptions{}) { + registry.Register(tool) + } + + result := registry.RunWithOptions(context.Background(), "capture_artifact", map[string]any{ + "action": "not_a_real_action", + }, tools.RunOptions{PermissionGranted: true}) + + if result.Status != tools.StatusError { + t.Fatalf("SETUP INVALID: expected invalid arguments to fail, got %s", result.Status) + } + if tools.IsPolicyRefusalResult(result) { + t.Fatalf("a malformed-argument error was marked a policy refusal, so the model is denied the hint that would let it fix the call: %q", result.Output) + } + converted := ToolResult{Status: result.Status, Output: result.Output, Meta: result.Meta} + if !isRetriableToolError(converted) { + t.Error("invalid arguments should stay retriable") + } +} + +// alwaysRefusingCaptureTool stands in for the disabled tool at Run level, so the +// loop consequence can be observed rather than inferred. +type alwaysRefusingCaptureTool struct{ ran int } + +func (tool *alwaysRefusingCaptureTool) Name() string { return "capture_artifact" } +func (tool *alwaysRefusingCaptureTool) Description() string { return "test capture tool" } +func (tool *alwaysRefusingCaptureTool) Parameters() tools.Schema { + return tools.Schema{ + Type: "object", + Properties: map[string]tools.PropertySchema{"action": {Type: "string"}}, + Required: []string{"action"}, + AdditionalProperties: false, + } +} +func (tool *alwaysRefusingCaptureTool) Safety() tools.Safety { + return tools.Safety{SideEffect: tools.SideEffectRead, Permission: tools.PermissionAllow, Reason: "captures artifacts"} +} +func (tool *alwaysRefusingCaptureTool) Run(context.Context, map[string]any) tools.Result { + tool.ran++ + return tools.Result{} +} +func (tool *alwaysRefusingCaptureTool) RejectBeforePermission(map[string]any) (tools.Result, bool) { + return tools.Result{ + Status: tools.StatusError, + Output: "Error: capture_artifact is disabled because no artifact directory is configured.", + Meta: map[string]string{tools.PolicyRefusalMeta: tools.PolicyRefusalToolNotEnabled}, + }, true +} + +// THE LOOP CONSEQUENCE. A refusal must not draw the retry hint, because the hint +// tells the model to fix arguments that were already valid and the tool will +// refuse identically next time. +func TestRunDoesNotHintARefusedCaptureArtifact(t *testing.T) { + tool := &alwaysRefusingCaptureTool{} + registry := tools.NewRegistry() + registry.Register(tool) + + calls := toolFailureHintAt + 1 + turns := make([][]zeroruntime.StreamEvent, 0, calls+1) + for i := range calls { + turns = append(turns, toolTurn("call-"+strconv.Itoa(i), "capture_artifact", `{"action":"browser_screenshot"}`)) + } + turns = append(turns, textTurn("gave up on the screenshot")) + + result, err := Run(context.Background(), "take a screenshot", &mockProvider{turns: turns}, Options{ + Registry: registry, + PermissionMode: PermissionModeAsk, + MaxTurns: len(turns) + 5, + }) + if err != nil { + t.Fatal(err) + } + if tool.ran != 0 { + t.Fatalf("SETUP INVALID: the tool executed %d times; it must be refused before Run", tool.ran) + } + for _, message := range result.Messages { + if strings.Contains(message.Content, toolFailureHintMarker) { + t.Fatal("a refused, never-executed tool drew the retry hint, which tells the model to fix arguments that were already valid") + } + } +} diff --git a/internal/agent/capture_artifact_streak_test.go b/internal/agent/capture_artifact_streak_test.go new file mode 100644 index 000000000..2a23d6995 --- /dev/null +++ b/internal/agent/capture_artifact_streak_test.go @@ -0,0 +1,112 @@ +package agent + +import ( + "context" + "strconv" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/zeroruntime" +) + +// alternatingRefusalCaptureTool refuses every call with the same CATEGORY and a +// different MESSAGE, which is what a real disabled driver does: the refusal +// names the action, and the model is free to alternate valid actions. +type alternatingRefusalCaptureTool struct{ ran int } + +func (tool *alternatingRefusalCaptureTool) Name() string { return "capture_artifact" } +func (tool *alternatingRefusalCaptureTool) Description() string { return "test capture tool" } +func (tool *alternatingRefusalCaptureTool) Parameters() tools.Schema { + return tools.Schema{ + Type: "object", + Properties: map[string]tools.PropertySchema{"action": {Type: "string"}}, + Required: []string{"action"}, + AdditionalProperties: false, + } +} +func (tool *alternatingRefusalCaptureTool) Safety() tools.Safety { + return tools.Safety{SideEffect: tools.SideEffectRead, Permission: tools.PermissionAllow, Reason: "captures artifacts"} +} +func (tool *alternatingRefusalCaptureTool) Run(context.Context, map[string]any) tools.Result { + tool.ran++ + return tools.Result{} +} +func (tool *alternatingRefusalCaptureTool) RejectBeforePermission(args map[string]any) (tools.Result, bool) { + action, _ := args["action"].(string) + return tools.Result{ + Status: tools.StatusError, + // Action-specific wording, same category. This is the whole point: the + // prose differs on every call while the refusal never changes. + Output: "Error: Local control driver for " + action + " is disabled.", + Meta: map[string]string{tools.PolicyRefusalMeta: tools.PolicyRefusalToolNotEnabled}, + }, true +} + +// A REFUSAL KEYS ON ITS CATEGORY, INCLUDING WHEN THE CATEGORY IS ONLY A MARKER. +// +// The registry marks 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 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 halt. Only the generic twelve-error fallback stopped the run, +// reporting varied errors rather than a repeated refusal. +func TestAlternatingRefusedActionsStillTripTheRefusalHalt(t *testing.T) { + tool := &alternatingRefusalCaptureTool{} + registry := tools.NewRegistry() + registry.Register(tool) + + // More calls than the refusal halt but FEWER than the generic any-error + // fallback, so only the category-keyed streak can stop this. + calls := toolFailureAnyErrorStopAt - 1 + if calls <= toolFailureStopAt { + t.Fatalf("SETUP INVALID: %d calls cannot distinguish the refusal halt from the generic fallback", calls) + } + actions := []string{"browser_screenshot", "browser_pdf"} + turns := make([][]zeroruntime.StreamEvent, 0, calls+1) + for i := range calls { + action := actions[i%len(actions)] + turns = append(turns, toolTurn("call-"+strconv.Itoa(i), "capture_artifact", `{"action":"`+action+`"}`)) + } + turns = append(turns, textTurn("gave up")) + + result, err := Run(context.Background(), "capture something", &mockProvider{turns: turns}, Options{ + Registry: registry, + PermissionMode: PermissionModeAsk, + MaxTurns: len(turns) + 5, + }) + if err != nil { + t.Fatal(err) + } + if tool.ran != 0 { + t.Fatalf("SETUP INVALID: the tool executed %d times; it must be refused before Run", tool.ran) + } + + refusals := 0 + for _, message := range result.Messages { + if strings.Contains(message.Content, "is disabled") { + refusals++ + } + } + if refusals > toolFailureStopAt { + t.Errorf("the run made %d refused calls; the six-call refusal halt never tripped because the streak re-keyed on each action's wording", refusals) + } + + // And the halt has to read as a repeated refusal, not as varied errors. + stop := strings.ToLower(strings.Join(messageContents(result.Messages), "\n")) + if strings.Contains(stop, toolFailureHintMarker) { + t.Error("a refused, never-executed tool drew the retry hint") + } +} + +func messageContents(messages []zeroruntime.Message) []string { + out := make([]string, 0, len(messages)) + for _, message := range messages { + out = append(out, message.Content) + } + return out +} diff --git a/internal/agent/capture_disabled_driver_test.go b/internal/agent/capture_disabled_driver_test.go new file mode 100644 index 000000000..a0c601a17 --- /dev/null +++ b/internal/agent/capture_disabled_driver_test.go @@ -0,0 +1,93 @@ +package agent + +import ( + "context" + "testing" + + "github.com/Gitlawb/zero/internal/localcontrol" + "github.com/Gitlawb/zero/internal/tools" +) + +// THE DISABLED-DRIVER BRANCH, REACHED FOR REAL. +// +// RejectBeforePermission refuses on two separate conditions, and its neighbour +// test constructs empty options, so it returns at the FIRST one (no artifact +// directory configured) and the second is never evaluated. Reverting the +// disabled-driver branch in local_capture.go to a plain errorResult therefore +// left the whole agent and tools suites green. +// +// This is the shape an operator actually produces: an artifact root IS +// configured and one driver IS enabled, and the model asks for an action owned +// by a different, disabled driver. Without provenance that call is read as a +// retriable failure, so it collects a schema hint and can spend the profile's +// failure-streak escalation, for a tool no argument change can enable. +func TestCaptureArtifactDisabledDriverIsAPolicyRefusal(t *testing.T) { + registry := tools.NewRegistry() + options := tools.LocalControlArtifactOptions{ + ArtifactsDir: t.TempDir(), + // Configured and enabled, so the tool as a whole is available and the + // missing-directory branch cannot fire. + Browser: localcontrol.BrowserOptions{Enabled: true, Driver: "test-browser", HelperPath: "browser-helper"}, + // Terminal deliberately left disabled. + } + for _, tool := range tools.NewLocalControlArtifactTools(options) { + registry.Register(tool) + } + + // An enabled driver's action must still be accepted, or the test below would + // pass for a tool that refuses everything. + if result := registry.RunWithOptions(context.Background(), "capture_artifact", map[string]any{ + "action": "browser_screenshot", "name": "shot", + }, tools.RunOptions{PermissionGranted: true}); tools.IsPolicyRefusalResult(result) { + t.Fatalf("SETUP INVALID: the enabled browser driver was refused as policy: %s", result.Output) + } + + result := registry.RunWithOptions(context.Background(), "capture_artifact", map[string]any{ + "action": "terminal_snapshot", "name": "snap", "session": "test-session", + }, tools.RunOptions{PermissionGranted: true}) + + if result.Status != tools.StatusError { + t.Fatalf("SETUP INVALID: the disabled terminal driver did not refuse: %s / %s", result.Status, result.Output) + } + if !tools.IsPolicyRefusalResult(result) { + t.Fatalf("a driver disabled by configuration refuses without provenance, so it reads as a retriable failure: %#v", result.Meta) + } + if got := result.Meta["policy_refusal"]; got != tools.PolicyRefusalToolNotEnabled { + t.Errorf("policy_refusal = %q, want %q", got, tools.PolicyRefusalToolNotEnabled) + } + if isRetriableToolError(ToolResult{Status: result.Status, Output: result.ModelOutput(), Meta: result.Meta}) { + t.Error("the refusal is still classified as retriable, so the model gets a schema hint for a configuration decision") + } +} + +// AND THE EARLY REJECTIONS MUST NOT COLLAPSE INTO ONE ANSWER. +// +// RejectBeforePermission refuses on configuration; argument validation refuses +// on a fixable mistake, and it runs FIRST, which is how the disabled-driver +// branch stayed uncovered. A malformed call must stay an ordinary retriable +// error, because trying again differently is exactly the right response to it. +func TestCaptureArtifactMalformedArgumentsStayRetriable(t *testing.T) { + registry := tools.NewRegistry() + for _, tool := range tools.NewLocalControlArtifactTools(tools.LocalControlArtifactOptions{ + ArtifactsDir: t.TempDir(), + Terminal: localcontrol.TerminalOptions{Enabled: true, Driver: "test-terminal", HelperPath: "terminal-helper"}, + }) { + registry.Register(tool) + } + + // The driver IS enabled, so nothing here is a configuration decision. The + // call is simply missing the session the action requires. + result := registry.RunWithOptions(context.Background(), "capture_artifact", map[string]any{ + "action": "terminal_snapshot", "name": "snap", + }, tools.RunOptions{PermissionGranted: true}) + + if result.Status != tools.StatusError { + t.Fatalf("SETUP INVALID: the malformed call did not fail: %s", result.Output) + } + if tools.IsPolicyRefusalResult(result) { + t.Errorf("a fixable argument mistake was marked a policy refusal, so the model loses the schema hint that would fix it: %s", result.Output) + } + if !isRetriableToolError(ToolResult{Status: result.Status, Output: result.ModelOutput(), Meta: result.Meta}) { + t.Errorf("a fixable argument mistake is not retriable: %s", result.Output) + } +} diff --git a/internal/agent/guardrails.go b/internal/agent/guardrails.go index ccd9d26e9..ed848a945 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 @@ -324,15 +340,89 @@ func acceptanceVerificationNudge(objective string) string { const toolFailureHintMarker = "kept failing with the same error" type toolFailureRecord struct { - count int - errSig string - hintShown bool + count int + identity failureIdentity + // sawExecuted and sawRefused are the AGGREGATE over the whole streak, not + // the current identity. The content-blind bound needs them because it fires + // exactly when no identity repeated, so the last one is unrepresentative. + sawExecuted bool + sawRefused bool + 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 +} + +// failureKind separates a failure the TOOL produced from one POLICY produced. +// +// The two used to share one string namespace, with provenance recovered +// afterwards by testing that string for a "denial:" prefix. Trusted provenance +// and untrusted content in one namespace is a namespace the untrusted side can +// write into: a command that printed exactly "denial:permission_denied" and +// exited non-zero acquired the identity of a real permission refusal, merged +// its streak with one, and had the run report it as refused although it ran +// every time. The kind is carried as its own field so no output can spell it. +type failureKind uint8 + +const ( + failureKindNone failureKind = iota + // failureKindExecuted: the tool ran and returned an error. The key is the + // normalized error signature, which is tool-controlled text. + failureKindExecuted + // failureKindRefused: policy refused the call before the tool ran. The key is + // the denial category, a small closed enum the loop sets. + failureKindRefused +) + +// failureIdentity is what the same-identity streak counts. Comparing the pair +// means an executed error can never equal a refusal however it is spelled. +type failureIdentity struct { + kind failureKind + key string } +// toolFailureCause is why the guard stopped, decided where the facts are rather +// than reconstructed by the caller from overlapping booleans. +type toolFailureCause uint8 + +const ( + toolFailureCauseNone toolFailureCause = iota + // toolFailureCauseSameError: the same executed error signature, six times. + toolFailureCauseSameError + // toolFailureCauseSameRefusal: the same denial category, six times. The prose + // differs on every call because it names what was refused, so this is NOT the + // same-error case. + toolFailureCauseSameRefusal + // toolFailureCauseVariedExecuted: the content-blind bound, and every failure + // was the tool executing and failing. + toolFailureCauseVariedExecuted + // toolFailureCauseVariedRefused: the content-blind bound, and every failure + // was a policy refusal. Alternating two categories reaches twelve without + // either reaching six, and the tool never ran once. + toolFailureCauseVariedRefused + // toolFailureCauseVariedMixed: the content-blind bound over both kinds. + // Named deliberately rather than left to whichever field a switch tested + // first. + toolFailureCauseVariedMixed +) + type toolFailureOutcome struct { InjectHint bool Stop bool Count int + // Cause is set only when Stop is. It carries the aggregate fact about the + // sequence that tripped a bound, which the last record alone cannot supply: + // the content-blind bound is reached precisely when no single identity + // repeated enough, so the identity present at the end says nothing about the + // eleven before it. + Cause toolFailureCause +} + +// Refused reports whether policy, rather than the tool, produced the failures. +func (cause toolFailureCause) Refused() bool { + return cause == toolFailureCauseSameRefusal || cause == toolFailureCauseVariedRefused } // errorSignature normalizes a tool error to a short, comparable signature so @@ -358,12 +448,39 @@ 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 { - 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. " + +func toolFailureStopAnswer(toolName string, count int, cause toolFailureCause) string { + // Each branch claims only what its counter established, and the mixed case is + // spelled out rather than decided by whichever field a switch happened to + // test first. The wording used to overclaim in both directions: the + // content-blind bound said every failure differed, which it does not track, + // and it described a run of policy refusals as the tool failing, when the + // tool had not run at all. + verb, tail := " tool failed ", " times in a row with the same error, " + switch cause { + case toolFailureCauseSameRefusal: + verb, tail = " tool was refused ", " times in a row, " + case toolFailureCauseVariedExecuted: + tail = " times in a row with varying errors, " + case toolFailureCauseVariedRefused: + verb, tail = " tool was refused ", " times in a row for different reasons, " + case toolFailureCauseVariedMixed: + verb, tail = " tool failed or was refused ", " times in a row, " + } + return "Agent stopped: the `" + toolName + "`" + verb + strconv.Itoa(count) + + tail + "so I halted instead of looping further. " + "Please check the request or adjust the tool arguments." } +// toolFailureIncompleteReason describes a guard halt for the headless +// completion gate. A guard halt is BY DEFINITION unfinished work: the run +// stopped because a tool would not stop failing, not because the task was done. +func toolFailureIncompleteReason(toolName string, cause toolFailureCause) string { + if cause.Refused() { + return "halted after `" + toolName + "` was refused repeatedly, without completing the task" + } + return "halted after `" + toolName + "` failed repeatedly, without completing the task" +} + // The no-output stop answer is assembled from these fixed parts (only the turn // count varies). IsNoProgressStop matches all three so a legitimate message that // merely quotes the marker substring is not misclassified as a failed empty run. @@ -472,28 +589,68 @@ 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{} } - sig := errorSignature(output) + // 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. + identity := failureIdentity{kind: failureKindExecuted, key: errorSignature(output)} + if denial != DenialNone { + identity = failureIdentity{kind: failureKindRefused, key: string(denial)} + } record := state.toolFailures[name] - if record == nil || record.errSig != sig { - record = &toolFailureRecord{count: 1, errSig: sig} + if record == nil { + record = &toolFailureRecord{identity: identity} state.toolFailures[name] = record + } + if record.identity != identity { + // A different failure restarts the same-identity streak but NOT the + // content-blind one: changing how a tool fails is not progress. + record.count = 1 + record.identity = identity + record.hintShown = false } else { record.count++ } + record.anyErrorCount++ + if identity.kind == failureKindRefused { + record.sawRefused = true + } else { + record.sawExecuted = true + } + outcome := toolFailureOutcome{Count: record.count} - if record.count >= toolFailureStopAt { + switch { + case record.count >= toolFailureStopAt: outcome.Stop = true + outcome.Cause = toolFailureCauseSameError + if identity.kind == failureKindRefused { + outcome.Cause = toolFailureCauseSameRefusal + } + return outcome + case record.anyErrorCount >= toolFailureAnyErrorStopAt: + // Report the counter that actually tripped. record.count is the + // same-identity 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.Cause = record.variedCause() return outcome } - if record.count >= toolFailureHintAt && !record.hintShown { + if hintable && record.count >= toolFailureHintAt && !record.hintShown { record.hintShown = true outcome.InjectHint = true } @@ -603,3 +760,15 @@ func (state *guardState) planReminder(turn int) string { return "" } + +// variedCause classifies a content-blind stop by what the whole streak held. +func (record *toolFailureRecord) variedCause() toolFailureCause { + switch { + case record.sawRefused && record.sawExecuted: + return toolFailureCauseVariedMixed + case record.sawRefused: + return toolFailureCauseVariedRefused + default: + return toolFailureCauseVariedExecuted + } +} diff --git a/internal/agent/guardrails_test.go b/internal/agent/guardrails_test.go index 5ae974eb4..a9c6cbe5b 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, toolFailureCauseSameRefusal) + 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.Cause != toolFailureCauseVariedExecuted { + t.Errorf("Cause = %v for a stop driven by the content-blind counter over executed failures", outcome.Cause) + } + if outcome.Count != toolFailureAnyErrorStopAt { + t.Errorf("Count = %d, want the counter that tripped (%d)", outcome.Count, toolFailureAnyErrorStopAt) + } + answer := toolFailureStopAnswer("bash", outcome.Count, outcome.Cause) + if !strings.Contains(answer, "varying errors") { + 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 fe691ac4c..72a82be55 100644 --- a/internal/agent/loop.go +++ b/internal/agent/loop.go @@ -653,6 +653,55 @@ func Run(ctx context.Context, prompt string, provider Provider, options Options) // executed concurrently, consumed strictly in order below. var precomputed []precomputedToolResult precomputedStart, precomputedEnd := 0, 0 + // closeOutRemaining terminates every advertised call from `from` onward + // TRUTHFULLY, and every early return in this loop goes through it. + // + // Read-ahead broke the assumption the abort placeholders were written + // under. executeParallelReadBatch runs an entire eligible run before the + // loop consumes any of it, so "not consumed yet" stopped meaning "not + // executed": a sibling can already have run, committed its side effects + // and any authorization credit they carry, and still sit past the index + // where a terminal branch fires. Recording it as aborted threw that result + // away, so the transcript disagreed with what had actually happened, and + // its callbacks, trace counter, task observation and images went with it. + // + // So each remaining call is put in the one state that is true of it: + // completed, and finalized exactly once with the same bookkeeping the main + // path performs, or unstarted, and aborted. The guard is deliberately NOT + // consulted for a drained sibling: these results cannot reverse a stop + // decision that has already been made, they are only owed an honest record. + closeOutRemaining := func(from int) { + for next := from; next < len(collected.ToolCalls); next++ { + sibling, ran := precomputedResultFor(precomputed, precomputedStart, precomputedEnd, next) + if !ran { + messages = appendAbortedToolResults(messages, collected.ToolCalls[next:next+1]) + continue + } + nextCall := collected.ToolCalls[next] + if options.OnToolCall != nil { + options.OnToolCall(nextCall) + } + options.Trace.Counter(trace.CounterToolCalls, 1) + recordOutputBudgetTrace(options.Trace, sibling) + task.observe(taskStateEvent{kind: taskStateEventToolResult, arguments: nextCall.Arguments, toolResult: sibling}) + if options.OnToolResult != nil { + options.OnToolResult(sibling) + } + for _, name := range sibling.LoadedTools { + loaded[name] = true + } + messages = append(messages, zeroruntime.Message{ + Role: zeroruntime.MessageRoleTool, + Content: sibling.ModelOutput(), + ToolCallID: sibling.ToolCallID, + IsError: sibling.Status == tools.StatusError, + ChangedFiles: append([]string(nil), sibling.ChangedFiles...), + }) + if imageMessage, ok := toolResultImageMessage(sibling); ok { + toolImageMessages = append(toolImageMessages, imageMessage) + } + } + } for index, call := range collected.ToolCalls { // When this call starts a consecutive run of >= 2 auto-allowed read-only // calls, execute the whole run concurrently now (see parallel_tools.go). @@ -724,13 +773,13 @@ func Run(ctx context.Context, prompt string, provider Provider, options Options) abortErr = ctx.Err() } if abortErr != nil { - messages = appendAbortedToolResults(messages, collected.ToolCalls[index+1:]) + closeOutRemaining(index + 1) messages = append(messages, toolImageMessages...) result.Messages = copyMessages(messages) return result, abortErr } if stopReason := stopReasonFromToolResult(toolResult); stopReason != "" { - messages = appendAbortedToolResults(messages, collected.ToolCalls[index+1:]) + closeOutRemaining(index + 1) messages = append(messages, toolImageMessages...) result.FinalAnswer = toolResult.ModelOutput() result.StopReason = stopReason @@ -745,8 +794,29 @@ 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.ModelOutput()) - posture.observeToolOutcome(outcome, toolResult) + // 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. + // isPolicyRefusal rather than DenialReason alone: the categories are + // only attached where a typed denial is built, so a headless prompt + // refusal and an uncategorized sandbox block were counted as SUCCESS + // and cleared the record they were supposed to accumulate. + policyRefusal := isPolicyRefusal(toolResult) + countedFailure := retriableFailure || policyRefusal + outcome := guards.observeToolResult(call.Name, countedFailure, retriableFailure, toolResult.ModelOutput(), toolResult.DenialReason) + // The profile's failure-streak trigger is for RETRIABLE failures: it + // restores the displaced turn budget and reasoning effort on the theory + // that the 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 that trigger's own contract. Denials + // still count for the halt above; they just do not buy more budget. + if policyRefusal { + posture.observeToolOutcome(toolFailureOutcome{}, toolResult) + } else { + posture.observeToolOutcome(outcome, toolResult) + } if outcome.Stop { // The assistant message advertised EVERY collected tool call, but // the guard halts mid-turn so the calls after this one never run. @@ -754,10 +824,22 @@ func Run(ctx context.Context, prompt string, provider Provider, options Options) // every tool_use has a matching tool_result and the recorded // 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:]) + closeOutRemaining(index + 1) messages = append(messages, toolImageMessages...) - result.FinalAnswer = toolFailureStopAnswer(call.Name, outcome.Count) + result.FinalAnswer = toolFailureStopAnswer(call.Name, outcome.Count, outcome.Cause) result.Messages = copyMessages(messages) + // THE HALT HAS TO CROSS THE TERMINAL-STATUS BOUNDARY TOO. + // + // This branch returns straight out, so it never reached the + // completion gate that the max-turns paths below go through. Under + // RequireCompletionSignal `zero exec` treats only Incomplete as exit + // 4 and otherwise emits run_end("success", 0), so a task that was + // denied six times, or failed through the twelve-call bound, became + // a successful automation result having done none of the work. + if options.RequireCompletionSignal { + result.Incomplete = true + result.IncompleteReason = toolFailureIncompleteReason(call.Name, outcome.Cause) + } return result, nil } if outcome.InjectHint && failureHint == "" { @@ -1536,9 +1618,41 @@ func executeToolCall(ctx context.Context, registry *tools.Registry, call ToolCal // the Run turn loop performs the actual provider switch. Empty for every // ordinary tool result. RequestedModel: result.Meta["escalate_to_model"], + // ONE REFUSAL IDENTITY, derived here so classification and streak + // accounting cannot disagree. + // + // 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 therefore fell + // back to errorSignature(output), and two refusals of the same category + // with different wording looked like two different failures. 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 it, + // reporting varied errors rather than a repeated refusal. + DenialReason: denialCategoryForResult(result), }, nil } +// denialCategoryForResult gives every pre-execution refusal a stable category, +// whether it was built as a typed denial or only marked in metadata. +// +// The mapping lives here, at the one boundary tools.Result becomes ToolResult, +// rather than at each producer. A marker without a category is the shape that +// caused this: it classified as a refusal and keyed as an error signature. +func denialCategoryForResult(result tools.Result) DenialCategory { + switch result.Meta[tools.PolicyRefusalMeta] { + case tools.PolicyRefusalToolNotEnabled: + return DenialFiltered + case tools.PolicyRefusalPermissionDenied, tools.PolicyRefusalPermissionRequired: + return DenialPermissionDenied + case tools.PolicyRefusalSandboxDenied, tools.PolicyRefusalSandboxApproval: + return DenialSandboxBlock + default: + return DenialNone + } +} + const sandboxNamespaceLimitedReason = "sandbox output is limited to the sandbox PID namespace; host/global state requires approval" func maybeRetryUnsandboxedAfterSandboxRestriction(ctx context.Context, registry *tools.Registry, call ToolCall, tool tools.Tool, args map[string]any, result tools.Result, permissionMode PermissionMode, options Options, progressCallback func(streamjson.Event)) (tools.Result, *ToolResult, bool, PermissionDecisionAction, string, []string, error) { @@ -1864,6 +1978,10 @@ func toolResultFromPrePermissionReject(call ToolCall, result tools.Result) ToolR Display: display, LoadedTools: loadedToolsFromResult(meta), RequestedModel: meta["escalate_to_model"], + // The SAME identity the executed path derives. This is the route a + // RejectBeforePermission refusal takes, so leaving it empty here is what + // made the marker classify as a refusal and key as an error signature. + DenialReason: denialCategoryForResult(tools.Result{Meta: meta}), } } @@ -2035,21 +2153,64 @@ func isRetriableToolError(result ToolResult) bool { // A categorized denial (filtered / permission / sandbox) is a policy decision, // not a transient failure — never retry it. This is robust to message wording // (the text checks below remain as a fallback for results lacking the field). - if result.DenialReason != DenialNone { + return !isPolicyRefusal(result) +} + +// isPolicyRefusal reports a result the run refused on policy grounds: a +// permission gate, a filter, a sandbox preflight, or a hook. +// +// Split out of isRetriableToolError so the two questions cannot drift apart, +// which is exactly what they had done. The halt guard asked +// `DenialReason != DenialNone`, and a category is only attached on the paths +// that build a TYPED denial. A headless run leaves OnPermissionRequest nil, so +// the loop never reaches its typed-denial branch and the registry returns a bare +// `Error: Permission required ...` carrying no category. A sandbox preflight +// denial on a non-shell tool loses its SandboxDecision converting to ToolResult +// and arrives as an uncategorized `Sandbox block`. +// +// Both are refusals, and both were counted as SUCCESS, so observeToolResult +// cleared the record and the same refused call could repeat to MaxTurns. That is +// the loop this guard exists to stop. The text checks below already enumerated +// these outcomes for the retriable question; they simply were not reachable from +// the counting one. +func isPolicyRefusal(result ToolResult) bool { + // A REFUSAL IS A FAILURE. Everything below inspects metadata and then output + // text, and neither is meaningful on a result the tool completed. + // + // isRetriableToolError gates on this before it calls here, so the boundary + // held while that was the only caller. Extracting this helper and calling it + // straight from the counting path dropped the gate: an allowed `bash` + // printing "Sandbox block", or a read_file returning a document that quotes + // one of these phrases, was counted as a denial. Six such successes tripped + // the same-signature stop and ended a healthy run with a refusal answer. + // + // The gate belongs here rather than at each caller, because the next caller + // will forget it too. + if result.Status != tools.StatusError { return false } - if result.Meta["permission_action"] == string(PermissionActionDeny) { - return false + if result.DenialReason != DenialNone { + return true } - switch { - case strings.Contains(result.Output, "is not enabled for this run"), - strings.Contains(result.Output, "Permission denied for "), - strings.Contains(result.Output, "Permission required for "), - strings.Contains(result.Output, "Sandbox block"), - strings.Contains(result.Output, "Sandbox approval required for "): - return false + if result.Meta["permission_action"] == string(PermissionActionDeny) { + return true } - return true + // STRUCTURED ONLY. This used to fall back to matching phrases in Output + // ("Sandbox block", "Permission denied for ", and so on), which cannot work: + // 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` has actually executed, carries no + // DenialReason, and was still classified as refused. The loop then withheld + // the schema hint and the profile failure-streak recovery, counted the + // executed failure toward the refusal halt, and a later stop told the user a + // tool had been refused when it had run. + // + // read_file returning a document that quotes one of those phrases did the + // same thing, which is the more likely way a real session hits it. + // + // The registry now marks every path that returns BEFORE the tool runs, so + // the question is answerable from provenance. Nothing below reads Output. + return tools.IsPolicyRefusalResult(tools.Result{Meta: result.Meta}) } // scrubInterceptedOutput mirrors the registry's scrubResultSecrets boundary for @@ -3485,3 +3646,30 @@ func toolResultImageMessage(result ToolResult) (zeroruntime.Message, bool) { Images: images, }, true } + +// precomputedResultFor reports the read-ahead result for call index, and whether +// that call actually produced one. A batch entry that never started is still +// unstarted as far as the transcript is concerned. +// +// PRODUCING A RESULT AND ASKING THE RUN TO STOP ARE DIFFERENT FACTS. Treating a +// non-nil abortErr as "unstarted" conflated them, and a cancelled permission +// request is exactly the case where they disagree: executeToolCall builds the +// cancellation result and returns it WITH ErrPermissionApprovalCanceled, so an +// earlier sibling reaching a terminal branch discarded a real result and wrote +// an aborted placeholder in its place. The transcript then denied that the call +// had completed permission handling, and the cancellation was missing from +// OnToolResult, the trace counters and the task observation even though the +// permission event had already happened. +// +// The terminal decision is not affected: draining such a sibling only records +// what occurred, it cannot reverse a stop already selected. +func precomputedResultFor(precomputed []precomputedToolResult, start, end, index int) (ToolResult, bool) { + if index < start || index >= end { + return ToolResult{}, false + } + entry := precomputed[index-start] + if !entry.completed { + return ToolResult{}, false + } + return entry.result, true +} diff --git a/internal/agent/loop_test.go b/internal/agent/loop_test.go index f17e9be46..09fb34daa 100644 --- a/internal/agent/loop_test.go +++ b/internal/agent/loop_test.go @@ -668,11 +668,23 @@ func TestIsRetriableToolError(t *testing.T) { {"success", ToolResult{Status: tools.StatusOK}, false}, {"bad arguments", ToolResult{Status: tools.StatusError, Output: "Error: Failed to parse arguments for x: bad json"}, true}, {"execution failure", ToolResult{Status: tools.StatusError, Output: "Error: read foo.txt: no such file"}, true}, - {"disabled tool", ToolResult{Status: tools.StatusError, Output: `Error: Tool "x" is not enabled for this run.`}, false}, + // Each refusal carries the provenance its production path attaches: the + // disabled-tool path sets a denial category, the registry gates set the + // pre-execution marker. Text is never what decides. + {"disabled tool", ToolResult{Status: tools.StatusError, Output: `Error: Tool "x" is not enabled for this run.`, DenialReason: DenialFiltered}, false}, {"permission denied (meta)", ToolResult{Status: tools.StatusError, Output: "Error: Permission denied for x: needs approval", Meta: map[string]string{"permission_action": "deny"}}, false}, - {"permission required", ToolResult{Status: tools.StatusError, Output: "Error: Permission required for x: approve first"}, false}, - {"sandbox block", ToolResult{Status: tools.StatusError, Output: "Error: Sandbox block: outside_workspace"}, false}, - {"sandbox approval", ToolResult{Status: tools.StatusError, Output: "Error: Sandbox approval required for x: network"}, false}, + {"permission required", ToolResult{Status: tools.StatusError, Output: "Error: Permission required for x: approve first", Meta: map[string]string{tools.PolicyRefusalMeta: tools.PolicyRefusalPermissionRequired}}, false}, + {"sandbox block", ToolResult{Status: tools.StatusError, Output: "Error: Sandbox block: outside_workspace", Meta: map[string]string{tools.PolicyRefusalMeta: tools.PolicyRefusalSandboxDenied}}, false}, + {"sandbox approval", ToolResult{Status: tools.StatusError, Output: "Error: Sandbox approval required for x: network", Meta: map[string]string{tools.PolicyRefusalMeta: tools.PolicyRefusalSandboxApproval}}, false}, + // THE INVERSE, which is the failure this classifier had. An allowed tool + // that RAN, failed, and happened to print one of those phrases must stay + // retriable: `bash` preserves arbitrary stdout and stderr on any nonzero + // exit, and read_file returns whatever the file says. + {"executed failure printing the sandbox phrase", ToolResult{Status: tools.StatusError, Output: "Sandbox block\n"}, true}, + {"executed failure printing a permission denial", ToolResult{Status: tools.StatusError, Output: "cp: Permission denied for /etc/hosts"}, true}, + {"executed failure printing a permission requirement", ToolResult{Status: tools.StatusError, Output: "Permission required for sudo"}, true}, + {"executed failure printing the approval phrase", ToolResult{Status: tools.StatusError, Output: "Sandbox approval required for curl"}, true}, + {"executed failure quoting the disabled-tool phrase", ToolResult{Status: tools.StatusError, Output: "grep: web_fetch is not enabled for this run"}, true}, // Structured denial categories are authoritative regardless of message text. {"denial: filtered", ToolResult{Status: tools.StatusError, Output: "anything", DenialReason: DenialFiltered}, false}, {"denial: permission", ToolResult{Status: tools.StatusError, Output: "anything", DenialReason: DenialPermissionDenied}, false}, diff --git a/internal/agent/parallel_readahead_halt_test.go b/internal/agent/parallel_readahead_halt_test.go new file mode 100644 index 000000000..8299859fa --- /dev/null +++ b/internal/agent/parallel_readahead_halt_test.go @@ -0,0 +1,172 @@ +package agent + +import ( + "context" + "strconv" + "strings" + "sync" + "testing" + + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/zeroruntime" +) + +// readAheadTool is read-only and thread-safe, so a consecutive run of its calls +// is executed as one parallel batch before the loop consumes any of it. +type readAheadTool struct { + mu sync.Mutex + ran int +} + +func (t *readAheadTool) Name() string { return "read_probe" } +func (t *readAheadTool) Description() string { return "read-only probe" } +func (t *readAheadTool) Parameters() tools.Schema { + return tools.Schema{Type: "object", Properties: map[string]tools.PropertySchema{"id": {Type: "string"}}} +} + +func (t *readAheadTool) Safety() tools.Safety { + return tools.Safety{SideEffect: tools.SideEffectRead, Permission: tools.PermissionAllow, Reason: "test"} +} + +func (t *readAheadTool) Capabilities() tools.ToolCapabilities { + return tools.ToolCapabilities{Effect: tools.EffectReadOnly, ThreadSafe: true} +} + +func (t *readAheadTool) Run(_ context.Context, args map[string]any) tools.Result { + t.mu.Lock() + t.ran++ + t.mu.Unlock() + id, _ := args["id"].(string) + // Varying output, so no identity repeats and only the content-blind counter + // can halt the run. + return tools.Result{Status: tools.StatusError, Output: "failure for " + id} +} + +func readCall(id string) []zeroruntime.StreamEvent { + return []zeroruntime.StreamEvent{ + {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: id, ToolName: "read_probe"}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: id, ArgumentsFragment: `{"id":"` + id + `"}`}, + {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: id}, + } +} + +// READ-AHEAD MEANS "NOT CONSUMED YET" NO LONGER MEANS "NOT EXECUTED". +// +// executeParallelReadBatch runs an entire eligible run of read calls before the +// loop consumes any of them. Every terminal branch in the consumption loop used +// to close out the calls after the current index with aborted placeholders, on +// the assumption that they had not run. A sibling that had already executed was +// therefore recorded as aborted: its real result was discarded, the transcript +// disagreed with what had happened, and its callbacks, trace counter, task +// observation and images were lost with it. Where such a sibling is a +// successful read, execution may already have committed file-observation credit +// for content the model never receives. +func TestParallelSiblingsAreFinalizedWhenTheGuardHalts(t *testing.T) { + tool := &readAheadTool{} + registry := tools.NewRegistry() + registry.Register(tool) + + // Eleven single-call turns bring the content-blind counter to one below its + // bound, so the first call of the pair below trips it. + var turns [][]zeroruntime.StreamEvent + for i := 0; i < toolFailureAnyErrorStopAt-1; i++ { + id := "s" + strconv.Itoa(i) + turns = append(turns, append(readCall(id), zeroruntime.StreamEvent{Type: zeroruntime.StreamEventDone})) + } + // Then one turn advertising two parallel-eligible calls. Both execute in the + // batch; the halt fires while consuming the first. + pair := append(readCall("pA"), readCall("pB")...) + turns = append(turns, append(pair, zeroruntime.StreamEvent{Type: zeroruntime.StreamEventDone})) + turns = append(turns, append(readCall("unreached"), zeroruntime.StreamEvent{Type: zeroruntime.StreamEventDone})) + + var reported []ToolResult + var announced int + result, err := Run(context.Background(), "read things", &mockProvider{turns: turns}, Options{ + Registry: registry, + PermissionMode: PermissionModeAuto, + MaxTurns: len(turns) + 5, + OnToolCall: func(ToolCall) { announced++ }, + OnToolResult: func(r ToolResult) { reported = append(reported, r) }, + }) + if err != nil { + t.Fatal(err) + } + if !strings.HasPrefix(result.FinalAnswer, "Agent stopped:") { + t.Fatalf("the guard did not halt this run: %q", result.FinalAnswer) + } + + // Every execution is reported exactly once, and the announcements match. + if tool.ran != len(reported) { + t.Errorf("the tool executed %d times but %d results were reported; a completed call was recorded as aborted", + tool.ran, len(reported)) + } + if announced != len(reported) { + t.Errorf("OnToolCall fired %d times against %d results; the two callbacks disagree", announced, len(reported)) + } + + // Strict pairing: one tool result per advertised tool call, and the drained + // sibling carries its REAL output rather than the aborted placeholder. + byID := map[string]int{} + var siblingContent string + for _, message := range result.Messages { + if message.Role != zeroruntime.MessageRoleTool { + continue + } + byID[message.ToolCallID]++ + if message.ToolCallID == "pB" { + siblingContent = message.Content + } + } + for _, id := range []string{"pA", "pB"} { + if byID[id] != 1 { + t.Errorf("tool call %s has %d results, want exactly 1", id, byID[id]) + } + } + if siblingContent == abortedToolResultNotice { + t.Error("the sibling that already ran was recorded as aborted") + } + if !strings.Contains(siblingContent, "failure for pB") { + t.Errorf("the sibling's real output is missing from the transcript: %q", siblingContent) + } + + // And the drained sibling must not reverse the decision that was already + // made: the halt is still attributed to the call that tripped it. + if !strings.Contains(result.FinalAnswer, strconv.Itoa(toolFailureAnyErrorStopAt)) { + t.Errorf("the stop answer changed after draining siblings: %q", result.FinalAnswer) + } +} + +// A call that genuinely never ran still gets an aborted placeholder, so the fix +// did not simply stop aborting anything. +func TestUnstartedCallsStillGetAbortedPlaceholders(t *testing.T) { + tool := &readAheadTool{} + registry := tools.NewRegistry() + registry.Register(tool) + + var turns [][]zeroruntime.StreamEvent + for i := 0; i < toolFailureAnyErrorStopAt-1; i++ { + id := "s" + strconv.Itoa(i) + turns = append(turns, append(readCall(id), zeroruntime.StreamEvent{Type: zeroruntime.StreamEventDone})) + } + // One eligible pair, then a THIRD call outside the batch window is not what + // happens here: the whole run is eligible, so instead advertise a single call + // and let the halt fire on it, leaving nothing precomputed beyond it. + turns = append(turns, append(readCall("solo"), zeroruntime.StreamEvent{Type: zeroruntime.StreamEventDone})) + turns = append(turns, append(readCall("unreached"), zeroruntime.StreamEvent{Type: zeroruntime.StreamEventDone})) + + result, err := Run(context.Background(), "read things", &mockProvider{turns: turns}, Options{ + Registry: registry, + PermissionMode: PermissionModeAuto, + MaxTurns: len(turns) + 5, + }) + if err != nil { + t.Fatal(err) + } + if !strings.HasPrefix(result.FinalAnswer, "Agent stopped:") { + t.Fatalf("the guard did not halt: %q", result.FinalAnswer) + } + if tool.ran != toolFailureAnyErrorStopAt { + t.Errorf("the tool ran %d times, want %d; nothing should execute past the halt", + tool.ran, toolFailureAnyErrorStopAt) + } +} diff --git a/internal/agent/parallel_tools.go b/internal/agent/parallel_tools.go index 11d6e28a3..ea1c77cd0 100644 --- a/internal/agent/parallel_tools.go +++ b/internal/agent/parallel_tools.go @@ -2,6 +2,7 @@ package agent import ( "context" + "strings" "sync" "github.com/Gitlawb/zero/internal/tools" @@ -36,6 +37,15 @@ const maxParallelReadTools = 8 type precomputedToolResult struct { result ToolResult abortErr error + // completed records whether this call PRODUCED a result, which is a + // different question from whether it asks the enclosing run to stop. + // + // A cancelled permission request is both: executeToolCall builds the + // cancellation result, with its call ID, its message and its denial category, + // and returns it together with ErrPermissionApprovalCanceled. Reading the + // error as "never started" throws that result away, and the transcript then + // denies that permission handling happened at all. + completed bool } // parallelSafeToolCall reports whether call may run concurrently with its @@ -189,7 +199,15 @@ func executeParallelReadBatch(ctx context.Context, registry *tools.Registry, cal semaphore <- struct{}{} defer func() { <-semaphore }() result, abortErr := executeToolCall(ctx, registry, calls[index], permissionMode, batchOptions) - results[index-start] = precomputedToolResult{result: result, abortErr: abortErr} + // Recorded HERE, where both halves are in hand, rather than inferred later + // from the error. A populated call ID is what every path that produced a + // result sets and what a context cancellation before execution leaves + // empty. + results[index-start] = precomputedToolResult{ + result: result, + abortErr: abortErr, + completed: strings.TrimSpace(result.ToolCallID) != "", + } }(index) } waitGroup.Wait() diff --git a/internal/agent/policy_refusal_provenance_run_test.go b/internal/agent/policy_refusal_provenance_run_test.go new file mode 100644 index 000000000..60fb597cc --- /dev/null +++ b/internal/agent/policy_refusal_provenance_run_test.go @@ -0,0 +1,111 @@ +package agent + +import ( + "context" + "strconv" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/zeroruntime" +) + +// forgedMarkerTool executes, fails, and claims in its metadata that the registry +// refused it before it ran. +type forgedMarkerTool struct { + ran int + value string +} + +func (tool *forgedMarkerTool) Name() string { return "bash" } +func (tool *forgedMarkerTool) Description() string { return "test shell tool" } +func (tool *forgedMarkerTool) Parameters() tools.Schema { + return tools.Schema{ + Type: "object", + Properties: map[string]tools.PropertySchema{"command": {Type: "string"}}, + Required: []string{"command"}, + AdditionalProperties: false, + } +} +func (tool *forgedMarkerTool) Safety() tools.Safety { + return tools.Safety{SideEffect: tools.SideEffectRead, Permission: tools.PermissionAllow, Reason: "reads files"} +} +func (tool *forgedMarkerTool) Run(context.Context, map[string]any) tools.Result { + tool.ran++ + return tools.Result{ + Status: tools.StatusError, + Output: "Error: the script exited 1", + Meta: map[string]string{tools.PolicyRefusalMeta: tool.value}, + } +} + +// THE FORGED MARKER MUST NOT CHANGE THE RUN. +// +// Classifying from metadata instead of from output text is only an improvement +// while the metadata is something an executed result cannot produce. The +// registry strips the key at the execution boundary; this is the same claim +// stated where it is spent, because the unit test proves the boundary and this +// proves the loop behaves. +// +// An executed failure that is misread as a refusal loses the schema hint, stops +// the profile failure streak from recovering, and is counted toward the refusal +// halt, so the run ends early and the final answer can say a tool was refused +// when it ran every time it was asked. +func TestRunTreatsAForgedRefusalMarkerAsAnExecutedFailure(t *testing.T) { + for _, value := range []string{ + tools.PolicyRefusalSandboxDenied, + tools.PolicyRefusalPermissionDenied, + tools.PolicyRefusalToolNotEnabled, + "something-the-registry-never-emits", + } { + t.Run(value, func(t *testing.T) { + tool := &forgedMarkerTool{value: value} + registry := tools.NewRegistry() + registry.Register(tool) + + // One more call than the hint threshold, so the hint has to have been + // injected by the last one, then a final text turn to end the run. + calls := toolFailureHintAt + 1 + turns := make([][]zeroruntime.StreamEvent, 0, calls+1) + for i := range calls { + turns = append(turns, toolTurn("call-"+strconv.Itoa(i), "bash", `{"command":"./flaky.sh"}`)) + } + turns = append(turns, textTurn("gave up on the script")) + + var denials []DenialCategory + result, err := Run(context.Background(), "run the script", &mockProvider{turns: turns}, Options{ + Registry: registry, + PermissionMode: PermissionModeAsk, + MaxTurns: len(turns) + 5, + OnToolResult: func(toolResult ToolResult) { + denials = append(denials, toolResult.DenialReason) + }, + OnPermissionRequest: func(context.Context, PermissionRequest) (PermissionDecision, error) { + t.Error("permission was requested for an allow-safety tool; this test must exercise the executed-failure path") + return PermissionDecision{Action: PermissionDecisionDeny}, nil + }, + }) + if err != nil { + t.Fatal(err) + } + if tool.ran != calls { + t.Fatalf("the tool ran %d times, want %d; the run was halted by a marker it set on its own result", tool.ran, calls) + } + for index, denial := range denials { + if denial != DenialNone { + t.Errorf("result %d was recorded as denial %q; the tool executed", index, denial) + } + } + var hinted bool + for _, message := range result.Messages { + if strings.Contains(message.Content, toolFailureHintMarker) { + hinted = true + break + } + } + if !hinted { + t.Errorf("no retry hint was injected for an executed failure carrying %q, so the model got no correction", value) + } + }) + } +} diff --git a/internal/agent/policy_refusal_run_path_test.go b/internal/agent/policy_refusal_run_path_test.go new file mode 100644 index 000000000..da48f672a --- /dev/null +++ b/internal/agent/policy_refusal_run_path_test.go @@ -0,0 +1,196 @@ +package agent + +import ( + "context" + "strconv" + "testing" + + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/trace" + "github.com/Gitlawb/zero/internal/zeroruntime" +) + +// THE UNCATEGORIZED REFUSALS ARE THE ONES THIS CHANGE EXISTS FOR, SO THEY ARE THE +// ONES THAT MUST BE DRIVEN THROUGH Run. +// +// A categorized denial is easy to test and was never the gap. The gap is a +// refusal that arrives with DenialReason empty, because a category is attached +// only where a TYPED denial is built: +// +// - a headless run leaves OnPermissionRequest nil, so the loop never reaches +// its typed-denial branch and the registry gate returns a bare +// `Permission required for ...` +// - a sandbox preflight denial on a non-shell tool loses its SandboxDecision +// converting to ToolResult and arrives as a bare `Sandbox block` +// +// Both were counted as SUCCESS, which cleared the very record they were meant to +// accumulate, and the refused call repeated to MaxTurns. +// +// Asserting that through the helper alone would prove 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. So each case +// below goes through Run and pins the three things the loop actually does with +// the classification: halt at the bound, never execute the tool, and withhold +// the profile's one-shot failure escalation. + +// headlessPromptTool needs approval and never gets it, because the run has no +// approver. The registry refuses it before Run() is reached, exactly as a +// headless run does, so nothing here fakes the refusal. +type headlessPromptTool struct{ ran int } + +func (tool *headlessPromptTool) Name() string { return "bash" } +func (tool *headlessPromptTool) Description() string { return "test shell tool" } +func (tool *headlessPromptTool) Parameters() tools.Schema { + return tools.Schema{ + Type: "object", + Properties: map[string]tools.PropertySchema{"command": {Type: "string"}}, + Required: []string{"command"}, + AdditionalProperties: false, + } +} +func (tool *headlessPromptTool) Safety() tools.Safety { + return tools.Safety{SideEffect: tools.SideEffectShell, Permission: tools.PermissionPrompt, Reason: "runs shell commands"} +} +func (tool *headlessPromptTool) Run(context.Context, map[string]any) tools.Result { + tool.ran++ + return tools.Result{Status: tools.StatusOK, Output: "should never run"} +} + +// A headless prompt refusal must halt at the same-signature bound rather than +// repeat to MaxTurns. +// +// It used to key on the refusal TEXT, which the registry happens to hold +// constant here, and the stop then described a repeated failure. The registry +// marks this path, so the category is now derived at the ToolResult boundary and +// the streak keys on that instead. The bound is unchanged; the wording is more +// honest, because a headless prompt refusal is a refusal and not a tool that +// keeps failing. +func TestRunStopsAnUncategorizedHeadlessRefusalAtTheFailureBound(t *testing.T) { + tool := &headlessPromptTool{} + 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} + + recorder := trace.NewRecorder("policy-refusal-session", "run-1", "fast") + result, err := Run(context.Background(), "do the thing", provider, Options{ + Registry: registry, + PermissionMode: PermissionModeAsk, + MaxTurns: len(turns) + 5, + Trace: recorder, + // Armed well below the halt bound, so a refusal counted as a retriable + // failure would visibly spend it. + Profile: &ProfilePolicy{ + Name: "fast", + Escalate: &PostureEscalation{MaxTurns: 999, OnToolFailureStreak: 2}, + }, + // OnPermissionRequest deliberately nil: this IS the headless path. + }) + if err != nil { + t.Fatal(err) + } + + if len(provider.requests) != toolFailureStopAt { + t.Errorf("the run made %d turns, want it halted at %d; an uncategorized refusal is not being counted", len(provider.requests), toolFailureStopAt) + } + if tool.ran != 0 { + t.Errorf("the refused tool executed %d times; the registry gate must precede execution", tool.ran) + } + // Categorized now, so the stop answer says refused. Previously the loop could + // not tell and reported a repeated failure. + want := toolFailureStopAnswer("bash", toolFailureStopAt, toolFailureCauseSameRefusal) + if result.FinalAnswer != want { + t.Errorf("final answer =\n %q\nwant\n %q", result.FinalAnswer, want) + } + assertNoPostureEscalation(t, recorder) +} + +// uncategorizedSandboxTool returns the shape a sandbox preflight denial has by +// the time the loop sees it on a non-shell tool: StatusError, `Sandbox block` +// prose, and no category, because the SandboxDecision does not survive the +// conversion to ToolResult. The path varies per call exactly as the real message +// does, since it names what was refused. +type uncategorizedSandboxTool struct{ ran int } + +func (tool *uncategorizedSandboxTool) Name() string { return "write_file" } +func (tool *uncategorizedSandboxTool) Description() string { return "test write tool" } +func (tool *uncategorizedSandboxTool) Parameters() tools.Schema { + return tools.Schema{ + Type: "object", + Properties: map[string]tools.PropertySchema{"path": {Type: "string"}}, + Required: []string{"path"}, + AdditionalProperties: false, + } +} +func (tool *uncategorizedSandboxTool) Safety() tools.Safety { + return tools.Safety{SideEffect: tools.SideEffectWrite, Permission: tools.PermissionAllow, Reason: "writes files"} +} +func (tool *uncategorizedSandboxTool) Run(context.Context, map[string]any) tools.Result { + tool.ran++ + return tools.Result{ + Status: tools.StatusError, + Output: "Sandbox block: write to /out-" + strconv.Itoa(tool.ran) + ".txt is outside the workspace", + } +} + +// Varying text plus no category means every call restarts the same-signature +// streak at 1, so the same-signature bound is never reached and only the +// content-blind counter can halt the run. Before the fix nothing halted it at +// all: each refusal was read as a success, deleted the record, and the call +// repeated until MaxTurns. +func TestRunStopsAnUncategorizedVaryingSandboxRefusalAtTheVariedBound(t *testing.T) { + tool := &uncategorizedSandboxTool{} + registry := tools.NewRegistry() + registry.Register(tool) + + turns := make([][]zeroruntime.StreamEvent, 0, toolFailureAnyErrorStopAt+4) + for i := range toolFailureAnyErrorStopAt + 4 { + turns = append(turns, toolTurn("call-"+strconv.Itoa(i), "write_file", + `{"path":"/out-`+strconv.Itoa(i)+`.txt"}`)) + } + provider := &mockProvider{turns: turns} + + recorder := trace.NewRecorder("policy-refusal-session", "run-2", "fast") + result, err := Run(context.Background(), "write the files", provider, Options{ + Registry: registry, + PermissionMode: PermissionModeAsk, + MaxTurns: len(turns) + 5, + Trace: recorder, + Profile: &ProfilePolicy{ + Name: "fast", + Escalate: &PostureEscalation{MaxTurns: 999, OnToolFailureStreak: 2}, + }, + }) + if err != nil { + t.Fatal(err) + } + + if tool.ran != toolFailureAnyErrorStopAt { + t.Errorf("the tool was called %d times, want the run halted at %d", tool.ran, toolFailureAnyErrorStopAt) + } + want := toolFailureStopAnswer("write_file", toolFailureAnyErrorStopAt, toolFailureCauseVariedExecuted) + if result.FinalAnswer != want { + t.Errorf("final answer =\n %q\nwant\n %q", result.FinalAnswer, want) + } + assertNoPostureEscalation(t, recorder) +} + +// The profile's failure-streak trigger restores turn budget and reasoning effort +// on the theory that a struggling tool needs room. A refusal is not a struggling +// tool, it is an answer, and spending the one-shot escalation on one buys turns +// that will be refused identically. The loop keeps this contract by passing an +// empty outcome to the controller for a refusal, which is only observable from +// outside as the escalation never firing. +func assertNoPostureEscalation(t *testing.T, recorder *trace.Recorder) { + t.Helper() + if got := recorder.Finish().Counter(trace.CounterPostureEscalations); got != 0 { + t.Errorf("posture_escalations = %d, want 0; a policy refusal spent the one-shot failure escalation", got) + } +} diff --git a/internal/agent/policy_refusal_status_test.go b/internal/agent/policy_refusal_status_test.go new file mode 100644 index 000000000..7dc26c5d4 --- /dev/null +++ b/internal/agent/policy_refusal_status_test.go @@ -0,0 +1,265 @@ +package agent + +import ( + "context" + "strconv" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/zeroruntime" +) + +// A REFUSAL IS A FAILURE, AND THE CLASSIFIER MUST SAY SO BEFORE IT READS ANYTHING +// ELSE. +// +// 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: +// a `bash` that greps the sandbox docs, a read_file returning this very source +// file, any successful command whose output happens to carry one of the phrases. +// +// 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, and every successful result was put to a +// question that only failures can answer. +// +// Each case below is a SUCCESS. None may be read as a refusal. +func TestPolicyRefusalNeverClassifiesASuccessfulResult(t *testing.T) { + cases := []struct { + name string + result ToolResult + // structured marks a result carrying real provenance: a denial category, + // permission metadata, or the registry's pre-execution marker. Only those + // may be read as a refusal when they fail. The rest carry nothing but + // text, which the tool controls, so they must stay ordinary failures no + // matter which phrase they happen to contain. + structured bool + }{ + { + name: "output quotes the sandbox block phrase", + result: ToolResult{Status: tools.StatusOK, Output: "grep: docs/sandbox.md: Sandbox block"}, + }, + { + name: "output quotes the disabled-tool phrase", + result: ToolResult{Status: tools.StatusOK, Output: "web_fetch is not enabled for this run"}, + }, + { + name: "output quotes a permission denial", + result: ToolResult{Status: tools.StatusOK, Output: "log line: Permission denied for bash"}, + }, + { + name: "output quotes a permission requirement", + result: ToolResult{Status: tools.StatusOK, Output: "log line: Permission required for bash"}, + }, + { + name: "output quotes a sandbox approval prompt", + result: ToolResult{Status: tools.StatusOK, Output: "Sandbox approval required for curl"}, + }, + { + // Metadata is no more trustworthy than text on a completed call. A + // result carrying a category with an OK status is not a denial; every + // path that attaches one fails the call. + name: "stale denial category on a completed call", + result: ToolResult{Status: tools.StatusOK, DenialReason: DenialSandboxBlock, Output: "ok"}, + structured: true, + }, + { + name: "stale permission metadata on a completed call", + result: ToolResult{ + Status: tools.StatusOK, + Meta: map[string]string{"permission_action": string(PermissionActionDeny)}, + Output: "ok", + }, + structured: true, + }, + { + name: "registry refusal marker on a completed call", + result: ToolResult{ + Status: tools.StatusOK, + Meta: map[string]string{tools.PolicyRefusalMeta: tools.PolicyRefusalSandboxDenied}, + Output: "ok", + }, + structured: true, + }, + } + + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + if isPolicyRefusal(testCase.result) { + t.Errorf("a successful result was classified as a policy refusal, so a healthy call counts toward the denial streak: %+v", testCase.result) + } + failed := testCase.result + failed.Status = tools.StatusError + switch { + case testCase.structured && !isPolicyRefusal(failed): + t.Errorf("a failure carrying real provenance was not classified as a refusal, so the gate broke the classifier: %+v", failed) + case !testCase.structured && isPolicyRefusal(failed): + t.Errorf("a failure carrying only text was classified as a refusal; output is tool-controlled and cannot decide this: %+v", failed) + } + }) + } +} + +// alwaysSucceedingTool runs freely and prints text the classifier keys on. This +// is not contrived: `Sandbox block` appears in this repo's own docs and error +// strings, so any grep, cat, or file read that crosses them produces exactly +// this result. +type alwaysSucceedingTool struct{ ran int } + +func (tool *alwaysSucceedingTool) Name() string { return "bash" } +func (tool *alwaysSucceedingTool) Description() string { return "test shell tool" } +func (tool *alwaysSucceedingTool) Parameters() tools.Schema { + return tools.Schema{ + Type: "object", + Properties: map[string]tools.PropertySchema{"command": {Type: "string"}}, + Required: []string{"command"}, + AdditionalProperties: false, + } +} +func (tool *alwaysSucceedingTool) Safety() tools.Safety { + return tools.Safety{SideEffect: tools.SideEffectRead, Permission: tools.PermissionAllow, Reason: "reads files"} +} +func (tool *alwaysSucceedingTool) Run(context.Context, map[string]any) tools.Result { + tool.ran++ + return tools.Result{ + Status: tools.StatusOK, + Output: "docs/windows-sandbox.md:41: Sandbox block (write jail) is reported as ...", + } +} + +// The same gap driven through Run, because the classifier being wrong only +// matters if the loop acts on it, and it does: the counting path calls +// isPolicyRefusal directly, and a true answer records a failure against the +// tool's signature. Six identical successes then look like six identical +// denials, and the run ends on a stop answer telling the user a tool it never +// refused was refused six times. +// +// The tool succeeds every turn and the run is given more turns than the bound, +// so anything that halts before the provider's final turn is the guard. +func TestRunDoesNotHaltOnSuccessfulOutputThatQuotesARefusal(t *testing.T) { + tool := &alwaysSucceedingTool{} + registry := tools.NewRegistry() + registry.Register(tool) + + calls := toolFailureStopAt + 4 + turns := make([][]zeroruntime.StreamEvent, 0, calls+1) + for i := range calls { + turns = append(turns, toolTurn("call-"+strconv.Itoa(i), "bash", + `{"command":"grep -n 'Sandbox block' docs/windows-sandbox.md"}`)) + } + turns = append(turns, textTurn("found the docs")) + provider := &mockProvider{turns: turns} + + result, err := Run(context.Background(), "search the docs", provider, Options{ + Registry: registry, + PermissionMode: PermissionModeAsk, + MaxTurns: len(turns) + 5, + OnPermissionRequest: func(context.Context, PermissionRequest) (PermissionDecision, error) { + t.Error("the loop asked permission for an allow-safety tool; this test must exercise the success path") + return PermissionDecision{Action: PermissionDecisionDeny}, nil + }, + }) + if err != nil { + t.Fatal(err) + } + + if tool.ran != calls { + t.Errorf("the tool ran %d times, want %d; the run was halted mid-way by its own success output", tool.ran, calls) + } + if result.FinalAnswer != "found the docs" { + t.Errorf("final answer =\n %q\nwant\n %q\n(a stop answer here means successful output was counted as a refusal)", + result.FinalAnswer, "found the docs") + } +} + +// failingPhraseTool RAN and failed. Its stderr happens to contain a phrase the +// classifier used to key on, which is the everyday case: `bash` preserves +// arbitrary stdout and stderr on any nonzero exit, so +// `printf 'Sandbox block\n' >&2; exit 1` produces exactly this. +type failingPhraseTool struct { + ran int + output string +} + +func (tool *failingPhraseTool) Name() string { return "bash" } +func (tool *failingPhraseTool) Description() string { return "test shell tool" } +func (tool *failingPhraseTool) Parameters() tools.Schema { + return tools.Schema{ + Type: "object", + Properties: map[string]tools.PropertySchema{"command": {Type: "string"}}, + Required: []string{"command"}, + AdditionalProperties: false, + } +} +func (tool *failingPhraseTool) Safety() tools.Safety { + return tools.Safety{SideEffect: tools.SideEffectRead, Permission: tools.PermissionAllow, Reason: "reads files"} +} +func (tool *failingPhraseTool) Run(context.Context, map[string]any) tools.Result { + tool.ran++ + return tools.Result{Status: tools.StatusError, Output: tool.output} +} + +// AN EXECUTED FAILURE IS RETRIABLE, WHATEVER IT PRINTED, and the observable +// difference is the schema hint. +// +// isRetriableToolError feeds `hintable` into the guard, and the guard injects +// the retry hint at toolFailureHintAt. Classifying an executed failure as a +// policy refusal makes hintable false, so the model is told nothing and simply +// repeats the call: the run loses the one correction it had. That is on top of +// the failure being accumulated as a denial. +// +// Each phrase below is one the classifier used to match on. The tool is +// allow-safety and runs every time, so nothing here is refused by anything. +func TestRunHintsAnExecutedFailureThatPrintsARefusalPhrase(t *testing.T) { + for _, output := range []string{ + "Sandbox block\n", + "cp: Permission denied for /etc/hosts", + "Permission required for sudo", + "Sandbox approval required for curl", + "grep: web_fetch is not enabled for this run", + } { + t.Run(output, func(t *testing.T) { + tool := &failingPhraseTool{output: output} + registry := tools.NewRegistry() + registry.Register(tool) + + // One more call than the hint threshold, so the hint has to have been + // injected by the last one, then a final text turn to end the run. + calls := toolFailureHintAt + 1 + turns := make([][]zeroruntime.StreamEvent, 0, calls+1) + for i := range calls { + turns = append(turns, toolTurn("call-"+strconv.Itoa(i), "bash", `{"command":"./flaky.sh"}`)) + } + turns = append(turns, textTurn("gave up on the script")) + provider := &mockProvider{turns: turns} + + result, err := Run(context.Background(), "run the script", provider, Options{ + Registry: registry, + PermissionMode: PermissionModeAsk, + MaxTurns: len(turns) + 5, + OnPermissionRequest: func(context.Context, PermissionRequest) (PermissionDecision, error) { + t.Error("permission was requested for an allow-safety tool; this test must exercise the executed-failure path") + return PermissionDecision{Action: PermissionDecisionDeny}, nil + }, + }) + if err != nil { + t.Fatal(err) + } + if tool.ran != calls { + t.Fatalf("the tool ran %d times, want %d; the run was halted by output it merely printed", tool.ran, calls) + } + + var hinted bool + for _, message := range result.Messages { + if strings.Contains(message.Content, toolFailureHintMarker) { + hinted = true + break + } + } + if !hinted { + t.Errorf("no retry hint was injected for an executed failure printing %q; it was classified as a policy refusal, so the model got no correction", output) + } + }) + } +} diff --git a/internal/agent/policy_refusal_test.go b/internal/agent/policy_refusal_test.go new file mode 100644 index 000000000..34e94eb10 --- /dev/null +++ b/internal/agent/policy_refusal_test.go @@ -0,0 +1,84 @@ +package agent + +import ( + "testing" + + "github.com/Gitlawb/zero/internal/tools" +) + +// UNCATEGORIZED REFUSALS MUST STILL COUNT. +// +// The halt guard 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 in the +// conversion and arrives as an uncategorized "Sandbox block". +// +// Both were counted as SUCCESS, which cleared the very record the guard +// accumulates, so the same refused call could repeat to MaxTurns. That is the +// loop this PR exists to stop. +func TestUncategorizedPolicyRefusalsAreCounted(t *testing.T) { + for _, testCase := range []struct { + name string + result ToolResult + }{ + { + // Still uncategorized in the DenialReason sense: a headless run never + // reaches the branch that builds a typed denial. It carries the + // registry's pre-execution marker instead, which is what makes it + // classifiable without reading the message. + name: "headless prompt refusal carries no denial category", + result: ToolResult{ + Status: tools.StatusError, + Output: `Error: Permission required for bash: The tool is marked "prompt" and was not executed.`, + Meta: map[string]string{tools.PolicyRefusalMeta: tools.PolicyRefusalPermissionRequired}, + }, + }, + { + // The SandboxDecision is still dropped in the conversion for a + // non-shell tool; the marker is what survives it. + name: "sandbox preflight denial loses its decision in conversion", + result: ToolResult{ + Status: tools.StatusError, + Output: "Sandbox block: write outside the workspace", + Meta: map[string]string{tools.PolicyRefusalMeta: tools.PolicyRefusalSandboxDenied}, + }, + }, + { + name: "an explicitly denied permission action", + result: ToolResult{Status: tools.StatusError, Output: "refused", Meta: map[string]string{"permission_action": string(PermissionActionDeny)}}, + }, + { + name: "a categorized denial still counts", + result: ToolResult{Status: tools.StatusError, Output: "denied", DenialReason: DenialFiltered}, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + if !isPolicyRefusal(testCase.result) { + t.Fatal("refusal was treated as a success, so the guard's record is cleared and the call can repeat to MaxTurns") + } + if isRetriableToolError(testCase.result) { + t.Error("a policy refusal is not retriable: repeating it verbatim cannot help") + } + }) + } +} + +// An ordinary failure is still retriable and still not a refusal, or the guard +// would halt on things the model can legitimately fix by trying again. +func TestOrdinaryFailuresAreNotPolicyRefusals(t *testing.T) { + for _, output := range []string{ + "Error: file not found", + "Error: invalid JSON in arguments", + "Error: connection reset", + } { + result := ToolResult{Status: tools.StatusError, Output: output} + if isPolicyRefusal(result) { + t.Errorf("%q was classified as a policy refusal", output) + } + if !isRetriableToolError(result) { + t.Errorf("%q should stay retriable", output) + } + } +} diff --git a/internal/agent/precomputed_completion_test.go b/internal/agent/precomputed_completion_test.go new file mode 100644 index 000000000..bc1f0a583 --- /dev/null +++ b/internal/agent/precomputed_completion_test.go @@ -0,0 +1,87 @@ +package agent + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/Gitlawb/zero/internal/tools" +) + +// PRODUCING A RESULT AND ASKING THE RUN TO STOP ARE DIFFERENT FACTS. +// +// closeOutRemaining decides between finalizing a completed sibling and writing +// an aborted placeholder, and it used to make that decision from abortErr. A +// cancelled permission request is exactly where the two facts disagree: +// executeToolCall builds the cancellation result, with its call ID, its message +// and its denial category, and returns it TOGETHER with +// ErrPermissionApprovalCanceled. Reading the error as "never started" discards a +// real result and records the opposite of what happened, and the cancellation +// then goes missing from OnToolResult, the trace counters and the task +// observation even though the permission event already fired. +func TestABatchEntryWithAResultAndAnAbortErrorIsDrained(t *testing.T) { + canceled := canceledPermissionResult( + ToolCall{ID: "call-b", Name: "read_probe"}, + "cancelled in TUI", + PermissionEvent{ToolName: "read_probe"}, + ) + precomputed := []precomputedToolResult{ + {result: ToolResult{ToolCallID: "call-a", Name: "read_probe", Status: tools.StatusOK}, completed: true}, + { + result: canceled, + abortErr: fmt.Errorf("%w for read_probe", ErrPermissionApprovalCanceled), + completed: true, + }, + } + + sibling, ran := precomputedResultFor(precomputed, 0, 2, 1) + if !ran { + t.Fatal("a cancelled permission result was treated as a call that never started") + } + if sibling.ToolCallID != "call-b" { + t.Errorf("ToolCallID = %q, want the cancellation result rather than an empty placeholder", sibling.ToolCallID) + } + if sibling.DenialReason != DenialApprovalCanceled { + t.Errorf("DenialReason = %q, want the cancellation preserved", sibling.DenialReason) + } +} + +// An entry that never produced a result is still unstarted, or the fix would +// have stopped aborting anything. +func TestABatchEntryWithNoResultIsStillUnstarted(t *testing.T) { + precomputed := []precomputedToolResult{ + {abortErr: errors.New("cancelled before the tool ran")}, + } + if _, ran := precomputedResultFor(precomputed, 0, 1, 0); ran { + t.Error("an entry that produced nothing was finalized as if it had run") + } +} + +// A call outside the batch window never started either. +func TestACallOutsideTheBatchWindowIsUnstarted(t *testing.T) { + precomputed := []precomputedToolResult{{result: ToolResult{ToolCallID: "call-a"}, completed: true}} + if _, ran := precomputedResultFor(precomputed, 0, 1, 1); ran { + t.Error("a call past the batch window was reported as executed") + } +} + +// And the flag is recorded where the truth is known rather than inferred later, +// so a real batch marks what it ran. +func TestTheBatchRecordsWhatItRan(t *testing.T) { + registry := tools.NewRegistry() + registry.Register(&readAheadTool{}) + calls := []ToolCall{ + {ID: "call-a", Name: "read_probe", Arguments: `{"id":"a"}`}, + {ID: "call-b", Name: "read_probe", Arguments: `{"id":"b"}`}, + } + results := executeParallelReadBatch(context.Background(), registry, calls, 0, 2, PermissionModeAuto, Options{Registry: registry}) + for index, entry := range results { + if !entry.completed { + t.Errorf("entry %d executed but was not recorded as completed: %#v", index, entry) + } + if entry.result.ToolCallID != calls[index].ID { + t.Errorf("entry %d is keyed to %q, want %q", index, entry.result.ToolCallID, calls[index].ID) + } + } +} diff --git a/internal/agent/refusal_provenance_test.go b/internal/agent/refusal_provenance_test.go new file mode 100644 index 000000000..67b303f7b --- /dev/null +++ b/internal/agent/refusal_provenance_test.go @@ -0,0 +1,176 @@ +package agent + +import ( + "context" + "strconv" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/zeroruntime" +) + +// legacyDenialSignaturePrefix is the string provenance used to be encoded in. +// The identity was one namespace holding both a normalized error signature and +// a synthetic "denial:" key, and provenance was recovered afterwards +// with strings.HasPrefix. It survives here, and only here, so the regression +// names the exact spelling that used to collide. +const legacyDenialSignaturePrefix = "denial:" + +func runUntilGuardHalt(t *testing.T, gate bool) Result { + t.Helper() + tool := &uncategorizedSandboxTool{} + registry := tools.NewRegistry() + registry.Register(tool) + turns := make([][]zeroruntime.StreamEvent, 0, toolFailureAnyErrorStopAt+4) + for i := range toolFailureAnyErrorStopAt + 4 { + turns = append(turns, toolTurn("c"+strconv.Itoa(i), "write_file", `{"path":"/o-`+strconv.Itoa(i)+`.txt"}`)) + } + result, err := Run(context.Background(), "write", &mockProvider{turns: turns}, Options{ + Registry: registry, + PermissionMode: PermissionModeAsk, + MaxTurns: len(turns) + 5, + RequireCompletionSignal: gate, + }) + if err != nil { + t.Fatal(err) + } + if !strings.HasPrefix(result.FinalAnswer, "Agent stopped:") { + t.Fatalf("the guard did not halt this run: %q", result.FinalAnswer) + } + return result +} + +// A GUARD HALT IS UNFINISHED WORK, AND HEADLESS HAS TO HEAR ABOUT IT. +// +// The halt returns straight out of the tool loop, so it never reached the +// completion gate the max-turns paths go through. zero exec treats only +// Result.Incomplete as exit 4 and otherwise reports run_end success with exit +// 0, so a task denied six times, or failed through the twelve-call bound, came +// back as a successful automation result having done none of the work asked. +func TestGuardHaltIsIncompleteUnderTheCompletionGate(t *testing.T) { + headless := runUntilGuardHalt(t, true) + if !headless.Incomplete { + t.Error("a guard halt reports the run as complete; zero exec would call a run that did nothing a success") + } + if !strings.Contains(headless.IncompleteReason, "write_file") { + t.Errorf("the incomplete reason does not name the tool that halted the run: %q", headless.IncompleteReason) + } + // And the interactive default is untouched: Incomplete exists for the + // headless gate, so setting it without one would be inventing a status. + if interactive := runUntilGuardHalt(t, false); interactive.Incomplete { + t.Error("Incomplete was set without RequireCompletionSignal, changing interactive behaviour") + } +} + +// executedDenialLookalikeTool runs, fails, and prints exactly the string the +// guard used to store for a real permission refusal. +type executedDenialLookalikeTool struct{ ran int } + +func (t *executedDenialLookalikeTool) Name() string { return "bash" } +func (t *executedDenialLookalikeTool) Description() string { return "probe" } +func (t *executedDenialLookalikeTool) Parameters() tools.Schema { return tools.Schema{Type: "object"} } +func (t *executedDenialLookalikeTool) Safety() tools.Safety { + return tools.Safety{SideEffect: tools.SideEffectShell, Permission: tools.PermissionAllow} +} + +func (t *executedDenialLookalikeTool) Run(context.Context, map[string]any) tools.Result { + t.ran++ + return tools.Result{Status: tools.StatusError, Output: legacyDenialSignaturePrefix + string(DenialPermissionDenied)} +} + +// TRUSTED PROVENANCE AND UNTRUSTED CONTENT MUST NOT SHARE A NAMESPACE. +// +// While the identity was a single string, a command that printed exactly +// "denial:permission_denied" and exited non-zero took on the identity of a real +// permission refusal: it merged streaks with one, and the run told the user the +// tool had been refused when it had executed every time. +func TestAnExecutedErrorCannotImpersonateARefusal(t *testing.T) { + tool := &executedDenialLookalikeTool{} + registry := tools.NewRegistry() + registry.Register(tool) + turns := make([][]zeroruntime.StreamEvent, 0, toolFailureStopAt+4) + for i := range toolFailureStopAt + 4 { + turns = append(turns, toolTurn("c"+strconv.Itoa(i), "bash", `{}`)) + } + result, err := Run(context.Background(), "run it", &mockProvider{turns: turns}, Options{ + Registry: registry, PermissionMode: PermissionModeAsk, MaxTurns: len(turns) + 5, + }) + if err != nil { + t.Fatal(err) + } + if tool.ran == 0 { + t.Fatal("the tool never ran, so this no longer covers an executed failure") + } + if strings.Contains(result.FinalAnswer, "was refused") { + t.Errorf("the tool executed %d times and nothing refused it, but the run says it was refused: %q", tool.ran, result.FinalAnswer) + } + if !strings.Contains(result.FinalAnswer, "with the same error") { + t.Errorf("an executed failure repeating identically should read as the same error: %q", result.FinalAnswer) + } +} + +// AND THE AGGREGATE IS ITS OWN FACT. +// +// The content-blind bound fires precisely when no single identity repeated +// enough, so the identity present at the end says nothing about the eleven +// before it. Alternating two refusal categories reaches twelve without either +// reaching six; the tool never ran once, and the answer used to describe that +// as the tool failing with varying errors. +func TestTheContentBlindBoundKeepsRefusalProvenance(t *testing.T) { + for _, testCase := range []struct { + name string + kinds []DenialCategory + wantCause toolFailureCause + wantAnswer string + }{ + { + name: "alternating refusal categories", + kinds: []DenialCategory{DenialPermissionDenied, DenialSandboxBlock}, + wantCause: toolFailureCauseVariedRefused, + wantAnswer: "was refused", + }, + { + // Mixed is DECIDED, not inherited from whichever field a switch + // tested first. Neither "failed" alone nor "refused" alone is true of + // this sequence, so the wording says both. + name: "executed failures mixed with refusals", + kinds: []DenialCategory{DenialNone, DenialPermissionDenied}, + wantCause: toolFailureCauseVariedMixed, + wantAnswer: "failed or was refused", + }, + { + name: "executed failures only", + kinds: []DenialCategory{DenialNone, DenialNone}, + wantCause: toolFailureCauseVariedExecuted, + wantAnswer: "with varying errors", + }, + } { + t.Run(testCase.name, func(t *testing.T) { + state := newGuardState() + var outcome toolFailureOutcome + for index := 0; index < toolFailureAnyErrorStopAt; index++ { + // Distinct prose every call, exactly as a real refusal or error + // naming what it touched would produce. + outcome = state.observeToolResult("bash", true, false, + "failure on item "+strconv.Itoa(index), testCase.kinds[index%len(testCase.kinds)]) + if outcome.Stop { + break + } + } + if !outcome.Stop { + t.Fatal("twelve consecutive failures did not trip the content-blind bound") + } + if outcome.Count != toolFailureAnyErrorStopAt { + t.Errorf("Count = %d, want the counter that tripped (%d)", outcome.Count, toolFailureAnyErrorStopAt) + } + if outcome.Cause != testCase.wantCause { + t.Errorf("Cause = %v, want %v", outcome.Cause, testCase.wantCause) + } + answer := toolFailureStopAnswer("bash", outcome.Count, outcome.Cause) + if !strings.Contains(answer, testCase.wantAnswer) { + t.Errorf("stop answer = %q, want it to contain %q", answer, testCase.wantAnswer) + } + }) + } +} diff --git a/internal/agent/sandbox_refusal_boundary_test.go b/internal/agent/sandbox_refusal_boundary_test.go new file mode 100644 index 000000000..b164a6842 --- /dev/null +++ b/internal/agent/sandbox_refusal_boundary_test.go @@ -0,0 +1,103 @@ +package agent + +import ( + "context" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/sandbox" + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/zeroruntime" +) + +// deniedWriteTool records every execution. Its body must never run: the sandbox +// refuses the call at the registry boundary, before the tool is reached. +type deniedWriteTool struct{ ran int } + +func (t *deniedWriteTool) Name() string { return "write_file" } +func (t *deniedWriteTool) Description() string { return "test write tool" } +func (t *deniedWriteTool) Parameters() tools.Schema { + return tools.Schema{ + Type: "object", + Properties: map[string]tools.PropertySchema{"path": {Type: "string"}}, + Required: []string{"path"}, + } +} + +func (t *deniedWriteTool) Safety() tools.Safety { + return tools.Safety{SideEffect: tools.SideEffectWrite, Permission: tools.PermissionAllow, Reason: "writes files"} +} + +func (t *deniedWriteTool) Run(context.Context, map[string]any) tools.Result { + t.ran++ + return tools.Result{Status: tools.StatusOK, Output: "wrote"} +} + +// THROUGH THE REAL SANDBOX, NOT A FAKE STANDING WHERE IT WOULD BE. +// +// The neighbouring uncategorized test deliberately hand-builds the shape a +// refusal has when no category survives, which is a fact about the LOOP. This +// one covers the producer: a real sandbox.Engine evaluation denies the call, +// the registry turns that into a categorized refusal, and the guard keys on the +// category. Without it, reverting refusalResult(..., PolicyRefusalSandboxDenied) +// in registry.go to a plain errorResult left both the agent and tools suites +// green, so nothing depended on the marker at all. +// +// Every assertion here fails if a different link breaks: ran proves the body was +// skipped, the request count proves the CATEGORY bound halted it rather than the +// content-blind one, and the wording proves the provenance survived conversion. +func TestRunHaltsARealSandboxDenialOnTheCategoryBound(t *testing.T) { + workspace := t.TempDir() + forbidden := t.TempDir() + policy := sandbox.DefaultPolicy() + // Applies whenever the sandbox is enforcing, so this does not depend on the + // workspace boundary being reachable from a synthetic request. + policy.DenyWrite = []string{forbidden} + + tool := &deniedWriteTool{} + registry := tools.NewRegistry() + registry.Register(tool) + + // A different path every call. Note that errorSignature truncates at 80 + // characters and the message opens with a fixed 40-character prefix followed + // by a long temp root, so the signatures collide here anyway: the request + // count below pins WHICH bound halted the run, but it does not by itself + // prove the streak was held by the category. The wording assertion is the one + // that discriminates, and it is the one that fails when the marker is gone. + turns := make([][]zeroruntime.StreamEvent, 0, toolFailureStopAt+4) + for i := range toolFailureStopAt + 4 { + target := filepath.Join(forbidden, "escape-"+strconv.Itoa(i)+".txt") + turns = append(turns, toolTurn("c"+strconv.Itoa(i), "write_file", `{"path":`+strconv.Quote(target)+`}`)) + } + provider := &mockProvider{turns: turns} + + result, err := Run(context.Background(), "write the files", provider, Options{ + Registry: registry, + PermissionMode: PermissionModeAsk, + MaxTurns: len(turns) + 5, + Cwd: workspace, + Sandbox: sandbox.NewEngine(sandbox.EngineOptions{ + WorkspaceRoot: workspace, + Policy: policy, + }), + }) + if err != nil { + t.Fatal(err) + } + + if tool.ran != 0 { + t.Errorf("the tool body ran %d times; the sandbox must refuse before execution", tool.ran) + } + if len(provider.requests) != toolFailureStopAt { + t.Errorf("the run made %d turns, want the category bound at %d; a categorized refusal whose prose varies must still halt on its category", + len(provider.requests), toolFailureStopAt) + } + if want := toolFailureStopAnswer("write_file", toolFailureStopAt, toolFailureCauseSameRefusal); result.FinalAnswer != want { + t.Errorf("final answer =\n %q\nwant\n %q", result.FinalAnswer, want) + } + if !strings.Contains(result.FinalAnswer, "was refused") { + t.Errorf("the refusal provenance did not survive to the answer: %q", result.FinalAnswer) + } +} diff --git a/internal/agent/stop_answer_wording_test.go b/internal/agent/stop_answer_wording_test.go new file mode 100644 index 000000000..3aaf10a42 --- /dev/null +++ b/internal/agent/stop_answer_wording_test.go @@ -0,0 +1,73 @@ +package agent + +import ( + "strings" + "testing" +) + +// THE ANSWER MUST CLAIM ONLY WHAT THE COUNTER ESTABLISHED. +// +// anyErrorCount establishes that the tool failed consecutively without a +// success. It does NOT establish that every failure differed: five A, five B and +// two C reach the content-blind bound of 12 without any signature repeating six +// times, and three of those errors were shared. The old wording called that +// "each with a different error". +// +// The signature bound has the inverse problem. A denial streak keys on the +// CATEGORY, and the prose behind it embeds the path or command refused, so it +// differs on every call. Calling that "the same error" is false in the other +// direction. +func TestStopAnswerDoesNotOverclaimTheFailurePattern(t *testing.T) { + varied := toolFailureStopAnswer("bash", 12, toolFailureCauseVariedExecuted) + if strings.Contains(varied, "each with a different error") { + t.Errorf("the content-blind bound does not track that every failure differed: %q", varied) + } + if !strings.Contains(varied, "varying errors") { + t.Errorf("the varied stop should still say the errors varied: %q", varied) + } + + refused := toolFailureStopAnswer("bash", 6, toolFailureCauseSameRefusal) + if strings.Contains(refused, "same error") { + t.Errorf("a denial streak covers refusals of different paths, so it is not the same error: %q", refused) + } + if !strings.Contains(refused, "refused") { + t.Errorf("a denial streak should say it was refused: %q", refused) + } + + // The one claim that IS justified: a signature streak really did repeat the + // same error signature, so that wording stays. + same := toolFailureStopAnswer("bash", 6, toolFailureCauseSameError) + if !strings.Contains(same, "same error") { + t.Errorf("a signature streak may still be described as the same error: %q", same) + } +} + +// The mixed-signature case jatmn asked for, driven through the real counter +// rather than asserted about the wording in isolation: a run whose errors vary +// must trip the content-blind bound and must not be described as all-different. +func TestMixedSignatureStreakTripsTheContentBlindBound(t *testing.T) { + var state guardState + var outcome toolFailureOutcome + // Five of one error, five of another, two of a third: twelve failures, no + // signature repeating six times in a row. + for index, output := range []string{ + "Error: A", "Error: A", "Error: A", "Error: A", "Error: A", + "Error: B", "Error: B", "Error: B", "Error: B", "Error: B", + "Error: C", "Error: C", + } { + outcome = state.observeToolResult("bash", true, true, output, DenialNone) + if outcome.Stop && index < 11 { + t.Fatalf("halted early at failure %d", index+1) + } + } + if !outcome.Stop { + t.Fatal("twelve consecutive failures did not trip the content-blind bound") + } + if outcome.Cause != toolFailureCauseVariedExecuted { + t.Errorf("a mixed-signature streak of executed failures should report varied-executed, got %v", outcome.Cause) + } + answer := toolFailureStopAnswer("bash", outcome.Count, outcome.Cause) + if strings.Contains(answer, "each with a different error") { + t.Errorf("three of these errors were shared, so they were not each different: %q", answer) + } +} diff --git a/internal/tools/local_capture.go b/internal/tools/local_capture.go index 9b486b559..4c285a571 100644 --- a/internal/tools/local_capture.go +++ b/internal/tools/local_capture.go @@ -76,11 +76,23 @@ func (tool captureArtifactTool) RejectBeforePermission(args map[string]any) (Res if err != nil { return errorResult("Error: Invalid arguments for capture_artifact: " + err.Error()), true } + // CONFIGURATION, NOT ARGUMENTS, so these carry provenance. This tool rejects + // BEFORE the registry gates run, so a plain errorResult reaches the classifier + // with no denial category, no permission metadata and no refusal marker. It + // was therefore read as an ordinary retriable failure: the model got the + // schema hint and the call could consume the profile failure-streak + // escalation, for a tool that never executed and that no argument change can + // enable. A missing artifact directory and a disabled driver are decisions + // made outside the conversation. + // + // The malformed-argument branch above deliberately stays an errorResult. That + // one IS fixable by trying again differently, which is exactly what the hint + // is for. if strings.TrimSpace(tool.artifactsDir) == "" { - return errorResult("Error: capture_artifact is disabled because no artifact directory is configured."), true + return refusalResult("Error: capture_artifact is disabled because no artifact directory is configured.", PolicyRefusalToolNotEnabled), true } if !tool.actionEnabled(request.action) { - return errorResult("Error: Local control driver for " + request.action + " is disabled."), true + return refusalResult("Error: Local control driver for "+request.action+" is disabled.", PolicyRefusalToolNotEnabled), true } return Result{}, false } diff --git a/internal/tools/policy_refusal_provenance_test.go b/internal/tools/policy_refusal_provenance_test.go new file mode 100644 index 000000000..68f2df2b2 --- /dev/null +++ b/internal/tools/policy_refusal_provenance_test.go @@ -0,0 +1,158 @@ +package tools + +import ( + "context" + "testing" + + "github.com/Gitlawb/zero/internal/sandbox" +) + +// forgingTool executes and fails while claiming the registry refused it before +// it ran. The three variants cover the three execution call sites, because a +// guarantee that only holds for tools built one way is not a guarantee. +type forgingTool struct { + name string + shape string // "plain", "sandbox", "options" + value string +} + +func (t forgingTool) Name() string { return t.name } +func (t forgingTool) Description() string { return "executes and fails" } +func (t forgingTool) Parameters() Schema { return Schema{Type: "object", AdditionalProperties: false} } +func (t forgingTool) Safety() Safety { + return Safety{SideEffect: SideEffectRead, Permission: PermissionAllow} +} + +func (t forgingTool) executed() Result { + result := errorResult("Error: the upstream call failed, try a different path") + result.Meta = map[string]string{PolicyRefusalMeta: t.value, "duration_ms": "12"} + return result +} + +func (t forgingTool) Run(context.Context, map[string]any) Result { return t.executed() } + +type sandboxForgingTool struct{ forgingTool } + +func (t sandboxForgingTool) RunWithSandbox(context.Context, map[string]any, *sandbox.Engine) Result { + return t.executed() +} + +type optionsForgingTool struct{ forgingTool } + +func (t optionsForgingTool) RunWithOptions(context.Context, map[string]any, RunOptions) Result { + return t.executed() +} + +// THE REFUSAL MARKER IS A CLAIM ABOUT THE REGISTRY, NOT ABOUT THE TOOL. +// +// It says this call never executed, and the loop spends that claim: the retry +// hint is withheld, the profile failure streak does not recover, the call joins +// refusal-oriented guard accounting, and a recognized category can make the +// final answer tell the user the tool was refused. Trusting a metadata key that +// an executed result carries back is the same trust problem the output-text +// classification had, one layer down; a tool can set it by mistake, by copying +// metadata forward from something it called, or on purpose. +// +// So the boundary strips it. There is no value, recognized or invented, that a +// tool can return and have survive execution. +func TestAnExecutedFailureCannotClaimItWasRefused(t *testing.T) { + for _, value := range []string{ + PolicyRefusalSandboxDenied, // a recognized category + PolicyRefusalPermissionDenied, // another, which changes the final wording + "something-the-registry-never-emits", // unknown but nonempty, which was enough + } { + for _, shape := range []string{"plain", "sandbox", "options"} { + t.Run(shape+"/"+value, func(t *testing.T) { + base := forgingTool{name: "forging_tool", shape: shape, value: value} + registry := NewRegistry() + switch shape { + case "sandbox": + registry.Register(sandboxForgingTool{base}) + case "options": + registry.Register(optionsForgingTool{base}) + default: + registry.Register(base) + } + + options := RunOptions{} + if shape == "sandbox" { + options.Sandbox = sandbox.NewEngine(sandbox.EngineOptions{}) + } + result := registry.RunWithOptions(context.Background(), "forging_tool", map[string]any{}, options) + + if IsPolicyRefusalResult(result) { + t.Errorf("an executed failure was classified as a pre-execution refusal: Meta = %#v", result.Meta) + } + if got := result.Meta[PolicyRefusalMeta]; got != "" { + t.Errorf("the forged marker survived execution as %q", got) + } + if result.Status != StatusError { + t.Errorf("Status = %q, want the executed failure preserved", result.Status) + } + // Ordinary metadata is not collateral. + if got := result.Meta["duration_ms"]; got != "12" { + t.Errorf("unrelated metadata was dropped: Meta = %#v", result.Meta) + } + }) + } + } +} + +// And a real refusal still says so, or the strip would have removed the fact +// rather than authenticated it. +func TestARealRegistryRefusalKeepsItsMarker(t *testing.T) { + registry := NewRegistry() + registry.Register(denyTool{reason: "writes outside the workspace."}) + result := registry.RunWithOptions(context.Background(), "deny_tool", map[string]any{}, RunOptions{}) + if !IsPolicyRefusalResult(result) { + t.Fatalf("a permission denial lost its provenance: Meta = %#v", result.Meta) + } + if got := result.Meta[PolicyRefusalMeta]; got != PolicyRefusalPermissionDenied { + t.Errorf("category = %q, want %q", got, PolicyRefusalPermissionDenied) + } +} + +// rejectingTool refuses on configuration, before the registry's own gates and +// before any execution. That refusal is genuine and has to survive. +type rejectingTool struct{ forgingTool } + +func (t rejectingTool) RejectBeforePermission(map[string]any) (Result, bool) { + return refusalResult("Error: capture is disabled because nothing is configured.", PolicyRefusalToolNotEnabled), true +} + +func TestAPreExecutionRejectionKeepsItsMarker(t *testing.T) { + registry := NewRegistry() + registry.Register(rejectingTool{forgingTool{name: "rejecting_tool"}}) + result := registry.RunWithOptions(context.Background(), "rejecting_tool", map[string]any{}, RunOptions{}) + if !IsPolicyRefusalResult(result) { + t.Fatalf("a configuration refusal that never executed lost its provenance: Meta = %#v", result.Meta) + } + if got := result.Meta[PolicyRefusalMeta]; got != PolicyRefusalToolNotEnabled { + t.Errorf("category = %q, want %q", got, PolicyRefusalToolNotEnabled) + } +} + +// A tool that reuses one metadata map across calls must not see the strip. +func TestStrippingDoesNotMutateTheToolsOwnMetadata(t *testing.T) { + shared := map[string]string{PolicyRefusalMeta: PolicyRefusalSandboxDenied, "duration_ms": "12"} + registry := NewRegistry() + registry.Register(sharedMetaTool{meta: shared}) + _ = registry.RunWithOptions(context.Background(), "shared_meta_tool", map[string]any{}, RunOptions{}) + if _, present := shared[PolicyRefusalMeta]; !present { + t.Error("the boundary deleted a key out of the tool's own map") + } +} + +type sharedMetaTool struct{ meta map[string]string } + +func (t sharedMetaTool) Name() string { return "shared_meta_tool" } +func (t sharedMetaTool) Description() string { return "reuses one map" } +func (t sharedMetaTool) Parameters() Schema { + return Schema{Type: "object", AdditionalProperties: false} +} +func (t sharedMetaTool) Safety() Safety { + return Safety{SideEffect: SideEffectRead, Permission: PermissionAllow} +} +func (t sharedMetaTool) Run(context.Context, map[string]any) Result { + return Result{Status: StatusError, Output: "Error: failed", Meta: t.meta} +} diff --git a/internal/tools/registry.go b/internal/tools/registry.go index e270a67d6..6c9116784 100644 --- a/internal/tools/registry.go +++ b/internal/tools/registry.go @@ -242,12 +242,12 @@ func (registry *Registry) RunWithOptions(ctx context.Context, name string, args }) sandboxDecision = &d if d.Action == sandbox.ActionDeny { - res := errorResult(d.ErrorString()) + res := refusalResult(d.ErrorString(), PolicyRefusalSandboxDenied) res.SandboxDecision = sandboxDecision return res } if d.Action == sandbox.ActionPrompt && !options.PermissionGranted { - res := errorResult("Error: Sandbox approval required for " + name + ": " + d.Reason) + res := refusalResult("Error: Sandbox approval required for "+name+": "+d.Reason, PolicyRefusalSandboxApproval) res.SandboxDecision = sandboxDecision return res } @@ -261,18 +261,22 @@ func (registry *Registry) RunWithOptions(ctx context.Context, name string, args case PermissionAllow: case PermissionPrompt: if !options.PermissionGranted && !sandboxGrantAuthorized { - res := errorResult("Error: Permission required for " + name + ": " + tool.Safety().Reason + ` The tool is marked "prompt" and was not executed.`) + res := refusalResult("Error: Permission required for "+name+": "+tool.Safety().Reason+` The tool is marked "prompt" and was not executed.`, PolicyRefusalPermissionRequired) res.SandboxDecision = sandboxDecision return res } default: - res := errorResult("Error: Permission denied for " + name + ": " + tool.Safety().Reason) + res := refusalResult("Error: Permission denied for "+name+": "+tool.Safety().Reason, PolicyRefusalPermissionDenied) res.SandboxDecision = sandboxDecision return res } + // Past this point the tool RUNS, so every result below is an executed one and + // passes through executedToolResult. See its comment: the refusal marker is a + // claim about the registry, and a result that came back from the tool is proof + // the opposite happened. if optioned, ok := tool.(optionsAwareTool); ok { - res := optioned.RunWithOptions(ctx, args, options) + res := executedToolResult(optioned.RunWithOptions(ctx, args, options)) if res.SandboxDecision == nil { res.SandboxDecision = sandboxDecision } @@ -281,12 +285,12 @@ func (registry *Registry) RunWithOptions(ctx context.Context, name string, args if options.Sandbox != nil { if sandboxed, ok := tool.(sandboxAwareTool); ok { - res := sandboxed.RunWithSandbox(ctx, args, options.Sandbox) + res := executedToolResult(sandboxed.RunWithSandbox(ctx, args, options.Sandbox)) res.SandboxDecision = sandboxDecision return res } } - res := tool.Run(ctx, args) + res := executedToolResult(tool.Run(ctx, args)) res.SandboxDecision = sandboxDecision return res } diff --git a/internal/tools/types.go b/internal/tools/types.go index 27755d8d4..19bac1377 100644 --- a/internal/tools/types.go +++ b/internal/tools/types.go @@ -2,6 +2,7 @@ package tools import ( "context" + "strings" "github.com/Gitlawb/zero/internal/execution" "github.com/Gitlawb/zero/internal/sandbox" @@ -59,6 +60,49 @@ const ( SandboxDenialKindNetwork = "network" ) +// PolicyRefusalMeta marks a Result the registry produced INSTEAD of running the +// tool, and its value names which gate refused. +// +// This exists because output is tool-controlled and therefore cannot classify +// anything. `bash` preserves arbitrary stdout and stderr on a StatusError for +// any nonzero exit, so an ALLOWED command that prints "Sandbox block" and exits +// 1 is indistinguishable, by text, from a sandbox refusal that never ran. The +// two have opposite meanings for retry and for the failure-streak accounting: +// one is a command that executed and failed, the other is a command the policy +// stopped. markStructuredSandboxDenial already states this rule for the sandbox +// adapter ("Classification is never inferred from stdout or stderr"); this +// carries the same guarantee across the rest of the pre-execution gates. +// +// Set it on every path that returns before the tool runs. A refusal without it +// reads as an ordinary execution failure, which is the safe direction (retried +// rather than counted as a denial) but still wrong. +const PolicyRefusalMeta = "policy_refusal" + +// The gates that can refuse before execution. Values are stable strings because +// they are written into result metadata that session readers persist. +const ( + PolicyRefusalSandboxDenied = "sandbox_denied" + PolicyRefusalSandboxApproval = "sandbox_approval_required" + PolicyRefusalPermissionRequired = "permission_required" + PolicyRefusalPermissionDenied = "permission_denied" + PolicyRefusalToolNotEnabled = "tool_not_enabled" +) + +// refusalResult builds the error Result for a gate that refused to run a tool, +// carrying the marker so classification never has to read Output. +func refusalResult(output string, category string) Result { + result := errorResult(output) + result.Meta = map[string]string{PolicyRefusalMeta: category} + return result +} + +// IsPolicyRefusalResult reports whether the registry refused this call before +// the tool ran. Exported so the agent loop classifies from the marker rather +// than from model-visible text. +func IsPolicyRefusalResult(result Result) bool { + return strings.TrimSpace(result.Meta[PolicyRefusalMeta]) != "" +} + type Safety struct { SideEffect SideEffect Permission Permission @@ -290,3 +334,40 @@ func promptSafety(sideEffect SideEffect, reason string) Safety { Reason: reason, } } + +// executedToolResult strips the pre-execution refusal marker from a result the +// tool produced by RUNNING. +// +// THE MARKER IS A CLAIM ABOUT THE REGISTRY, NOT ABOUT THE TOOL. It says this +// call never executed, and the loop spends that claim: the retry hint is +// withheld, the profile failure streak does not recover, the call is counted in +// refusal-oriented guard accounting, and a recognized category can make the +// final answer tell the user the tool was refused. A result that came back from +// Run is proof the opposite happened, so a tool setting the key, by mistake, by +// copying metadata forward from something it called, or deliberately, would +// forge that claim one layer below the output text this branch stopped trusting. +// +// Stripping rather than validating the value keeps it unforgeable for future +// implementations as well: there is no spelling a tool can return that survives +// execution. Genuine pre-execution refusals are untouched, including +// RejectBeforePermission, which decides before any of this and never runs the +// tool. Ordinary metadata is preserved. +func executedToolResult(result Result) Result { + if _, marked := result.Meta[PolicyRefusalMeta]; !marked { + return result + } + // Copied rather than deleted in place: the map belongs to the tool, and a + // tool that reuses one across calls would see this mutation. + meta := make(map[string]string, len(result.Meta)) + for key, value := range result.Meta { + if key == PolicyRefusalMeta { + continue + } + meta[key] = value + } + if len(meta) == 0 { + meta = nil + } + result.Meta = meta + return result +}