fix(agent): stop a denied tool looping past the repeated-failure halt

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")
+		}
+	}
+}

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
+}

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)
+	}
+}