From e16c02099848a480307b1ebf66889f5909b5f435 Mon Sep 17 00:00:00 2001 From: Trung Nguyen Date: Thu, 23 Jul 2026 16:06:23 +0200 Subject: [PATCH 1/9] feat: three-mode safety policy with decoupled classifier Replace the two-mode safer/unsafe shell approval model with three explicit safety modes and a pure-labeller classifier. - Modes: Strict (ask always), Balanced (auto-approve classifier-safe calls, ask on destructive/unknown), Autonomous (auto-approve every call). - Classifier is now a pure labeller emitting safe / destructive / unknown as metadata; the runtime consumes it via a mode x label decision table. - The per-toolset 'safer: true' opt-in is gone. Any agent that declares a shell toolset gets the classifier automatically. - Custom Allow/Ask/Deny rules override the mode; session-scoped Ask rules beat Autonomous, YAML-declared Ask rules are advisory and yield to it. --- agent-schema.json | 6 +- cmd/root/run_session_test.go | 6 +- examples/golibrary/stream/main.go | 2 +- examples/shell_safer.yaml | 12 +- pkg/a2a/adapter.go | 2 +- pkg/acp/agent.go | 2 +- pkg/acp/runagent_test.go | 2 +- pkg/cli/printer.go | 12 +- pkg/cli/runner.go | 6 +- pkg/config/latest/types.go | 85 ++-- pkg/config/latest/validate.go | 3 - pkg/config/latest/validate_test.go | 3 +- pkg/hooks/builtins/safer_shell.go | 102 ++--- pkg/hooks/builtins/safer_shell_test.go | 360 ++++------------- pkg/hooks/types.go | 21 +- pkg/leantui/update.go | 2 +- pkg/runtime/agent_delegation_test.go | 8 +- pkg/runtime/hooks.go | 28 +- pkg/runtime/on_tool_approval_decision_test.go | 4 +- pkg/runtime/resume.go | 50 +-- pkg/runtime/resume_test.go | 34 +- pkg/runtime/runtime_test.go | 71 +++- pkg/runtime/tool_dispatch.go | 6 +- pkg/runtime/toolexec/dispatcher.go | 267 ++++++------- pkg/runtime/toolexec/dispatcher_test.go | 366 +++++++++++++++--- pkg/runtime/toolexec/helpers_test.go | 2 +- pkg/runtime/toolexec/permissions.go | 111 ++++-- pkg/runtime/toolexec/permissions_test.go | 162 +++++--- pkg/server/session_manager.go | 17 +- pkg/session/safety_policy_test.go | 68 ++-- pkg/session/session.go | 50 ++- pkg/teamloader/teamloader.go | 2 +- pkg/tui/components/toolconfirm/toolconfirm.go | 15 +- .../toolconfirm/toolconfirm_test.go | 8 +- pkg/tui/components/tour/steps.go | 5 +- pkg/tui/components/tour/tour_test.go | 6 +- pkg/tui/dialog/tool_confirmation.go | 4 +- 37 files changed, 988 insertions(+), 922 deletions(-) diff --git a/agent-schema.json b/agent-schema.json index 661de4c991..aa428ebfcb 100644 --- a/agent-schema.json +++ b/agent-schema.json @@ -1363,7 +1363,7 @@ }, "preempt_yolo": { "type": "boolean", - "description": "Opt a pre_tool_use entry into firing BEFORE the deterministic approval pipeline (--yolo, permission allow/deny patterns). A deny/ask verdict from a preempting hook cannot be bypassed by --yolo or permission allow-rules; an allow verdict is advisory (the pipeline still runs Decide() and the rest of pre_tool_use). Default pre_tool_use entries fire AFTER Decide(). Only meaningful on pre_tool_use; ignored on other events. Used by the safer_shell builtin (auto-registered with this flag when a shell toolset has `safer: true`)." + "description": "Opt a pre_tool_use entry into firing BEFORE the deterministic approval pipeline (custom allow/ask/deny rules + safety mode). A deny/ask verdict from a preempting hook cannot be bypassed by the safety mode (including Autonomous); an allow verdict is advisory. Default pre_tool_use entries fire AFTER the safety mode's verdict. Only meaningful on pre_tool_use; ignored on other events. Used by the safer_shell builtin (auto-registered whenever an agent has a shell toolset) as a pure labeller — it always returns Allow with classification metadata that the runtime's (mode × label) table consumes." } }, "required": [ @@ -2330,10 +2330,6 @@ "type": "boolean", "description": "Opt in to background-job recall for the background_jobs toolset (only valid for type 'background_jobs'). When enabled, run_background_job exposes a recall boolean parameter. If the agent sets recall=true for a background job, the runtime injects a steering message with a short completion sentence and the job output when the job finishes. Default false." }, - "safer": { - "type": "boolean", - "description": "Opt in to the safer_shell classifier for the shell toolset (only valid for type 'shell'). When enabled, the agent auto-registers the safer_shell builtin under pre_tool_use with preempt_yolo:true. Verdict adapts to the session's SafetyPolicy (unsafe / safer / safe-auto / strict). See docs/configuration/hooks/index.md for the per-policy behaviour matrix. Default false." - }, "url": { "type": "string", "description": "URL for the a2a, openapi or open_url tool", diff --git a/cmd/root/run_session_test.go b/cmd/root/run_session_test.go index 6b6cc29b9c..4888144ae3 100644 --- a/cmd/root/run_session_test.go +++ b/cmd/root/run_session_test.go @@ -66,7 +66,7 @@ func TestCreateLocalRuntimeAndSession_ResumeWithYoloBackfillsSafetyPolicy(t *tes _, sess, err := f.createLocalRuntimeAndSession(t.Context(), newSessionTestLoadResult(), req, store) require.NoError(t, err) assert.True(t, sess.ToolsApproved) - assert.Equal(t, session.SafetyPolicyUnsafe, sess.SafetyPolicy) + assert.Equal(t, session.SafetyPolicyAutonomous, sess.SafetyPolicy) } // An explicit stored SafetyPolicy survives a --yolo resume: the backfill @@ -77,7 +77,7 @@ func TestCreateLocalRuntimeAndSession_ResumeKeepsExplicitSafetyPolicy(t *testing store := session.NewInMemorySessionStore() require.NoError(t, store.AddSession(t.Context(), session.New( session.WithID("existing"), - session.WithSafetyPolicy(session.SafetyPolicySafer), + session.WithSafetyPolicy(session.SafetyPolicyBalanced), ))) f := &runExecFlags{} @@ -85,7 +85,7 @@ func TestCreateLocalRuntimeAndSession_ResumeKeepsExplicitSafetyPolicy(t *testing _, sess, err := f.createLocalRuntimeAndSession(t.Context(), newSessionTestLoadResult(), req, store) require.NoError(t, err) - assert.Equal(t, session.SafetyPolicySafer, sess.SafetyPolicy) + assert.Equal(t, session.SafetyPolicyBalanced, sess.SafetyPolicy) } // A relative ref (e.g. -1) is resume-only: it must resolve against existing diff --git a/examples/golibrary/stream/main.go b/examples/golibrary/stream/main.go index 014c1ca55e..696c347cbb 100644 --- a/examples/golibrary/stream/main.go +++ b/examples/golibrary/stream/main.go @@ -63,7 +63,7 @@ func run(ctx context.Context) error { case *runtime.StreamStoppedEvent: log.Println("Stream stopped for session") case *runtime.ToolCallConfirmationEvent: - rt.Resume(ctx, runtime.ResumeRequest{Type: runtime.ResumeTypeApproveSession}) + rt.Resume(ctx, runtime.ResumeRequest{Type: runtime.ResumeTypeApproveAutonomous}) case *runtime.ToolCallEvent: log.Printf("Tool call: %s\n", e.ToolCall.Function.Name) case *runtime.ToolCallResponseEvent: diff --git a/examples/shell_safer.yaml b/examples/shell_safer.yaml index 1893220dfe..13e67ecfe4 100644 --- a/examples/shell_safer.yaml +++ b/examples/shell_safer.yaml @@ -3,12 +3,12 @@ agents: model: anthropic/claude-haiku-4-5 description: Shell agent with the safer_shell classifier enabled welcome_message: | - The safer_shell classifier is enabled. Its verdict follows the - session's SafetyPolicy (unsafe / safer / safe-auto / strict, set - by the caller — CLI `--yolo` maps to unsafe, the API accepts - `safety_policy` on session create, and users can opt into - `safe-auto` from a confirmation prompt). + The safer_shell classifier runs automatically for every shell + toolset. It labels each shell command (safe / destructive / + unknown) and attaches the label as tool-call metadata; the + runtime's safety mode (strict / balanced / autonomous) is what + actually gates the call. Confirmation prompts render a + destructive-tier badge when the classifier flagged the command. instruction: Use the shell tool to run the command the user asks for. toolsets: - type: shell - safer: true diff --git a/pkg/a2a/adapter.go b/pkg/a2a/adapter.go index 2b5437022a..89343f0e10 100644 --- a/pkg/a2a/adapter.go +++ b/pkg/a2a/adapter.go @@ -76,7 +76,7 @@ func runDockerAgent(ctx agent.InvocationContext, t *team.Team, agentName string, if existing, err := sessStore.GetSession(ctx, sessionID); err == nil && existing != nil { sess = existing sess.AddMessage(session.UserMessage(message)) - sess.ToolsApproved = true + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) sess.NonInteractive = true } else { workingDir, _ := os.Getwd() diff --git a/pkg/acp/agent.go b/pkg/acp/agent.go index 30cf47f7ae..f23468ba99 100644 --- a/pkg/acp/agent.go +++ b/pkg/acp/agent.go @@ -748,7 +748,7 @@ func (a *Agent) handleToolCallConfirmation(ctx context.Context, acpSess *Session case "allow": acpSess.rt.Resume(ctx, runtime.ResumeRequest{Type: runtime.ResumeTypeApprove}) case "allow-always": - acpSess.rt.Resume(ctx, runtime.ResumeRequest{Type: runtime.ResumeTypeApproveSession}) + acpSess.rt.Resume(ctx, runtime.ResumeRequest{Type: runtime.ResumeTypeApproveAutonomous}) case "reject": acpSess.rt.Resume(ctx, runtime.ResumeRequest{Type: runtime.ResumeTypeReject}) default: diff --git a/pkg/acp/runagent_test.go b/pkg/acp/runagent_test.go index 3e16909315..311e46763b 100644 --- a/pkg/acp/runagent_test.go +++ b/pkg/acp/runagent_test.go @@ -633,7 +633,7 @@ func TestRunAgent_ToolCallConfirmationOutcomes(t *testing.T) { { name: "allow-always approves session", result: permissionSelected("allow-always"), - wantResume: []runtime.ResumeRequest{{Type: runtime.ResumeTypeApproveSession}}, + wantResume: []runtime.ResumeRequest{{Type: runtime.ResumeTypeApproveAutonomous}}, }, { name: "reject rejects", diff --git a/pkg/cli/printer.go b/pkg/cli/printer.go index cc435e9c24..dc17270c29 100644 --- a/pkg/cli/printer.go +++ b/pkg/cli/printer.go @@ -21,10 +21,10 @@ import ( type ConfirmationResult string const ( - ConfirmationApprove ConfirmationResult = "approve" - ConfirmationApproveSession ConfirmationResult = "approve_session" - ConfirmationReject ConfirmationResult = "reject" - ConfirmationAbort ConfirmationResult = "abort" + ConfirmationApprove ConfirmationResult = "approve" + ConfirmationApproveAutonomous ConfirmationResult = "approve_autonomous" + ConfirmationReject ConfirmationResult = "reject" + ConfirmationAbort ConfirmationResult = "abort" ) var bold = color.New(color.Bold).SprintfFunc() @@ -111,7 +111,7 @@ func (p *Printer) PrintToolCallWithConfirmation(ctx context.Context, toolCall to return ConfirmationApprove case 'a', 'A': p.Print(bold("Yes to all 👍")) - return ConfirmationApproveSession + return ConfirmationApproveAutonomous case 'n', 'N': p.Print(bold("No 👎")) return ConfirmationReject @@ -135,7 +135,7 @@ func (p *Printer) PrintToolCallWithConfirmation(ctx context.Context, toolCall to case "y": return ConfirmationApprove case "a": - return ConfirmationApproveSession + return ConfirmationApproveAutonomous case "n": return ConfirmationReject default: diff --git a/pkg/cli/runner.go b/pkg/cli/runner.go index 5be8c2a3cf..a40a007b41 100644 --- a/pkg/cli/runner.go +++ b/pkg/cli/runner.go @@ -179,9 +179,9 @@ func Run(ctx context.Context, out *Printer, cfg Config, rt runtime.Runtime, sess switch result { case ConfirmationApprove: rt.Resume(ctx, runtime.ResumeApprove()) - case ConfirmationApproveSession: - sess.ToolsApproved = true - rt.Resume(ctx, runtime.ResumeApproveSession()) + case ConfirmationApproveAutonomous: + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) + rt.Resume(ctx, runtime.ResumeApproveAutonomous()) case ConfirmationReject: rt.Resume(ctx, runtime.ResumeReject("")) lastConfirmedToolCallID = "" // Clear on reject since tool won't execute diff --git a/pkg/config/latest/types.go b/pkg/config/latest/types.go index 86b693083d..e2f46cccf3 100644 --- a/pkg/config/latest/types.go +++ b/pkg/config/latest/types.go @@ -996,28 +996,17 @@ func (a *AgentConfig) SessionCompactionEnabled() bool { return *a.SessionCompaction } -// SaferShellEnabled reports whether any of the agent's shell toolsets -// has opted into destructive-command detection. The flag lives on the -// toolset (not the agent), so this aggregates across all of an agent's -// toolsets — one match anywhere flips the agent-level flag the runtime -// uses to register the safer_shell builtin. -// -// Off by default: a missing pointer or explicit `safer: false` does -// not enable the feature. Only an explicit `safer: true` on at least -// one shell toolset switches it on. -// -// Multi-toolset note: when an agent declares more than one shell -// toolset and only some opt in, the safer_shell builtin still runs -// for every shell call on this agent (it filters by ToolName, not -// by toolset identity). Author one shell toolset per agent when you -// need toolset-scoped granularity. -func (a *AgentConfig) SaferShellEnabled() bool { +// HasShellToolset reports whether any of the agent's toolsets is a +// shell toolset. The runtime uses this to decide whether to auto-inject +// the safer_shell classifier as a pre_tool_use hook — the classifier +// is a pure labeller (never blocks) that emits blast_radius metadata +// consumed by the runtime's (mode × classifier-label) verdict table. +func (a *AgentConfig) HasShellToolset() bool { if a == nil { return false } for i := range a.Toolsets { - ts := &a.Toolsets[i] - if ts.Type == "shell" && ts.Safer != nil && *ts.Safer { + if a.Toolsets[i].Type == "shell" { return true } } @@ -1614,17 +1603,6 @@ type Toolset struct { // finishes. Recall *bool `json:"recall,omitempty" yaml:"recall,omitempty"` - // For the `shell` toolset — opt in to destructive-command detection. - // When enabled, the agent auto-registers the safer_shell builtin under - // pre_tool_use with preempt_yolo:true. Destructive commands (rm -rf, docker - // volume rm, mkfs, …) get an Ask verdict carrying a blast-radius - // classification; known-safe reads (ls, git status, docker ps, …) - // flow through silently; everything else asks with blast_radius=unknown - // so the user sees the prompt before --yolo or permission allow-rules - // can auto-approve it. nil/false leaves the agent's shell calls subject - // only to the regular approval pipeline. - Safer *bool `json:"safer,omitempty" yaml:"safer,omitempty"` - // For the `rag` tool RAGConfig *RAGConfig `json:"rag_config,omitempty" yaml:"rag_config,omitempty"` @@ -2440,14 +2418,16 @@ type RAGFusionConfig struct { } // PermissionsConfig represents tool permission configuration. -// Allow/Ask/Deny model. This controls tool call approval behavior: -// - Allow: Tools matching these patterns are auto-approved (like --yolo for specific tools) -// - Ask: Tools matching these patterns always require user approval, even if the tool is read-only -// - Deny: Tools matching these patterns are always rejected, even with --yolo +// Allow/Ask/Deny model. Custom rules always override the session's +// SafetyPolicy (strict / balanced / autonomous): +// - Allow: Tools matching these patterns are auto-approved, even under Strict. +// - Ask: Tools matching these patterns always require user approval, even under Autonomous. +// - Deny: Tools matching these patterns are always rejected. // -// Patterns support glob-style matching (e.g., "shell", "read_*", "mcp:github:*") -// The evaluation order is: Deny (checked first), then Allow, then Ask (explicit), then default -// (read-only tools auto-approved, others ask) +// Patterns support glob-style matching (e.g., "shell", "read_*", "mcp:github:*"). +// Within a single checker, priority is Deny > Allow > Ask; when no +// custom rule matches, the safety mode is applied against the tool's +// classifier label (safe / destructive / unknown) to pick allow / ask. type PermissionsConfig struct { // Allow lists tool name patterns that are auto-approved without user confirmation Allow []string `json:"allow,omitempty"` @@ -2473,11 +2453,12 @@ type HooksConfig struct { PostToolUse HookMatcherConfigs `json:"post_tool_use,omitempty" yaml:"post_tool_use,omitempty"` // PermissionRequest hooks run just before the runtime would prompt - // the user to approve a tool call (i.e. when neither --yolo nor a - // permissions rule short-circuited the decision). Hooks may auto-allow - // or auto-deny via hook_specific_output.permission_decision so the - // user is not prompted; otherwise the runtime falls through to the - // usual interactive confirmation. Tool-matched, like pre_tool_use. + // the user to approve a tool call (i.e. when neither a custom + // rule nor the safety mode auto-approved the call). Hooks may + // auto-allow or auto-deny via hook_specific_output.permission_decision + // so the user is not prompted; otherwise the runtime falls + // through to the usual interactive confirmation. Tool-matched, + // like pre_tool_use. PermissionRequest HookMatcherConfigs `json:"permission_request,omitempty" yaml:"permission_request,omitempty"` // SessionStart hooks run when a session begins @@ -2684,17 +2665,19 @@ type HookMatcherConfig struct { Hooks HookDefinitions `json:"hooks" yaml:"hooks"` // PreemptYolo opts a pre_tool_use entry into firing BEFORE the - // deterministic approval pipeline (--yolo, permission patterns). - // A deny/ask verdict from a preempting hook cannot be bypassed by - // auto-approval rules; an allow verdict is advisory (the pipeline - // still runs Decide() and the rest of pre_tool_use). Default - // pre_tool_use entries fire AFTER Decide(), as before. Only valid - // on pre_tool_use; ignored on other events. + // deterministic approval pipeline (custom rules + safety mode). + // A deny/ask verdict from a preempting hook cannot be bypassed + // by the safety mode (including Autonomous); an allow verdict is + // advisory (the pipeline still consults custom rules + the mode + // table + the default pre_tool_use lane). Default pre_tool_use + // entries fire AFTER the safety mode's verdict. Only valid on + // pre_tool_use; ignored on other events. // - // Used by the safer_shell builtin (auto-registered with this flag - // when a shell toolset has `safer: true`). Custom hooks set it to - // true when they implement a security-critical check that must - // not be bypassed by --yolo. + // Used by the safer_shell builtin (auto-registered whenever an + // agent has a shell toolset) as a pure labeller — it always + // returns Allow with classification metadata. Custom hooks set + // this to true when they implement a security-critical check + // that must not be bypassed by the safety mode. PreemptYolo *bool `json:"preempt_yolo,omitempty" yaml:"preempt_yolo,omitempty"` } diff --git a/pkg/config/latest/validate.go b/pkg/config/latest/validate.go index 9087e9e9de..eeb3ed25a5 100644 --- a/pkg/config/latest/validate.go +++ b/pkg/config/latest/validate.go @@ -298,9 +298,6 @@ func (t *Toolset) validate() error { if t.Recall != nil && t.Type != "background_jobs" { return errors.New("recall can only be used with type 'background_jobs'") } - if t.Safer != nil && t.Type != "shell" { - return errors.New("safer can only be used with type 'shell'") - } if len(t.AllowedDomains) > 0 && len(t.BlockedDomains) > 0 { return errors.New("allowed_domains and blocked_domains are mutually exclusive") } diff --git a/pkg/config/latest/validate_test.go b/pkg/config/latest/validate_test.go index 51960ec984..5e9d5594b6 100644 --- a/pkg/config/latest/validate_test.go +++ b/pkg/config/latest/validate_test.go @@ -191,7 +191,6 @@ func TestToolsetValidateAttributeTypeMismatch(t *testing.T) { {name: "allow_private_ips on wrong type", toolset: Toolset{Type: "shell", AllowPrivateIPs: new(true)}, wantErr: "allow_private_ips can only be used with type 'fetch', 'api', 'openapi', 'a2a' or remote MCP toolsets"}, {name: "sudo_askpass on non-shell", toolset: Toolset{Type: "fetch", SudoAskpass: new(true)}, wantErr: "sudo_askpass can only be used with type 'shell'"}, {name: "recall on non-background_jobs", toolset: Toolset{Type: "shell", Recall: new(true)}, wantErr: "recall can only be used with type 'background_jobs'"}, - {name: "safer on non-shell", toolset: Toolset{Type: "fetch", Safer: new(true)}, wantErr: "safer can only be used with type 'shell'"}, {name: "allowed and blocked domains", toolset: Toolset{Type: "fetch", AllowedDomains: []string{"a.example.com"}, BlockedDomains: []string{"b.example.com"}}, wantErr: "allowed_domains and blocked_domains are mutually exclusive"}, {name: "invalid allowed_domains pattern", toolset: Toolset{Type: "fetch", AllowedDomains: []string{"foo.*"}}, wantErr: `allowed_domains[0] "foo.*" is invalid`}, {name: "invalid blocked_domains pattern", toolset: Toolset{Type: "fetch", BlockedDomains: []string{"10.0.0.0/33"}}, wantErr: `blocked_domains[0] "10.0.0.0/33" is invalid: not a valid CIDR`}, @@ -299,7 +298,7 @@ func TestToolsetValidateValidToolsets(t *testing.T) { name string toolset Toolset }{ - {name: "shell", toolset: Toolset{Type: "shell", Env: map[string]string{"A": "b"}, SudoAskpass: new(true), Safer: new(true)}}, + {name: "shell", toolset: Toolset{Type: "shell", Env: map[string]string{"A": "b"}, SudoAskpass: new(true)}}, {name: "background_jobs", toolset: Toolset{Type: "background_jobs", Env: map[string]string{"A": "b"}, Recall: new(true)}}, {name: "memory with path", toolset: Toolset{Type: "memory", Path: "/tmp/memory.db"}}, {name: "memory without path", toolset: Toolset{Type: "memory"}}, diff --git a/pkg/hooks/builtins/safer_shell.go b/pkg/hooks/builtins/safer_shell.go index cd764dbf98..df47754ab6 100644 --- a/pkg/hooks/builtins/safer_shell.go +++ b/pkg/hooks/builtins/safer_shell.go @@ -1,15 +1,9 @@ package builtins -// safer_shell classifies shell tool calls against a taxonomy and -// adapts its verdict to hooks.Input.SafetyPolicy. Registered on -// pre_tool_use with preempt_yolo:true so it runs before --yolo. -// -// Per-policy verdict: -// unsafe — silent. -// safer — destructive ask; safe/unknown silent. -// safe-auto — safe allow; destructive/unknown ask. -// strict — safe/destructive/unknown all ask (with metadata). -// +// safer_shell labels shell calls against an embedded taxonomy of +// safe reads and destructive patterns. Pure labeller: always returns +// Allow + metadata (blast_radius, category, reason). The mode × label +// table in pkg/runtime/toolexec is what actually gates the call. // Compound shell (a && b, a; b, a | b) skips the safe-list. import ( @@ -135,11 +129,10 @@ func compileSafe(value any) ([]safePattern, error) { return out, nil } -// saferShell is the [hooks.BuiltinFunc] registered under [SaferShell]. -// See the file-level comment for the policy-aware logic. Taxonomy -// load failure asks with blast_radius=unknown regardless of policy -// (fail-closed), except under unsafe which stays silent. -func saferShell(_ context.Context, in *hooks.Input, args []string) (*hooks.Output, error) { +// blast_radius wire values: "safe" | "low" | "medium" | "high" | +// "unknown". low/medium/high all collapse to "destructive" at the +// runtime layer (see LabelFromBlastRadius). +func saferShell(_ context.Context, in *hooks.Input, _ []string) (*hooks.Output, error) { if in == nil || in.HookEventName != hooks.EventPreToolUse { return nil, nil } @@ -147,76 +140,32 @@ func saferShell(_ context.Context, in *hooks.Input, args []string) (*hooks.Outpu return nil, nil } - policy := effectiveSafetyPolicy(in.SafetyPolicy, args) - if policy == policyUnsafe { - return nil, nil - } - command, _ := shellCommandArg(in.ToolInput) patterns, err := loadSafetyPatterns() if err != nil { - return askWithMetadata(radiusUnknown, "", "Safety pattern load failed: "+err.Error()), nil + return allowWithMetadata(radiusUnknown, "", "Safety pattern load failed: "+err.Error()), nil } if command != "" { if match := bestDestructiveMatch(command, patterns.destructive); match != nil { - return askWithMetadata(match.BlastRadius, match.Category, + return allowWithMetadata(match.BlastRadius, match.Category, "Command matches destructive operation: "+match.Pattern), nil } if match := bestSafeMatch(command, patterns.safe); match != nil { - reason := "Command matches safe read-only pattern: " + match.Pattern - switch policy { - case policySafer: - return nil, nil - case policySafeAuto: - return allowWithMetadata(radiusSafe, match.Category, reason), nil - default: - return askWithMetadata(radiusSafe, match.Category, reason), nil - } + return allowWithMetadata(radiusSafe, match.Category, + "Command matches safe read-only pattern: "+match.Pattern), nil } } - // Unknown command: safer defers, strict / safe-auto ask. - if policy == policySafer { - return nil, nil - } - return askWithMetadata(radiusUnknown, "", - "Shell command requires safer-mode confirmation."), nil + return allowWithMetadata(radiusUnknown, "", + "Command does not match any known safe or destructive pattern."), nil } -// Mirrors pkg/session.SafetyPolicy strings; the hooks package must -// stay free of a session dependency. -const ( - policyUnsafe = "unsafe" - policySafer = "safer" - policySafeAuto = "safe-auto" - policyStrict = "strict" -) - const ( radiusSafe = "safe" radiusUnknown = "unknown" ) -// effectiveSafetyPolicy picks the policy for one invocation. -// Precedence: args[0] (YAML pin) > session > strict. Unrecognised -// args[0] falls through to session (so a YAML typo doesn't flip -// modes silently). -func effectiveSafetyPolicy(sessionPolicy string, args []string) string { - if len(args) > 0 { - switch args[0] { - case policyUnsafe, policySafer, policySafeAuto, policyStrict: - return args[0] - } - } - switch sessionPolicy { - case policyUnsafe, policySafer, policySafeAuto, policyStrict: - return sessionPolicy - default: - return policyStrict - } -} - func shellCommandArg(input map[string]any) (string, bool) { if v, ok := input["cmd"].(string); ok { return v, true @@ -275,15 +224,7 @@ func containsShellSeparator(command string) bool { return false } -func askWithMetadata(blastRadius, category, reason string) *hooks.Output { - return verdictWithMetadata(hooks.DecisionAsk, blastRadius, category, reason) -} - func allowWithMetadata(blastRadius, category, reason string) *hooks.Output { - return verdictWithMetadata(hooks.DecisionAllow, blastRadius, category, reason) -} - -func verdictWithMetadata(decision hooks.Decision, blastRadius, category, reason string) *hooks.Output { meta := map[string]string{ metaBlastRadius: blastRadius, metaReason: reason, @@ -294,13 +235,26 @@ func verdictWithMetadata(decision hooks.Decision, blastRadius, category, reason return &hooks.Output{ HookSpecificOutput: &hooks.HookSpecificOutput{ HookEventName: hooks.EventPreToolUse, - PermissionDecision: decision, + PermissionDecision: hooks.DecisionAllow, PermissionDecisionReason: reason, Metadata: meta, }, } } +// LabelFromBlastRadius collapses the wire radius to the three-way +// classifier label consumed by the mode table. +func LabelFromBlastRadius(blastRadius string) string { + switch blastRadius { + case radiusSafe: + return "safe" + case "low", "medium", "high": + return "destructive" + default: + return "unknown" + } +} + // collectDestructiveEntries walks the JSON destructive section. The // shape is map[category-name][]entry where each entry has pattern + // blast_radius (+ optional category override). diff --git a/pkg/hooks/builtins/safer_shell_test.go b/pkg/hooks/builtins/safer_shell_test.go index 650ecb912d..ff2bd1f943 100644 --- a/pkg/hooks/builtins/safer_shell_test.go +++ b/pkg/hooks/builtins/safer_shell_test.go @@ -9,10 +9,14 @@ import ( "github.com/docker/docker-agent/pkg/hooks" ) -// TestSaferShell_MatchesDestructivePatterns covers the destructive -// taxonomy: each fixture must produce an Ask verdict with the -// expected blast-radius level when run under EventPreToolUse against -// a shell tool call. Metadata carries blast_radius + category + reason. +// safer_shell is now a pure labeller: it always returns an Allow +// verdict for shell tool calls with classification metadata attached. +// The runtime's (mode × label) verdict table is what actually gates +// the call — see pkg/runtime/toolexec. + +// Destructive fixtures produce an Allow verdict with the expected +// blast-radius level in metadata. Metadata carries blast_radius + +// category + reason. func TestSaferShell_MatchesDestructivePatterns(t *testing.T) { t.Parallel() @@ -38,10 +42,11 @@ func TestSaferShell_MatchesDestructivePatterns(t *testing.T) { ToolInput: map[string]any{"cmd": tc.cmd}, }, nil) require.NoError(t, err) - require.NotNil(t, out, "destructive command %q should produce a verdict", tc.cmd) + require.NotNil(t, out, "destructive command %q should produce metadata", tc.cmd) require.NotNil(t, out.HookSpecificOutput) assert.Equal(t, hooks.EventPreToolUse, out.HookSpecificOutput.HookEventName) - assert.Equal(t, hooks.DecisionAsk, out.HookSpecificOutput.PermissionDecision) + assert.Equal(t, hooks.DecisionAllow, out.HookSpecificOutput.PermissionDecision, + "classifier is a pure labeller — always Allow") assert.Equal(t, tc.wantLevel, out.HookSpecificOutput.Metadata[metaBlastRadius], "unexpected blast radius for %q", tc.cmd) assert.Contains(t, out.HookSpecificOutput.PermissionDecisionReason, tc.wantPattern, @@ -52,65 +57,18 @@ func TestSaferShell_MatchesDestructivePatterns(t *testing.T) { } } -// Under strict (default), safe-list matches Ask with blast_radius=safe -// so the UI can render a SAFE chip. The verdict is still Ask — safe -// metadata is decoration, not a bypass. -func TestSaferShell_SafeCommandsUnderStrictAskWithSafeRadius(t *testing.T) { +// Safe patterns produce an Allow verdict with blast_radius=safe. +func TestSaferShell_SafeCommandsProduceSafeRadius(t *testing.T) { t.Parallel() safeCases := []string{ "ls", - "ls /tmp", "ls -la", - "ls -la /tmp", "cat README.md", - "head -n 50 main.go", - "tail -n 20 logs/app.log", "pwd", - "whoami", - "hostname", - "date", - "env", - "printenv PATH", - "which docker", - "echo hello world", - "printf %s\\n value", - "basename /tmp/x", - "dirname /tmp/x", - "df -h", - "du -sh /tmp", - "grep -n pattern file.go", - "rg pattern", - "rg pattern src", - "sort file.txt", - "uniq file.txt", - "wc file.txt", - "wc -l file.txt", - "stat go.mod", - "file go.mod", - "ps aux", - "top -n 1", "git status", - "git diff", - "git diff HEAD~1", - "git log --oneline -10", - "git show HEAD", - "git branch", - "git remote -v", "docker ps", - "docker ps -a", - "docker images", - "docker inspect web", - "docker logs web", - "docker logs --tail 100 web", - "docker stats --no-stream", - "docker version", - "docker info", - "docker system df", "kubectl get pods", - "kubectl describe pod web", - "kubectl logs web", - "kubectl version", } for _, cmd := range safeCases { t.Run(cmd, func(t *testing.T) { @@ -120,34 +78,34 @@ func TestSaferShell_SafeCommandsUnderStrictAskWithSafeRadius(t *testing.T) { ToolInput: map[string]any{"cmd": cmd}, }, nil) require.NoError(t, err, "safe command %q must produce a verdict", cmd) - require.NotNil(t, out, "safe command %q must produce a verdict under strict", cmd) + require.NotNil(t, out) require.NotNil(t, out.HookSpecificOutput) - assert.Equal(t, hooks.DecisionAsk, out.HookSpecificOutput.PermissionDecision, - "strict must still ask; the safe verdict is UI decoration, not a bypass") + assert.Equal(t, hooks.DecisionAllow, out.HookSpecificOutput.PermissionDecision, + "classifier is a pure labeller — always Allow") assert.Equal(t, radiusSafe, out.HookSpecificOutput.Metadata[metaBlastRadius]) assert.Contains(t, out.HookSpecificOutput.PermissionDecisionReason, "safe read-only pattern") }) } } -// TestSaferShell_CompoundShellFallsThroughToAsk pins the contract -// that a destructive command hidden inside a chain still triggers -// the prompt — directly via a destructive match on the inner -// command — and that a safe-looking compound never silently passes -// the safe allowlist (the matcher refuses safe-match on any string -// containing a shell separator). -func TestSaferShell_CompoundShellFallsThroughToAsk(t *testing.T) { +// Compound shell: destructive matches short-circuit first (worst +// wins); otherwise the compound is rejected wholesale by +// [bestSafeMatch] and falls through to unknown, since a single-line +// compound can smuggle unsafe segments past a naive per-segment +// check. +func TestSaferShell_CompoundShellRadius(t *testing.T) { t.Parallel() cases := []struct { - name string - cmd string + name string + cmd string + wantRadius string }{ - {"safe-then-destructive AND", "cd /tmp && rm -rf foo"}, - {"safe-then-destructive semicolon", "cd /tmp; rm -rf foo"}, - {"safe-then-destructive pipe", "find /tmp | xargs rm -rf"}, - {"two safes chained does NOT silently match safe list", "ls && pwd"}, - {"safe OR safe still asks", "git status || git diff"}, + {"safe-then-destructive AND", "cd /tmp && rm -rf foo", "high"}, + {"safe-then-destructive semicolon", "cd /tmp; rm -rf foo", "high"}, + {"safe-then-destructive pipe", "find /tmp | xargs rm -rf", "low"}, + {"two safes chained via && stay unknown", "ls && pwd", "unknown"}, + {"safe OR safe stays unknown", "git status || git diff", "unknown"}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -157,16 +115,17 @@ func TestSaferShell_CompoundShellFallsThroughToAsk(t *testing.T) { ToolInput: map[string]any{"cmd": tc.cmd}, }, nil) require.NoError(t, err) - require.NotNil(t, out, "compound command %q must produce a verdict", tc.cmd) + require.NotNil(t, out) require.NotNil(t, out.HookSpecificOutput) - assert.Equal(t, hooks.DecisionAsk, out.HookSpecificOutput.PermissionDecision) + assert.Equal(t, hooks.DecisionAllow, out.HookSpecificOutput.PermissionDecision) + assert.Equal(t, tc.wantRadius, out.HookSpecificOutput.Metadata[metaBlastRadius]) }) } } -// TestSaferShell_AcceptsCommandAliasKey pins the "command" alias for -// the canonical "cmd" arg — the shell tool accepts both. Without -// this the alias path would silently bypass the matcher. +// The "command" alias for the canonical "cmd" arg — the shell tool +// accepts both. Without this the alias path would silently bypass +// classification. func TestSaferShell_AcceptsCommandAliasKey(t *testing.T) { t.Parallel() @@ -180,10 +139,8 @@ func TestSaferShell_AcceptsCommandAliasKey(t *testing.T) { assert.Equal(t, "high", out.HookSpecificOutput.Metadata[metaBlastRadius]) } -// TestSaferShell_NoOpForNonShellTool keeps the no-op contract: the -// builtin is registered under matcher "*", so it sees every -// pre_tool_use dispatch. It must return nil for tools it doesn't -// classify. +// The builtin is registered under matcher "*", so it sees every +// pre_tool_use dispatch. It must return nil for non-shell tools. func TestSaferShell_NoOpForNonShellTool(t *testing.T) { t.Parallel() @@ -196,9 +153,7 @@ func TestSaferShell_NoOpForNonShellTool(t *testing.T) { assert.Nil(t, out) } -// TestSaferShell_NoOpUnderWrongEvent: the builtin only acts on -// EventPreToolUse. An operator who wires it under a different event -// (e.g. post_tool_use) gets a no-op rather than a misleading verdict. +// The builtin only acts on EventPreToolUse. Anywhere else it no-ops. func TestSaferShell_NoOpUnderWrongEvent(t *testing.T) { t.Parallel() @@ -211,11 +166,10 @@ func TestSaferShell_NoOpUnderWrongEvent(t *testing.T) { assert.Nil(t, out) } -// TestSaferShell_UnknownCommandAsksWithUnknownRadius pins the -// fail-closed default: a shell command that matches neither the -// destructive taxonomy nor the safe allowlist still asks, with -// blast_radius=unknown. -func TestSaferShell_UnknownCommandAsksWithUnknownRadius(t *testing.T) { +// A shell command that matches neither destructive nor safe patterns +// produces an Allow verdict with blast_radius=unknown so the runtime +// still sees a label to feed the mode table. +func TestSaferShell_UnknownCommandProducesUnknownRadius(t *testing.T) { t.Parallel() out, err := saferShell(t.Context(), &hooks.Input{ @@ -226,16 +180,12 @@ func TestSaferShell_UnknownCommandAsksWithUnknownRadius(t *testing.T) { require.NoError(t, err) require.NotNil(t, out) require.NotNil(t, out.HookSpecificOutput) - assert.Equal(t, hooks.DecisionAsk, out.HookSpecificOutput.PermissionDecision) + assert.Equal(t, hooks.DecisionAllow, out.HookSpecificOutput.PermissionDecision) assert.Equal(t, "unknown", out.HookSpecificOutput.Metadata[metaBlastRadius]) } -// TestSaferShell_EmptyOrMissingCommandAsksWithUnknown: empty or -// missing cmd / command keys still produce an Ask verdict. The shell -// tool shouldn't be emitting empty cmds in practice, but if the LLM -// does, safer mode wants the user to see the prompt rather than -// rubber-stamping it. -func TestSaferShell_EmptyOrMissingCommandAsksWithUnknown(t *testing.T) { +// Empty or missing cmd / command keys produce Allow + blast_radius=unknown. +func TestSaferShell_EmptyOrMissingCommandProducesUnknown(t *testing.T) { t.Parallel() cases := []map[string]any{ @@ -253,13 +203,12 @@ func TestSaferShell_EmptyOrMissingCommandAsksWithUnknown(t *testing.T) { require.NoError(t, err, "case %d", i) require.NotNil(t, out, "case %d: %v", i, in) require.NotNil(t, out.HookSpecificOutput) - assert.Equal(t, hooks.DecisionAsk, out.HookSpecificOutput.PermissionDecision, "case %d", i) + assert.Equal(t, hooks.DecisionAllow, out.HookSpecificOutput.PermissionDecision, "case %d", i) assert.Equal(t, "unknown", out.HookSpecificOutput.Metadata[metaBlastRadius], "case %d", i) } } -// TestSaferShell_NilInputIsNoOp covers the executor's defensive nil -// passthrough. +// Nil input passes through with no output (defensive). func TestSaferShell_NilInputIsNoOp(t *testing.T) { t.Parallel() @@ -268,10 +217,9 @@ func TestSaferShell_NilInputIsNoOp(t *testing.T) { assert.Nil(t, out) } -// TestSaferShell_ApplyAgentDefaultsAutoInjectsBuiltin pins the YAML -// sugar contract: setting AgentDefaults.SaferShell=true must produce -// a pre_tool_use entry that names the safer_shell builtin and flags -// PreemptYolo so the entry fires before Decide()/--yolo. +// ApplyAgentDefaults must still auto-inject the classifier with +// preempt_yolo:true so its metadata is available to the runtime +// before Decide(). func TestSaferShell_ApplyAgentDefaultsAutoInjectsBuiltin(t *testing.T) { t.Parallel() @@ -283,200 +231,30 @@ func TestSaferShell_ApplyAgentDefaultsAutoInjectsBuiltin(t *testing.T) { "wildcard matcher keeps the hook generic so other pre_tool_use hooks can coexist") require.NotNil(t, mc.PreemptYolo, "preempt_yolo must be set on the auto-injected entry") assert.True(t, *mc.PreemptYolo, - "preempt_yolo must be true so the entry fires before Decide()/--yolo") + "preempt_yolo must be true so metadata reaches the dispatcher before the mode-verdict step") require.Len(t, mc.Hooks, 1) assert.Equal(t, hooks.HookTypeBuiltin, mc.Hooks[0].Type) assert.Equal(t, SaferShell, mc.Hooks[0].Command) } -func TestSaferShell_PolicyMatrix(t *testing.T) { - const destructive = "rm -rf /tmp/x" - const safe = "docker ps" - const unknown = "myproject-cli deploy --prod" - - type want struct { - emit bool - decision hooks.Decision - blastRadius string - } - - cases := []struct { - name string - policy string - cmd string - want want - }{ - {"unsafe + destructive → silent", policyUnsafe, destructive, want{emit: false}}, - {"unsafe + safe → silent", policyUnsafe, safe, want{emit: false}}, - {"unsafe + unknown → silent", policyUnsafe, unknown, want{emit: false}}, - - {"safer + destructive → ask (high)", policySafer, destructive, want{emit: true, decision: hooks.DecisionAsk, blastRadius: "high"}}, - {"safer + safe → silent", policySafer, safe, want{emit: false}}, - {"safer + unknown → silent", policySafer, unknown, want{emit: false}}, - - {"safe-auto + destructive → ask (high)", policySafeAuto, destructive, want{emit: true, decision: hooks.DecisionAsk, blastRadius: "high"}}, - {"safe-auto + safe → allow (safe)", policySafeAuto, safe, want{emit: true, decision: hooks.DecisionAllow, blastRadius: "safe"}}, - {"safe-auto + unknown → ask (unknown)", policySafeAuto, unknown, want{emit: true, decision: hooks.DecisionAsk, blastRadius: "unknown"}}, - - {"strict + destructive → ask (high)", policyStrict, destructive, want{emit: true, decision: hooks.DecisionAsk, blastRadius: "high"}}, - {"strict + safe → ask (safe)", policyStrict, safe, want{emit: true, decision: hooks.DecisionAsk, blastRadius: "safe"}}, - {"strict + unknown → ask (unknown)", policyStrict, unknown, want{emit: true, decision: hooks.DecisionAsk, blastRadius: "unknown"}}, - - {"empty + destructive → ask (high)", "", destructive, want{emit: true, decision: hooks.DecisionAsk, blastRadius: "high"}}, - {"empty + safe → ask (safe)", "", safe, want{emit: true, decision: hooks.DecisionAsk, blastRadius: "safe"}}, - {"empty + unknown → ask (unknown)", "", unknown, want{emit: true, decision: hooks.DecisionAsk, blastRadius: "unknown"}}, - {"unrecognised policy → strict fallback", "future", destructive, want{emit: true, decision: hooks.DecisionAsk, blastRadius: "high"}}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - out, err := saferShell(t.Context(), &hooks.Input{ - HookEventName: hooks.EventPreToolUse, - ToolName: shellToolName, - ToolInput: map[string]any{"cmd": tc.cmd}, - SafetyPolicy: tc.policy, - }, nil) - require.NoError(t, err) - if !tc.want.emit { - assert.Nil(t, out, "policy=%q cmd=%q must be silent", tc.policy, tc.cmd) - return - } - require.NotNil(t, out, "policy=%q cmd=%q must produce a verdict", tc.policy, tc.cmd) - require.NotNil(t, out.HookSpecificOutput) - assert.Equal(t, tc.want.decision, out.HookSpecificOutput.PermissionDecision) - assert.Equal(t, tc.want.blastRadius, out.HookSpecificOutput.Metadata[metaBlastRadius]) - }) - } -} - -// args[0] pins the mode regardless of Input.SafetyPolicy; -// unrecognised args fall through to the session policy. -func TestSaferShell_ArgsOverrideSessionPolicy(t *testing.T) { - cases := []struct { - name string - sessionPolicy string - args []string - cmd string - wantEmit bool - wantRadius string - }{ - { - name: "args=safer overrides unsafe session: destructive gates", - sessionPolicy: policyUnsafe, - args: []string{"safer"}, - cmd: "rm -rf /tmp/x", - wantEmit: true, - wantRadius: "high", - }, - { - name: "args=safer overrides unsafe session: unknown stays silent", - sessionPolicy: policyUnsafe, - args: []string{"safer"}, - cmd: "docker build .", - wantEmit: false, - }, - { - name: "args=safer overrides unsafe session: safe stays silent", - sessionPolicy: policyUnsafe, - args: []string{"safer"}, - cmd: "docker ps", - wantEmit: false, - }, - { - name: "args=safe-auto overrides unsafe session: safe allow", - sessionPolicy: policyUnsafe, - args: []string{"safe-auto"}, - cmd: "docker ps", - wantEmit: true, - wantRadius: "safe", - }, - { - name: "args=safe-auto overrides unsafe session: destructive ask", - sessionPolicy: policyUnsafe, - args: []string{"safe-auto"}, - cmd: "rm -rf /tmp/x", - wantEmit: true, - wantRadius: "high", - }, - { - name: "args=safe-auto overrides unsafe session: unknown ask", - sessionPolicy: policyUnsafe, - args: []string{"safe-auto"}, - cmd: "myproject-cli deploy", - wantEmit: true, - wantRadius: "unknown", - }, - { - name: "args=strict overrides safer session: unknown now gates", - sessionPolicy: policySafer, - args: []string{"strict"}, - cmd: "docker build .", - wantEmit: true, - wantRadius: "unknown", - }, - { - name: "args=unsafe overrides strict session: silent on destructive", - sessionPolicy: policyStrict, - args: []string{"unsafe"}, - cmd: "rm -rf /tmp/x", - wantEmit: false, - }, - { - name: "unrecognised args value falls through to session policy (unsafe → silent)", - sessionPolicy: policyUnsafe, - args: []string{"yolo"}, // typo - cmd: "rm -rf /tmp/x", - wantEmit: false, - }, - { - name: "unrecognised args value falls through to session policy (safer → ask)", - sessionPolicy: policySafer, - args: []string{"yolo"}, // typo - cmd: "rm -rf /tmp/x", - wantEmit: true, - wantRadius: "high", - }, - { - name: "empty args + empty session: strict fallback gates destructive", - sessionPolicy: "", - args: nil, - cmd: "rm -rf /tmp/x", - wantEmit: true, - wantRadius: "high", - }, +// LabelFromBlastRadius maps every wire-format radius onto the runtime +// classifier label. safe → safe; low/medium/high → destructive; +// anything else → unknown. +func TestLabelFromBlastRadius(t *testing.T) { + t.Parallel() + cases := map[string]string{ + "safe": "safe", + "low": "destructive", + "medium": "destructive", + "high": "destructive", + "unknown": "unknown", + "": "unknown", + "garbage": "unknown", } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - out, err := saferShell(t.Context(), &hooks.Input{ - HookEventName: hooks.EventPreToolUse, - ToolName: shellToolName, - ToolInput: map[string]any{"cmd": tc.cmd}, - SafetyPolicy: tc.sessionPolicy, - }, tc.args) - require.NoError(t, err) - if !tc.wantEmit { - assert.Nil(t, out) - return - } - require.NotNil(t, out) - require.NotNil(t, out.HookSpecificOutput) - wantDecision := hooks.DecisionAsk - if tc.wantRadius == "safe" { - wantDecision = hooks.DecisionAllow - } - assert.Equal(t, wantDecision, out.HookSpecificOutput.PermissionDecision) - assert.Equal(t, tc.wantRadius, out.HookSpecificOutput.Metadata[metaBlastRadius]) + for radius, wantLabel := range cases { + t.Run(radius, func(t *testing.T) { + t.Parallel() + assert.Equal(t, wantLabel, LabelFromBlastRadius(radius)) }) } } - -// unsafe short-circuits before the taxonomy loader is called. -func TestSaferShell_UnsafeReturnsBeforeTaxonomyLoad(t *testing.T) { - out, err := saferShell(t.Context(), &hooks.Input{ - HookEventName: hooks.EventPreToolUse, - ToolName: shellToolName, - ToolInput: map[string]any{"cmd": "rm -rf /tmp/x"}, - SafetyPolicy: policyUnsafe, - }, nil) - require.NoError(t, err) - assert.Nil(t, out, "unsafe must short-circuit before classification") -} diff --git a/pkg/hooks/types.go b/pkg/hooks/types.go index 7794ac66ce..298c821c7c 100644 --- a/pkg/hooks/types.go +++ b/pkg/hooks/types.go @@ -348,13 +348,24 @@ type Input struct { // OnToolApprovalDecision specific: the verdict resolved by the // approval chain ("allow", "deny", "canceled") and a stable - // classifier for what produced it ("yolo", - // "session_permissions_allow", "session_permissions_deny", - // "team_permissions_allow", "team_permissions_deny", - // "readonly_hint", "user_approved", "user_approved_session", - // "user_approved_tool", "user_rejected", "context_canceled"). + // classifier for what produced it ("session_permissions_allow", + // "session_permissions_deny", "team_permissions_allow", + // "team_permissions_deny", "pre_tool_use_hook_allow", + // "pre_tool_use_hook_deny", "permission_request_hook_allow", + // "permission_request_hook_deny", "mode_strict", "mode_balanced", + // "mode_autonomous", "user_approved", "user_approved_balanced", + // "user_approved_autonomous", "user_approved_tool", + // "user_rejected", "context_canceled", "non_interactive_deny"). ApprovalDecision string `json:"approval_decision,omitempty"` ApprovalSource string `json:"approval_source,omitempty"` + // SafetyLabel is the safety classifier's verdict for the call + // (safe / destructive / unknown). For shell tools this is + // derived from safer_shell's blast_radius metadata; for other + // tools from readOnlyHint / DestructiveHint annotations. + // Populated on every [EventOnToolApprovalDecision] dispatch so + // audit sinks always see the classification even when the call + // auto-approved under Autonomous. + SafetyLabel string `json:"safety_label,omitempty"` // AfterLLMCall specific: per-turn token usage and the computed USD // cost of the model response the runtime just received. Both are diff --git a/pkg/leantui/update.go b/pkg/leantui/update.go index d2ac8eee0e..a2ba6a33c3 100644 --- a/pkg/leantui/update.go +++ b/pkg/leantui/update.go @@ -443,7 +443,7 @@ func (m *model) handleConfirmKey(k ui.Key) { case 'a', 'A': m.resolveConfirm(runtime.ResumeApproveTool(m.screen.Confirm.Tool)) case 's', 'S': - m.resolveConfirm(runtime.ResumeApproveSession()) + m.resolveConfirm(runtime.ResumeApproveAutonomous()) case 'n', 'N': m.resolveConfirm(runtime.ResumeReject("rejected by user")) } diff --git a/pkg/runtime/agent_delegation_test.go b/pkg/runtime/agent_delegation_test.go index 25fe27f0e7..3df8021877 100644 --- a/pkg/runtime/agent_delegation_test.go +++ b/pkg/runtime/agent_delegation_test.go @@ -305,8 +305,8 @@ func TestSubSessionInheritsPermissions(t *testing.T) { assert.Equal(t, perms.Deny, s.Permissions.Deny) assert.Equal(t, perms.Ask, s.Permissions.Ask) - // Even with ToolsApproved set (yolo), an inherited Deny must win during dispatch. - s.ToolsApproved = true + // Even under Autonomous mode, an inherited Deny must win during dispatch. + s.SetSafetyPolicy(session.SafetyPolicyAutonomous) checker := permissions.NewChecker(&latest.PermissionsConfig{ Allow: s.Permissions.Allow, @@ -317,8 +317,8 @@ func TestSubSessionInheritsPermissions(t *testing.T) { {Checker: checker, Source: "session permissions"}, } - decision := toolexec.Decide(s.ToolsApproved, namedCheckers, "write_file", map[string]any{"path": "foo"}, false) - assert.Equal(t, toolexec.OutcomeDeny, decision.Outcome, "Inherited Deny should override ToolsApproved: true (yolo)") + decision := toolexec.Decide(s.SafetyPolicy, toolexec.SafetyLabelUnknown, namedCheckers, "write_file", map[string]any{"path": "foo"}) + assert.Equal(t, toolexec.OutcomeDeny, decision.Outcome, "Inherited Deny should override Autonomous mode") } func TestNewSubSession_PermissionsIsolation(t *testing.T) { diff --git a/pkg/runtime/hooks.go b/pkg/runtime/hooks.go index 8e3c776ead..88b444634a 100644 --- a/pkg/runtime/hooks.go +++ b/pkg/runtime/hooks.go @@ -456,38 +456,44 @@ const ( ApprovalDecisionDeny = "deny" ApprovalDecisionCanceled = "canceled" - ApprovalSourceYolo = "yolo" ApprovalSourceSessionPermissionsAllow = "session_permissions_allow" ApprovalSourceSessionPermissionsDeny = "session_permissions_deny" ApprovalSourceTeamPermissionsAllow = "team_permissions_allow" ApprovalSourceTeamPermissionsDeny = "team_permissions_deny" ApprovalSourcePreToolUseHookAllow = "pre_tool_use_hook_allow" ApprovalSourcePreToolUseHookDeny = "pre_tool_use_hook_deny" - ApprovalSourceReadOnlyHint = "readonly_hint" - ApprovalSourceUserApproved = "user_approved" - ApprovalSourceUserApprovedSession = "user_approved_session" - ApprovalSourceUserApprovedSafe = "user_approved_safe" - ApprovalSourceUserApprovedSafer = "user_approved_safer" - ApprovalSourceUserApprovedTool = "user_approved_tool" - ApprovalSourceUserRejected = "user_rejected" - ApprovalSourceContextCanceled = "context_canceled" + // ApprovalSourceModeStrict / ModeBalanced / ModeAutonomous are + // recorded when the (mode × classifier-label) table produced the + // verdict (i.e. no custom rule matched). + ApprovalSourceModeStrict = "mode_strict" + ApprovalSourceModeBalanced = "mode_balanced" + ApprovalSourceModeAutonomous = "mode_autonomous" + ApprovalSourceUserApproved = "user_approved" + ApprovalSourceUserApprovedBalanced = "user_approved_balanced" + ApprovalSourceUserApprovedAutonomous = "user_approved_autonomous" + ApprovalSourceUserApprovedTool = "user_approved_tool" + ApprovalSourceUserRejected = "user_rejected" + ApprovalSourceContextCanceled = "context_canceled" ) // executeOnToolApprovalDecisionHooks fires on_tool_approval_decision // after the runtime's approval chain has resolved a verdict for a // tool call. Fired once per call from each return path of // [executeWithApproval], so a single hook gets one record per tool -// call regardless of which step decided. +// call regardless of which step decided. safetyLabel carries the +// classifier's verdict for the call (safe / destructive / unknown); +// empty when classification didn't run before the decision. func (r *LocalRuntime) executeOnToolApprovalDecisionHooks( ctx context.Context, sess *session.Session, a *agent.Agent, toolCall tools.ToolCall, - decision, source string, + decision, source, safetyLabel string, ) { input := toolexec.NewHooksInput(sess, toolCall) input.ApprovalDecision = decision input.ApprovalSource = source + input.SafetyLabel = safetyLabel r.dispatchHook(ctx, a, hooks.EventOnToolApprovalDecision, input, nil) } diff --git a/pkg/runtime/on_tool_approval_decision_test.go b/pkg/runtime/on_tool_approval_decision_test.go index 9e2db170b1..5106b8e696 100644 --- a/pkg/runtime/on_tool_approval_decision_test.go +++ b/pkg/runtime/on_tool_approval_decision_test.go @@ -64,7 +64,7 @@ func TestExecuteOnToolApprovalDecisionHooks_ForwardsVerdictAndSource(t *testing. Arguments: `{"path":"/tmp/x"}`, }, } - r.executeOnToolApprovalDecisionHooks(t.Context(), sess, a, tc, ApprovalDecisionAllow, ApprovalSourceReadOnlyHint) + r.executeOnToolApprovalDecisionHooks(t.Context(), sess, a, tc, ApprovalDecisionAllow, ApprovalSourceModeBalanced, "safe") got := rb.snapshot() require.Len(t, got, 1) @@ -72,7 +72,7 @@ func TestExecuteOnToolApprovalDecisionHooks_ForwardsVerdictAndSource(t *testing. assert.Equal(t, "read_file", in.ToolName) assert.Equal(t, "call-1", in.ToolUseID) assert.Equal(t, ApprovalDecisionAllow, in.ApprovalDecision) - assert.Equal(t, ApprovalSourceReadOnlyHint, in.ApprovalSource) + assert.Equal(t, ApprovalSourceModeBalanced, in.ApprovalSource) } // TestApprovalSourceMappersAreStable pins the stable classifier diff --git a/pkg/runtime/resume.go b/pkg/runtime/resume.go index e7f318a373..3826cea70e 100644 --- a/pkg/runtime/resume.go +++ b/pkg/runtime/resume.go @@ -16,17 +16,14 @@ type ResumeType = toolexec.ResumeType const ( // ResumeTypeApprove approves the single pending tool call. ResumeTypeApprove = toolexec.ResumeTypeApprove - // ResumeTypeApproveSession approves the pending tool call and every - // subsequent permission-gated call for the rest of the session. - ResumeTypeApproveSession = toolexec.ResumeTypeApproveSession - // ResumeTypeApproveSafe approves the pending call and flips the - // session to SafetyPolicySafeAuto so subsequent classifier-verified - // safe calls auto-approve. - ResumeTypeApproveSafe = toolexec.ResumeTypeApproveSafe - // ResumeTypeApproveSafer approves the pending call and flips the - // session to SafetyPolicySafer so subsequent non-destructive calls - // auto-approve (superset of safe-auto — includes unclassified). - ResumeTypeApproveSafer = toolexec.ResumeTypeApproveSafer + // ResumeTypeApproveBalanced approves the pending call and flips the + // session to [session.SafetyPolicyBalanced] so subsequent safe + // tool calls auto-approve. + ResumeTypeApproveBalanced = toolexec.ResumeTypeApproveBalanced + // ResumeTypeApproveAutonomous approves the pending call and flips + // the session to [session.SafetyPolicyAutonomous] so subsequent + // tool calls auto-approve. + ResumeTypeApproveAutonomous = toolexec.ResumeTypeApproveAutonomous // ResumeTypeApproveTool approves the pending call and every future // call to the same tool name within the session. ResumeTypeApproveTool = toolexec.ResumeTypeApproveTool @@ -45,21 +42,16 @@ func ResumeApprove() ResumeRequest { return ResumeRequest{Type: ResumeTypeApprove} } -// ResumeApproveSession creates a ResumeRequest to approve all tool calls for the session. -func ResumeApproveSession() ResumeRequest { - return ResumeRequest{Type: ResumeTypeApproveSession} +// ResumeApproveBalanced creates a ResumeRequest that approves the pending +// call and flips the session to [session.SafetyPolicyBalanced]. +func ResumeApproveBalanced() ResumeRequest { + return ResumeRequest{Type: ResumeTypeApproveBalanced} } -// ResumeApproveSafe creates a ResumeRequest that approves the pending -// call and flips the session to SafetyPolicySafeAuto. -func ResumeApproveSafe() ResumeRequest { - return ResumeRequest{Type: ResumeTypeApproveSafe} -} - -// ResumeApproveSafer creates a ResumeRequest that approves the pending -// call and flips the session to SafetyPolicySafer. -func ResumeApproveSafer() ResumeRequest { - return ResumeRequest{Type: ResumeTypeApproveSafer} +// ResumeApproveAutonomous creates a ResumeRequest that approves the pending +// call and flips the session to [session.SafetyPolicyAutonomous]. +func ResumeApproveAutonomous() ResumeRequest { + return ResumeRequest{Type: ResumeTypeApproveAutonomous} } // ResumeApproveTool creates a ResumeRequest to always approve a specific tool for the session. @@ -81,9 +73,8 @@ func ResumeReject(reason string) ResumeRequest { func IsValidResumeType(t ResumeType) bool { switch t { case ResumeTypeApprove, - ResumeTypeApproveSession, - ResumeTypeApproveSafe, - ResumeTypeApproveSafer, + ResumeTypeApproveBalanced, + ResumeTypeApproveAutonomous, ResumeTypeApproveTool, ResumeTypeReject: return true @@ -96,9 +87,8 @@ func IsValidResumeType(t ResumeType) bool { func ValidResumeTypes() []ResumeType { return []ResumeType{ ResumeTypeApprove, - ResumeTypeApproveSession, - ResumeTypeApproveSafe, - ResumeTypeApproveSafer, + ResumeTypeApproveBalanced, + ResumeTypeApproveAutonomous, ResumeTypeApproveTool, ResumeTypeReject, } diff --git a/pkg/runtime/resume_test.go b/pkg/runtime/resume_test.go index 7e78b3bf3c..68717a9f93 100644 --- a/pkg/runtime/resume_test.go +++ b/pkg/runtime/resume_test.go @@ -15,13 +15,15 @@ func TestIsValidResumeType(t *testing.T) { want bool }{ {"approve", ResumeTypeApprove, true}, - {"approve-session", ResumeTypeApproveSession, true}, - {"approve-safe", ResumeTypeApproveSafe, true}, - {"approve-safer", ResumeTypeApproveSafer, true}, + {"approve-balanced", ResumeTypeApproveBalanced, true}, + {"approve-autonomous", ResumeTypeApproveAutonomous, true}, {"approve-tool", ResumeTypeApproveTool, true}, {"reject", ResumeTypeReject, true}, {"empty", ResumeType(""), false}, {"unknown", ResumeType("yolo"), false}, + {"legacy-approve-session", ResumeType("approve-session"), false}, + {"legacy-approve-safe", ResumeType("approve-safe"), false}, + {"legacy-approve-safer", ResumeType("approve-safer"), false}, } for _, tt := range tests { @@ -37,16 +39,14 @@ func TestValidResumeTypes(t *testing.T) { got := ValidResumeTypes() - // Every returned type must round-trip through IsValidResumeType. for _, rt := range got { assert.Truef(t, IsValidResumeType(rt), "ValidResumeTypes() returned %q which IsValidResumeType rejects", rt) } assert.ElementsMatch(t, []ResumeType{ ResumeTypeApprove, - ResumeTypeApproveSession, - ResumeTypeApproveSafe, - ResumeTypeApproveSafer, + ResumeTypeApproveBalanced, + ResumeTypeApproveAutonomous, ResumeTypeApproveTool, ResumeTypeReject, }, got) @@ -63,26 +63,18 @@ func TestResumeApproveHelpers(t *testing.T) { assert.Empty(t, r.ToolName) }) - t.Run("approve-session", func(t *testing.T) { + t.Run("approve-balanced", func(t *testing.T) { t.Parallel() - r := ResumeApproveSession() - assert.Equal(t, ResumeTypeApproveSession, r.Type) + r := ResumeApproveBalanced() + assert.Equal(t, ResumeTypeApproveBalanced, r.Type) assert.Empty(t, r.Reason) assert.Empty(t, r.ToolName) }) - t.Run("approve-safe", func(t *testing.T) { + t.Run("approve-autonomous", func(t *testing.T) { t.Parallel() - r := ResumeApproveSafe() - assert.Equal(t, ResumeTypeApproveSafe, r.Type) - assert.Empty(t, r.Reason) - assert.Empty(t, r.ToolName) - }) - - t.Run("approve-safer", func(t *testing.T) { - t.Parallel() - r := ResumeApproveSafer() - assert.Equal(t, ResumeTypeApproveSafer, r.Type) + r := ResumeApproveAutonomous() + assert.Equal(t, ResumeTypeApproveAutonomous, r.Type) assert.Empty(t, r.Reason) assert.Empty(t, r.ToolName) }) diff --git a/pkg/runtime/runtime_test.go b/pkg/runtime/runtime_test.go index 0033b84638..eec30b4074 100644 --- a/pkg/runtime/runtime_test.go +++ b/pkg/runtime/runtime_test.go @@ -2627,19 +2627,68 @@ func TestDenyOverridesYoloMode(t *testing.T) { require.False(t, executed, "expected tool to NOT be executed in --yolo mode because Deny wins") } -// TestYoloMode_OverridesForceAsk verifies that the yolo flag takes precedence over ForceAsk permissions. -func TestYoloMode_OverridesForceAsk(t *testing.T) { +// TestSessionForceAsk_BeatsAutonomous verifies that a session-level +// ask: pattern (from the user's Custom rules UI) still forces a +// confirmation even under Autonomous mode. Team-level ask patterns +// from YAML are advisory — see TestTeamForceAsk_YieldsToAutonomous +// below. +func TestSessionForceAsk_BeatsAutonomous(t *testing.T) { + t.Parallel() + + var executed bool + agentTools := []tools.Tool{{ + Name: "careful_tool", + Parameters: map[string]any{}, + Handler: func(_ context.Context, _ tools.ToolCall, _ tools.Runtime) (*tools.ToolCallResult, error) { + executed = true + return tools.ResultSuccess("executed"), nil + }, + }} + + prov := &mockProvider{id: "test/mock-model", stream: &mockStream{}} + root := agent.New("root", "You are a test agent", + agent.WithModel(prov), + agent.WithToolSets(newStubToolSet(nil, agentTools, nil)), + ) + tm := team.New(team.WithAgents(root)) + + rt, err := NewLocalRuntime(t.Context(), tm, WithSessionCompaction(false), WithModelStore(mockModelStore{})) + require.NoError(t, err) + + sess := session.New(session.WithUserMessage("Test"), session.WithToolsApproved(true)) + sess.Permissions = &session.PermissionsConfig{Ask: []string{"careful_tool"}} + sess.NonInteractive = true // ForceAsk in non-interactive → auto-deny (no hang, no run) + + calls := []tools.ToolCall{{ + ID: "call_1", + Type: "function", + Function: tools.FunctionCall{Name: "careful_tool", Arguments: "{}"}, + }} + + events := make(chan Event, 10) + rt.processToolCalls(t.Context(), sess, calls, agentTools, NewChannelSink(events)) + close(events) + + require.False(t, executed, "session-level ask: pattern must beat Autonomous") +} + +// TestTeamForceAsk_YieldsToAutonomous verifies that a team-level +// ask: pattern (from the agent YAML) is advisory: it forces asking +// only when the mode itself would ask. Under Autonomous, the mode +// wins so agents shipping `ask: "*"` in YAML don't defeat the user's +// mode choice. +func TestTeamForceAsk_YieldsToAutonomous(t *testing.T) { t.Parallel() - // Test that --yolo flag takes precedence over ForceAsk permissions permChecker := permissions.NewChecker(&latest.PermissionsConfig{ Ask: []string{"careful_tool"}, }) var executed bool agentTools := []tools.Tool{{ - Name: "careful_tool", - Parameters: map[string]any{}, + Name: "careful_tool", + Parameters: map[string]any{}, + Annotations: tools.ToolAnnotations{ReadOnlyHint: true}, Handler: func(_ context.Context, _ tools.ToolCall, _ tools.Runtime) (*tools.ToolCallResult, error) { executed = true return tools.ResultSuccess("executed"), nil @@ -2660,8 +2709,6 @@ func TestYoloMode_OverridesForceAsk(t *testing.T) { require.NoError(t, err) sess := session.New(session.WithUserMessage("Test"), session.WithToolsApproved(true)) - sess.NonInteractive = true // fail fast instead of hanging on askUser if this regresses - require.True(t, sess.ToolsApproved) calls := []tools.ToolCall{{ ID: "call_1", @@ -2673,9 +2720,7 @@ func TestYoloMode_OverridesForceAsk(t *testing.T) { rt.processToolCalls(t.Context(), sess, calls, agentTools, NewChannelSink(events)) close(events) - // YOLO overrides ForceAsk: the checker's ForceAsk verdict is bypassed - // and the tool executes automatically. - require.True(t, executed, "expected tool to be executed in --yolo mode because YOLO wins over ForceAsk") + require.True(t, executed, "team-level ask: pattern must yield to Autonomous") } // TestSessionDenyOverridesYoloMode verifies that session-level Deny permissions take precedence over the yolo flag. @@ -3399,7 +3444,7 @@ func TestReprobe_NewToolsAvailableAfterToolCall(t *testing.T) { sess := session.New(session.WithUserMessage("Install and use MCP")) sess.Title = "reprobe test" - sess.ToolsApproved = true + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) evCh := rt.RunStream(t.Context(), sess) var events []Event @@ -3476,7 +3521,7 @@ func TestReprobe_NoChangeMeansNoExtraEvents(t *testing.T) { sess := session.New(session.WithUserMessage("Do the thing")) sess.Title = "no-change reprobe test" - sess.ToolsApproved = true + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) evCh := rt.RunStream(t.Context(), sess) var events []Event @@ -4684,7 +4729,7 @@ func TestEmptyTrailingTurnAfterToolCallsIsSilent(t *testing.T) { sess := session.New(session.WithUserMessage("Do the thing")) sess.Title = "empty trailing turn test" - sess.ToolsApproved = true + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) evCh := rt.RunStream(t.Context(), sess) var events []Event diff --git a/pkg/runtime/tool_dispatch.go b/pkg/runtime/tool_dispatch.go index 3f78bcf6a2..8c6dad9d9f 100644 --- a/pkg/runtime/tool_dispatch.go +++ b/pkg/runtime/tool_dispatch.go @@ -65,7 +65,7 @@ func (r *LocalRuntime) permissionCheckers(sess *session.Session) []toolexec.Name Ask: perms.Ask, Deny: perms.Deny, }), - Source: "session permissions", + Source: toolexec.SessionPermissionsSource, }) } if tc := r.team.Permissions(); tc != nil { @@ -126,8 +126,8 @@ func (h *hookDispatcher) NotifyUserInput(ctx context.Context, sessionID, label s h.r.executeOnUserInputHooks(ctx, sessionID, label) } -func (h *hookDispatcher) NotifyApprovalDecision(ctx context.Context, sess *session.Session, a *agent.Agent, tc tools.ToolCall, decision, source string) { - h.r.executeOnToolApprovalDecisionHooks(ctx, sess, a, tc, decision, source) +func (h *hookDispatcher) NotifyApprovalDecision(ctx context.Context, sess *session.Session, a *agent.Agent, tc tools.ToolCall, decision, source, safetyLabel string) { + h.r.executeOnToolApprovalDecisionHooks(ctx, sess, a, tc, decision, source, safetyLabel) } // allowSourceFor maps a permission-checker source label to the diff --git a/pkg/runtime/toolexec/dispatcher.go b/pkg/runtime/toolexec/dispatcher.go index fd0d66cb22..b2a23cf120 100644 --- a/pkg/runtime/toolexec/dispatcher.go +++ b/pkg/runtime/toolexec/dispatcher.go @@ -19,6 +19,7 @@ import ( "github.com/docker/docker-agent/pkg/chat" "github.com/docker/docker-agent/pkg/concurrent" "github.com/docker/docker-agent/pkg/hooks" + "github.com/docker/docker-agent/pkg/hooks/builtins" "github.com/docker/docker-agent/pkg/permissions" "github.com/docker/docker-agent/pkg/session" "github.com/docker/docker-agent/pkg/telemetry" @@ -34,7 +35,6 @@ const ( ApprovalDecisionDeny = "deny" ApprovalDecisionCanceled = "canceled" - ApprovalSourceYolo = "yolo" ApprovalSourceSessionPermissionsAllow = "session_permissions_allow" ApprovalSourceSessionPermissionsDeny = "session_permissions_deny" ApprovalSourceTeamPermissionsAllow = "team_permissions_allow" @@ -43,14 +43,21 @@ const ( ApprovalSourcePreToolUseHookDeny = "pre_tool_use_hook_deny" ApprovalSourcePermissionRequestHookDeny = "permission_request_hook_deny" ApprovalSourcePermissionRequestHookAllow = "permission_request_hook_allow" - ApprovalSourceReadOnlyHint = "readonly_hint" - ApprovalSourceUserApproved = "user_approved" - ApprovalSourceUserApprovedSession = "user_approved_session" - ApprovalSourceUserApprovedSafe = "user_approved_safe" - ApprovalSourceUserApprovedSafer = "user_approved_safer" - ApprovalSourceUserApprovedTool = "user_approved_tool" - ApprovalSourceUserRejected = "user_rejected" - ApprovalSourceContextCanceled = "context_canceled" + // ApprovalSourceModeStrict / ModeBalanced / ModeAutonomous are + // recorded when the (mode × classifier-label) table produced the + // verdict (i.e. no custom rule matched). + ApprovalSourceModeStrict = "mode_strict" + ApprovalSourceModeBalanced = "mode_balanced" + ApprovalSourceModeAutonomous = "mode_autonomous" + ApprovalSourceUserApproved = "user_approved" + // ApprovalSourceUserApprovedBalanced / Autonomous are recorded + // when the user picked a bulk-approve verb from a confirmation + // prompt that also flipped the session's SafetyPolicy. + ApprovalSourceUserApprovedBalanced = "user_approved_balanced" + ApprovalSourceUserApprovedAutonomous = "user_approved_autonomous" + ApprovalSourceUserApprovedTool = "user_approved_tool" + ApprovalSourceUserRejected = "user_rejected" + ApprovalSourceContextCanceled = "context_canceled" // ApprovalSourceNonInteractiveDeny is recorded when a tool call // reaches [call.askUser] in a non-interactive session (eval, MCP // serve, A2A adapter, …). With no human at the keyboard and no @@ -110,10 +117,12 @@ type HookDispatcher interface { // NotifyApprovalDecision is invoked once per tool call after the // approval pipeline (auto-allow, deny, user confirmation, ...) has // resolved a verdict. Implementations typically fire - // [hooks.EventOnToolApprovalDecision] with decision and source set - // to the supplied strings (see ApprovalDecision* / ApprovalSource* - // constants). - NotifyApprovalDecision(ctx context.Context, sess *session.Session, a *agent.Agent, tc tools.ToolCall, decision, source string) + // [hooks.EventOnToolApprovalDecision] with decision, source, and + // the classifier's safety label set to the supplied strings (see + // ApprovalDecision* / ApprovalSource* constants and + // SafetyLabel*). safetyLabel may be empty for calls that reach + // approval before classification runs (e.g. context cancellation). + NotifyApprovalDecision(ctx context.Context, sess *session.Session, a *agent.Agent, tc tools.ToolCall, decision, source, safetyLabel string) } // ToolHandler is the signature for runtime-managed tool handlers @@ -137,12 +146,20 @@ type ResumeRequest struct { type ResumeType string const ( - ResumeTypeApprove ResumeType = "approve" - ResumeTypeApproveSession ResumeType = "approve-session" - ResumeTypeApproveSafe ResumeType = "approve-safe" - ResumeTypeApproveSafer ResumeType = "approve-safer" - ResumeTypeApproveTool ResumeType = "approve-tool" - ResumeTypeReject ResumeType = "reject" + // ResumeTypeApprove approves the single pending tool call. + ResumeTypeApprove ResumeType = "approve" + // ResumeTypeApproveBalanced approves the pending call and flips + // the session to [session.SafetyPolicyBalanced]. + ResumeTypeApproveBalanced ResumeType = "approve-balanced" + // ResumeTypeApproveAutonomous approves the pending call and flips + // the session to [session.SafetyPolicyAutonomous]. + ResumeTypeApproveAutonomous ResumeType = "approve-autonomous" + // ResumeTypeApproveTool approves the pending call and appends the + // tool (or supplied pattern) to the session's Allow list. + ResumeTypeApproveTool ResumeType = "approve-tool" + // ResumeTypeReject rejects the pending tool call, with an + // optional reason surfaced to the model. + ResumeTypeReject ResumeType = "reject" ) // Dispatcher executes batches of tool calls. Construct one per runtime @@ -347,38 +364,21 @@ func (c *call) run(ctx context.Context) CallOutcome { return outcome } -// approveAndRun runs runTool if the configured approval pipeline allows -// it, otherwise records an error or asks the user. +// approveAndRun runs runTool if the approval pipeline allows it. +// Pipeline: // -// The pipeline order is: -// -// 0. pre_tool_use entries with preempt_yolo:true — fire BEFORE the -// deterministic checkers so a Deny or Ask verdict here preempts -// --yolo and permission allow rules. Allow / no-opinion fall -// through. The safer_shell builtin is the canonical user; its -// verdict's metadata is surfaced to the confirmation event via -// [call.confirmationMetadata]. -// 1. yolo / permission checkers (delegated to [Decide]) — deterministic -// verdicts win first. ForceAsk goes straight to the user. -// 2. pre_tool_use hooks (LLM-judge, shell scripts, ...) — the -// default lane, consulted ONLY when no deterministic checker -// matched. The hook can Deny (block), Allow (skip the user -// prompt) or Ask (force the prompt). Hooks may also rewrite tool -// arguments via UpdatedInput, in which case the rewrite is -// applied here so the user prompt and the tool handler both see -// the modified call. -// 3. read-only hint — auto-approve when the tool advertises it. -// 4. user confirmation — fallback prompt. -// -// Splitting the read-only hint out of [Decide] is deliberate: it lets -// the pre_tool_use hook see (and override) calls that would otherwise -// auto-approve via the read-only hint. This matches the PR's intent -// that an LLM judge gets a turn on every call that isn't covered by an -// explicit allow/deny rule. +// 0. preempt_yolo pre_tool_use — safer_shell (labeller only); +// user-authored hooks may Deny/Ask to override every stage below. +// 1. Decide — custom Deny/Allow/ForceAsk win outright; else +// (mode × classifier-label) → allow/ask. +// 2. default pre_tool_use — consulted on Ask+ReasonMode; may +// Deny/Allow/Ask and rewrite args via UpdatedInput. +// 3. askUser — prompt (or auto-deny non-interactively). func (c *call) approveAndRun(ctx context.Context, runTool func() CallOutcome) CallOutcome { - // Stage 0: pre_tool_use entries flagged with preempt_yolo:true fire - // before the deterministic checkers so destructive-command - // verdicts can preempt --yolo / permissions. + // Stage 0: pre_tool_use entries flagged with preempt_yolo:true. + // The safer_shell classifier lives here as a pure labeller (always + // Allow + metadata); user-authored hooks can still Deny/Ask to + // override every downstream stage. if r := c.consultPreToolUsePreYolo(ctx); r != nil { switch r.Decision { case hooks.DecisionDeny: @@ -394,10 +394,7 @@ func (c *call) approveAndRun(ctx context.Context, runTool func() CallOutcome) Ca // A session-scoped allow grant (the interactive "T = always // allow this tool" decision, stored in sess.Permissions) is an // informed opt-in the user made in response to this very safety - // prompt. Honor it instead of asking again, otherwise "always - // allow" would re-prompt on every matching call. The blanket - // --yolo and the team/config permission layer stay subordinate - // to the preempt-yolo verdict — see [call.sessionPermissionsAllow]. + // prompt. Honor it instead of asking again. if c.sessionPermissionsAllow() { slog.DebugContext(ctx, "preempt-yolo Ask overridden by session permission allow", "tool", c.tc.Function.Name, "session_id", c.sess.ID) c.notifyApproval(ctx, ApprovalDecisionAllow, ApprovalSourceSessionPermissionsAllow) @@ -408,17 +405,8 @@ func (c *call) approveAndRun(ctx context.Context, runTool func() CallOutcome) Ca // DecisionAllow / "" → advisory; fall through to Decide(). } - // safe-auto / safer opt-ins bypass the permissions layer for the - // calls each policy considers auto-approvable. Otherwise - // permissions.ask: "*" would defeat the opt-in. - if source, ok := c.policyAutoApprove(); ok { - c.notifyApproval(ctx, ApprovalDecisionAllow, source) - return runTool() - } - - // readOnlyHint is intentionally false here so the pre_tool_use hook - // gets a turn before the read-only fast-path applies. - decision := c.permissionDecision(false) + // Stage 1: custom rules + safety mode. + decision := c.permissionDecision() switch decision.Outcome { case OutcomeAllow: @@ -434,51 +422,77 @@ func (c *call) approveAndRun(ctx context.Context, runTool func() CallOutcome) Ca if decision.Reason == ReasonChecker { // Explicit ask pattern from a checker: skip the hook and // prompt the user directly. The user is the source of - // truth for these calls. + // truth for explicit ask rules. slog.DebugContext(ctx, "Tool requires confirmation (ask pattern)", "tool", c.tc.Function.Name, "source", decision.Source, "session_id", c.sess.ID) return c.askUser(ctx, runTool) } } - // No deterministic verdict: consult the pre_tool_use hook chain. + // Stage 2: consult the default pre_tool_use hook chain (non-preempt). if outcome, handled := c.consultPreToolUseHook(ctx, runTool); handled { return outcome } - if c.tool.Annotations.ReadOnlyHint { - c.notifyApproval(ctx, ApprovalDecisionAllow, ApprovalSourceReadOnlyHint) - return runTool() - } + // Stage 3: fallback prompt. return c.askUser(ctx, runTool) } -func (c *call) permissionDecision(readOnlyHint bool) PermissionDecision { +// permissionDecision applies the runtime approval pipeline for this +// call: custom checkers first (Deny / Allow / ForceAsk always win), +// then the (mode × classifier-label) verdict table. See [Decide] for +// the full semantics. +func (c *call) permissionDecision() PermissionDecision { var checkers []NamedChecker if c.d.Permissions != nil { checkers = c.d.Permissions(c.sess) } - return Decide( - c.sess.IsToolsApproved(), + label := c.classifierLabel() + d := Decide( + c.sess.SafetyPolicy, + label, checkers, c.tc.Function.Name, ParseToolInput(c.tc.Function.Arguments), - readOnlyHint, ) + slog.Debug("permissionDecision", + "session_id", c.sess.ID, + "tool", c.tc.Function.Name, + "safety_policy", string(c.sess.SafetyPolicy), + "label", label, + "outcome", d.Outcome, + "reason", d.Reason, + "source", d.Source) + return d +} + +// classifierLabel returns the safety label the mode table consumes: +// safer_shell's blast_radius for shell, MCP annotations otherwise. +func (c *call) classifierLabel() string { + if c.tc.Function.Name == shellToolName { + if c.preYoloResult != nil { + if radius := c.preYoloResult.Metadata["blast_radius"]; radius != "" { + return builtins.LabelFromBlastRadius(radius) + } + } + return SafetyLabelUnknown + } + return LabelWithDestructiveHint(c.tool.Annotations.ReadOnlyHint, c.tool.Annotations.DestructiveHint) } func (c *call) autoApprovalAfterConfirmationWait() (PermissionDecision, bool) { if c.preYoloResult != nil && c.preYoloResult.Decision == hooks.DecisionAsk { - // Even under a preempt-yolo Ask, a session-scoped allow grant that + // Under a preempt-yolo Ask a session-scoped allow grant that // landed while we were blocked on the resume channel (e.g. a // concurrent "always allow this tool" decision) is an informed // opt-in and takes effect. Mirrors approveAndRun's Stage 0: the - // blanket --yolo and the team/config layer stay subordinate. + // team/config layer stays subordinate to the preempt-yolo + // verdict. if c.sessionPermissionsAllow() { - return PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonChecker, Source: sessionPermissionsSource}, true + return PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonChecker, Source: SessionPermissionsSource}, true } return PermissionDecision{}, false } - decision := c.permissionDecision(c.tool.Annotations.ReadOnlyHint) + decision := c.permissionDecision() return decision, decision.Outcome == OutcomeAllow } @@ -517,30 +531,6 @@ func (c *call) sessionPermissionsAllow() bool { return true } -// policyAutoApprove returns (source, true) when the session's safety -// policy allows this call to skip the permissions layer. Under -// safe-auto that requires a positive safe verdict (preempt-yolo Allow -// or ReadOnlyHint). Under safer it just requires the tool not to -// declare itself destructive — destructive preempt-yolo verdicts -// already short-circuited to askUser at Stage 0. -func (c *call) policyAutoApprove() (string, bool) { - switch c.sess.SafetyPolicy { - case session.SafetyPolicySafeAuto: - if c.preYoloResult != nil && c.preYoloResult.Decision == hooks.DecisionAllow { - return ApprovalSourcePreToolUseHookAllow, true - } - if c.tool.Annotations.ReadOnlyHint { - return ApprovalSourceReadOnlyHint, true - } - case session.SafetyPolicySafer: - if c.tool.Annotations.DestructiveHint != nil && *c.tool.Annotations.DestructiveHint { - return "", false - } - return ApprovalSourcePreToolUseHookAllow, true - } - return "", false -} - func (c *call) cancellationMessage(ctx context.Context) string { switch { case errors.Is(context.Cause(ctx), errBatchCanceledByUser): @@ -649,67 +639,61 @@ func (c *call) applyHookModifiedInput(result *hooks.Result) { // notifyApproval forwards the resolved approval decision to the // HookDispatcher, when one is configured. Also stamps the decision + -// source on the active runtime.tool.call span so denied / canceled -// calls are visible in trace dashboards (without it, denied tool calls -// are indistinguishable from user-canceled ones at the span level). +// source + safety label on the active runtime.tool.call span so +// denied / canceled calls are visible in trace dashboards (without +// it, denied tool calls are indistinguishable from user-canceled +// ones at the span level). func (c *call) notifyApproval(ctx context.Context, decision, source string) { + label := c.classifierLabel() if span := trace.SpanFromContext(ctx); span.IsRecording() { span.SetAttributes( attribute.String("cagent.approval.decision", decision), attribute.String("cagent.approval.source", source), ) + if label != "" { + span.SetAttributes(attribute.String("cagent.approval.safety_label", label)) + } } if c.d.Hooks == nil { return } - c.d.Hooks.NotifyApprovalDecision(ctx, c.sess, c.a, c.tc, decision, source) + c.d.Hooks.NotifyApprovalDecision(ctx, c.sess, c.a, c.tc, decision, source, label) } // logAllow emits the auto-approval debug log appropriate to the reason // that produced the [OutcomeAllow] decision. func (c *call) logAllow(d PermissionDecision) { switch d.Reason { - case ReasonYolo: - slog.Debug("Tool auto-approved by --yolo flag", "tool", c.tc.Function.Name, "session_id", c.sess.ID) case ReasonChecker: slog.Debug("Tool auto-approved by permissions", "tool", c.tc.Function.Name, "source", d.Source, "session_id", c.sess.ID) - // ReasonReadOnlyHint is intentionally silent (matches prior behaviour). + case ReasonMode: + slog.Debug("Tool auto-approved by safety mode", "tool", c.tc.Function.Name, "mode", d.Source, "session_id", c.sess.ID) } } // allowSourceForDecision maps a [PermissionDecision] with [OutcomeAllow] -// onto the corresponding ApprovalSource* constant. +// onto the corresponding ApprovalSource* constant. ReasonMode carries +// the source verbatim (applyMode fills it with an ApprovalSourceMode* +// constant); ReasonChecker goes through allowSourceForChecker. func allowSourceForDecision(d PermissionDecision) string { - switch d.Reason { - case ReasonYolo: - return ApprovalSourceYolo - case ReasonReadOnlyHint: - return ApprovalSourceReadOnlyHint - case ReasonChecker: - return allowSourceForChecker(d.Source) + if d.Reason == ReasonMode { + return d.Source } return allowSourceForChecker(d.Source) } -// sessionPermissionsSource is the checker source label the runtime -// attaches to the session-scoped permission checker (see -// pkg/runtime.permissionCheckers). It distinguishes interactive / -// session-scoped grants from the team/config layer. -const sessionPermissionsSource = "session permissions" - -// allowSourceForChecker maps a checker source label ("session permissions" -// or "permissions configuration") onto the corresponding ApprovalSource* -// allow constant. +// allowSourceForChecker maps a checker source label to its +// ApprovalSource* allow constant. func allowSourceForChecker(checkerSource string) string { - if checkerSource == sessionPermissionsSource { + if checkerSource == SessionPermissionsSource { return ApprovalSourceSessionPermissionsAllow } return ApprovalSourceTeamPermissionsAllow } -// denySourceForChecker mirrors allowSourceForChecker for the deny path. +// denySourceForChecker mirrors allowSourceForChecker for deny. func denySourceForChecker(checkerSource string) string { - if checkerSource == sessionPermissionsSource { + if checkerSource == SessionPermissionsSource { return ApprovalSourceSessionPermissionsDeny } return ApprovalSourceTeamPermissionsDeny @@ -884,28 +868,23 @@ func blastRadiusFromAnnotations(a tools.ToolAnnotations) map[string]string { } // handleResume applies the user's confirmation decision: run the tool -// (with optional session/tool-wide approval persistence) or emit a -// rejection error response. +// (with optional session-scoped mode / allow-list persistence) or +// emit a rejection error response. func (c *call) handleResume(ctx context.Context, req ResumeRequest, runTool func() CallOutcome) CallOutcome { switch req.Type { case ResumeTypeApprove: slog.DebugContext(ctx, "Resume signal received, approving tool", "tool", c.tc.Function.Name, "session_id", c.sess.ID) c.notifyApproval(ctx, ApprovalDecisionAllow, ApprovalSourceUserApproved) return runTool() - case ResumeTypeApproveSession: - slog.DebugContext(ctx, "Resume signal received, approving session", "tool", c.tc.Function.Name, "session_id", c.sess.ID) - c.sess.SetToolsApproved(true) - c.notifyApproval(ctx, ApprovalDecisionAllow, ApprovalSourceUserApprovedSession) - return runTool() - case ResumeTypeApproveSafe: - slog.DebugContext(ctx, "Resume signal received, opting into safe-auto", "tool", c.tc.Function.Name, "session_id", c.sess.ID) - c.sess.SetSafetyPolicy(session.SafetyPolicySafeAuto) - c.notifyApproval(ctx, ApprovalDecisionAllow, ApprovalSourceUserApprovedSafe) + case ResumeTypeApproveBalanced: + slog.DebugContext(ctx, "Resume signal received, opting into balanced", "tool", c.tc.Function.Name, "session_id", c.sess.ID) + c.sess.SetSafetyPolicy(session.SafetyPolicyBalanced) + c.notifyApproval(ctx, ApprovalDecisionAllow, ApprovalSourceUserApprovedBalanced) return runTool() - case ResumeTypeApproveSafer: - slog.DebugContext(ctx, "Resume signal received, opting into safer", "tool", c.tc.Function.Name, "session_id", c.sess.ID) - c.sess.SetSafetyPolicy(session.SafetyPolicySafer) - c.notifyApproval(ctx, ApprovalDecisionAllow, ApprovalSourceUserApprovedSafer) + case ResumeTypeApproveAutonomous: + slog.DebugContext(ctx, "Resume signal received, opting into autonomous", "tool", c.tc.Function.Name, "session_id", c.sess.ID) + c.sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) + c.notifyApproval(ctx, ApprovalDecisionAllow, ApprovalSourceUserApprovedAutonomous) return runTool() case ResumeTypeApproveTool: approvedTool := req.ToolName diff --git a/pkg/runtime/toolexec/dispatcher_test.go b/pkg/runtime/toolexec/dispatcher_test.go index 21d794e652..a864657ffe 100644 --- a/pkg/runtime/toolexec/dispatcher_test.go +++ b/pkg/runtime/toolexec/dispatcher_test.go @@ -104,7 +104,7 @@ func TestDispatcher_RoutesToToolsetHandler(t *testing.T) { t.Parallel() a := newAgent() sess := session.New() - sess.ToolsApproved = true // skip approval so we exercise the happy path + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) // skip approval so we exercise the happy path var handlerCalls int tool := tools.Tool{ @@ -135,7 +135,7 @@ func TestDispatcher_RunsToolHandlersInParallel(t *testing.T) { t.Parallel() a := newAgent() sess := session.New() - sess.ToolsApproved = true + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) started := make(chan string, 2) release := make(chan struct{}) @@ -196,7 +196,7 @@ func TestDispatcher_EmitsToolOutputFromHandlerContext(t *testing.T) { t.Parallel() a := newAgent() sess := session.New() - sess.ToolsApproved = true + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) tool := tools.Tool{ Name: "streamer", @@ -225,7 +225,7 @@ func TestDispatcher_RecordsDocumentToolResult(t *testing.T) { t.Parallel() a := newAgent() sess := session.New() - sess.ToolsApproved = true + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) tool := tools.Tool{ Name: "report", @@ -264,7 +264,7 @@ func TestDispatcher_RoutesToRuntimeHandler(t *testing.T) { t.Parallel() a := newAgent() sess := session.New() - sess.ToolsApproved = true + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) var handlerCalls int d := &toolexec.Dispatcher{ @@ -431,7 +431,7 @@ func TestDispatcher_ResumeRejectEmitsErrorResponseWithReason(t *testing.T) { assert.Contains(t, em.responses[0].Output, "wrong arguments") } -func TestDispatcher_ResumeApproveSafeFlipsSessionToSafeAuto(t *testing.T) { +func TestDispatcher_ResumeApproveBalancedFlipsSessionToBalanced(t *testing.T) { t.Parallel() a := newAgent() sess := session.New() @@ -452,17 +452,17 @@ func TestDispatcher_ResumeApproveSafeFlipsSessionToSafeAuto(t *testing.T) { } em := &captureEmitter{} - resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeApproveSafe} + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeApproveBalanced} d.Process(t.Context(), sess, []tools.ToolCall{{ ID: "x", Function: tools.FunctionCall{Name: "shell", Arguments: "{}"}, }}, []tools.Tool{tool}, em) - assert.True(t, ran, "approve-safe must run the pending tool") - assert.Equal(t, session.SafetyPolicySafeAuto, sess.SafetyPolicy, - "approve-safe must persist safe-auto on the session") - assert.False(t, sess.ToolsApproved, "safe-auto must not backfill ToolsApproved") + assert.True(t, ran, "approve-balanced must run the pending tool") + assert.Equal(t, session.SafetyPolicyBalanced, sess.SafetyPolicy, + "approve-balanced must persist balanced on the session") + assert.False(t, sess.ToolsApproved, "balanced must not backfill ToolsApproved") } func TestDispatcher_ResumeApproveToolPersistsToSessionPermissions(t *testing.T) { @@ -498,10 +498,12 @@ func TestDispatcher_ResumeApproveToolPersistsToSessionPermissions(t *testing.T) assert.Contains(t, sess.Permissions.Allow, "shell") } -func TestDispatcher_ReadOnlyHintAutoApproves(t *testing.T) { +// Under Balanced mode, a ReadOnlyHint tool auto-approves without +// prompting: mode(balanced) × label(safe) = allow. +func TestDispatcher_ReadOnlyHintAutoApprovesUnderBalanced(t *testing.T) { t.Parallel() a := newAgent() - sess := session.New() // ToolsApproved=false; no permissions configured + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyBalanced)) var ran bool tool := tools.Tool{ @@ -526,14 +528,221 @@ func TestDispatcher_ReadOnlyHintAutoApproves(t *testing.T) { }}, []tools.Tool{tool}, em) assert.True(t, ran) - assert.Empty(t, em.confirmations, "read-only tool must not prompt the user") + assert.Empty(t, em.confirmations, "balanced + read-only must not prompt the user") require.Len(t, em.responses, 1) assert.False(t, em.responses[0].IsError) } -// DestructiveHint on a non-approved tool must surface as -// blast_radius=high in the confirmation event metadata so the UI can -// render a warning tier without duplicating the classification logic. +// A batch of shell calls the classifier can't recognise (label = +// unknown): user picks approve-tool with a per-command shell pattern +// on the first prompt. Subsequent sibling calls that match the same +// pattern should auto-approve without emitting a second confirmation. +// Mirrors Gordon in prod where most commands (docker build, npm +// install, mise run …) fall into the unknown bucket and users grant +// per-command allow rules from the confirmation card. +func TestDispatcher_ApproveToolAutoApprovesSiblingCallsMatchingPattern(t *testing.T) { + t.Parallel() + a := newAgent() + sess := session.New() + + tool := tools.Tool{ + Name: "shell", + Handler: func(context.Context, tools.ToolCall, tools.Runtime) (*tools.ToolCallResult, error) { + return tools.ResultSuccess("ok"), nil + }, + } + + // safer_shell can't classify these — falls back to unknown, which + // under Balanced/Strict would keep asking. approve-tool is the + // escape hatch. + hd := &stubHookDispatcher{ + on: map[hooks.EventType]*hooks.Result{ + hooks.EventPreToolUsePreYolo: { + Allowed: true, + Decision: hooks.DecisionAllow, + Metadata: map[string]string{"blast_radius": "unknown"}, + }, + }, + } + + resume := make(chan toolexec.ResumeRequest, 1) + d := &toolexec.Dispatcher{ + AgentFor: func(*session.Session) *agent.Agent { return a }, + Hooks: hd, + Resume: resume, + // Real runtimes expose the session's own Permissions here (see + // pkg/runtime/tool_dispatch.go). Without it, approve-tool's + // session-level allow rule never reaches Decide(). + Permissions: func(s *session.Session) []toolexec.NamedChecker { + perms := s.ClonePermissions() + if perms == nil { + return nil + } + return []toolexec.NamedChecker{{ + Checker: permissions.NewCheckerFromRules(perms.Allow, perms.Ask, perms.Deny), + Source: "session permissions", + }} + }, + } + em := &captureEmitter{confirmed: make(chan struct{})} + + go func() { + <-em.confirmed + resume <- toolexec.ResumeRequest{ + Type: toolexec.ResumeTypeApproveTool, + ToolName: "shell:cmd=docker*", + } + }() + + d.Process(t.Context(), sess, []tools.ToolCall{ + {ID: "a", Function: tools.FunctionCall{Name: "shell", Arguments: `{"cmd":"docker ps"}`}}, + {ID: "b", Function: tools.FunctionCall{Name: "shell", Arguments: `{"cmd":"docker images"}`}}, + {ID: "c", Function: tools.FunctionCall{Name: "shell", Arguments: `{"cmd":"docker build ."}`}}, + }, []tools.Tool{tool}, em) + + require.Len(t, em.responses, 3) + for i, r := range em.responses { + assert.Falsef(t, r.IsError, "response %d must not be an error", i) + } + assert.Len(t, em.confirmations, 1, + "approve-tool with a shell pattern must auto-approve every matching sibling") +} + +// A batch of safe shell calls under Strict, with the safer_shell +// classifier wired in: user approve-balanced on the first prompt +// should auto-approve every remaining safe shell call in the batch. +// Mirrors production Gordon where the classifier tags each shell +// call with blast_radius via the preempt-yolo lane. +func TestDispatcher_ApproveBalancedAutoApprovesSiblingSafeShellCalls(t *testing.T) { + t.Parallel() + a := newAgent() + sess := session.New() + + tool := tools.Tool{ + Name: "shell", + Handler: func(context.Context, tools.ToolCall, tools.Runtime) (*tools.ToolCallResult, error) { + return tools.ResultSuccess("ok"), nil + }, + } + + // safer_shell always returns Allow + blast_radius=safe for safe + // commands under the new pure-labeller design. + hd := &stubHookDispatcher{ + on: map[hooks.EventType]*hooks.Result{ + hooks.EventPreToolUsePreYolo: { + Allowed: true, + Decision: hooks.DecisionAllow, + Metadata: map[string]string{"blast_radius": "safe"}, + }, + }, + } + + resume := make(chan toolexec.ResumeRequest, 1) + d := &toolexec.Dispatcher{ + AgentFor: func(*session.Session) *agent.Agent { return a }, + Hooks: hd, + Resume: resume, + } + em := &captureEmitter{confirmed: make(chan struct{})} + + go func() { + <-em.confirmed + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeApproveBalanced} + }() + + d.Process(t.Context(), sess, []tools.ToolCall{ + {ID: "a", Function: tools.FunctionCall{Name: "shell", Arguments: `{"cmd":"ls"}`}}, + {ID: "b", Function: tools.FunctionCall{Name: "shell", Arguments: `{"cmd":"pwd"}`}}, + {ID: "c", Function: tools.FunctionCall{Name: "shell", Arguments: `{"cmd":"git status"}`}}, + }, []tools.Tool{tool}, em) + + require.Len(t, em.responses, 3) + for i, r := range em.responses { + assert.Falsef(t, r.IsError, "response %d must not be an error", i) + } + assert.Len(t, em.confirmations, 1, + "only the first call should prompt; balanced flip must auto-approve safe siblings") +} + +// A batch of safe tool calls under Strict: user approve-balanced on +// the first prompt should auto-approve every remaining call in the +// batch without emitting a second confirmation event. Regression test +// for a Gordon bug where each parallel call kept surfacing its own +// prompt even after the mode flipped. +func TestDispatcher_ApproveBalancedAutoApprovesSiblingSafeCalls(t *testing.T) { + t.Parallel() + a := newAgent() + sess := session.New() + + var ran atomic.Int32 + tool := tools.Tool{ + Name: "read_file", + Annotations: tools.ToolAnnotations{ReadOnlyHint: true}, + Handler: func(context.Context, tools.ToolCall, tools.Runtime) (*tools.ToolCallResult, error) { + ran.Add(1) + return tools.ResultSuccess("contents"), nil + }, + } + + resume := make(chan toolexec.ResumeRequest, 1) + d := &toolexec.Dispatcher{ + AgentFor: func(*session.Session) *agent.Agent { return a }, + Resume: resume, + } + em := &captureEmitter{confirmed: make(chan struct{})} + + go func() { + <-em.confirmed + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeApproveBalanced} + }() + + d.Process(t.Context(), sess, []tools.ToolCall{ + {ID: "a", Function: tools.FunctionCall{Name: "read_file", Arguments: `{"path":"a"}`}}, + {ID: "b", Function: tools.FunctionCall{Name: "read_file", Arguments: `{"path":"b"}`}}, + {ID: "c", Function: tools.FunctionCall{Name: "read_file", Arguments: `{"path":"c"}`}}, + }, []tools.Tool{tool}, em) + + assert.Equal(t, int32(3), ran.Load(), "every safe sibling must run") + assert.Len(t, em.confirmations, 1, + "only the first call should prompt; the balanced flip must auto-approve the siblings") +} + +// Under Strict (default), ReadOnlyHint tools prompt the user. +func TestDispatcher_ReadOnlyHintPromptsUnderStrict(t *testing.T) { + t.Parallel() + a := newAgent() + sess := session.New() // empty policy ≡ strict + + tool := tools.Tool{ + Name: "read_file", + Annotations: tools.ToolAnnotations{ + ReadOnlyHint: true, + }, + Handler: func(context.Context, tools.ToolCall, tools.Runtime) (*tools.ToolCallResult, error) { + return tools.ResultSuccess("contents"), nil + }, + } + + resume := make(chan toolexec.ResumeRequest, 1) + d := &toolexec.Dispatcher{ + AgentFor: func(*session.Session) *agent.Agent { return a }, + Resume: resume, + } + em := &captureEmitter{} + + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeReject} + + d.Process(t.Context(), sess, []tools.ToolCall{{ + ID: "r", + Function: tools.FunctionCall{Name: "read_file", Arguments: "{}"}, + }}, []tools.Tool{tool}, em) + + require.Len(t, em.confirmations, 1, "strict must prompt even for read-only tools") +} + +// DestructiveHint on a tool that reaches the confirmation event must +// surface blast_radius=high metadata so the UI can render a warning +// tier without duplicating the classification logic. func TestDispatcher_DestructiveHintEmitsHighBlastRadiusMetadata(t *testing.T) { t.Parallel() a := newAgent() @@ -604,89 +813,84 @@ func TestDispatcher_UnannotatedToolEmitsNoBlastRadius(t *testing.T) { } } -// Under safe-auto, a ReadOnlyHint tool must auto-approve even when the -// session-level permissions checker asks for confirmation. Without this, -// callers configuring permissions.ask: "*" defeat the safe-auto opt-in. -func TestDispatcher_SafeAutoBypassesAskForReadOnlyTools(t *testing.T) { +// Under Autonomous mode, tools auto-approve without prompting even +// when marked destructive: the mode wins over the classifier's +// label. The user opted in explicitly. +func TestDispatcher_AutonomousAutoApprovesDestructiveTools(t *testing.T) { t.Parallel() a := newAgent() - sess := session.New(session.WithSafetyPolicy(session.SafetyPolicySafeAuto)) - perms := session.PermissionsConfig{Ask: []string{"*"}} - sess.Permissions = &perms + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) + destructive := true var ran bool tool := tools.Tool{ - Name: "read_file", + Name: "delete_task", Annotations: tools.ToolAnnotations{ - ReadOnlyHint: true, + DestructiveHint: &destructive, }, Handler: func(context.Context, tools.ToolCall, tools.Runtime) (*tools.ToolCallResult, error) { ran = true - return tools.ResultSuccess("contents"), nil + return tools.ResultSuccess("ok"), nil }, } d := &toolexec.Dispatcher{ AgentFor: func(*session.Session) *agent.Agent { return a }, - Permissions: func(s *session.Session) []toolexec.NamedChecker { - return []toolexec.NamedChecker{{ - Checker: permissions.NewCheckerFromRules(nil, s.Permissions.Ask, nil), - Source: "session", - }} - }, } em := &captureEmitter{} d.Process(t.Context(), sess, []tools.ToolCall{{ - ID: "r", - Function: tools.FunctionCall{Name: "read_file", Arguments: "{}"}, + ID: "x", + Function: tools.FunctionCall{Name: "delete_task", Arguments: "{}"}, }}, []tools.Tool{tool}, em) - assert.True(t, ran, "safe-auto + ReadOnlyHint must auto-approve") - assert.Empty(t, em.confirmations, - "safe-auto + ReadOnlyHint must not prompt the user") + assert.True(t, ran, "autonomous must auto-approve every tool") + assert.Empty(t, em.confirmations) } -func TestDispatcher_SaferBypassesAskForUnclassifiedTools(t *testing.T) { +// Custom ask: pattern beats Autonomous — new behaviour under the +// mode model. +func TestDispatcher_CustomAskBeatsAutonomous(t *testing.T) { t.Parallel() a := newAgent() - sess := session.New(session.WithSafetyPolicy(session.SafetyPolicySafer)) - perms := session.PermissionsConfig{Ask: []string{"*"}} - sess.Permissions = &perms + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) - var ran bool tool := tools.Tool{ - Name: "custom_tool", + Name: "shell", Handler: func(context.Context, tools.ToolCall, tools.Runtime) (*tools.ToolCallResult, error) { - ran = true - return tools.ResultSuccess("ok"), nil + panic("must not run without confirmation") }, } + resume := make(chan toolexec.ResumeRequest, 1) d := &toolexec.Dispatcher{ AgentFor: func(*session.Session) *agent.Agent { return a }, - Permissions: func(s *session.Session) []toolexec.NamedChecker { + Resume: resume, + Permissions: func(*session.Session) []toolexec.NamedChecker { return []toolexec.NamedChecker{{ - Checker: permissions.NewCheckerFromRules(nil, s.Permissions.Ask, nil), - Source: "session", + Checker: permissions.NewCheckerFromRules(nil, []string{"shell"}, nil), + Source: toolexec.SessionPermissionsSource, }} }, } em := &captureEmitter{} + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeReject} + d.Process(t.Context(), sess, []tools.ToolCall{{ ID: "x", - Function: tools.FunctionCall{Name: "custom_tool", Arguments: "{}"}, + Function: tools.FunctionCall{Name: "shell", Arguments: "{}"}, }}, []tools.Tool{tool}, em) - assert.True(t, ran, "safer + unclassified tool must auto-approve") - assert.Empty(t, em.confirmations) + require.Len(t, em.confirmations, 1, + "session-level ask must prompt even under autonomous") } -func TestDispatcher_SaferPromptsForDestructiveHintTools(t *testing.T) { +// Balanced mode prompts on destructive tools (mode × destructive = ask). +func TestDispatcher_BalancedPromptsForDestructiveHintTools(t *testing.T) { t.Parallel() a := newAgent() - sess := session.New(session.WithSafetyPolicy(session.SafetyPolicySafer)) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyBalanced)) destructive := true tool := tools.Tool{ @@ -716,7 +920,39 @@ func TestDispatcher_SaferPromptsForDestructiveHintTools(t *testing.T) { require.Len(t, em.confirmations, 1) } -func TestDispatcher_ResumeApproveSaferFlipsSessionToSafer(t *testing.T) { +// Balanced mode also prompts on unknown (readOnlyHint=false, no +// destructive hint) tools. +func TestDispatcher_BalancedPromptsForUnknownTools(t *testing.T) { + t.Parallel() + a := newAgent() + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyBalanced)) + + tool := tools.Tool{ + Name: "custom_tool", + Handler: func(context.Context, tools.ToolCall, tools.Runtime) (*tools.ToolCallResult, error) { + return tools.ResultSuccess("ok"), nil + }, + } + + resume := make(chan toolexec.ResumeRequest, 1) + d := &toolexec.Dispatcher{ + AgentFor: func(*session.Session) *agent.Agent { return a }, + Resume: resume, + } + em := &captureEmitter{} + + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeReject} + + d.Process(t.Context(), sess, []tools.ToolCall{{ + ID: "x", + Function: tools.FunctionCall{Name: "custom_tool", Arguments: "{}"}, + }}, []tools.Tool{tool}, em) + + require.Len(t, em.confirmations, 1, + "balanced + unknown tool must prompt the user") +} + +func TestDispatcher_ResumeApproveAutonomousFlipsSessionToAutonomous(t *testing.T) { t.Parallel() a := newAgent() sess := session.New() @@ -737,7 +973,7 @@ func TestDispatcher_ResumeApproveSaferFlipsSessionToSafer(t *testing.T) { } em := &captureEmitter{} - resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeApproveSafer} + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeApproveAutonomous} d.Process(t.Context(), sess, []tools.ToolCall{{ ID: "x", @@ -745,7 +981,9 @@ func TestDispatcher_ResumeApproveSaferFlipsSessionToSafer(t *testing.T) { }}, []tools.Tool{tool}, em) assert.True(t, ran) - assert.Equal(t, session.SafetyPolicySafer, sess.SafetyPolicy) + assert.Equal(t, session.SafetyPolicyAutonomous, sess.SafetyPolicy) + assert.True(t, sess.ToolsApproved, + "approve-autonomous must backfill ToolsApproved for legacy branches") } func TestDispatcher_DenyByPermissionsEmitsErrorResponse(t *testing.T) { @@ -792,7 +1030,7 @@ func TestDispatcher_ToolResponseTransformRewritesOutput(t *testing.T) { t.Parallel() a := newAgent() sess := session.New() - sess.ToolsApproved = true + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) original := "raw output with a secret" rewritten := "output with [REDACTED]" @@ -845,7 +1083,7 @@ func TestDispatcher_ToolResponseTransformIsNoOpWithoutHooks(t *testing.T) { t.Parallel() a := newAgent() sess := session.New() - sess.ToolsApproved = true + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) original := "untouched output" @@ -1050,7 +1288,7 @@ func TestDispatcher_PreToolUsePreYoloAskPreemptsYolo(t *testing.T) { a := newAgent() sess := session.New() - sess.ToolsApproved = true + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) tool := tools.Tool{ Name: "shell", @@ -1270,7 +1508,7 @@ func TestDispatcher_PreToolUsePreYoloDenyShortCircuits(t *testing.T) { a := newAgent() sess := session.New() - sess.ToolsApproved = true + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) tool := tools.Tool{ Name: "shell", @@ -1316,7 +1554,7 @@ func TestDispatcher_PreToolUsePreYoloAllowIsAdvisory(t *testing.T) { a := newAgent() sess := session.New() - sess.ToolsApproved = true + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) ran := false tool := tools.Tool{ @@ -1363,7 +1601,7 @@ func TestDispatcher_PreToolUseDefaultLaneSkippedUnderYolo(t *testing.T) { a := newAgent() sess := session.New() - sess.ToolsApproved = true + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) ran := false tool := tools.Tool{ @@ -1415,7 +1653,7 @@ func TestDispatcher_PreToolUseDefaultLaneSkippedUnderYolo(t *testing.T) { func TestDispatcher_NonInteractiveAskAutoDenies(t *testing.T) { a := newAgent() sess := session.New() - sess.ToolsApproved = true + sess.SetSafetyPolicy(session.SafetyPolicyAutonomous) sess.NonInteractive = true tool := tools.Tool{ diff --git a/pkg/runtime/toolexec/helpers_test.go b/pkg/runtime/toolexec/helpers_test.go index 0df3b01ce3..a924d599e8 100644 --- a/pkg/runtime/toolexec/helpers_test.go +++ b/pkg/runtime/toolexec/helpers_test.go @@ -51,5 +51,5 @@ func (s *stubHookDispatcher) Dispatch(_ context.Context, _ *agent.Agent, event h } func (s *stubHookDispatcher) NotifyUserInput(context.Context, string, string) {} -func (s *stubHookDispatcher) NotifyApprovalDecision(context.Context, *session.Session, *agent.Agent, tools.ToolCall, string, string) { +func (s *stubHookDispatcher) NotifyApprovalDecision(context.Context, *session.Session, *agent.Agent, tools.ToolCall, string, string, string) { } diff --git a/pkg/runtime/toolexec/permissions.go b/pkg/runtime/toolexec/permissions.go index dbf8fdda4d..6cf171e5b9 100644 --- a/pkg/runtime/toolexec/permissions.go +++ b/pkg/runtime/toolexec/permissions.go @@ -2,6 +2,7 @@ package toolexec import ( "github.com/docker/docker-agent/pkg/permissions" + "github.com/docker/docker-agent/pkg/session" ) // PermissionOutcome is the resolved decision after evaluating the full @@ -20,22 +21,28 @@ const ( // PermissionReason explains *why* a [PermissionDecision] was reached. // Callers use it to produce accurate log messages and to know which -// auto-approval path was taken (yolo, checker rule, read-only hint, or -// default). +// stage of the pipeline produced the verdict (custom rule vs. safety +// mode). type PermissionReason int const ( - // ReasonYolo: --yolo (sess.ToolsApproved) auto-approved the tool. - ReasonYolo PermissionReason = iota // ReasonChecker: a configured permission checker (session-level or // team-level) produced a definitive Allow/Deny/ForceAsk verdict. // PermissionDecision.Source identifies which checker. - ReasonChecker - // ReasonReadOnlyHint: no checker matched and the tool's ReadOnlyHint - // annotation auto-approved it. - ReasonReadOnlyHint - // ReasonDefault: nothing matched; the user must confirm. - ReasonDefault + ReasonChecker PermissionReason = iota + // ReasonMode: no custom checker matched; the session's SafetyPolicy + // was applied against the tool's classifier label. Source is the + // mode name ("strict", "balanced", "autonomous"). + ReasonMode +) + +// Safety classifier labels. Every tool call carries one of these +// three; they are the runtime's canonical safety taxonomy and drive +// the mode × label verdict table. +const ( + SafetyLabelSafe = "safe" + SafetyLabelDestructive = "destructive" + SafetyLabelUnknown = "unknown" ) // NamedChecker pairs a [permissions.Checker] with a human-readable source @@ -48,32 +55,39 @@ type NamedChecker struct { // PermissionDecision is the result of [Decide]: an outcome plus the // reason and (when the reason is [ReasonChecker]) the source label of the -// checker that produced it. +// checker that produced it. When the reason is [ReasonMode], Source +// carries the mode name. type PermissionDecision struct { Outcome PermissionOutcome Reason PermissionReason Source string } -// Decide resolves the final permission outcome for a tool call by walking -// the configured pipeline in priority order: +// SessionPermissionsSource is the checker source label the runtime +// uses for the session-scoped permission layer (interactive "T = +// always allow" grants and mid-session API mutations). Session +// ForceAsk beats the mode; team ForceAsk is advisory. +const SessionPermissionsSource = "session permissions" + +// Decide resolves the final permission outcome: +// +// - Deny (any checker) → Deny. +// - Allow (any checker) → Allow. +// - ForceAsk (session-level only) → Ask, beats the mode. +// - Otherwise → (mode × label) verdict table. // -// 1. checkers (in order; typically session-level first, then team-level) -// — the first checker that returns Allow / Deny / ForceAsk wins. -// However, if the outcome is ForceAsk and yoloApproved is true, -// YOLO overrides ForceAsk and auto-allows the call. -// 2. yoloApproved (--yolo) — auto-allow everything else. -// 3. readOnlyHint — auto-allow. -// 4. default — Ask. +// Team-level ForceAsk is deliberately advisory: a YAML rule like +// `ask: "*"` should not defeat Balanced/Autonomous. Users who want +// targeted asks that beat the mode add them via the Custom rules UI, +// which populates the session-level checker. // -// Decide is pure (no I/O, no side effects) so the entire approval matrix -// can be exhaustively unit-tested without a runtime. +// Pure so the matrix is unit-testable. func Decide( - yoloApproved bool, + mode session.SafetyPolicy, + label string, checkers []NamedChecker, toolName string, toolArgs map[string]any, - readOnlyHint bool, ) PermissionDecision { for _, pc := range checkers { switch pc.Checker.CheckWithArgs(toolName, toolArgs) { @@ -82,21 +96,54 @@ func Decide( case permissions.Allow: return PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonChecker, Source: pc.Source} case permissions.ForceAsk: - if yoloApproved { - return PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonYolo} + if pc.Source == SessionPermissionsSource { + return PermissionDecision{Outcome: OutcomeAsk, Reason: ReasonChecker, Source: pc.Source} } - return PermissionDecision{Outcome: OutcomeAsk, Reason: ReasonChecker, Source: pc.Source} + // Team-level ForceAsk falls through so the mode can override. case permissions.Ask: - // No explicit match at this level; fall through to next checker. + // No explicit match at this level; fall through. } } + return applyMode(mode, label) +} - if yoloApproved { - return PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonYolo} +// applyMode implements the (mode × label) → verdict table. Empty / +// unrecognised modes fall back to Strict. Source is the +// ApprovalSourceMode* constant so [allowSourceForDecision] can return +// it verbatim without a second lookup. +// +// safe destructive unknown +// Strict ask ask ask +// Balanced allow ask ask +// Autonomous allow allow allow +func applyMode(mode session.SafetyPolicy, label string) PermissionDecision { + switch mode { + case session.SafetyPolicyAutonomous: + return PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonMode, Source: ApprovalSourceModeAutonomous} + case session.SafetyPolicyBalanced: + if label == SafetyLabelSafe { + return PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonMode, Source: ApprovalSourceModeBalanced} + } + return PermissionDecision{Outcome: OutcomeAsk, Reason: ReasonMode, Source: ApprovalSourceModeBalanced} + default: + return PermissionDecision{Outcome: OutcomeAsk, Reason: ReasonMode, Source: ApprovalSourceModeStrict} } +} +// LabelFromReadOnlyHint: readOnlyHint=true ⇒ safe; false ⇒ unknown. +func LabelFromReadOnlyHint(readOnlyHint bool) string { if readOnlyHint { - return PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonReadOnlyHint} + return SafetyLabelSafe + } + return SafetyLabelUnknown +} + +// LabelWithDestructiveHint upgrades to destructive when the tool's +// DestructiveHint annotation is set; otherwise falls back to +// [LabelFromReadOnlyHint]. +func LabelWithDestructiveHint(readOnlyHint bool, destructiveHint *bool) string { + if destructiveHint != nil && *destructiveHint { + return SafetyLabelDestructive } - return PermissionDecision{Outcome: OutcomeAsk, Reason: ReasonDefault} + return LabelFromReadOnlyHint(readOnlyHint) } diff --git a/pkg/runtime/toolexec/permissions_test.go b/pkg/runtime/toolexec/permissions_test.go index 1803f95f0c..8325f5bf5f 100644 --- a/pkg/runtime/toolexec/permissions_test.go +++ b/pkg/runtime/toolexec/permissions_test.go @@ -7,6 +7,7 @@ import ( "github.com/docker/docker-agent/pkg/config/latest" "github.com/docker/docker-agent/pkg/permissions" + "github.com/docker/docker-agent/pkg/session" ) func newChecker(t *testing.T, allow, ask, deny []string) *permissions.Checker { @@ -18,111 +19,154 @@ func newChecker(t *testing.T, allow, ask, deny []string) *permissions.Checker { }) } -func TestDecide_DenyOverridesYolo(t *testing.T) { +// Custom deny beats every mode, including Autonomous. This is the +// hardest guarantee custom rules give the user. +func TestDecide_CustomDenyBeatsAutonomous(t *testing.T) { t.Parallel() - d := Decide(true, []NamedChecker{ + d := Decide(session.SafetyPolicyAutonomous, SafetyLabelSafe, []NamedChecker{ {Checker: newChecker(t, nil, nil, []string{"shell"}), Source: "team"}, - }, "shell", nil, false) + }, "shell", nil) assert.Equal(t, PermissionDecision{Outcome: OutcomeDeny, Reason: ReasonChecker, Source: "team"}, d) } -func TestDecide_YoloAllowsWhenNoCheckerMatches(t *testing.T) { +// Custom allow beats the safety mode's Ask under Strict/Balanced. +func TestDecide_CustomAllowBeatsMode(t *testing.T) { t.Parallel() - d := Decide(true, nil, "shell", nil, false) - assert.Equal(t, PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonYolo}, d) -} - -func TestDecide_YoloOverridesForceAsk(t *testing.T) { - t.Parallel() - d := Decide(true, []NamedChecker{ - {Checker: newChecker(t, nil, []string{"shell"}, nil), Source: "team"}, - }, "shell", nil, false) - assert.Equal(t, PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonYolo}, d) -} - -func TestDecide_DenyFromCheckerWins(t *testing.T) { - t.Parallel() - d := Decide(false, []NamedChecker{ - {Checker: newChecker(t, nil, nil, []string{"shell"}), Source: "session"}, - }, "shell", nil, true /* read-only doesn't bypass deny */) + d := Decide(session.SafetyPolicyStrict, SafetyLabelDestructive, []NamedChecker{ + {Checker: newChecker(t, []string{"shell"}, nil, nil), Source: "session"}, + }, "shell", nil) - assert.Equal(t, PermissionDecision{Outcome: OutcomeDeny, Reason: ReasonChecker, Source: "session"}, d) + assert.Equal(t, PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonChecker, Source: "session"}, d) } -func TestDecide_AllowFromChecker(t *testing.T) { +// Session-level ask beats Autonomous. Users who add explicit ask +// patterns via the Custom rules UI get their targeted prompt even +// under yolo. +func TestDecide_SessionAskBeatsAutonomous(t *testing.T) { t.Parallel() - d := Decide(false, []NamedChecker{ - {Checker: newChecker(t, []string{"read_*"}, nil, nil), Source: "session"}, - }, "read_file", nil, false) + d := Decide(session.SafetyPolicyAutonomous, SafetyLabelSafe, []NamedChecker{ + {Checker: newChecker(t, nil, []string{"shell:cmd=git push --force*"}, nil), Source: SessionPermissionsSource}, + }, "shell", map[string]any{"cmd": "git push --force origin main"}) - assert.Equal(t, PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonChecker, Source: "session"}, d) + assert.Equal(t, PermissionDecision{Outcome: OutcomeAsk, Reason: ReasonChecker, Source: SessionPermissionsSource}, d) } -func TestDecide_ForceAskFromCheckerOverridesReadOnly(t *testing.T) { +// Team-level ask (e.g. `ask: "*"` in an agent YAML) is advisory: it +// forces asking only when the mode itself would ask. Under Balanced +// with a classifier-safe tool, the mode overrides and auto-approves +// so YAML wildcards don't defeat the user's mode choice. +func TestDecide_TeamAskFallsThroughToMode(t *testing.T) { t.Parallel() - d := Decide(false, []NamedChecker{ - {Checker: newChecker(t, nil, []string{"read_file"}, nil), Source: "team"}, - }, "read_file", nil, true /* read-only would normally bypass ask */) + d := Decide(session.SafetyPolicyBalanced, SafetyLabelSafe, []NamedChecker{ + {Checker: newChecker(t, nil, []string{"*"}, nil), Source: "permissions configuration"}, + }, "read_multiple_files", nil) - assert.Equal(t, PermissionDecision{Outcome: OutcomeAsk, Reason: ReasonChecker, Source: "team"}, d) + assert.Equal(t, PermissionDecision{ + Outcome: OutcomeAllow, Reason: ReasonMode, Source: "mode_balanced", + }, d, "team-level ask: * must not defeat Balanced for classifier-safe tools") } +// First matching checker wins; session (checked first) overrides team. func TestDecide_FirstCheckerWins_SessionBeforeTeam(t *testing.T) { t.Parallel() - // Session allows; team denies. Session is checked first → Allow. - d := Decide(false, []NamedChecker{ + d := Decide(session.SafetyPolicyStrict, SafetyLabelUnknown, []NamedChecker{ {Checker: newChecker(t, []string{"shell"}, nil, nil), Source: "session permissions"}, {Checker: newChecker(t, nil, nil, []string{"shell"}), Source: "permissions configuration"}, - }, "shell", nil, false) + }, "shell", nil) assert.Equal(t, PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonChecker, Source: "session permissions"}, d) } +// Empty first checker falls through to the second. func TestDecide_FallsThroughWhenNoCheckerMatches(t *testing.T) { t.Parallel() - // First checker doesn't match anything (no patterns) → falls through to second. - d := Decide(false, []NamedChecker{ + d := Decide(session.SafetyPolicyStrict, SafetyLabelUnknown, []NamedChecker{ {Checker: newChecker(t, nil, nil, nil), Source: "session permissions"}, {Checker: newChecker(t, []string{"shell"}, nil, nil), Source: "permissions configuration"}, - }, "shell", nil, false) + }, "shell", nil) assert.Equal(t, PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonChecker, Source: "permissions configuration"}, d) } -func TestDecide_ReadOnlyHintAutoApproves(t *testing.T) { +// Arg-pattern matching is unchanged: `shell:cmd=ls*` matches `ls -la`. +func TestDecide_ArgPatternMatching(t *testing.T) { t.Parallel() - d := Decide(false, nil, "read_file", nil, true) - assert.Equal(t, PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonReadOnlyHint}, d) + d := Decide(session.SafetyPolicyStrict, SafetyLabelSafe, []NamedChecker{ + {Checker: newChecker(t, []string{"shell:cmd=ls*"}, nil, nil), Source: "session"}, + }, "shell", map[string]any{"cmd": "ls -la"}) + + assert.Equal(t, PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonChecker, Source: "session"}, d) } -func TestDecide_DefaultAsk(t *testing.T) { +// Arg-pattern non-match falls through to the mode table. +func TestDecide_ArgPatternNoMatchFallsToMode(t *testing.T) { t.Parallel() - d := Decide(false, nil, "shell", nil, false) - assert.Equal(t, PermissionDecision{Outcome: OutcomeAsk, Reason: ReasonDefault}, d) + d := Decide(session.SafetyPolicyStrict, SafetyLabelSafe, []NamedChecker{ + {Checker: newChecker(t, []string{"shell:cmd=ls*"}, nil, nil), Source: "session"}, + }, "shell", map[string]any{"cmd": "rm -rf /"}) + + assert.Equal(t, PermissionDecision{Outcome: OutcomeAsk, Reason: ReasonMode, Source: "mode_strict"}, d) } -func TestDecide_NoCheckersWithReadOnly(t *testing.T) { +// TestDecide_ModeVerdictTable pins the (mode × label) → verdict +// table. Every cell is exercised because a subtle regression here +// would silently either over-approve or over-prompt. +func TestDecide_ModeVerdictTable(t *testing.T) { t.Parallel() - d := Decide(false, []NamedChecker{}, "read_file", nil, true) - assert.Equal(t, PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonReadOnlyHint}, d) + cases := []struct { + mode session.SafetyPolicy + label string + wantOutcome PermissionOutcome + wantSource string + }{ + {session.SafetyPolicyStrict, SafetyLabelSafe, OutcomeAsk, "mode_strict"}, + {session.SafetyPolicyStrict, SafetyLabelDestructive, OutcomeAsk, "mode_strict"}, + {session.SafetyPolicyStrict, SafetyLabelUnknown, OutcomeAsk, "mode_strict"}, + {session.SafetyPolicyBalanced, SafetyLabelSafe, OutcomeAllow, "mode_balanced"}, + {session.SafetyPolicyBalanced, SafetyLabelDestructive, OutcomeAsk, "mode_balanced"}, + {session.SafetyPolicyBalanced, SafetyLabelUnknown, OutcomeAsk, "mode_balanced"}, + {session.SafetyPolicyAutonomous, SafetyLabelSafe, OutcomeAllow, "mode_autonomous"}, + {session.SafetyPolicyAutonomous, SafetyLabelDestructive, OutcomeAllow, "mode_autonomous"}, + {session.SafetyPolicyAutonomous, SafetyLabelUnknown, OutcomeAllow, "mode_autonomous"}, + } + for _, tc := range cases { + t.Run(string(tc.mode)+"_"+tc.label, func(t *testing.T) { + t.Parallel() + d := Decide(tc.mode, tc.label, nil, "some_tool", nil) + assert.Equal(t, tc.wantOutcome, d.Outcome) + assert.Equal(t, ReasonMode, d.Reason) + assert.Equal(t, tc.wantSource, d.Source) + }) + } } -func TestDecide_ArgPatternMatching(t *testing.T) { +// Empty mode is treated as Strict — the safe default when no session +// preference has been recorded. +func TestDecide_EmptyModeDefaultsToStrict(t *testing.T) { t.Parallel() - // A checker that only allows shell when cmd starts with "ls". - d := Decide(false, []NamedChecker{ - {Checker: newChecker(t, []string{"shell:cmd=ls*"}, nil, nil), Source: "session"}, - }, "shell", map[string]any{"cmd": "ls -la"}, false) - - assert.Equal(t, PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonChecker, Source: "session"}, d) + d := Decide("", SafetyLabelSafe, nil, "read_file", nil) + assert.Equal(t, PermissionDecision{Outcome: OutcomeAsk, Reason: ReasonMode, Source: "mode_strict"}, d) } -func TestDecide_ArgPatternNoMatchFallsToDefault(t *testing.T) { +// LabelFromReadOnlyHint: readOnlyHint=true → safe, false → unknown. +func TestLabelFromReadOnlyHint(t *testing.T) { t.Parallel() - d := Decide(false, []NamedChecker{ - {Checker: newChecker(t, []string{"shell:cmd=ls*"}, nil, nil), Source: "session"}, - }, "shell", map[string]any{"cmd": "rm -rf /"}, false) + assert.Equal(t, SafetyLabelSafe, LabelFromReadOnlyHint(true)) + assert.Equal(t, SafetyLabelUnknown, LabelFromReadOnlyHint(false)) +} - assert.Equal(t, PermissionDecision{Outcome: OutcomeAsk, Reason: ReasonDefault}, d) +// LabelWithDestructiveHint: an explicit destructive annotation +// upgrades a non-shell tool to the destructive label regardless of +// readOnlyHint. +func TestLabelWithDestructiveHint(t *testing.T) { + t.Parallel() + trueVal := true + falseVal := false + assert.Equal(t, SafetyLabelDestructive, LabelWithDestructiveHint(false, &trueVal)) + assert.Equal(t, SafetyLabelDestructive, LabelWithDestructiveHint(true, &trueVal)) + assert.Equal(t, SafetyLabelSafe, LabelWithDestructiveHint(true, &falseVal)) + assert.Equal(t, SafetyLabelUnknown, LabelWithDestructiveHint(false, &falseVal)) + assert.Equal(t, SafetyLabelUnknown, LabelWithDestructiveHint(false, nil)) + assert.Equal(t, SafetyLabelSafe, LabelWithDestructiveHint(true, nil)) } diff --git a/pkg/server/session_manager.go b/pkg/server/session_manager.go index 7555b92a5e..da1e64756e 100644 --- a/pkg/server/session_manager.go +++ b/pkg/server/session_manager.go @@ -926,19 +926,19 @@ func (sm *SessionManager) ResumeSession(ctx context.Context, sessionID, confirma return errors.New("session not found") } + slog.DebugContext(ctx, "ResumeSession received", + "session_id", sessionID, "confirmation", confirmation, "tool_name", toolName) + // Mirror + persist mid-turn session mutations synchronously — // PersistenceObserver only persists on OnRunStart. if rt.session != nil { mutated := false switch runtime.ResumeType(confirmation) { - case runtime.ResumeTypeApproveSafe: - rt.session.SetSafetyPolicy(session.SafetyPolicySafeAuto) - mutated = true - case runtime.ResumeTypeApproveSafer: - rt.session.SetSafetyPolicy(session.SafetyPolicySafer) + case runtime.ResumeTypeApproveBalanced: + rt.session.SetSafetyPolicy(session.SafetyPolicyBalanced) mutated = true - case runtime.ResumeTypeApproveSession: - rt.session.SetToolsApproved(true) + case runtime.ResumeTypeApproveAutonomous: + rt.session.SetSafetyPolicy(session.SafetyPolicyAutonomous) mutated = true case runtime.ResumeTypeApproveTool: // Skip when toolName is empty — the dispatcher's own @@ -949,6 +949,9 @@ func (sm *SessionManager) ResumeSession(ctx context.Context, sessionID, confirma } } if mutated { + slog.DebugContext(ctx, "ResumeSession mutated session", + "session_id", sessionID, "safety_policy", string(rt.session.SafetyPolicy), + "tools_approved", rt.session.ToolsApproved) if err := sm.sessionStore.UpdateSession(ctx, rt.session); err != nil { slog.WarnContext(ctx, "failed to persist mid-turn session state", "session_id", sessionID, "confirmation", confirmation, "err", err) diff --git a/pkg/session/safety_policy_test.go b/pkg/session/safety_policy_test.go index 9843574a46..7a944b040f 100644 --- a/pkg/session/safety_policy_test.go +++ b/pkg/session/safety_policy_test.go @@ -9,41 +9,47 @@ import ( func TestSafetyPolicy_IsValid(t *testing.T) { t.Parallel() cases := map[SafetyPolicy]bool{ - "": true, - SafetyPolicyUnsafe: true, - SafetyPolicySafer: true, - SafetyPolicySafeAuto: true, - SafetyPolicyStrict: true, - SafetyPolicy("yolo"): false, - SafetyPolicy("Safer"): false, // case-sensitive on purpose + "": true, + SafetyPolicyStrict: true, + SafetyPolicyBalanced: true, + SafetyPolicyAutonomous: true, + SafetyPolicy("yolo"): false, + SafetyPolicy("safer"): false, + SafetyPolicy("safe-auto"): false, + SafetyPolicy("unsafe"): false, + SafetyPolicy("Strict"): false, // case-sensitive on purpose } for in, want := range cases { assert.Equalf(t, want, in.IsValid(), "SafetyPolicy(%q).IsValid()", string(in)) } } -// WithSafetyPolicy(unsafe) must flip ToolsApproved=true so legacy -// branches on ToolsApproved (Decide's --yolo short-circuit) still fire. -// Safer/strict intentionally leave ToolsApproved alone. -func TestWithSafetyPolicy_UnsafeSyncsToolsApproved(t *testing.T) { +// WithSafetyPolicy(autonomous) must flip ToolsApproved=true so legacy +// branches on ToolsApproved still fire. Strict/Balanced intentionally +// leave ToolsApproved alone. +func TestWithSafetyPolicy_AutonomousSyncsToolsApproved(t *testing.T) { t.Parallel() - s := New(WithSafetyPolicy(SafetyPolicyUnsafe)) - assert.Equal(t, SafetyPolicyUnsafe, s.SafetyPolicy) + s := New(WithSafetyPolicy(SafetyPolicyAutonomous)) + assert.Equal(t, SafetyPolicyAutonomous, s.SafetyPolicy) assert.True(t, s.ToolsApproved) - s = New(WithSafetyPolicy(SafetyPolicySafer)) - assert.Equal(t, SafetyPolicySafer, s.SafetyPolicy) + s = New(WithSafetyPolicy(SafetyPolicyBalanced)) + assert.Equal(t, SafetyPolicyBalanced, s.SafetyPolicy) + assert.False(t, s.ToolsApproved) + + s = New(WithSafetyPolicy(SafetyPolicyStrict)) + assert.Equal(t, SafetyPolicyStrict, s.SafetyPolicy) assert.False(t, s.ToolsApproved) } -// WithToolsApproved(true) must backfill SafetyPolicy=unsafe so hooks -// reading Input.SafetyPolicy see the correct value for legacy --yolo -// callers (gordon-Slack, MCP, eval) that haven't migrated. +// WithToolsApproved(true) must backfill SafetyPolicy=autonomous so +// hooks reading Input.SafetyPolicy see the correct value for legacy +// --yolo callers that haven't migrated. func TestWithToolsApproved_BackfillsSafetyPolicy(t *testing.T) { t.Parallel() s := New(WithToolsApproved(true)) assert.True(t, s.ToolsApproved) - assert.Equal(t, SafetyPolicyUnsafe, s.SafetyPolicy) + assert.Equal(t, SafetyPolicyAutonomous, s.SafetyPolicy) s = New(WithToolsApproved(false)) assert.False(t, s.ToolsApproved) @@ -51,34 +57,34 @@ func TestWithToolsApproved_BackfillsSafetyPolicy(t *testing.T) { } // Explicit WithSafetyPolicy after WithToolsApproved wins over the -// backfill (e.g. yolo + safer = "auto-approve except destructive"). +// backfill (e.g. yolo + balanced would be a rare combination). func TestWithSafetyPolicy_ExplicitWinsOverToolsApproved(t *testing.T) { t.Parallel() s := New( WithToolsApproved(true), - WithSafetyPolicy(SafetyPolicySafer), + WithSafetyPolicy(SafetyPolicyBalanced), ) assert.True(t, s.ToolsApproved) - assert.Equal(t, SafetyPolicySafer, s.SafetyPolicy) + assert.Equal(t, SafetyPolicyBalanced, s.SafetyPolicy) } // SetSafetyPolicy mid-session mirrors WithSafetyPolicy: setting -// unsafe backfills ToolsApproved; other modes leave it alone. -// Used by the dispatcher's approve-safe resume handler to opt an -// existing session into safe-auto without recreating it. +// autonomous backfills ToolsApproved; other modes leave it alone. +// Used by the dispatcher's approve-balanced / approve-autonomous +// resume handlers to opt an existing session into a new mode. func TestSetSafetyPolicy_MidSession(t *testing.T) { t.Parallel() s := New() assert.Equal(t, SafetyPolicy(""), s.SafetyPolicy) assert.False(t, s.ToolsApproved) - s.SetSafetyPolicy(SafetyPolicySafeAuto) - assert.Equal(t, SafetyPolicySafeAuto, s.SafetyPolicy) - assert.False(t, s.ToolsApproved, "safe-auto must not backfill ToolsApproved") + s.SetSafetyPolicy(SafetyPolicyBalanced) + assert.Equal(t, SafetyPolicyBalanced, s.SafetyPolicy) + assert.False(t, s.ToolsApproved, "balanced must not backfill ToolsApproved") - s.SetSafetyPolicy(SafetyPolicyUnsafe) - assert.Equal(t, SafetyPolicyUnsafe, s.SafetyPolicy) - assert.True(t, s.ToolsApproved, "unsafe must backfill ToolsApproved for legacy branches") + s.SetSafetyPolicy(SafetyPolicyAutonomous) + assert.Equal(t, SafetyPolicyAutonomous, s.SafetyPolicy) + assert.True(t, s.ToolsApproved, "autonomous must backfill ToolsApproved for legacy branches") s.SetSafetyPolicy(SafetyPolicyStrict) assert.Equal(t, SafetyPolicyStrict, s.SafetyPolicy) diff --git a/pkg/session/session.go b/pkg/session/session.go index 864e1718d5..03664142b9 100644 --- a/pkg/session/session.go +++ b/pkg/session/session.go @@ -33,32 +33,26 @@ const ( toolResultTruncationMarker = "\n[...tool result truncated: middle omitted...]\n" ) -// SafetyPolicy is the per-session safety preference. It is data only: -// the runtime forwards it to hooks via [hooks.Input.SafetyPolicy] and -// classifiers (e.g. safer_shell) adapt on it. Empty ⇒ derive from -// ToolsApproved (true ⇒ unsafe, false ⇒ strict). +// SafetyPolicy is the per-session safety preference. The runtime +// routes tool calls to allow/ask via the mode × classifier-label +// table (pkg/runtime/toolexec). Empty ⇒ derive from ToolsApproved +// (true ⇒ autonomous, false ⇒ strict). type SafetyPolicy string const ( - // SafetyPolicyUnsafe: --yolo / ToolsApproved=true equivalent. - // Classifiers stay silent, tool calls auto-approve. - SafetyPolicyUnsafe SafetyPolicy = "unsafe" - // SafetyPolicySafer: auto-approve except classifier-flagged - // destructive calls (blast_radius low/medium/high). - SafetyPolicySafer SafetyPolicy = "safer" - // SafetyPolicySafeAuto: auto-approve shell calls the classifier - // positively recognises as safe (blast_radius=safe); ask on - // destructive and unknown. Sits between safer and strict: - // safer waves through unknown too, strict prompts for safe. - SafetyPolicySafeAuto SafetyPolicy = "safe-auto" - // SafetyPolicyStrict: today's no-yolo CLI default — prompt for - // anything not auto-approved by a checker rule. + // SafetyPolicyStrict prompts on every tool call. SafetyPolicyStrict SafetyPolicy = "strict" + // SafetyPolicyBalanced auto-approves classifier-safe calls; asks + // on destructive and unknown. + SafetyPolicyBalanced SafetyPolicy = "balanced" + // SafetyPolicyAutonomous auto-approves every call (legacy yolo). + // Classifiers still emit metadata for audit but never gate. + SafetyPolicyAutonomous SafetyPolicy = "autonomous" ) func (p SafetyPolicy) IsValid() bool { switch p { - case "", SafetyPolicyUnsafe, SafetyPolicySafer, SafetyPolicySafeAuto, SafetyPolicyStrict: + case "", SafetyPolicyStrict, SafetyPolicyBalanced, SafetyPolicyAutonomous: return true } return false @@ -1201,24 +1195,24 @@ func WithMessages(messages []Item) Opt { // WithToolsApproved is the legacy --yolo setter. Prefer // [WithSafetyPolicy]. With toolsApproved=true and no explicit -// SafetyPolicy, pins the policy to [SafetyPolicyUnsafe]. +// SafetyPolicy, pins the policy to [SafetyPolicyAutonomous]. func WithToolsApproved(toolsApproved bool) Opt { return func(s *Session) { s.ToolsApproved = toolsApproved if toolsApproved && s.SafetyPolicy == "" { - s.SafetyPolicy = SafetyPolicyUnsafe + s.SafetyPolicy = SafetyPolicyAutonomous } } } // WithSafetyPolicy sets the session's safety preference. -// [SafetyPolicyUnsafe] also flips ToolsApproved=true so legacy branches -// on ToolsApproved keep working. The other modes leave ToolsApproved -// alone — set both if you want auto-approve + selective gating. +// [SafetyPolicyAutonomous] also flips ToolsApproved=true so legacy +// branches on ToolsApproved keep working. The other modes leave +// ToolsApproved alone. func WithSafetyPolicy(policy SafetyPolicy) Opt { return func(s *Session) { s.SafetyPolicy = policy - if policy == SafetyPolicyUnsafe { + if policy == SafetyPolicyAutonomous { s.ToolsApproved = true } } @@ -1462,20 +1456,20 @@ func (s *Session) SetToolsApproved(approved bool) { defer s.mu.Unlock() s.ToolsApproved = approved if approved && s.SafetyPolicy == "" { - s.SafetyPolicy = SafetyPolicyUnsafe + s.SafetyPolicy = SafetyPolicyAutonomous } } // SetSafetyPolicy updates the session's SafetyPolicy under s.mu. -// Mirrors WithSafetyPolicy: setting unsafe also flips ToolsApproved +// Mirrors WithSafetyPolicy: setting autonomous also flips ToolsApproved // so legacy branches on ToolsApproved keep working. Runtime callers // use this to persist a user's mid-session mode change (e.g. opting -// into safe-auto from a confirmation prompt). +// into balanced from a confirmation prompt). func (s *Session) SetSafetyPolicy(policy SafetyPolicy) { s.mu.Lock() defer s.mu.Unlock() s.SafetyPolicy = policy - if policy == SafetyPolicyUnsafe { + if policy == SafetyPolicyAutonomous { s.ToolsApproved = true } } diff --git a/pkg/teamloader/teamloader.go b/pkg/teamloader/teamloader.go index 01b519463d..57ae311fec 100644 --- a/pkg/teamloader/teamloader.go +++ b/pkg/teamloader/teamloader.go @@ -300,7 +300,7 @@ func LoadWithConfig(ctx context.Context, agentSource config.Source, runConfig *c agent.WithAddEnvironmentInfo(agentConfig.AddEnvironmentInfo), agent.WithAddDescriptionParameter(agentConfig.AddDescriptionParameter), agent.WithRedactSecrets(agentConfig.RedactSecretsEnabled()), - agent.WithSaferShell(agentConfig.SaferShellEnabled()), + agent.WithSaferShell(agentConfig.HasShellToolset()), agent.WithAddPromptFiles(promptFiles), agent.WithMaxIterations(agentConfig.MaxIterations), agent.WithMaxConsecutiveToolCalls(agentConfig.MaxConsecutiveToolCalls), diff --git a/pkg/tui/components/toolconfirm/toolconfirm.go b/pkg/tui/components/toolconfirm/toolconfirm.go index e7d95b4e03..0706eb32c8 100644 --- a/pkg/tui/components/toolconfirm/toolconfirm.go +++ b/pkg/tui/components/toolconfirm/toolconfirm.go @@ -36,9 +36,10 @@ const ( // ApproveTool runs the call and always allows the tool, scoped by the // permission pattern from BuildPermissionPattern. ApproveTool - // ApproveSession runs the call and approves all tools for the rest of - // the session. - ApproveSession + // ApproveAutonomous runs the call and flips the session to + // SafetyPolicyAutonomous (auto-approve everything for the rest of + // the session). + ApproveAutonomous // Reject rejects the call with an optional reason shown to the model. Reject ) @@ -51,8 +52,8 @@ func (d Decision) Resume(pattern, reason string) runtime.ResumeRequest { switch d { case ApproveTool: return runtime.ResumeApproveTool(pattern) - case ApproveSession: - return runtime.ResumeApproveSession() + case ApproveAutonomous: + return runtime.ResumeApproveAutonomous() case Reject: return runtime.ResumeReject(reason) default: @@ -109,7 +110,7 @@ func DecisionForAction(action string) (decision Decision, ok bool) { case "T": return ApproveTool, true case "A": - return ApproveSession, true + return ApproveAutonomous, true } return 0, false } @@ -146,7 +147,7 @@ func (k KeyMap) DecisionFor(msg tea.KeyPressMsg) (decision Decision, ok bool) { case key.Matches(msg, k.ThisTool): return ApproveTool, true case key.Matches(msg, k.All): - return ApproveSession, true + return ApproveAutonomous, true } return 0, false } diff --git a/pkg/tui/components/toolconfirm/toolconfirm_test.go b/pkg/tui/components/toolconfirm/toolconfirm_test.go index 33c0100790..62d5725820 100644 --- a/pkg/tui/components/toolconfirm/toolconfirm_test.go +++ b/pkg/tui/components/toolconfirm/toolconfirm_test.go @@ -86,7 +86,7 @@ func TestDecisionResume(t *testing.T) { t.Parallel() assert.Equal(t, runtime.ResumeApprove(), Approve.Resume("", "")) assert.Equal(t, runtime.ResumeApproveTool("shell:cmd=ls*"), ApproveTool.Resume("shell:cmd=ls*", "")) - assert.Equal(t, runtime.ResumeApproveSession(), ApproveSession.Resume("", "")) + assert.Equal(t, runtime.ResumeApproveAutonomous(), ApproveAutonomous.Resume("", "")) assert.Equal(t, runtime.ResumeReject("too risky"), Reject.Resume("", "too risky")) } @@ -117,8 +117,8 @@ func TestKeyMapDecisionFor(t *testing.T) { {"N", Reject}, {"t", ApproveTool}, {"T", ApproveTool}, - {"a", ApproveSession}, - {"A", ApproveSession}, + {"a", ApproveAutonomous}, + {"A", ApproveAutonomous}, } { decision, ok := keyMap.DecisionFor(tea.KeyPressMsg{Code: rune(tt.key[0]), Text: tt.key}) require.True(t, ok, "key %q", tt.key) @@ -132,7 +132,7 @@ func TestKeyMapDecisionFor(t *testing.T) { func TestDecisionForAction(t *testing.T) { t.Parallel() - for i, want := range []Decision{Approve, Reject, ApproveTool, ApproveSession} { + for i, want := range []Decision{Approve, Reject, ApproveTool, ApproveAutonomous} { action := string(ActionKeys[i]) decision, ok := DecisionForAction(action) require.True(t, ok, "action %q", action) diff --git a/pkg/tui/components/tour/steps.go b/pkg/tui/components/tour/steps.go index 91dae96c02..586d28848f 100644 --- a/pkg/tui/components/tour/steps.go +++ b/pkg/tui/components/tour/steps.go @@ -81,7 +81,10 @@ func checkToolCallApproved(msg tea.Msg) bool { return false } switch resume.Request.Type { - case runtime.ResumeTypeApprove, runtime.ResumeTypeApproveTool, runtime.ResumeTypeApproveSession: + case runtime.ResumeTypeApprove, + runtime.ResumeTypeApproveTool, + runtime.ResumeTypeApproveBalanced, + runtime.ResumeTypeApproveAutonomous: return true } return false diff --git a/pkg/tui/components/tour/tour_test.go b/pkg/tui/components/tour/tour_test.go index df3b91a082..4567cc00b2 100644 --- a/pkg/tui/components/tour/tour_test.go +++ b/pkg/tui/components/tour/tour_test.go @@ -103,9 +103,9 @@ func TestObserve_ToolStepChecks(t *testing.T) { t.Parallel() approvals := map[string]tea.Msg{ - "approve": dialog.RuntimeResumeMsg{Request: runtime.ResumeApprove()}, - "approve-tool": dialog.RuntimeResumeMsg{Request: runtime.ResumeApproveTool("shell:cmd=ls*")}, - "approve-session": dialog.RuntimeResumeMsg{Request: runtime.ResumeApproveSession()}, + "approve": dialog.RuntimeResumeMsg{Request: runtime.ResumeApprove()}, + "approve-tool": dialog.RuntimeResumeMsg{Request: runtime.ResumeApproveTool("shell:cmd=ls*")}, + "approve-autonomous": dialog.RuntimeResumeMsg{Request: runtime.ResumeApproveAutonomous()}, } for name, msg := range approvals { m := startedTour() diff --git a/pkg/tui/dialog/tool_confirmation.go b/pkg/tui/dialog/tool_confirmation.go index 156a5b4bc1..2d0a195ec5 100644 --- a/pkg/tui/dialog/tool_confirmation.go +++ b/pkg/tui/dialog/tool_confirmation.go @@ -281,11 +281,11 @@ func (d *toolConfirmationDialog) executeAction(decision toolconfirm.Decision) (l core.CmdHandler(CloseDialogMsg{}), core.CmdHandler(RuntimeResumeMsg{Request: toolconfirm.ApproveTool.Resume(d.permissionPattern, "")}), ) - case toolconfirm.ApproveSession: + case toolconfirm.ApproveAutonomous: d.sessionState.SetYoloMode(true) return d, tea.Sequence( core.CmdHandler(CloseDialogMsg{}), - core.CmdHandler(RuntimeResumeMsg{Request: toolconfirm.ApproveSession.Resume("", "")}), + core.CmdHandler(RuntimeResumeMsg{Request: toolconfirm.ApproveAutonomous.Resume("", "")}), ) } return d, nil From dbda8ff894eda4878fcb95b017e3498e6e6494c1 Mon Sep 17 00:00:00 2001 From: Trung Nguyen Date: Thu, 23 Jul 2026 16:52:21 +0200 Subject: [PATCH 2/9] fix: guard SafetyPolicy reads with GetSafetyPolicy accessor SetSafetyPolicy writes under s.mu but concurrent readers in the dispatcher (permissionDecision, hook input construction) read the field bare, racing with 'approve + switch to Balanced/Autonomous' resumes on sibling in-flight calls. Add a locked GetSafetyPolicy() accessor mirroring IsToolsApproved() and route all runtime reads through it. The debug logs in the dispatcher and session_manager ResumeSession path are dropped; they existed only for local development and were the origin of the racy field reads. --- pkg/runtime/toolexec/dispatcher.go | 18 ++++-------------- pkg/runtime/toolexec/hooks_input.go | 2 +- pkg/server/session_manager.go | 6 ------ pkg/session/session.go | 8 ++++++++ 4 files changed, 13 insertions(+), 21 deletions(-) diff --git a/pkg/runtime/toolexec/dispatcher.go b/pkg/runtime/toolexec/dispatcher.go index b2a23cf120..0e03cea9a6 100644 --- a/pkg/runtime/toolexec/dispatcher.go +++ b/pkg/runtime/toolexec/dispatcher.go @@ -446,23 +446,13 @@ func (c *call) permissionDecision() PermissionDecision { if c.d.Permissions != nil { checkers = c.d.Permissions(c.sess) } - label := c.classifierLabel() - d := Decide( - c.sess.SafetyPolicy, - label, + return Decide( + c.sess.GetSafetyPolicy(), + c.classifierLabel(), checkers, c.tc.Function.Name, ParseToolInput(c.tc.Function.Arguments), ) - slog.Debug("permissionDecision", - "session_id", c.sess.ID, - "tool", c.tc.Function.Name, - "safety_policy", string(c.sess.SafetyPolicy), - "label", label, - "outcome", d.Outcome, - "reason", d.Reason, - "source", d.Source) - return d } // classifierLabel returns the safety label the mode table consumes: @@ -794,7 +784,7 @@ func (c *call) runPermissionRequestHook(ctx context.Context, runTool func() Call ToolName: toolName, ToolUseID: c.tc.ID, ToolInput: ParseToolInput(c.tc.Function.Arguments), - SafetyPolicy: string(c.sess.SafetyPolicy), + SafetyPolicy: string(c.sess.GetSafetyPolicy()), }) if result == nil { return CallOutcome{}, false, nil diff --git a/pkg/runtime/toolexec/hooks_input.go b/pkg/runtime/toolexec/hooks_input.go index 4997d7f692..9ffd126794 100644 --- a/pkg/runtime/toolexec/hooks_input.go +++ b/pkg/runtime/toolexec/hooks_input.go @@ -19,7 +19,7 @@ func NewHooksInput(sess *session.Session, toolCall tools.ToolCall) *hooks.Input ToolName: toolCall.Function.Name, ToolUseID: toolCall.ID, ToolInput: ParseToolInput(toolCall.Function.Arguments), - SafetyPolicy: string(sess.SafetyPolicy), + SafetyPolicy: string(sess.GetSafetyPolicy()), } } diff --git a/pkg/server/session_manager.go b/pkg/server/session_manager.go index da1e64756e..e820b422bf 100644 --- a/pkg/server/session_manager.go +++ b/pkg/server/session_manager.go @@ -926,9 +926,6 @@ func (sm *SessionManager) ResumeSession(ctx context.Context, sessionID, confirma return errors.New("session not found") } - slog.DebugContext(ctx, "ResumeSession received", - "session_id", sessionID, "confirmation", confirmation, "tool_name", toolName) - // Mirror + persist mid-turn session mutations synchronously — // PersistenceObserver only persists on OnRunStart. if rt.session != nil { @@ -949,9 +946,6 @@ func (sm *SessionManager) ResumeSession(ctx context.Context, sessionID, confirma } } if mutated { - slog.DebugContext(ctx, "ResumeSession mutated session", - "session_id", sessionID, "safety_policy", string(rt.session.SafetyPolicy), - "tools_approved", rt.session.ToolsApproved) if err := sm.sessionStore.UpdateSession(ctx, rt.session); err != nil { slog.WarnContext(ctx, "failed to persist mid-turn session state", "session_id", sessionID, "confirmation", confirmation, "err", err) diff --git a/pkg/session/session.go b/pkg/session/session.go index 03664142b9..1fba9cf9b9 100644 --- a/pkg/session/session.go +++ b/pkg/session/session.go @@ -1433,6 +1433,14 @@ func (s *Session) IsToolsApproved() bool { return s.ToolsApproved } +// GetSafetyPolicy returns a consistent snapshot of the SafetyPolicy. +// This is safe to call concurrently with SetSafetyPolicy. +func (s *Session) GetSafetyPolicy() SafetyPolicy { + s.mu.RLock() + defer s.mu.RUnlock() + return s.SafetyPolicy +} + // ClonePermissions returns a deep copy of the session's PermissionsConfig. // This is safe to call concurrently with session mutations. func (s *Session) ClonePermissions() *PermissionsConfig { From 8105dcb00b573ca0d86352d1300e0369143dfbc5 Mon Sep 17 00:00:00 2001 From: Trung Nguyen Date: Thu, 23 Jul 2026 16:52:28 +0200 Subject: [PATCH 3/9] fix: propagate SafetyPolicy to sub-sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SubSessionConfig only carried ToolsApproved. Balanced-mode parents would spawn delegated agents, transferred tasks, and skill runs with an empty SafetyPolicy, silently downgrading them to Strict — Balanced sub-sessions would prompt on every read-only call while their parent flowed through silently. Carry SafetyPolicy alongside ToolsApproved and propagate it via session.WithSafetyPolicy at every sub-session construction site. --- pkg/runtime/agent_delegation.go | 6 ++++++ pkg/runtime/skill_runner.go | 1 + 2 files changed, 7 insertions(+) diff --git a/pkg/runtime/agent_delegation.go b/pkg/runtime/agent_delegation.go index 7ebcfe9fb5..f01325b75e 100644 --- a/pkg/runtime/agent_delegation.go +++ b/pkg/runtime/agent_delegation.go @@ -92,6 +92,9 @@ type SubSessionConfig struct { Title string // ToolsApproved overrides whether tools are pre-approved in the child session. ToolsApproved bool + // SafetyPolicy inherits the parent's mode so a Balanced parent + // doesn't silently downgrade its sub-agents / skill runs to Strict. + SafetyPolicy session.SafetyPolicy // Permissions defines session-level tool permission overrides. Permissions *session.PermissionsConfig // NonInteractive marks the child session as running without a user present @@ -178,6 +181,7 @@ func newSubSession(parent *session.Session, cfg SubSessionConfig, childAgent *ag session.WithMaxToolResultTokens(childAgent.MaxToolResultTokens()), session.WithTitle(cfg.Title), session.WithToolsApproved(cfg.ToolsApproved), + session.WithSafetyPolicy(cfg.SafetyPolicy), session.WithNonInteractive(cfg.NonInteractive), session.WithSendUserMessage(false), session.WithParentID(parent.ID), @@ -532,6 +536,7 @@ func (r *LocalRuntime) RunAgent(ctx context.Context, params agenttool.RunParams) AgentName: params.AgentName, Title: "Background agent task", ToolsApproved: params.ParentSession.IsToolsApproved(), + SafetyPolicy: params.ParentSession.GetSafetyPolicy(), Permissions: params.ParentSession.ClonePermissions(), NonInteractive: true, PinAgent: true, @@ -592,6 +597,7 @@ func (r *LocalRuntime) handleTaskTransfer(ctx context.Context, sess *session.Ses AgentName: params.Agent, Title: "Transferred task", ToolsApproved: sess.IsToolsApproved(), + SafetyPolicy: sess.GetSafetyPolicy(), Permissions: sess.ClonePermissions(), NonInteractive: sess.NonInteractive, }, diff --git a/pkg/runtime/skill_runner.go b/pkg/runtime/skill_runner.go index ba58f51e4b..300c761754 100644 --- a/pkg/runtime/skill_runner.go +++ b/pkg/runtime/skill_runner.go @@ -110,6 +110,7 @@ func (r *LocalRuntime) RunSkillFork(ctx context.Context, sess *session.Session, AgentName: ca, Title: "Skill: " + prepared.SkillName, ToolsApproved: sess.IsToolsApproved(), + SafetyPolicy: sess.GetSafetyPolicy(), Permissions: sess.ClonePermissions(), NonInteractive: sess.NonInteractive, ExcludedTools: []string{skills.ToolNameRunSkill}, From 41c682d0ffbc523d359f89cc138a5c1d49db9814 Mon Sep 17 00:00:00 2001 From: Trung Nguyen Date: Thu, 23 Jul 2026 16:52:33 +0200 Subject: [PATCH 4/9] feat(toolconfirm): expose ApproveBalanced in the shared confirmation surface Add Decision, key binding (B), OptionsHelp entry, and Resume mapping for ApproveBalanced alongside the existing ApproveAutonomous so 'approve + switch to Balanced' is reachable from the confirmation dialog, matching the existing 'approve + switch to Autonomous' shortcut. --- pkg/tui/components/toolconfirm/toolconfirm.go | 20 +++++++++++++++++-- .../toolconfirm/toolconfirm_test.go | 9 ++++++--- pkg/tui/dialog/tool_confirmation.go | 5 +++++ 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/pkg/tui/components/toolconfirm/toolconfirm.go b/pkg/tui/components/toolconfirm/toolconfirm.go index 0706eb32c8..36daf6cd7d 100644 --- a/pkg/tui/components/toolconfirm/toolconfirm.go +++ b/pkg/tui/components/toolconfirm/toolconfirm.go @@ -36,6 +36,10 @@ const ( // ApproveTool runs the call and always allows the tool, scoped by the // permission pattern from BuildPermissionPattern. ApproveTool + // ApproveBalanced runs the call and flips the session to + // SafetyPolicyBalanced (auto-approve classifier-safe calls; ask on + // destructive and unknown). + ApproveBalanced // ApproveAutonomous runs the call and flips the session to // SafetyPolicyAutonomous (auto-approve everything for the rest of // the session). @@ -52,6 +56,8 @@ func (d Decision) Resume(pattern, reason string) runtime.ResumeRequest { switch d { case ApproveTool: return runtime.ResumeApproveTool(pattern) + case ApproveBalanced: + return runtime.ResumeApproveBalanced() case ApproveAutonomous: return runtime.ResumeApproveAutonomous() case Reject: @@ -97,10 +103,10 @@ func AlwaysAllowLabel(pattern string) string { // ActionKeys are the uppercase action letters of the decision row, in // display order. Click hit-testing on the rendered options walks these // letters; map a hit back to its decision with DecisionForAction. -const ActionKeys = "YNTA" +const ActionKeys = "YNTBA" // DecisionForAction maps an action letter from ActionKeys ("Y", "N", "T", -// "A") to its decision. ok is false for any other string. +// "B", "A") to its decision. ok is false for any other string. func DecisionForAction(action string) (decision Decision, ok bool) { switch action { case "Y": @@ -109,6 +115,8 @@ func DecisionForAction(action string) (decision Decision, ok bool) { return Reject, true case "T": return ApproveTool, true + case "B": + return ApproveBalanced, true case "A": return ApproveAutonomous, true } @@ -122,6 +130,7 @@ func OptionsHelp(pattern string) []string { "Y", "yes", "N", "no", "T", AlwaysAllowLabel(pattern), + "B", "balanced", "A", "all tools", } } @@ -131,6 +140,7 @@ type KeyMap struct { Yes key.Binding No key.Binding All key.Binding + Balanced key.Binding ThisTool key.Binding } @@ -146,6 +156,8 @@ func (k KeyMap) DecisionFor(msg tea.KeyPressMsg) (decision Decision, ok bool) { return Reject, true case key.Matches(msg, k.ThisTool): return ApproveTool, true + case key.Matches(msg, k.Balanced): + return ApproveBalanced, true case key.Matches(msg, k.All): return ApproveAutonomous, true } @@ -167,6 +179,10 @@ func DefaultKeyMap() KeyMap { key.WithKeys("a", "A"), key.WithHelp("A", "approve all"), ), + Balanced: key.NewBinding( + key.WithKeys("b", "B"), + key.WithHelp("B", "approve + switch to balanced"), + ), ThisTool: key.NewBinding( key.WithKeys("t", "T"), key.WithHelp("T", "always allow this tool"), diff --git a/pkg/tui/components/toolconfirm/toolconfirm_test.go b/pkg/tui/components/toolconfirm/toolconfirm_test.go index 62d5725820..42d19b03e0 100644 --- a/pkg/tui/components/toolconfirm/toolconfirm_test.go +++ b/pkg/tui/components/toolconfirm/toolconfirm_test.go @@ -78,14 +78,15 @@ func TestAlwaysAllowLabel(t *testing.T) { func TestOptionsHelpUsesThePattern(t *testing.T) { t.Parallel() opts := OptionsHelp("shell:cmd=rm*") - require.Len(t, opts, 8) - assert.Equal(t, []string{"Y", "yes", "N", "no", "T", "always allow rm*", "A", "all tools"}, opts) + require.Len(t, opts, 10) + assert.Equal(t, []string{"Y", "yes", "N", "no", "T", "always allow rm*", "B", "balanced", "A", "all tools"}, opts) } func TestDecisionResume(t *testing.T) { t.Parallel() assert.Equal(t, runtime.ResumeApprove(), Approve.Resume("", "")) assert.Equal(t, runtime.ResumeApproveTool("shell:cmd=ls*"), ApproveTool.Resume("shell:cmd=ls*", "")) + assert.Equal(t, runtime.ResumeApproveBalanced(), ApproveBalanced.Resume("", "")) assert.Equal(t, runtime.ResumeApproveAutonomous(), ApproveAutonomous.Resume("", "")) assert.Equal(t, runtime.ResumeReject("too risky"), Reject.Resume("", "too risky")) } @@ -117,6 +118,8 @@ func TestKeyMapDecisionFor(t *testing.T) { {"N", Reject}, {"t", ApproveTool}, {"T", ApproveTool}, + {"b", ApproveBalanced}, + {"B", ApproveBalanced}, {"a", ApproveAutonomous}, {"A", ApproveAutonomous}, } { @@ -132,7 +135,7 @@ func TestKeyMapDecisionFor(t *testing.T) { func TestDecisionForAction(t *testing.T) { t.Parallel() - for i, want := range []Decision{Approve, Reject, ApproveTool, ApproveAutonomous} { + for i, want := range []Decision{Approve, Reject, ApproveTool, ApproveBalanced, ApproveAutonomous} { action := string(ActionKeys[i]) decision, ok := DecisionForAction(action) require.True(t, ok, "action %q", action) diff --git a/pkg/tui/dialog/tool_confirmation.go b/pkg/tui/dialog/tool_confirmation.go index 2d0a195ec5..de10337dc0 100644 --- a/pkg/tui/dialog/tool_confirmation.go +++ b/pkg/tui/dialog/tool_confirmation.go @@ -281,6 +281,11 @@ func (d *toolConfirmationDialog) executeAction(decision toolconfirm.Decision) (l core.CmdHandler(CloseDialogMsg{}), core.CmdHandler(RuntimeResumeMsg{Request: toolconfirm.ApproveTool.Resume(d.permissionPattern, "")}), ) + case toolconfirm.ApproveBalanced: + return d, tea.Sequence( + core.CmdHandler(CloseDialogMsg{}), + core.CmdHandler(RuntimeResumeMsg{Request: toolconfirm.ApproveBalanced.Resume("", "")}), + ) case toolconfirm.ApproveAutonomous: d.sessionState.SetYoloMode(true) return d, tea.Sequence( From ab6c42bf8aeb6af8c976958d682b9dd69aec23e0 Mon Sep 17 00:00:00 2001 From: Trung Nguyen Date: Thu, 23 Jul 2026 16:57:05 +0200 Subject: [PATCH 5/9] docs: update WithSaferShell godoc for auto-inject + pure-labeller model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Doc-comment still described the old model (opt-in via 'safer: true' on the toolset; hook routes destructive shell commands to confirmation regardless of --yolo). The classifier is now auto-registered whenever an agent has a shell toolset and is a pure labeller — it never blocks, only attaches blast_radius metadata that the runtime consumes. --- pkg/agent/opts.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/agent/opts.go b/pkg/agent/opts.go index 1d15003e59..580b8e2d82 100644 --- a/pkg/agent/opts.go +++ b/pkg/agent/opts.go @@ -169,10 +169,11 @@ func WithRedactSecrets(redactSecrets bool) Opt { } // WithSaferShell registers the safer_shell builtin under pre_tool_use -// with preempt_yolo:true when any of the agent's shell toolsets has -// `safer: true`. The builtin runs pre-Decide() and routes destructive -// shell commands to user confirmation regardless of --yolo or -// permission allow-rules. See [builtins.SaferShell]. +// with preempt_yolo:true whenever the agent declares a shell toolset +// (see [config.AgentConfig.HasShellToolset]). The builtin runs +// pre-Decide() as a pure labeller — it always allows the call and +// attaches classification metadata (blast_radius) that the runtime's +// (mode × label) verdict table consumes. See [builtins.SaferShell]. func WithSaferShell(saferShell bool) Opt { return func(a *Agent) { a.saferShell = saferShell From f5a6524e4840becad43a86a5a6d457af6a27ddfa Mon Sep 17 00:00:00 2001 From: Trung Nguyen Date: Thu, 23 Jul 2026 17:07:16 +0200 Subject: [PATCH 6/9] docs: trim verbose comment blocks Cut narrative prose that restates what the code does or duplicates adjacent godoc. No behaviour change. --- pkg/agent/opts.go | 9 ++--- pkg/config/latest/types.go | 53 +++++++++-------------------- pkg/hooks/builtins/safer_shell.go | 25 ++++---------- pkg/hooks/types.go | 24 ++++--------- pkg/runtime/hooks.go | 15 +++----- pkg/runtime/resume.go | 46 +++++++------------------ pkg/runtime/toolexec/dispatcher.go | 42 +++++++---------------- pkg/runtime/toolexec/permissions.go | 27 +++++---------- 8 files changed, 72 insertions(+), 169 deletions(-) diff --git a/pkg/agent/opts.go b/pkg/agent/opts.go index 580b8e2d82..0e8a9972c7 100644 --- a/pkg/agent/opts.go +++ b/pkg/agent/opts.go @@ -168,12 +168,9 @@ func WithRedactSecrets(redactSecrets bool) Opt { } } -// WithSaferShell registers the safer_shell builtin under pre_tool_use -// with preempt_yolo:true whenever the agent declares a shell toolset -// (see [config.AgentConfig.HasShellToolset]). The builtin runs -// pre-Decide() as a pure labeller — it always allows the call and -// attaches classification metadata (blast_radius) that the runtime's -// (mode × label) verdict table consumes. See [builtins.SaferShell]. +// WithSaferShell registers the safer_shell classifier as a +// preempt_yolo pre_tool_use hook. Pure labeller: always allows, +// tags blast_radius metadata for the mode × label table. func WithSaferShell(saferShell bool) Opt { return func(a *Agent) { a.saferShell = saferShell diff --git a/pkg/config/latest/types.go b/pkg/config/latest/types.go index e2f46cccf3..ad20e1a76b 100644 --- a/pkg/config/latest/types.go +++ b/pkg/config/latest/types.go @@ -996,11 +996,8 @@ func (a *AgentConfig) SessionCompactionEnabled() bool { return *a.SessionCompaction } -// HasShellToolset reports whether any of the agent's toolsets is a -// shell toolset. The runtime uses this to decide whether to auto-inject -// the safer_shell classifier as a pre_tool_use hook — the classifier -// is a pure labeller (never blocks) that emits blast_radius metadata -// consumed by the runtime's (mode × classifier-label) verdict table. +// HasShellToolset gates auto-injection of the safer_shell classifier +// (a pure labeller consumed by the runtime's mode × label table). func (a *AgentConfig) HasShellToolset() bool { if a == nil { return false @@ -2417,17 +2414,11 @@ type RAGFusionConfig struct { Weights map[string]float64 `json:"weights,omitempty"` // Strategy weights for weighted fusion } -// PermissionsConfig represents tool permission configuration. -// Allow/Ask/Deny model. Custom rules always override the session's -// SafetyPolicy (strict / balanced / autonomous): -// - Allow: Tools matching these patterns are auto-approved, even under Strict. -// - Ask: Tools matching these patterns always require user approval, even under Autonomous. -// - Deny: Tools matching these patterns are always rejected. -// -// Patterns support glob-style matching (e.g., "shell", "read_*", "mcp:github:*"). -// Within a single checker, priority is Deny > Allow > Ask; when no -// custom rule matches, the safety mode is applied against the tool's -// classifier label (safe / destructive / unknown) to pick allow / ask. +// PermissionsConfig configures per-tool overrides that layer on top +// of the session's SafetyPolicy. Allow/Deny always win; Ask beats the +// mode only for session-scoped rules (YAML `ask:` is advisory — see +// [toolexec.Decide]). Patterns are glob-style ("shell", "read_*", +// "mcp:github:*"). type PermissionsConfig struct { // Allow lists tool name patterns that are auto-approved without user confirmation Allow []string `json:"allow,omitempty"` @@ -2452,13 +2443,10 @@ type HooksConfig struct { // in the handler when you only want to act on one of them. PostToolUse HookMatcherConfigs `json:"post_tool_use,omitempty" yaml:"post_tool_use,omitempty"` - // PermissionRequest hooks run just before the runtime would prompt - // the user to approve a tool call (i.e. when neither a custom - // rule nor the safety mode auto-approved the call). Hooks may - // auto-allow or auto-deny via hook_specific_output.permission_decision - // so the user is not prompted; otherwise the runtime falls - // through to the usual interactive confirmation. Tool-matched, - // like pre_tool_use. + // PermissionRequest hooks fire right before an interactive + // confirmation prompt. Return DecisionAllow/DecisionDeny to + // short-circuit; anything else falls through to the prompt. + // Tool-matched, like pre_tool_use. PermissionRequest HookMatcherConfigs `json:"permission_request,omitempty" yaml:"permission_request,omitempty"` // SessionStart hooks run when a session begins @@ -2664,20 +2652,11 @@ type HookMatcherConfig struct { // Hooks are the hooks to execute when the matcher matches Hooks HookDefinitions `json:"hooks" yaml:"hooks"` - // PreemptYolo opts a pre_tool_use entry into firing BEFORE the - // deterministic approval pipeline (custom rules + safety mode). - // A deny/ask verdict from a preempting hook cannot be bypassed - // by the safety mode (including Autonomous); an allow verdict is - // advisory (the pipeline still consults custom rules + the mode - // table + the default pre_tool_use lane). Default pre_tool_use - // entries fire AFTER the safety mode's verdict. Only valid on - // pre_tool_use; ignored on other events. - // - // Used by the safer_shell builtin (auto-registered whenever an - // agent has a shell toolset) as a pure labeller — it always - // returns Allow with classification metadata. Custom hooks set - // this to true when they implement a security-critical check - // that must not be bypassed by the safety mode. + // PreemptYolo fires the entry BEFORE the mode/custom-rules + // pipeline. Deny/Ask verdicts cannot be bypassed by any mode + // (including Autonomous); Allow is advisory. Only valid on + // pre_tool_use. Used by the safer_shell classifier (labeller + // only) and by user-authored security-critical checks. PreemptYolo *bool `json:"preempt_yolo,omitempty" yaml:"preempt_yolo,omitempty"` } diff --git a/pkg/hooks/builtins/safer_shell.go b/pkg/hooks/builtins/safer_shell.go index df47754ab6..5ca1de68fe 100644 --- a/pkg/hooks/builtins/safer_shell.go +++ b/pkg/hooks/builtins/safer_shell.go @@ -18,25 +18,15 @@ import ( "github.com/docker/docker-agent/pkg/hooks" ) -// SaferShell is the registered name of the builtin that classifies -// destructive shell commands and forces confirmation by preempting -// --yolo on the pre_tool_use chain. +// SaferShell is the registered name of the builtin. const SaferShell = "safer_shell" -// shellToolName is the tool name that this builtin acts on. The shell -// builtin's canonical name is duplicated here as a string literal so -// pkg/hooks/builtins does not depend on pkg/tools/builtin/shell. The -// name is part of the user-facing wire protocol — if it ever changes, -// the rename is caught by tests in both packages. +// shellToolName duplicated as a literal to avoid depending on +// pkg/tools/builtin/shell. Rename drift is caught by tests in both. const shellToolName = "shell" -// Metadata keys the safer_shell builtin emits. The runtime carries -// these through hooks.Result.Metadata into the tool-call confirmation -// event so renderers can highlight destructive calls. The TUI -// confirmation dialog renders the blast_radius key as a colored badge -// (see pkg/tui/dialog/tool_confirmation.go). Treated as opaque -// strings by the runtime; documented for hook authors who want to -// match the same key names. +// Metadata keys emitted onto hooks.Result.Metadata; consumed by +// LabelFromBlastRadius and by UI renderers for the confirmation card. const ( metaBlastRadius = "blast_radius" metaCategory = "category" @@ -129,9 +119,8 @@ func compileSafe(value any) ([]safePattern, error) { return out, nil } -// blast_radius wire values: "safe" | "low" | "medium" | "high" | -// "unknown". low/medium/high all collapse to "destructive" at the -// runtime layer (see LabelFromBlastRadius). +// blast_radius wire values: safe | low | medium | high | unknown. +// The runtime collapses low/medium/high to "destructive" (see LabelFromBlastRadius). func saferShell(_ context.Context, in *hooks.Input, _ []string) (*hooks.Output, error) { if in == nil || in.HookEventName != hooks.EventPreToolUse { return nil, nil diff --git a/pkg/hooks/types.go b/pkg/hooks/types.go index 298c821c7c..c5eec9a1ec 100644 --- a/pkg/hooks/types.go +++ b/pkg/hooks/types.go @@ -346,25 +346,15 @@ type Input struct { PreviousMaxIterations int `json:"previous_max_iterations,omitempty"` NewMaxIterations int `json:"new_max_iterations,omitempty"` - // OnToolApprovalDecision specific: the verdict resolved by the - // approval chain ("allow", "deny", "canceled") and a stable - // classifier for what produced it ("session_permissions_allow", - // "session_permissions_deny", "team_permissions_allow", - // "team_permissions_deny", "pre_tool_use_hook_allow", - // "pre_tool_use_hook_deny", "permission_request_hook_allow", - // "permission_request_hook_deny", "mode_strict", "mode_balanced", - // "mode_autonomous", "user_approved", "user_approved_balanced", - // "user_approved_autonomous", "user_approved_tool", - // "user_rejected", "context_canceled", "non_interactive_deny"). + // OnToolApprovalDecision specific: verdict + a stable source + // label (mode_*, session_permissions_*, pre_tool_use_hook_*, + // user_*, etc. — see the ApprovalSource* constants in the + // runtime package for the full set). ApprovalDecision string `json:"approval_decision,omitempty"` ApprovalSource string `json:"approval_source,omitempty"` - // SafetyLabel is the safety classifier's verdict for the call - // (safe / destructive / unknown). For shell tools this is - // derived from safer_shell's blast_radius metadata; for other - // tools from readOnlyHint / DestructiveHint annotations. - // Populated on every [EventOnToolApprovalDecision] dispatch so - // audit sinks always see the classification even when the call - // auto-approved under Autonomous. + // SafetyLabel is the classifier verdict (safe / destructive / + // unknown). Populated even when the call auto-approved so audit + // sinks always see the classification. SafetyLabel string `json:"safety_label,omitempty"` // AfterLLMCall specific: per-turn token usage and the computed USD diff --git a/pkg/runtime/hooks.go b/pkg/runtime/hooks.go index 88b444634a..3e1e2acb8f 100644 --- a/pkg/runtime/hooks.go +++ b/pkg/runtime/hooks.go @@ -462,9 +462,8 @@ const ( ApprovalSourceTeamPermissionsDeny = "team_permissions_deny" ApprovalSourcePreToolUseHookAllow = "pre_tool_use_hook_allow" ApprovalSourcePreToolUseHookDeny = "pre_tool_use_hook_deny" - // ApprovalSourceModeStrict / ModeBalanced / ModeAutonomous are - // recorded when the (mode × classifier-label) table produced the - // verdict (i.e. no custom rule matched). + // ApprovalSourceModeStrict / Balanced / Autonomous mark the + // verdict as coming from the mode × classifier-label table. ApprovalSourceModeStrict = "mode_strict" ApprovalSourceModeBalanced = "mode_balanced" ApprovalSourceModeAutonomous = "mode_autonomous" @@ -476,13 +475,9 @@ const ( ApprovalSourceContextCanceled = "context_canceled" ) -// executeOnToolApprovalDecisionHooks fires on_tool_approval_decision -// after the runtime's approval chain has resolved a verdict for a -// tool call. Fired once per call from each return path of -// [executeWithApproval], so a single hook gets one record per tool -// call regardless of which step decided. safetyLabel carries the -// classifier's verdict for the call (safe / destructive / unknown); -// empty when classification didn't run before the decision. +// executeOnToolApprovalDecisionHooks fires exactly once per tool +// call after the approval chain resolves. safetyLabel is empty when +// classification didn't run before the decision (e.g. context cancel). func (r *LocalRuntime) executeOnToolApprovalDecisionHooks( ctx context.Context, sess *session.Session, diff --git a/pkg/runtime/resume.go b/pkg/runtime/resume.go index 3826cea70e..9b6cf8bc7a 100644 --- a/pkg/runtime/resume.go +++ b/pkg/runtime/resume.go @@ -3,73 +3,53 @@ package runtime import "github.com/docker/docker-agent/pkg/runtime/toolexec" // ResumeType identifies the user's response to a confirmation request. -// -// The runtime emits a TOOL_PERMISSION_REQUEST event whenever a tool call -// requires user approval, then blocks until the embedder calls Resume(...) -// with one of the values below. -// -// ResumeType, ResumeRequest, and the ResumeType* constants are aliased -// from [toolexec] so the dispatcher and the runtime share one definition -// without circular imports. +// Aliased from [toolexec] to keep dispatcher + runtime in sync without +// circular imports. type ResumeType = toolexec.ResumeType const ( // ResumeTypeApprove approves the single pending tool call. ResumeTypeApprove = toolexec.ResumeTypeApprove - // ResumeTypeApproveBalanced approves the pending call and flips the - // session to [session.SafetyPolicyBalanced] so subsequent safe - // tool calls auto-approve. + // ResumeTypeApproveBalanced approves + flips session to + // [session.SafetyPolicyBalanced]. ResumeTypeApproveBalanced = toolexec.ResumeTypeApproveBalanced - // ResumeTypeApproveAutonomous approves the pending call and flips - // the session to [session.SafetyPolicyAutonomous] so subsequent - // tool calls auto-approve. + // ResumeTypeApproveAutonomous approves + flips session to + // [session.SafetyPolicyAutonomous]. ResumeTypeApproveAutonomous = toolexec.ResumeTypeApproveAutonomous - // ResumeTypeApproveTool approves the pending call and every future - // call to the same tool name within the session. + // ResumeTypeApproveTool approves + appends the tool to the + // session's Allow list. ResumeTypeApproveTool = toolexec.ResumeTypeApproveTool // ResumeTypeReject rejects the pending tool call. ResumeTypeReject = toolexec.ResumeTypeReject ) -// ResumeRequest carries the user's confirmation decision along with an optional -// reason (used when rejecting a tool call to help the model understand why). -// The struct fields live in [toolexec.ResumeRequest]; this alias is kept -// for readers who land here from the runtime API. +// ResumeRequest carries the user's confirmation decision plus an +// optional reason (used with Reject to help the model understand why). type ResumeRequest = toolexec.ResumeRequest -// ResumeApprove creates a ResumeRequest to approve a single tool call. func ResumeApprove() ResumeRequest { return ResumeRequest{Type: ResumeTypeApprove} } -// ResumeApproveBalanced creates a ResumeRequest that approves the pending -// call and flips the session to [session.SafetyPolicyBalanced]. func ResumeApproveBalanced() ResumeRequest { return ResumeRequest{Type: ResumeTypeApproveBalanced} } -// ResumeApproveAutonomous creates a ResumeRequest that approves the pending -// call and flips the session to [session.SafetyPolicyAutonomous]. func ResumeApproveAutonomous() ResumeRequest { return ResumeRequest{Type: ResumeTypeApproveAutonomous} } -// ResumeApproveTool creates a ResumeRequest to always approve a specific tool for the session. func ResumeApproveTool(toolName string) ResumeRequest { return ResumeRequest{Type: ResumeTypeApproveTool, ToolName: toolName} } -// ResumeReject creates a ResumeRequest to reject a tool call with an optional reason. func ResumeReject(reason string) ResumeRequest { return ResumeRequest{Type: ResumeTypeReject, Reason: reason} } -// IsValidResumeType validates confirmation values coming from /resume. -// -// The runtime may be resumed by multiple entry points (API, CLI, TUI, tests). -// Even if upstream layers perform validation, the runtime must never assume -// the ResumeType is valid; accepting invalid values leads to confusing -// downstream behaviour where tool execution fails without a clear cause. +// IsValidResumeType rejects unknown ResumeType values arriving from +// external callers (API, CLI, TUI, tests). Unvalidated values lead to +// tool execution failing without a clear cause. func IsValidResumeType(t ResumeType) bool { switch t { case ResumeTypeApprove, diff --git a/pkg/runtime/toolexec/dispatcher.go b/pkg/runtime/toolexec/dispatcher.go index 0e03cea9a6..5264e07dfb 100644 --- a/pkg/runtime/toolexec/dispatcher.go +++ b/pkg/runtime/toolexec/dispatcher.go @@ -43,27 +43,22 @@ const ( ApprovalSourcePreToolUseHookDeny = "pre_tool_use_hook_deny" ApprovalSourcePermissionRequestHookDeny = "permission_request_hook_deny" ApprovalSourcePermissionRequestHookAllow = "permission_request_hook_allow" - // ApprovalSourceModeStrict / ModeBalanced / ModeAutonomous are - // recorded when the (mode × classifier-label) table produced the - // verdict (i.e. no custom rule matched). + // ApprovalSourceModeStrict / Balanced / Autonomous mark the + // verdict as coming from the mode × classifier-label table. ApprovalSourceModeStrict = "mode_strict" ApprovalSourceModeBalanced = "mode_balanced" ApprovalSourceModeAutonomous = "mode_autonomous" - ApprovalSourceUserApproved = "user_approved" - // ApprovalSourceUserApprovedBalanced / Autonomous are recorded - // when the user picked a bulk-approve verb from a confirmation - // prompt that also flipped the session's SafetyPolicy. + // ApprovalSourceUserApproved / Balanced / Autonomous / Tool / + // Rejected mark bulk-approve verbs picked from a prompt. + ApprovalSourceUserApproved = "user_approved" ApprovalSourceUserApprovedBalanced = "user_approved_balanced" ApprovalSourceUserApprovedAutonomous = "user_approved_autonomous" ApprovalSourceUserApprovedTool = "user_approved_tool" ApprovalSourceUserRejected = "user_rejected" ApprovalSourceContextCanceled = "context_canceled" - // ApprovalSourceNonInteractiveDeny is recorded when a tool call - // reaches [call.askUser] in a non-interactive session (eval, MCP - // serve, A2A adapter, …). With no human at the keyboard and no - // Resume listener, the deterministic safe answer is Deny; without - // this guard the dispatcher would block on the Resume channel - // forever. + // ApprovalSourceNonInteractiveDeny is the auto-deny recorded when + // askUser is reached in a non-interactive session (eval, MCP + // serve, A2A) with no Resume listener. ApprovalSourceNonInteractiveDeny = "non_interactive_deny" ) @@ -375,10 +370,7 @@ func (c *call) run(ctx context.Context) CallOutcome { // Deny/Allow/Ask and rewrite args via UpdatedInput. // 3. askUser — prompt (or auto-deny non-interactively). func (c *call) approveAndRun(ctx context.Context, runTool func() CallOutcome) CallOutcome { - // Stage 0: pre_tool_use entries flagged with preempt_yolo:true. - // The safer_shell classifier lives here as a pure labeller (always - // Allow + metadata); user-authored hooks can still Deny/Ask to - // override every downstream stage. + // Stage 0: preempt_yolo pre_tool_use. if r := c.consultPreToolUsePreYolo(ctx); r != nil { switch r.Decision { case hooks.DecisionDeny: @@ -391,10 +383,8 @@ func (c *call) approveAndRun(ctx context.Context, runTool func() CallOutcome) Ca c.errorResponse(ctx, rejectMsg) return CallOutcome{} case hooks.DecisionAsk: - // A session-scoped allow grant (the interactive "T = always - // allow this tool" decision, stored in sess.Permissions) is an - // informed opt-in the user made in response to this very safety - // prompt. Honor it instead of asking again. + // Honor a session-scoped Allow grant — it's an informed opt-in + // from the user, not something to re-prompt on. if c.sessionPermissionsAllow() { slog.DebugContext(ctx, "preempt-yolo Ask overridden by session permission allow", "tool", c.tc.Function.Name, "session_id", c.sess.ID) c.notifyApproval(ctx, ApprovalDecisionAllow, ApprovalSourceSessionPermissionsAllow) @@ -420,15 +410,13 @@ func (c *call) approveAndRun(ctx context.Context, runTool func() CallOutcome) Ca return CallOutcome{} case OutcomeAsk: if decision.Reason == ReasonChecker { - // Explicit ask pattern from a checker: skip the hook and - // prompt the user directly. The user is the source of - // truth for explicit ask rules. + // Explicit ask rule: prompt directly, skip the hook chain. slog.DebugContext(ctx, "Tool requires confirmation (ask pattern)", "tool", c.tc.Function.Name, "source", decision.Source, "session_id", c.sess.ID) return c.askUser(ctx, runTool) } } - // Stage 2: consult the default pre_tool_use hook chain (non-preempt). + // Stage 2: default pre_tool_use. if outcome, handled := c.consultPreToolUseHook(ctx, runTool); handled { return outcome } @@ -437,10 +425,6 @@ func (c *call) approveAndRun(ctx context.Context, runTool func() CallOutcome) Ca return c.askUser(ctx, runTool) } -// permissionDecision applies the runtime approval pipeline for this -// call: custom checkers first (Deny / Allow / ForceAsk always win), -// then the (mode × classifier-label) verdict table. See [Decide] for -// the full semantics. func (c *call) permissionDecision() PermissionDecision { var checkers []NamedChecker if c.d.Permissions != nil { diff --git a/pkg/runtime/toolexec/permissions.go b/pkg/runtime/toolexec/permissions.go index 6cf171e5b9..e924a06bd3 100644 --- a/pkg/runtime/toolexec/permissions.go +++ b/pkg/runtime/toolexec/permissions.go @@ -63,25 +63,16 @@ type PermissionDecision struct { Source string } -// SessionPermissionsSource is the checker source label the runtime -// uses for the session-scoped permission layer (interactive "T = -// always allow" grants and mid-session API mutations). Session -// ForceAsk beats the mode; team ForceAsk is advisory. +// SessionPermissionsSource labels the session-scoped checker layer. +// Session ForceAsk beats the mode; team ForceAsk is advisory. const SessionPermissionsSource = "session permissions" // Decide resolves the final permission outcome: -// -// - Deny (any checker) → Deny. -// - Allow (any checker) → Allow. -// - ForceAsk (session-level only) → Ask, beats the mode. -// - Otherwise → (mode × label) verdict table. -// -// Team-level ForceAsk is deliberately advisory: a YAML rule like -// `ask: "*"` should not defeat Balanced/Autonomous. Users who want -// targeted asks that beat the mode add them via the Custom rules UI, -// which populates the session-level checker. -// -// Pure so the matrix is unit-testable. +// - Deny/Allow from any checker win outright. +// - ForceAsk wins only when it comes from the session layer; +// team-level ForceAsk falls through so Balanced/Autonomous can +// override it. +// - Otherwise applies the (mode × label) table. func Decide( mode session.SafetyPolicy, label string, @@ -108,9 +99,7 @@ func Decide( } // applyMode implements the (mode × label) → verdict table. Empty / -// unrecognised modes fall back to Strict. Source is the -// ApprovalSourceMode* constant so [allowSourceForDecision] can return -// it verbatim without a second lookup. +// unrecognised modes fall back to Strict. // // safe destructive unknown // Strict ask ask ask From 05c538c4c57dfbc8d991cd9036b6f5c6ff2bb954 Mon Sep 17 00:00:00 2001 From: Trung Nguyen Date: Thu, 23 Jul 2026 17:31:29 +0200 Subject: [PATCH 7/9] fix: restore read-only auto-approve so Strict matches pre-PR default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PR claimed 'Strict is unchanged from today's no-yolo default — every call prompts.' In practice the old no-yolo default did auto- approve tools annotated ReadOnlyHint; removing that short-circuit broke headless callers (--exec, MCP serve, A2A) that relied on read-only tools flowing through without a user to prompt. Reintroduce the short-circuit right before the fallback ask so Balanced/Autonomous keep going through the mode table and Strict regains the read-only escape hatch. --- pkg/runtime/hooks.go | 1 + pkg/runtime/toolexec/dispatcher.go | 12 +++++ pkg/runtime/toolexec/dispatcher_test.go | 58 ++++--------------------- 3 files changed, 21 insertions(+), 50 deletions(-) diff --git a/pkg/runtime/hooks.go b/pkg/runtime/hooks.go index 3e1e2acb8f..6e3f5809b9 100644 --- a/pkg/runtime/hooks.go +++ b/pkg/runtime/hooks.go @@ -462,6 +462,7 @@ const ( ApprovalSourceTeamPermissionsDeny = "team_permissions_deny" ApprovalSourcePreToolUseHookAllow = "pre_tool_use_hook_allow" ApprovalSourcePreToolUseHookDeny = "pre_tool_use_hook_deny" + ApprovalSourceReadOnlyHint = "readonly_hint" // ApprovalSourceModeStrict / Balanced / Autonomous mark the // verdict as coming from the mode × classifier-label table. ApprovalSourceModeStrict = "mode_strict" diff --git a/pkg/runtime/toolexec/dispatcher.go b/pkg/runtime/toolexec/dispatcher.go index 5264e07dfb..b7421278d2 100644 --- a/pkg/runtime/toolexec/dispatcher.go +++ b/pkg/runtime/toolexec/dispatcher.go @@ -43,6 +43,10 @@ const ( ApprovalSourcePreToolUseHookDeny = "pre_tool_use_hook_deny" ApprovalSourcePermissionRequestHookDeny = "permission_request_hook_deny" ApprovalSourcePermissionRequestHookAllow = "permission_request_hook_allow" + // ApprovalSourceReadOnlyHint short-circuits the ask stage for + // tools annotated read-only. Preserves the pre-three-mode default + // of auto-approving read-only calls under Strict. + ApprovalSourceReadOnlyHint = "readonly_hint" // ApprovalSourceModeStrict / Balanced / Autonomous mark the // verdict as coming from the mode × classifier-label table. ApprovalSourceModeStrict = "mode_strict" @@ -421,6 +425,14 @@ func (c *call) approveAndRun(ctx context.Context, runTool func() CallOutcome) Ca return outcome } + // Read-only tools bypass the prompt — this preserves the + // pre-three-mode default. Balanced/Autonomous already allowed + // them via the mode table; this catches Strict. + if c.tool.Annotations.ReadOnlyHint { + c.notifyApproval(ctx, ApprovalDecisionAllow, ApprovalSourceReadOnlyHint) + return runTool() + } + // Stage 3: fallback prompt. return c.askUser(ctx, runTool) } diff --git a/pkg/runtime/toolexec/dispatcher_test.go b/pkg/runtime/toolexec/dispatcher_test.go index a864657ffe..10fa8aaf6a 100644 --- a/pkg/runtime/toolexec/dispatcher_test.go +++ b/pkg/runtime/toolexec/dispatcher_test.go @@ -664,80 +664,38 @@ func TestDispatcher_ApproveBalancedAutoApprovesSiblingSafeShellCalls(t *testing. "only the first call should prompt; balanced flip must auto-approve safe siblings") } -// A batch of safe tool calls under Strict: user approve-balanced on -// the first prompt should auto-approve every remaining call in the -// batch without emitting a second confirmation event. Regression test -// for a Gordon bug where each parallel call kept surfacing its own -// prompt even after the mode flipped. -func TestDispatcher_ApproveBalancedAutoApprovesSiblingSafeCalls(t *testing.T) { - t.Parallel() - a := newAgent() - sess := session.New() - - var ran atomic.Int32 - tool := tools.Tool{ - Name: "read_file", - Annotations: tools.ToolAnnotations{ReadOnlyHint: true}, - Handler: func(context.Context, tools.ToolCall, tools.Runtime) (*tools.ToolCallResult, error) { - ran.Add(1) - return tools.ResultSuccess("contents"), nil - }, - } - - resume := make(chan toolexec.ResumeRequest, 1) - d := &toolexec.Dispatcher{ - AgentFor: func(*session.Session) *agent.Agent { return a }, - Resume: resume, - } - em := &captureEmitter{confirmed: make(chan struct{})} - - go func() { - <-em.confirmed - resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeApproveBalanced} - }() - - d.Process(t.Context(), sess, []tools.ToolCall{ - {ID: "a", Function: tools.FunctionCall{Name: "read_file", Arguments: `{"path":"a"}`}}, - {ID: "b", Function: tools.FunctionCall{Name: "read_file", Arguments: `{"path":"b"}`}}, - {ID: "c", Function: tools.FunctionCall{Name: "read_file", Arguments: `{"path":"c"}`}}, - }, []tools.Tool{tool}, em) - - assert.Equal(t, int32(3), ran.Load(), "every safe sibling must run") - assert.Len(t, em.confirmations, 1, - "only the first call should prompt; the balanced flip must auto-approve the siblings") -} - -// Under Strict (default), ReadOnlyHint tools prompt the user. -func TestDispatcher_ReadOnlyHintPromptsUnderStrict(t *testing.T) { +// Under Strict (default), ReadOnlyHint tools bypass the prompt — +// preserves the pre-three-mode default of auto-approving read-only +// calls so headless callers ({--exec}, MCP, A2A) don't regress. +func TestDispatcher_ReadOnlyHintAutoApprovesUnderStrict(t *testing.T) { t.Parallel() a := newAgent() sess := session.New() // empty policy ≡ strict + var ran atomic.Int32 tool := tools.Tool{ Name: "read_file", Annotations: tools.ToolAnnotations{ ReadOnlyHint: true, }, Handler: func(context.Context, tools.ToolCall, tools.Runtime) (*tools.ToolCallResult, error) { + ran.Add(1) return tools.ResultSuccess("contents"), nil }, } - resume := make(chan toolexec.ResumeRequest, 1) d := &toolexec.Dispatcher{ AgentFor: func(*session.Session) *agent.Agent { return a }, - Resume: resume, } em := &captureEmitter{} - resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeReject} - d.Process(t.Context(), sess, []tools.ToolCall{{ ID: "r", Function: tools.FunctionCall{Name: "read_file", Arguments: "{}"}, }}, []tools.Tool{tool}, em) - require.Len(t, em.confirmations, 1, "strict must prompt even for read-only tools") + assert.Equal(t, int32(1), ran.Load(), "read-only tool must run") + assert.Empty(t, em.confirmations, "read-only tool must not prompt under Strict") } // DestructiveHint on a tool that reaches the confirmation event must From 7e1ca2a1d4a93f44c225bbd1dd213b45ba5627ff Mon Sep 17 00:00:00 2001 From: Trung Nguyen Date: Thu, 23 Jul 2026 17:31:29 +0200 Subject: [PATCH 8/9] feat(leantui): add [b] key for approve-balanced The full TUI dialog exposes ApproveBalanced via 'B'; leantui only offered [s] for Autonomous. Add [b] so leantui users can opt into Balanced from a confirmation prompt. --- pkg/leantui/ui/screen.go | 2 +- pkg/leantui/update.go | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/leantui/ui/screen.go b/pkg/leantui/ui/screen.go index 91abe1d84e..af18a792b1 100644 --- a/pkg/leantui/ui/screen.go +++ b/pkg/leantui/ui/screen.go @@ -56,6 +56,6 @@ type ConfirmModel struct { func (c *ConfirmModel) Render(width int) []string { lines := []string{Truncate(StWarning().Render("● Approve tool call"), width)} lines = append(lines, RenderTool(c.View, width)...) - lines = append(lines, Truncate(StMuted().Render("[y] yes [a] always this tool [s] whole session [n] no"), width)) + lines = append(lines, Truncate(StMuted().Render("[y] yes [a] always this tool [b] balanced [s] whole session [n] no"), width)) return lines } diff --git a/pkg/leantui/update.go b/pkg/leantui/update.go index a2ba6a33c3..dd6c4a6e40 100644 --- a/pkg/leantui/update.go +++ b/pkg/leantui/update.go @@ -442,6 +442,8 @@ func (m *model) handleConfirmKey(k ui.Key) { m.resolveConfirm(runtime.ResumeApprove()) case 'a', 'A': m.resolveConfirm(runtime.ResumeApproveTool(m.screen.Confirm.Tool)) + case 'b', 'B': + m.resolveConfirm(runtime.ResumeApproveBalanced()) case 's', 'S': m.resolveConfirm(runtime.ResumeApproveAutonomous()) case 'n', 'N': From 341315249408c9808a44d20f0f544a6409c2804e Mon Sep 17 00:00:00 2001 From: Trung Nguyen Date: Thu, 23 Jul 2026 17:46:27 +0200 Subject: [PATCH 9/9] refactor: collapse LabelFromReadOnlyHint + LabelWithDestructiveHint Two-function chain where the second was the sole caller of the first. Merge into a single LabelFromAnnotations that takes the ToolAnnotations struct directly, matching the shape of the caller. --- pkg/runtime/toolexec/dispatcher.go | 2 +- pkg/runtime/toolexec/permissions.go | 21 +++++-------- pkg/runtime/toolexec/permissions_test.go | 39 +++++++++++++----------- pkg/session/session.go | 8 ++--- 4 files changed, 34 insertions(+), 36 deletions(-) diff --git a/pkg/runtime/toolexec/dispatcher.go b/pkg/runtime/toolexec/dispatcher.go index b7421278d2..e892d590bd 100644 --- a/pkg/runtime/toolexec/dispatcher.go +++ b/pkg/runtime/toolexec/dispatcher.go @@ -462,7 +462,7 @@ func (c *call) classifierLabel() string { } return SafetyLabelUnknown } - return LabelWithDestructiveHint(c.tool.Annotations.ReadOnlyHint, c.tool.Annotations.DestructiveHint) + return LabelFromAnnotations(c.tool.Annotations) } func (c *call) autoApprovalAfterConfirmationWait() (PermissionDecision, bool) { diff --git a/pkg/runtime/toolexec/permissions.go b/pkg/runtime/toolexec/permissions.go index e924a06bd3..58fed2a376 100644 --- a/pkg/runtime/toolexec/permissions.go +++ b/pkg/runtime/toolexec/permissions.go @@ -3,6 +3,7 @@ package toolexec import ( "github.com/docker/docker-agent/pkg/permissions" "github.com/docker/docker-agent/pkg/session" + "github.com/docker/docker-agent/pkg/tools" ) // PermissionOutcome is the resolved decision after evaluating the full @@ -119,20 +120,14 @@ func applyMode(mode session.SafetyPolicy, label string) PermissionDecision { } } -// LabelFromReadOnlyHint: readOnlyHint=true ⇒ safe; false ⇒ unknown. -func LabelFromReadOnlyHint(readOnlyHint bool) string { - if readOnlyHint { +// LabelFromAnnotations maps a tool's MCP hints to a safety label: +// DestructiveHint wins, then ReadOnlyHint → safe, else unknown. +func LabelFromAnnotations(a tools.ToolAnnotations) string { + if a.DestructiveHint != nil && *a.DestructiveHint { + return SafetyLabelDestructive + } + if a.ReadOnlyHint { return SafetyLabelSafe } return SafetyLabelUnknown } - -// LabelWithDestructiveHint upgrades to destructive when the tool's -// DestructiveHint annotation is set; otherwise falls back to -// [LabelFromReadOnlyHint]. -func LabelWithDestructiveHint(readOnlyHint bool, destructiveHint *bool) string { - if destructiveHint != nil && *destructiveHint { - return SafetyLabelDestructive - } - return LabelFromReadOnlyHint(readOnlyHint) -} diff --git a/pkg/runtime/toolexec/permissions_test.go b/pkg/runtime/toolexec/permissions_test.go index 8325f5bf5f..edceab44cb 100644 --- a/pkg/runtime/toolexec/permissions_test.go +++ b/pkg/runtime/toolexec/permissions_test.go @@ -8,6 +8,7 @@ import ( "github.com/docker/docker-agent/pkg/config/latest" "github.com/docker/docker-agent/pkg/permissions" "github.com/docker/docker-agent/pkg/session" + "github.com/docker/docker-agent/pkg/tools" ) func newChecker(t *testing.T, allow, ask, deny []string) *permissions.Checker { @@ -149,24 +150,28 @@ func TestDecide_EmptyModeDefaultsToStrict(t *testing.T) { assert.Equal(t, PermissionDecision{Outcome: OutcomeAsk, Reason: ReasonMode, Source: "mode_strict"}, d) } -// LabelFromReadOnlyHint: readOnlyHint=true → safe, false → unknown. -func TestLabelFromReadOnlyHint(t *testing.T) { - t.Parallel() - assert.Equal(t, SafetyLabelSafe, LabelFromReadOnlyHint(true)) - assert.Equal(t, SafetyLabelUnknown, LabelFromReadOnlyHint(false)) -} - -// LabelWithDestructiveHint: an explicit destructive annotation -// upgrades a non-shell tool to the destructive label regardless of -// readOnlyHint. -func TestLabelWithDestructiveHint(t *testing.T) { +// LabelFromAnnotations: DestructiveHint wins, then ReadOnlyHint → +// safe, else unknown. +func TestLabelFromAnnotations(t *testing.T) { t.Parallel() trueVal := true falseVal := false - assert.Equal(t, SafetyLabelDestructive, LabelWithDestructiveHint(false, &trueVal)) - assert.Equal(t, SafetyLabelDestructive, LabelWithDestructiveHint(true, &trueVal)) - assert.Equal(t, SafetyLabelSafe, LabelWithDestructiveHint(true, &falseVal)) - assert.Equal(t, SafetyLabelUnknown, LabelWithDestructiveHint(false, &falseVal)) - assert.Equal(t, SafetyLabelUnknown, LabelWithDestructiveHint(false, nil)) - assert.Equal(t, SafetyLabelSafe, LabelWithDestructiveHint(true, nil)) + cases := []struct { + name string + in tools.ToolAnnotations + want string + }{ + {"destructive beats read-only", tools.ToolAnnotations{ReadOnlyHint: true, DestructiveHint: &trueVal}, SafetyLabelDestructive}, + {"destructive alone", tools.ToolAnnotations{DestructiveHint: &trueVal}, SafetyLabelDestructive}, + {"read-only, destructive=false", tools.ToolAnnotations{ReadOnlyHint: true, DestructiveHint: &falseVal}, SafetyLabelSafe}, + {"read-only, destructive nil", tools.ToolAnnotations{ReadOnlyHint: true}, SafetyLabelSafe}, + {"neither", tools.ToolAnnotations{}, SafetyLabelUnknown}, + {"destructive=false only", tools.ToolAnnotations{DestructiveHint: &falseVal}, SafetyLabelUnknown}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, LabelFromAnnotations(tc.in)) + }) + } } diff --git a/pkg/session/session.go b/pkg/session/session.go index 1fba9cf9b9..48f0eae8aa 100644 --- a/pkg/session/session.go +++ b/pkg/session/session.go @@ -1468,11 +1468,9 @@ func (s *Session) SetToolsApproved(approved bool) { } } -// SetSafetyPolicy updates the session's SafetyPolicy under s.mu. -// Mirrors WithSafetyPolicy: setting autonomous also flips ToolsApproved -// so legacy branches on ToolsApproved keep working. Runtime callers -// use this to persist a user's mid-session mode change (e.g. opting -// into balanced from a confirmation prompt). +// SetSafetyPolicy updates SafetyPolicy under s.mu. Setting Autonomous +// also flips ToolsApproved to keep legacy branches on that flag +// working. func (s *Session) SetSafetyPolicy(policy SafetyPolicy) { s.mu.Lock() defer s.mu.Unlock()