From 500312ce36b2bb0822b707e3bef1961c4b867ac9 Mon Sep 17 00:00:00 2001 From: Giuseppe Scuglia Date: Sun, 11 Oct 2026 19:00:22 +0200 Subject: [PATCH 1/8] fix(engine): evaluate tool permissions on the same argument decode the tool executes Permission evaluation, approval display and tool execution now read a tool call's arguments through one decode, governance.DecodeToolArgs, which session.ParseArgs and the policy extractors share. governance.CheckToolArgs refuses argument objects that repeat a key, have top-level keys equal under Unicode case folding, or spell a policy-read argument in another case. Engine.permissionDecision applies it before policy evaluation, so such calls are rejected with a tool error before any rule, guardrail, approval, hook or tool sees them. The evaluator and ParseArgs apply the same check. The tests that pinned the old case-insensitive, last-key-wins reading now assert rejection, including the ambiguous cases added by #2263. Closes #2264 Co-Authored-By: Claude Opus 5.5 (1M context) --- engine/CHANGELOG.md | 27 +- engine/adapter/fstools/local_operands_test.go | 86 +++-- engine/agent/actionreview.go | 2 +- engine/agent/authority_disclosure.go | 6 +- engine/agent/dispatch.go | 22 +- engine/agent/policy_args_decode_test.go | 293 +++++++++++++++++ engine/agent/repair2b_test.go | 18 +- engine/agent/subagent.go | 2 +- engine/agent/temp_scope_permission_test.go | 47 ++- engine/api/governance.txt | 4 + engine/governance/args.go | 298 ++++++++++++++++++ engine/governance/args_test.go | 249 +++++++++++++++ engine/governance/evaluator.go | 97 ++++-- engine/session/toolcall.go | 9 +- engine/session/toolcall_test.go | 20 ++ engine/tool/toolsearch.go | 6 +- .../app/contextual_build_evidence_test.go | 109 ++++--- internal/app/escapeclassifier.go | 3 +- internal/app/escapepolicy.go | 2 +- .../permissions-and-posture.md | 6 + 20 files changed, 1156 insertions(+), 150 deletions(-) create mode 100644 engine/agent/policy_args_decode_test.go create mode 100644 engine/governance/args.go create mode 100644 engine/governance/args_test.go diff --git a/engine/CHANGELOG.md b/engine/CHANGELOG.md index 2e8fd643b5..b9f4394730 100644 --- a/engine/CHANGELOG.md +++ b/engine/CHANGELOG.md @@ -31,6 +31,13 @@ The covered surface is the eight core packages (`session`, `governance`, `learni and `ExecutionTemplateRevision` labels for exact public-create retry matching. Execution reattachment still uses the durable environment reference. Added (minor). +- **Shared tool-argument decode (issue #2264)** — adds + `governance.CheckToolArgs`, `governance.DecodeToolArgs`, + `governance.ErrAmbiguousToolArgs`, and `governance.AmbiguousArgsReason`. + `DecodeToolArgs` is the one decode of a tool call's arguments used by + permission evaluation, approval display, and execution (`session.ParseArgs` + delegates to it). Added (minor). + - **File-backed hierarchy prompt flag** — adds `prompt.Config.ProjectInstructionHierarchy` to render the next-request discovery limitation through the default builder, without appending to host-owned prompts. Added (minor). @@ -388,7 +395,7 @@ The covered surface is the eight core packages (`session`, `governance`, `learni `temp_scope: "system"` previously surfaced an approval whose args held only the first two words of the command (`command_summary`). The ask now carries `{"command", "timeout_ms", "background", "temp_scope"}` re-encoded from the - decoded call, so duplicate or case-variant keys show the value that runs. + decoded call. The `command_summary` field is removed; no in-repo client read it. Runner-owned temporary paths and environment values still never appear. Subagent asks follow suit: the isolation auto-approve no longer clears a @@ -422,6 +429,24 @@ The covered surface is the eight core packages (`session`, `governance`, `learni pending recovery with explicit settlement. Changed (breaking behavioral contract, pre-v1 minor); exported signatures and API snapshots are unchanged. +- **Ambiguous tool arguments are rejected (issue #2264)** — permission + evaluation and tool execution now read a tool call's arguments through one + decode, so the policy evaluates exactly the values the tool runs with. An + argument object that repeats a key at any depth, has two top-level keys equal + under Unicode case folding, or spells a policy-read argument (`command`, `cmd`, + `temp_scope`, `path`, `file_path`, `pattern`, `url`, `query`, `source`, + `destination`, `server`, `tool`) in another case is refused before policy + evaluation with a `permission denied: ambiguous tool arguments: …` tool + error; no approval is surfaced and nothing executes. `session.ParseArgs` also + refuses a key that names a tagged field of its destination only in another + case. `governance.Evaluator.EvaluateWith` denies such calls, + `Evaluator.LearnableRule` refuses them, and `governance.ShellCommandFromArgs` + returns `("", false)` for them. This replaces the earlier case-insensitive, + last-key-wins reading of `tool.LocalFileOperands` (issue #2094) for those + shapes. Callers that sent case-variant or repeated keys must send each + argument once, spelled as in the tool schema. Changed (pre-v1 minor); no + exported signatures changed. + - **Target-scoped project guidance and ephemeral snapshot** — `prompt.InstructionAssembler` now requires `Assemble(context.Context, []string, *session.InstructionSnapshot, int)` and `TargetScoped() bool`. `prompt.AssembleWithManifest` requires the directory diff --git a/engine/adapter/fstools/local_operands_test.go b/engine/adapter/fstools/local_operands_test.go index d5e2f206cb..715245ce01 100644 --- a/engine/adapter/fstools/local_operands_test.go +++ b/engine/adapter/fstools/local_operands_test.go @@ -5,6 +5,7 @@ import ( "errors" "io/fs" "slices" + "strings" "testing" "github.com/stacklok/mecatl/engine/adapter/memfs" @@ -23,13 +24,15 @@ func TestLocalFileOperandsReadExecution(t *testing.T) { wantError bool }{ {`{"path":"a"}`, "a", false}, - {`{"Path":"b"}`, "b", false}, - {`{"path":"a","Path":"b"}`, "b", false}, - {`{"Path":"b","path":"a"}`, "a", false}, - {`{"path":"a","path":"b"}`, "b", false}, - {`{"path":"b","path":"a"}`, "a", false}, - {`{"path":"a","path":null}`, "a", false}, - {`{"path":null,"path":"a"}`, "a", false}, + // Ambiguous argument objects (a repeated key, or a field spelled in + // another case) yield no operands and are refused by the tool. + {`{"Path":"b"}`, "", true}, + {`{"path":"a","Path":"b"}`, "", true}, + {`{"Path":"b","path":"a"}`, "", true}, + {`{"path":"a","path":"b"}`, "", true}, + {`{"path":"b","path":"a"}`, "", true}, + {`{"path":"a","path":null}`, "", true}, + {`{"path":null,"path":"a"}`, "", true}, {`{"path":1,"path":"a"}`, "", true}, {`{"path":"a","path":1}`, "", true}, {`{"path":null}`, "", true}, @@ -63,6 +66,9 @@ func TestLocalFileOperandsReadExecution(t *testing.T) { } } +// TestLocalFileOperandsPathToolExecution pins that a path tool given a path +// field in two spellings derives no operand and refuses the call, leaving both +// candidate files untouched. func TestLocalFileOperandsPathToolExecution(t *testing.T) { for _, tt := range []struct { tool tool.Tool @@ -88,34 +94,20 @@ func TestLocalFileOperandsPathToolExecution(t *testing.T) { } } call := rawOperandCall(name, tt.args) - if got := tool.LocalFileOperands(name, call.Args); !slices.Equal(got, []string{"b"}) { - t.Fatalf("operands=%q", got) + if got := tool.LocalFileOperands(name, call.Args); got != nil { + t.Fatalf("operands=%q, want none", got) } result := exec(t, tt.tool, call, ws) - if result.IsError { - t.Fatal(result) + if !result.IsError || !strings.Contains(result.Content, "ambiguous tool arguments") { + t.Fatalf("result=%+v, want an ambiguous-arguments error", result) } - switch name { - case "ListDir": - if result.Content != "RIGHT\n" { - t.Fatal(result) - } - case "Read": - if result.Content != " 1\tMARKER_B\n" { - t.Fatal(result) - } - default: - data, err := ws.Read(context.Background(), "b") - if name == "Remove" { - if !errors.Is(err, fs.ErrNotExist) { - t.Fatalf("removed b still exists: %q err=%v", data, err) - } - } else if err != nil || string(data) != "changed" { - t.Fatalf("b=%q err=%v", data, err) - } - data, err = ws.Read(context.Background(), "a") - if err != nil || string(data) != "MARKER_A" { - t.Fatalf("unselected a changed: %q err=%v", data, err) + if name == "ListDir" { + return + } + for path, marker := range map[string]string{"a": "MARKER_A", "b": "MARKER_B"} { + data, err := ws.Read(context.Background(), path) + if err != nil || string(data) != marker { + t.Fatalf("%s changed: %q err=%v", path, data, err) } } }) @@ -130,13 +122,14 @@ func TestLocalFileOperandsCopyMoveExecution(t *testing.T) { source, destination string }{ {`{"source":"a","destination":"x"}`, "a", "x"}, - {`{"source":"a","Source":"b","destination":"x","Destination":"y"}`, "b", "y"}, - {`{"Source":"b","source":"a","Destination":"y","destination":"x"}`, "a", "x"}, - {`{"source":"a","source":"b","destination":"x","destination":"y"}`, "b", "y"}, - {`{"source":"b","source":"a","destination":"y","destination":"x"}`, "a", "x"}, - {`{"source":"a","source":null,"destination":"x","destination":null}`, "a", "x"}, - {`{"source":null,"source":"a","destination":null,"destination":"x"}`, "a", "x"}, - {`{"ſource":"b","destination":"y"}`, "b", "y"}, + // Ambiguous argument objects yield no operands and are refused. + {`{"source":"a","Source":"b","destination":"x","Destination":"y"}`, "", ""}, + {`{"Source":"b","source":"a","Destination":"y","destination":"x"}`, "", ""}, + {`{"source":"a","source":"b","destination":"x","destination":"y"}`, "", ""}, + {`{"source":"b","source":"a","destination":"y","destination":"x"}`, "", ""}, + {`{"source":"a","source":null,"destination":"x","destination":null}`, "", ""}, + {`{"source":null,"source":"a","destination":null,"destination":"x"}`, "", ""}, + {`{"ſource":"b","destination":"y"}`, "", ""}, {`{"source":1,"source":"a","destination":"x"}`, "", ""}, {`{"source":"a","source":1,"destination":"x"}`, "", ""}, {`{"source":"a","destination":1,"destination":"x"}`, "", ""}, @@ -195,13 +188,14 @@ func (r *operandRecordingRunner) Run(_ context.Context, command string) (tool.Co func TestLocalFileOperandsShellExecution(t *testing.T) { for _, tt := range []struct{ args, command, path string }{ {`{"command":"./a.sh"}`, "./a.sh", "./a.sh"}, - {`{"Command":"./b.sh"}`, "./b.sh", "./b.sh"}, - {`{"command":"./a.sh","Command":"./b.sh"}`, "./b.sh", "./b.sh"}, - {`{"Command":"./b.sh","command":"./a.sh"}`, "./a.sh", "./a.sh"}, - {`{"command":"./a.sh","command":"./b.sh"}`, "./b.sh", "./b.sh"}, - {`{"command":"./b.sh","command":"./a.sh"}`, "./a.sh", "./a.sh"}, - {`{"command":"./a.sh","command":null}`, "./a.sh", "./a.sh"}, - {`{"command":null,"command":"./a.sh"}`, "./a.sh", "./a.sh"}, + // Ambiguous argument objects yield no operands and never run. + {`{"Command":"./b.sh"}`, "", ""}, + {`{"command":"./a.sh","Command":"./b.sh"}`, "", ""}, + {`{"Command":"./b.sh","command":"./a.sh"}`, "", ""}, + {`{"command":"./a.sh","command":"./b.sh"}`, "", ""}, + {`{"command":"./b.sh","command":"./a.sh"}`, "", ""}, + {`{"command":"./a.sh","command":null}`, "", ""}, + {`{"command":null,"command":"./a.sh"}`, "", ""}, {`{"command":1,"command":"./a.sh"}`, "", ""}, {`{"command":"./a.sh","command":1}`, "", ""}, {`{}`, "", ""}, diff --git a/engine/agent/actionreview.go b/engine/agent/actionreview.go index e17203840b..1857d8905a 100644 --- a/engine/agent/actionreview.go +++ b/engine/agent/actionreview.go @@ -492,7 +492,7 @@ func reviewTarget(call session.ToolCall) (ReviewTarget, []string) { var args struct { URL string `json:"url"` } - if json.Unmarshal(call.Args, &args) == nil && args.URL != "" { + if _, ok := session.ParseArgs(call, &args); ok && args.URL != "" { return ReviewTarget{Kind: "external", Display: args.URL, DestinationID: args.URL}, nil } } diff --git a/engine/agent/authority_disclosure.go b/engine/agent/authority_disclosure.go index 8108927a7a..228e54047f 100644 --- a/engine/agent/authority_disclosure.go +++ b/engine/agent/authority_disclosure.go @@ -85,10 +85,8 @@ func authorityToolSearch(call session.ToolCall, cat *tool.Catalog, set governanc var args struct { Query string `json:"query"` } - if len(call.Args) > 0 { - if err := json.Unmarshal(call.Args, &args); err != nil { - return session.NewToolError(call.ID, fmt.Sprintf("ToolSearch: invalid args: %v", err)) - } + if msg, ok := session.ParseArgs(call, &args); !ok { + return session.NewToolError(call.ID, "ToolSearch: "+msg) } query := strings.ToLower(strings.TrimSpace(args.Query)) matches := make([]authorityMatchedSpec, 0) diff --git a/engine/agent/dispatch.go b/engine/agent/dispatch.go index b8dbfd4a6d..e8d38c1b11 100644 --- a/engine/agent/dispatch.go +++ b/engine/agent/dispatch.go @@ -1206,7 +1206,16 @@ func recordValidatedApproval(r *Run, sessionID session.SessionID, ask session.Pe // permissionDecision evaluates normal Shell authority and, only for the declared // system temporary scope, the independent tool-wide escape capability. The // synthetic capability is never dispatched or registered as a tool. +// +// It is the single entry to permission evaluation for every dispatched call +// (fresh, hook-mutated, or resumed after approval), so it first refuses an +// argument object that is not in the canonical form governance.CheckToolArgs +// requires. No policy, guardrail, approval surface, hook or tool then sees +// arguments whose fields different decoders would read differently. func (e *Engine) permissionDecision(ctx context.Context, r *Run, sess *session.Session, env tool.Environment, c session.ToolCall) governance.PermissionDecision { + if err := governance.CheckToolArgs(c.Args); err != nil { + return governance.PermissionDecision{Effect: governance.Deny, Reason: governance.AmbiguousArgsReason(err)} + } evaluate := func(c session.ToolCall) governance.PermissionDecision { result := e.deps.Policy.Evaluate(ctx, sess.ID, sess.Mode, c, env.Workspace()) if r != nil { @@ -1247,10 +1256,11 @@ type systemScopeApproval struct { // systemScopeApprovalArgs projects the system-scope Shell call the engine will // execute: the full command, a non-zero timeout, the background flag, and the -// requested scope, re-encoded from the decoded shellArgs so duplicate or -// case-variant keys show the value that runs. Runner-owned temporary paths and -// environment values are chosen when the command runs and never enter the call, -// so the projection cannot carry them. +// requested scope, re-encoded from the decoded shellArgs (permissionDecision has +// already refused ambiguous argument objects, so this is the decode the Shell +// tool executes). Runner-owned temporary paths and environment values are +// chosen when the command runs and never enter the call, so the projection +// cannot carry them. func systemScopeApprovalArgs(c session.ToolCall) []byte { args, _, ok := parseShellArgs(c) if !ok { @@ -1866,7 +1876,7 @@ func authorityTarget(call session.ToolCall, _ governance.CapabilitySet) (string, Server string `json:"server"` Tool string `json:"tool"` } - if err := json.Unmarshal(call.Args, &args); err != nil || strings.TrimSpace(args.Server) == "" || strings.TrimSpace(args.Tool) == "" { + if _, ok := session.ParseArgs(call, &args); !ok || strings.TrimSpace(args.Server) == "" || strings.TrimSpace(args.Tool) == "" { return "", errors.New("CallMcpWithQuery target is invalid") } return "mcp__" + strings.TrimSpace(args.Server) + "__" + strings.TrimSpace(args.Tool), nil @@ -1874,7 +1884,7 @@ func authorityTarget(call session.ToolCall, _ governance.CapabilitySet) (string, var args struct { Server string `json:"server"` } - if err := json.Unmarshal(call.Args, &args); err != nil || strings.TrimSpace(args.Server) == "" { + if _, ok := session.ParseArgs(call, &args); !ok || strings.TrimSpace(args.Server) == "" { return "", errors.New("MCP resource target is invalid") } return governance.MCPResourceCapability(strings.TrimSpace(args.Server)), nil diff --git a/engine/agent/policy_args_decode_test.go b/engine/agent/policy_args_decode_test.go new file mode 100644 index 0000000000..b9d6591fc7 --- /dev/null +++ b/engine/agent/policy_args_decode_test.go @@ -0,0 +1,293 @@ +package agent + +import ( + "context" + "encoding/json" + "strings" + "sync" + "testing" + "time" + + "github.com/stacklok/mecatl/engine/adapter/fstools" + "github.com/stacklok/mecatl/engine/adapter/memfs" + "github.com/stacklok/mecatl/engine/adapter/mockllm" + "github.com/stacklok/mecatl/engine/adapter/permpolicy" + "github.com/stacklok/mecatl/engine/governance" + "github.com/stacklok/mecatl/engine/port" + "github.com/stacklok/mecatl/engine/session" + "github.com/stacklok/mecatl/engine/tool" +) + +// policyArgsRecorder is an allow-all PermissionPolicy that records, for every +// evaluated call, the Shell command and the resource pattern the governance +// package derives from the call's arguments: the values a configured rule would +// be matched against. +type policyArgsRecorder struct { + inner port.PermissionPolicy + + mu sync.Mutex + evaluated []policyArgsObservation +} + +type policyArgsObservation struct { + tool string + command string + pattern string +} + +func newPolicyArgsRecorder() *policyArgsRecorder { + return &policyArgsRecorder{inner: permpolicy.NewPolicy([]governance.Rule{{Scope: governance.ScopeCLI, Effect: governance.Allow}}, nil)} +} + +func (p *policyArgsRecorder) Evaluate(ctx context.Context, id session.SessionID, mode session.PermissionMode, c session.ToolCall, ws tool.WorkspaceReader) port.PermissionResult { + command, _ := governance.ShellCommandFromArgs(c.Args) + rule, _ := governance.NewEvaluator(nil).LearnableRule(c.Name, c.Args) + p.mu.Lock() + p.evaluated = append(p.evaluated, policyArgsObservation{tool: c.Name, command: command, pattern: rule.Pattern}) + p.mu.Unlock() + return p.inner.Evaluate(ctx, id, mode, c, ws) +} + +func (*policyArgsRecorder) Learn(session.SessionID, session.ToolCall) {} + +func (p *policyArgsRecorder) observations() []policyArgsObservation { + p.mu.Lock() + defer p.mu.Unlock() + return append([]policyArgsObservation(nil), p.evaluated...) +} + +func runPolicyArgsCall(t *testing.T, catalog *tool.Catalog, policy port.PermissionPolicy, env tool.Environment, call session.ToolCall) session.ToolResult { + t.Helper() + eng := NewEngine(Deps{LLM: mockllm.New(mockllm.ToolCallTurn(call), mockllm.TextTurn("done")), Catalog: catalog, Policy: policy}) + sess := session.New("policy-args", session.ModeDefault, env.Ref(), session.Limits{MaxTurns: 3}, time.Now()) + run := eng.Run(context.Background(), sess, env, RunRequest{Text: "run"}) + var result session.ToolResult + seen := false + for event := range run.Events() { + if event.Type == session.EvPermissionAsk { + // The recording policy allows every call; an ask means the test setup is + // wrong, so stop rather than wait for an approval that never comes. + run.Cancel() + t.Errorf("unexpected permission ask: %+v", event.Ask) + } + if event.Type == session.EvToolResult && event.ToolResult != nil && event.ToolResult.CallID == call.ID { + result = *event.ToolResult + seen = true + } + } + if !seen { + t.Fatalf("no tool result for %s", call.ID) + } + return result +} + +// TestShellPolicyEvaluatesTheExecutedCommand asserts, across argument +// encodings, that the command the permission policy evaluates is the command the +// Shell runner receives, and that an ambiguous argument object is rejected before +// policy evaluation without reaching the runner. +func TestShellPolicyEvaluatesTheExecutedCommand(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + args string + ambiguous bool + }{ + {name: "canonical", args: `{"command":"echo canonical"}`}, + {name: "canonical with options", args: `{"command":"echo options","timeout_ms":1000,"temp_scope":"managed"}`}, + {name: "escaped canonical key", args: `{"\u0063ommand":"echo escaped"}`}, + {name: "cmd alias alongside command", args: `{"command":"echo primary","cmd":"echo alias"}`}, + {name: "cmd alias alone", args: `{"cmd":"echo alias"}`}, + {name: "repeated key", args: `{"command":"echo one","command":"echo two"}`, ambiguous: true}, + {name: "repeated key via escape", args: `{"command":"echo one","\u0063ommand":"echo two"}`, ambiguous: true}, + {name: "case variant of field", args: `{"Command":"echo variant"}`, ambiguous: true}, + {name: "case variant alongside field", args: `{"command":"echo one","COMMAND":"echo two"}`, ambiguous: true}, + {name: "field alongside case variant", args: `{"COMMAND":"echo one","command":"echo two"}`, ambiguous: true}, + {name: "case variant of option", args: `{"command":"echo one","Temp_Scope":"system"}`, ambiguous: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + runner := &fakeShellRunner{res: tool.CommandResult{Stdout: "ok"}} + catalog := tool.NewCatalog() + catalog.MustRegister(NewShellTool()) + policy := newPolicyArgsRecorder() + result := runPolicyArgsCall(t, catalog, policy, shellEnvRunner(runner), session.NewToolCall("c", tool.ShellToolName, []byte(tc.args))) + + runner.mu.Lock() + executed := runner.command + runner.mu.Unlock() + observed := policy.observations() + + if tc.ambiguous { + if len(observed) != 0 { + t.Errorf("ambiguous arguments reached policy evaluation: %+v", observed) + } + if executed != "" { + t.Errorf("ambiguous arguments executed %q", executed) + } + if !result.IsError || !strings.Contains(result.Content, "ambiguous") { + t.Errorf("result = %+v, want an ambiguous-arguments tool error", result) + } + return + } + if executed == "" { + // A call the tool refuses runs nothing, whatever was evaluated. + if !result.IsError { + t.Fatalf("call neither executed nor failed: %+v", result) + } + return + } + if len(observed) == 0 { + t.Fatalf("command %q executed without a policy evaluation", executed) + } + for _, o := range observed { + if o.command != executed { + t.Errorf("policy evaluated command %q, runner executed %q", o.command, executed) + } + } + }) + } +} + +// TestFileToolPolicyEvaluatesTheExecutedPath asserts, across argument encodings, +// that the resource the permission policy evaluates for a file tool is the one +// the tool acts on, and that ambiguous argument objects are rejected before +// policy evaluation without touching the workspace. +func TestFileToolPolicyEvaluatesTheExecutedPath(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + tool tool.Tool + args string + ambiguous bool + // wantRead is the file content a successful Read must return. + wantRead string + // wantFile, when set, is a path that must exist after a successful call. + wantFile string + }{ + {name: "read canonical", tool: fstools.ReadTool{}, args: `{"path":"a.txt"}`, wantRead: "alpha"}, + {name: "read repeated key", tool: fstools.ReadTool{}, args: `{"path":"a.txt","path":"b.txt"}`, ambiguous: true}, + {name: "read case variant alongside field", tool: fstools.ReadTool{}, args: `{"path":"a.txt","PATH":"b.txt"}`, ambiguous: true}, + {name: "read case variant of field", tool: fstools.ReadTool{}, args: `{"Path":"b.txt"}`, ambiguous: true}, + {name: "list case variant alongside field", tool: fstools.ListDirTool{}, args: `{"path":"dir","Path":"."}`, ambiguous: true}, + {name: "grep case variant alongside field", tool: fstools.GrepTool{}, args: `{"pattern":"alpha","path":"a.txt","Path":"b.txt"}`, ambiguous: true}, + {name: "glob case variant alongside field", tool: fstools.GlobTool{}, args: `{"pattern":"*.txt","PATTERN":"**"}`, ambiguous: true}, + {name: "write repeated key", tool: fstools.WriteTool{}, args: `{"path":"new-a.txt","path":"new-b.txt","content":"x"}`, ambiguous: true}, + {name: "remove case variant alongside field", tool: fstools.RemoveTool{}, args: `{"path":"a.txt","pAth":"b.txt"}`, ambiguous: true}, + {name: "copy canonical", tool: fstools.CopyTool{}, args: `{"source":"a.txt","destination":"copy.txt"}`, wantFile: "copy.txt"}, + {name: "copy unicode fold variant alongside field", tool: fstools.CopyTool{}, args: `{"source":"a.txt","destination":"c1.txt","de\u017ftination":"c2.txt"}`, ambiguous: true}, + {name: "move repeated key", tool: fstools.MoveTool{}, args: `{"source":"a.txt","destination":"m1.txt","destination":"m2.txt"}`, ambiguous: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ws := memfs.NewWorkspace("/ws") + for path, body := range map[string]string{"a.txt": "alpha", "b.txt": "beta", "dir/c.txt": "gamma"} { + if err := ws.Write(t.Context(), path, []byte(body)); err != nil { + t.Fatal(err) + } + } + catalog := tool.NewCatalog() + catalog.MustRegister(tc.tool) + policy := newPolicyArgsRecorder() + name := tc.tool.Spec().Name + result := runPolicyArgsCall(t, catalog, policy, testEnvironment(ws, nil), session.NewToolCall("c", name, []byte(tc.args))) + observed := policy.observations() + + if tc.ambiguous { + if len(observed) != 0 { + t.Errorf("ambiguous arguments reached policy evaluation: %+v", observed) + } + if !result.IsError || !strings.Contains(result.Content, "ambiguous") { + t.Errorf("result = %+v, want an ambiguous-arguments tool error", result) + } + for _, path := range []string{"a.txt", "b.txt"} { + if _, err := ws.Read(t.Context(), path); err != nil { + t.Errorf("workspace file %s changed: %v", path, err) + } + } + for _, path := range []string{"new-a.txt", "new-b.txt", "c1.txt", "c2.txt", "m1.txt", "m2.txt"} { + if _, err := ws.Read(t.Context(), path); err == nil { + t.Errorf("ambiguous call created %s", path) + } + } + return + } + if result.IsError { + t.Fatalf("canonical call failed: %+v", result) + } + if len(observed) == 0 { + t.Fatal("call executed without a policy evaluation") + } + if tc.wantRead != "" { + if !strings.Contains(result.Content, tc.wantRead) { + t.Fatalf("read content = %q, want %q", result.Content, tc.wantRead) + } + var args struct { + Path string `json:"path"` + } + if err := json.Unmarshal([]byte(tc.args), &args); err != nil { + t.Fatal(err) + } + if observed[0].pattern != args.Path { + t.Errorf("policy evaluated %q, tool read %q", observed[0].pattern, args.Path) + } + } + if tc.wantFile != "" { + if _, err := ws.Read(t.Context(), tc.wantFile); err != nil { + t.Errorf("expected %s after call: %v", tc.wantFile, err) + } + } + }) + } +} + +// TestArgumentDecodeRejectsAmbiguityBeforeEveryTool asserts the ambiguity check +// is tool-agnostic: a call to a tool whose arguments are forwarded verbatim is +// rejected before policy evaluation and never reaches the tool. +func TestArgumentDecodeRejectsAmbiguityBeforeEveryTool(t *testing.T) { + t.Parallel() + for _, name := range []string{"WebFetch", "WebSearch", "mcp__srv__fetch"} { + t.Run(name, func(t *testing.T) { + t.Parallel() + stub := &argsRecordingTool{name: name} + catalog := tool.NewCatalog() + catalog.MustRegister(stub) + policy := newPolicyArgsRecorder() + args := `{"url":"https://one.example","URL":"https://two.example","query":"q"}` + result := runPolicyArgsCall(t, catalog, policy, memEnv("/ws"), session.NewToolCall("c", name, []byte(args))) + if observed := policy.observations(); len(observed) != 0 { + t.Errorf("ambiguous arguments reached policy evaluation: %+v", observed) + } + if stub.calls() != 0 { + t.Errorf("ambiguous arguments reached the tool") + } + if !result.IsError || !strings.Contains(result.Content, "ambiguous") { + t.Errorf("result = %+v, want an ambiguous-arguments tool error", result) + } + }) + } +} + +type argsRecordingTool struct { + name string + mu sync.Mutex + n int +} + +func (s *argsRecordingTool) Spec() tool.ToolSpec { + return tool.ToolSpec{Name: s.name, Description: "records calls", Schema: json.RawMessage(`{"type":"object"}`)} +} + +func (*argsRecordingTool) ReadOnly() bool { return true } + +func (s *argsRecordingTool) Execute(_ context.Context, in session.ToolCall, _ tool.Environment) (session.ToolResult, error) { + s.mu.Lock() + s.n++ + s.mu.Unlock() + return session.NewToolResult(in.ID, "ok"), nil +} + +func (s *argsRecordingTool) calls() int { + s.mu.Lock() + defer s.mu.Unlock() + return s.n +} diff --git a/engine/agent/repair2b_test.go b/engine/agent/repair2b_test.go index afc753a733..992e273db4 100644 --- a/engine/agent/repair2b_test.go +++ b/engine/agent/repair2b_test.go @@ -48,7 +48,11 @@ func TestReviewTargetUsesOnlyKnownLocalFileSemantics(t *testing.T) { } } -func TestReviewTargetMatchesBuiltinReadWithConflictingPathVariants(t *testing.T) { +// TestReviewTargetMatchesBuiltinReadDecode pins that the review target and the +// built-in Read share one decode: path variants in two spellings give no +// workspace target and the Read refuses them, while the canonical path is both +// the reviewed dependency and the file read. +func TestReviewTargetMatchesBuiltinReadDecode(t *testing.T) { ctx := context.Background() workspace := memfs.NewWorkspace("/review") for path, marker := range map[string]string{ @@ -59,12 +63,20 @@ func TestReviewTargetMatchesBuiltinReadWithConflictingPathVariants(t *testing.T) t.Fatal(err) } } - call := session.NewToolCall("c", "Read", json.RawMessage(`{"path":"website/a.ts","Path":"services/a.ts"}`)) + env := tool.MustEnvironment(session.EnvironmentRef{Kind: session.EnvKindMem, ID: "review"}, workspace, memledger.New(), nil) + conflicting := session.NewToolCall("c", "Read", json.RawMessage(`{"path":"website/a.ts","Path":"services/a.ts"}`)) + if target, paths := reviewTarget(conflicting); target.Kind != "tool" || len(paths) != 0 { + t.Fatalf("conflicting variants review target=%+v paths=%v, want no workspace target", target, paths) + } + if result, err := (fstools.ReadTool{}).Execute(ctx, conflicting, env); err != nil || !result.IsError || !strings.Contains(result.Content, "ambiguous tool arguments") { + t.Fatalf("conflicting variants Read result=%+v err=%v, want an ambiguous-arguments error", result, err) + } + + call := session.NewToolCall("c", "Read", json.RawMessage(`{"path":"services/a.ts"}`)) target, paths := reviewTarget(call) if target.Display != "services/a.ts" || len(paths) != 1 || paths[0] != "services/a.ts" { t.Fatalf("review target=%+v paths=%v", target, paths) } - env := tool.MustEnvironment(session.EnvironmentRef{Kind: session.EnvKindMem, ID: "review"}, workspace, memledger.New(), nil) result, err := (fstools.ReadTool{}).Execute(ctx, call, env) if err != nil || result.IsError || !strings.Contains(result.Content, "SERVICES_MARKER") || strings.Contains(result.Content, "WEBSITE_MARKER") { t.Fatalf("Read result=%+v err=%v", result, err) diff --git a/engine/agent/subagent.go b/engine/agent/subagent.go index d21d15fef0..27abf6439b 100644 --- a/engine/agent/subagent.go +++ b/engine/agent/subagent.go @@ -1529,7 +1529,7 @@ func (t *SubagentTool) MutatesParent(call session.ToolCall) bool { return false } var args subagentArgs - if err := json.Unmarshal(call.Args, &args); err != nil { + if _, ok := session.ParseArgs(call, &args); !ok { return false } return strings.TrimSpace(args.Mode) == subagentModeReadWrite diff --git a/engine/agent/temp_scope_permission_test.go b/engine/agent/temp_scope_permission_test.go index 4e00dff744..a986b5c5b4 100644 --- a/engine/agent/temp_scope_permission_test.go +++ b/engine/agent/temp_scope_permission_test.go @@ -170,7 +170,7 @@ func firstPermissionAsk(t *testing.T, events []session.Event) session.PendingAsk // TestSystemTempApprovalShowsFullCommand pins that the system-scope approval // shows the whole command and every execution-relevant field as the engine -// decoded them, so duplicate and case-variant keys project the value that runs. +// decoded them. func TestSystemTempApprovalShowsFullCommand(t *testing.T) { t.Parallel() for _, tc := range []struct{ name, args, want string }{ @@ -184,16 +184,6 @@ func TestSystemTempApprovalShowsFullCommand(t *testing.T) { args: `{"temp_scope":"system","background":true,"timeout_ms":5000,"command":"go test ./... | tee out.txt"}`, want: `{"command":"go test ./... | tee out.txt","timeout_ms":5000,"background":true,"temp_scope":"system"}`, }, - { - name: "duplicate key", - args: `{"command":"git status","temp_scope":"system","command":"git status && echo done"}`, - want: `{"command":"git status && echo done","temp_scope":"system"}`, - }, - { - name: "case-variant keys", - args: `{"command":"git status","COMMAND":"git status && echo done","Temp_Scope":"system","Timeout_MS":7}`, - want: `{"command":"git status && echo done","timeout_ms":7,"temp_scope":"system"}`, - }, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() @@ -207,14 +197,45 @@ func TestSystemTempApprovalShowsFullCommand(t *testing.T) { } } +// TestSystemTempAmbiguousArgsAreRejectedBeforeApproval pins that a system-scope +// Shell call whose argument object repeats a key or spells a field in another +// case is refused before policy evaluation: no approval is surfaced and nothing +// reaches the runner. +func TestSystemTempAmbiguousArgsAreRejectedBeforeApproval(t *testing.T) { + t.Parallel() + for _, tc := range []struct{ name, args string }{ + {name: "duplicate key", args: `{"command":"git status","temp_scope":"system","command":"git status && echo done"}`}, + {name: "case-variant keys", args: `{"command":"git status","COMMAND":"git status && echo done","Temp_Scope":"system","Timeout_MS":7}`}, + {name: "case-variant scope", args: `{"command":"git status","TEMP_SCOPE":"system"}`}, + {name: "case-variant option", args: `{"command":"git status","temp_scope":"system","Timeout_MS":7}`}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + runner := &fakeShellRunner{res: tool.CommandResult{Stdout: "ok"}} + call := session.NewToolCall("c", tool.ShellToolName, []byte(tc.args)) + events, result := runScopedShell(t, []governance.Rule{{Tool: tool.ShellToolName, Effect: governance.Allow}}, call, runner) + for _, event := range events { + if event.Type == session.EvPermissionAsk { + t.Fatalf("ambiguous arguments surfaced an approval: %s", event.Ask.Args) + } + } + if runner.command != "" { + t.Fatalf("ambiguous arguments executed %q", runner.command) + } + if !result.IsError || !strings.Contains(result.Content, "ambiguous tool arguments") { + t.Fatalf("result = %+v, want an ambiguous-arguments tool error", result) + } + }) + } +} + // TestSystemTempApprovalMatchesExecutedCommand approves the system-scope ask // and proves the command the approver saw is the command the runner received. func TestSystemTempApprovalMatchesExecutedCommand(t *testing.T) { t.Parallel() for _, args := range []string{ `{"command":"git status && echo done","temp_scope":"system"}`, - `{"command":"git status","temp_scope":"system","command":"git status && echo done"}`, - `{"command":"git status","COMMAND":"git status && echo done","TEMP_SCOPE":"system"}`, + `{"temp_scope":"system","timeout_ms":5000,"command":"git status && echo done"}`, } { t.Run(args, func(t *testing.T) { t.Parallel() diff --git a/engine/api/governance.txt b/engine/api/governance.txt index 2e35736042..207df1f00c 100644 --- a/engine/api/governance.txt +++ b/engine/api/governance.txt @@ -30,8 +30,11 @@ const ScopeManaged Scope = 0 const ScopeSharedProject Scope = 3 const ScopeUser Scope = 4 const UntrustedFence untyped string = "<< maxArgNameInError { + runes := []rune(name) + name = string(runes[:maxArgNameInError]) + "…" + } + return fmt.Sprintf("%q", name) +} + +// checkFieldSpelling rejects a key that names one of dst's explicitly tagged +// struct fields only under case folding. Untagged fields keep encoding/json's +// case-insensitive matching, since their Go name is not a wire spelling. +func checkFieldSpelling(keys []string, dst any) error { + fields := taggedFieldNames(reflect.TypeOf(dst)) + if fields == nil { + return nil + } + for _, key := range keys { + if _, exact := fields.exact[key]; exact { + continue + } + if name, ok := fields.folded[foldArgName(key)]; ok { + return fmt.Errorf("%w: %s must be spelled %q", ErrAmbiguousToolArgs, quoteArgName(key), name) + } + } + return nil +} + +// taggedFields indexes a struct's explicitly tagged JSON field names exactly and +// by their case-folding class. +type taggedFields struct { + exact map[string]struct{} + folded map[string]string +} + +var taggedFieldCache sync.Map // reflect.Type -> *taggedFields + +// taggedFieldNames returns the explicitly tagged JSON field names of the struct +// t points to, including those promoted from untagged embedded structs, or nil +// when t is not a pointer to a struct. +func taggedFieldNames(t reflect.Type) *taggedFields { + if t == nil || t.Kind() != reflect.Pointer || t.Elem().Kind() != reflect.Struct { + return nil + } + if cached, ok := taggedFieldCache.Load(t); ok { + return cached.(*taggedFields) + } + fields := &taggedFields{exact: map[string]struct{}{}, folded: map[string]string{}} + collectTaggedFields(t.Elem(), fields, 0) + taggedFieldCache.Store(t, fields) + return fields +} + +// maxEmbeddingDepth bounds the walk through embedded structs. +const maxEmbeddingDepth = 8 + +func collectTaggedFields(t reflect.Type, fields *taggedFields, depth int) { + if depth > maxEmbeddingDepth { + return + } + for i := range t.NumField() { + field := t.Field(i) + tag := field.Tag.Get("json") + if tag == "-" { + continue + } + name, _, _ := strings.Cut(tag, ",") + if field.Anonymous && name == "" { + embedded := field.Type + if embedded.Kind() == reflect.Pointer { + embedded = embedded.Elem() + } + if embedded.Kind() == reflect.Struct { + collectTaggedFields(embedded, fields, depth+1) + } + continue + } + if !field.IsExported() || name == "" { + continue + } + fields.exact[name] = struct{}{} + fields.folded[foldArgName(name)] = name + } +} diff --git a/engine/governance/args_test.go b/engine/governance/args_test.go new file mode 100644 index 0000000000..5e12a0ebda --- /dev/null +++ b/engine/governance/args_test.go @@ -0,0 +1,249 @@ +package governance + +import ( + "encoding/json" + "errors" + "strings" + "testing" +) + +// toolArgsSeeds are argument encodings covering canonical objects, JSON key +// escapes, repeated keys, case and Unicode case-folding variants, nesting and +// non-object payloads. They seed the decode-agreement fuzzer and the tables. +var toolArgsSeeds = []string{ + ``, + `{}`, + `null`, + `[]`, + `"text"`, + `{bad`, + `{"command":"ls"} {"command":"pwd"}`, + `{"command":"go test ./..."}`, + `{"command":"go test ./...","timeout_ms":5000,"temp_scope":"managed"}`, + `{"command":"ls"}`, + `{"command":"ls","cmd":"pwd"}`, + `{"cmd":"pwd"}`, + `{"command":"","cmd":"pwd"}`, + `{"command":7,"cmd":"pwd"}`, + `{"command":null}`, + `{"command":"ls","command":"pwd"}`, + `{"command":"ls","command":"pwd"}`, + `{"Command":"pwd"}`, + `{"command":"ls","COMMAND":"pwd"}`, + `{"COMMAND":"pwd","command":"ls"}`, + `{"command":"ls","Temp_Scope":"system"}`, + `{"command":"ls","Timeout_MS":1}`, + `{"path":"a"}`, + `{"path":"a","Path":"b"}`, + `{"file_path":"a"}`, + `{"FILE_PATH":"a"}`, + `{"pattern":"*.go","path":"src"}`, + `{"url":"https://example.com","URL":"https://example.org"}`, + `{"query":"q","Query":"r"}`, + `{"source":"a","destination":"b"}`, + `{"source":"a","ſource":"b","destination":"c"}`, + `{"ſource":"b","destination":"c"}`, + `{"key":"a","Key":"b"}`, + `{"server":"s","tool":"t","Tool":"u"}`, + `{"edits":[{"old_string":"a","old_string":"b"}]}`, + `{"meta":{"x":1,"x":2}}`, + `{"meta":{"x":1,"X":2}}`, + `{"filePath":"a","FilePath":"b"}`, + `{"label":"x","Label":"y"}`, + `{"a\\\"b":1,"a\\\"b":2}`, + `{"s":"{\"command\":1}","command":"ls"}`, + `{"bad\xffkey":1}`, +} + +// TestCheckToolArgsClassification pins which argument objects are ambiguous. +func TestCheckToolArgsClassification(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + args string + ambiguous bool + }{ + {``, false}, + {`{}`, false}, + {`null`, false}, + {`[]`, false}, + {`{bad`, false}, + {`{"command":"ls"}`, false}, + {`{"command":"ls"}`, false}, + {`{"command":"ls","cmd":"pwd"}`, false}, + {`{"filePath":"a"}`, false}, + {`{"meta":{"x":1,"X":2}}`, false}, + {`{"command":"ls","command":"pwd"}`, true}, + {`{"command":"ls","command":"pwd"}`, true}, + {`{"Command":"pwd"}`, true}, + {`{"command":"ls","COMMAND":"pwd"}`, true}, + {`{"command":"ls","Temp_Scope":"system"}`, true}, + {`{"path":"a","Path":"b"}`, true}, + {`{"FILE_PATH":"a"}`, true}, + {`{"url":"https://example.com","URL":"https://example.org"}`, true}, + {`{"source":"a","ſource":"b","destination":"c"}`, true}, + {`{"ſource":"b","destination":"c"}`, true}, + {`{"key":"a","Key":"b"}`, true}, + {`{"server":"s","tool":"t","Tool":"u"}`, true}, + {`{"edits":[{"old_string":"a","old_string":"b"}]}`, true}, + {`{"meta":{"x":1,"x":2}}`, true}, + {`{"filePath":"a","FilePath":"b"}`, true}, + {`{"label":"x","Label":"y"}`, true}, + } { + err := CheckToolArgs(json.RawMessage(tc.args)) + if got := errors.Is(err, ErrAmbiguousToolArgs); got != tc.ambiguous { + t.Errorf("CheckToolArgs(%s) = %v, want ambiguous=%v", tc.args, err, tc.ambiguous) + } + } +} + +// TestDecodeToolArgsRequiresExactFieldSpelling pins that a key naming a tagged +// destination field only under case folding is refused, while untagged fields +// and unrelated keys keep decoding. +func TestDecodeToolArgsRequiresExactFieldSpelling(t *testing.T) { + t.Parallel() + type embedded struct { + Offset int `json:"offset"` + } + type dst struct { + embedded + Limit int `json:"limit"` + Untagged string + Skipped string `json:"-"` + } + for _, tc := range []struct { + args string + ok bool + }{ + {`{"limit":1,"offset":2}`, true}, + {`{"untagged":"x"}`, true}, + {`{"other":1}`, true}, + {`{"Limit":1}`, false}, + {`{"OFFSET":2}`, false}, + } { + var d dst + err := DecodeToolArgs(json.RawMessage(tc.args), &d) + if (err == nil) != tc.ok { + t.Errorf("DecodeToolArgs(%s) = %v, want ok=%v", tc.args, err, tc.ok) + } + if err != nil && !errors.Is(err, ErrAmbiguousToolArgs) { + t.Errorf("DecodeToolArgs(%s) error %v does not wrap ErrAmbiguousToolArgs", tc.args, err) + } + } +} + +// TestEvaluatorDeniesAmbiguousArgs pins the evaluator's own check: an ambiguous +// argument object is denied before any rule, plan mode or learned allow, and is +// never learnable. +func TestEvaluatorDeniesAmbiguousArgs(t *testing.T) { + t.Parallel() + e := NewEvaluator([]Rule{{Scope: ScopeCLI, Effect: Allow}}) + for _, tc := range []struct{ tool, args string }{ + {"Shell", `{"command":"ls","COMMAND":"pwd"}`}, + {"Read", `{"path":"a","path":"b"}`}, + {"Copy", `{"source":"a","destination":"b","Destination":"c"}`}, + {"mcp__srv__fetch", `{"url":"https://example.com","Url":"https://example.org"}`}, + } { + d := e.EvaluateWith(tc.tool, json.RawMessage(tc.args), false, []Rule{{Scope: ScopeUser, Tool: tc.tool, Effect: Allow}}) + if d.Effect != Deny || !strings.Contains(d.Reason, "ambiguous tool arguments") { + t.Errorf("%s %s: decision %+v, want an ambiguous-arguments deny", tc.tool, tc.args, d) + } + if _, ok := e.LearnableRule(tc.tool, json.RawMessage(tc.args)); ok { + t.Errorf("%s %s: ambiguous arguments produced a learnable rule", tc.tool, tc.args) + } + } +} + +// shellExecArgs mirrors the Shell tool's argument struct: the fields its +// execution decodes. +type shellExecArgs struct { + Command string `json:"command"` + TimeoutMS int `json:"timeout_ms"` + Background bool `json:"background"` + TempScope string `json:"temp_scope"` +} + +// pathExecArgs mirrors the single-path built-in tools' argument struct. +type pathExecArgs struct { + Path string `json:"path"` +} + +// copyExecArgs mirrors the Copy/Move tools' argument struct. +type copyExecArgs struct { + Source string `json:"source"` + Destination string `json:"destination"` +} + +// assertPolicyMatchesExecution checks, for one argument payload, that whenever +// the execution decode accepts the payload, the policy extraction reads the +// same value; and that an ambiguous payload is refused by both. +func assertPolicyMatchesExecution(t *testing.T, args string) { + t.Helper() + raw := json.RawMessage(args) + ambiguous := CheckToolArgs(raw) != nil + + // The key scanner sees every top-level key encoding/json sees, once. + var object map[string]json.RawMessage + if !ambiguous && json.Unmarshal(raw, &object) == nil && object != nil { + keys, err := scanToolArgs(raw) + if err != nil || len(keys) != len(object) { + t.Errorf("%q: scanned keys %q (err=%v), encoding/json keys %d", args, keys, err, len(object)) + } + for _, key := range keys { + if _, ok := object[key]; !ok { + t.Errorf("%q: scanned key %q that encoding/json does not decode", args, key) + } + } + } + + var shell shellExecArgs + if err := DecodeToolArgs(raw, &shell); err == nil { + if ambiguous { + t.Errorf("%q: Shell decode accepted an ambiguous object", args) + } + cmd, ok := ShellCommandFromArgs(raw) + if strings.TrimSpace(shell.Command) != "" && (!ok || cmd != shell.Command) { + t.Errorf("%q: policy command %q (ok=%v), executed command %q", args, cmd, ok, shell.Command) + } + } else if cmd, ok := ShellCommandFromArgs(raw); ambiguous && ok { + t.Errorf("%q: policy read command %q from an ambiguous object", args, cmd) + } + + var path pathExecArgs + if err := DecodeToolArgs(raw, &path); err == nil && path.Path != "" { + if got := nonShellPattern("Read", raw); got != path.Path { + t.Errorf("%q: policy path %q, executed path %q", args, got, path.Path) + } + } else if ambiguous && nonShellPattern("Read", raw) != "" { + t.Errorf("%q: policy read a path from an ambiguous object", args) + } + + var operands copyExecArgs + if err := DecodeToolArgs(raw, &operands); err == nil && operands.Source != "" && operands.Destination != "" { + got, ok := copyMovePatterns(raw) + if !ok || len(got) != 2 || got[0] != operands.Source || got[1] != operands.Destination { + t.Errorf("%q: policy operands %q (ok=%v), executed %q -> %q", args, got, ok, operands.Source, operands.Destination) + } + } else if _, ok := copyMovePatterns(raw); ambiguous && ok { + t.Errorf("%q: policy read operands from an ambiguous object", args) + } +} + +// TestPolicyExtractionMatchesExecutionDecode runs the decode-agreement property +// over the seed encodings. +func TestPolicyExtractionMatchesExecutionDecode(t *testing.T) { + t.Parallel() + for _, args := range toolArgsSeeds { + assertPolicyMatchesExecution(t, args) + } +} + +// FuzzPolicyExtractionMatchesExecutionDecode asserts the decode-agreement +// property for arbitrary argument payloads. +func FuzzPolicyExtractionMatchesExecutionDecode(f *testing.F) { + for _, seed := range toolArgsSeeds { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, args string) { + assertPolicyMatchesExecution(t, args) + }) +} diff --git a/engine/governance/evaluator.go b/engine/governance/evaluator.go index 356803450f..c1163b9ac0 100644 --- a/engine/governance/evaluator.go +++ b/engine/governance/evaluator.go @@ -118,6 +118,9 @@ func (e *Evaluator) Evaluate(tool string, args json.RawMessage, planMode bool) P // The static rules are never mutated: a fresh slice (static followed by extra) is // resolved per call, preserving the Evaluator's immutability. func (e *Evaluator) EvaluateWith(tool string, args json.RawMessage, planMode bool, extra []Rule) PermissionDecision { + if d, ambiguous := ambiguousArgsDecision(args); ambiguous { + return d + } if planMode { if d, blocked := planModeDecision(tool, args); blocked { return d @@ -132,6 +135,24 @@ func (e *Evaluator) EvaluateWith(tool string, args json.RawMessage, planMode boo return e.resolve(merged, tool, args) } +// ambiguousArgsDecision denies a call whose arguments are not in the canonical +// form CheckToolArgs requires. It runs before plan mode and every rule, so no +// rule, learned allow or posture can admit a call whose fields different readers +// would decode differently. +func ambiguousArgsDecision(args json.RawMessage) (PermissionDecision, bool) { + err := CheckToolArgs(args) + if err == nil { + return PermissionDecision{}, false + } + return PermissionDecision{Effect: Deny, Reason: AmbiguousArgsReason(err)}, true +} + +// AmbiguousArgsReason renders the model-facing reason for a call refused by +// CheckToolArgs, telling the model how to resend it. +func AmbiguousArgsReason(err error) string { + return err.Error() + "; send each argument once, spelled exactly as in the tool schema" +} + // LearnableRule derives the per-session allow rule the Evaluator WOULD learn for // a (tool, args) pair the user chose to "allow always", reusing the SAME pattern // derivation the evaluator matches against — so a learned rule matches exactly the @@ -176,6 +197,9 @@ func (*Evaluator) LearnableRule(tool string, args json.RawMessage) (Rule, bool) // every other tool it reuses nonShellPattern (the path/target field). An empty // derived pattern is returned as-is so the caller refuses a tool-wide grant. func learnablePattern(tool string, args json.RawMessage) (string, bool) { + if CheckToolArgs(args) != nil { + return "", false + } if tool == "Shell" { cmd, _ := ShellCommandFromArgs(args) if cmd == "" { @@ -556,9 +580,10 @@ func describeTarget(tool, rulePattern, pattern string) string { return tool } -// copyMovePatterns extracts both resource operands from a Copy or Move call. -// Both must be strings; malformed shapes return false and fall back to the safe -// ordinary no-pattern evaluation. +// copyMovePatterns extracts both resource operands from a Copy or Move call +// through DecodeToolArgs, the decode the tools execute. Both must be strings; +// malformed or ambiguous shapes return false and fall back to the safe ordinary +// no-pattern evaluation. func copyMovePatterns(args json.RawMessage) ([]string, bool) { if len(args) == 0 { return nil, false @@ -567,59 +592,79 @@ func copyMovePatterns(args json.RawMessage) ([]string, bool) { Source string `json:"source"` Destination string `json:"destination"` } - if err := json.Unmarshal(args, &values); err != nil { + if err := DecodeToolArgs(args, &values); err != nil { return nil, false } return []string{values.Source, values.Destination}, true } +// resourceArgs is the policy view of a single-resource call's arguments, decoded +// through DecodeToolArgs with the field names the built-in tools declare. +type resourceArgs struct { + Path json.RawMessage `json:"path"` + FilePath json.RawMessage `json:"file_path"` + Pattern json.RawMessage `json:"pattern"` + URL json.RawMessage `json:"url"` + Query json.RawMessage `json:"query"` +} + // nonShellPattern derives the pattern string to match a single-resource non-Shell tool call // against. It uses the most common path/target field of the built-in tools so -// rules can target specific files; unknown shapes fall back to the empty string -// (which only matches tool-wide rules). +// rules can target specific files; unknown or ambiguous shapes fall back to the +// empty string (which only matches tool-wide rules). func nonShellPattern(_ string, args json.RawMessage) string { if len(args) == 0 { return "" } - var m map[string]json.RawMessage - if err := json.Unmarshal(args, &m); err != nil { + var fields resourceArgs + if err := DecodeToolArgs(args, &fields); err != nil { return "" } - for _, key := range []string{"path", "file_path", "pattern", "url", "query"} { - if raw, ok := m[key]; ok { - var s string - if json.Unmarshal(raw, &s) == nil { - return s - } + for _, raw := range []json.RawMessage{fields.Path, fields.FilePath, fields.Pattern, fields.URL, fields.Query} { + if raw == nil { + continue + } + var s string + if json.Unmarshal(raw, &s) == nil { + return s } } return "" } +// shellCommandArgs is the policy view of a Shell call's arguments, decoded +// through DecodeToolArgs with the Shell tool's field name plus the cmd alias. +type shellCommandArgs struct { + Command json.RawMessage `json:"command"` + Cmd json.RawMessage `json:"cmd"` +} + // ShellCommandFromArgs extracts the command string from a Shell tool call's raw -// args JSON. It reads the "command" field, then "cmd" as a fallback, returning -// the first non-empty (after TrimSpace) string. It is FAIL-SAFE: a JSON parse -// error OR neither field carrying a non-empty string returns ("", false), so a -// caller that gates a safety decision on the result INSPECTS rather than skips -// (an unreadable args object must never be presumed read-only). This is the -// single shared extraction for the Shell tool-call args schema — the governance +// args JSON. It decodes through DecodeToolArgs, the decode the Shell tool +// executes, and reads the "command" field, then "cmd" as a fallback, returning +// the first non-empty (after TrimSpace) string. The Shell tool runs only +// "command", so a call that reaches execution was evaluated on the command it +// runs. It is FAIL-SAFE: a JSON parse error, an ambiguous argument object, OR +// neither field carrying a non-empty string returns ("", false), so a caller +// that gates a safety decision on the result INSPECTS rather than skips (an +// unreadable args object must never be presumed read-only). This is the single +// shared extraction for the Shell tool-call args schema — the governance // evaluator, the Subagent isolation gate, and the guardrail Shell pre-filter all // call it, so a schema change lands in one place. func ShellCommandFromArgs(args json.RawMessage) (string, bool) { if len(args) == 0 { return "", false } - var m map[string]json.RawMessage - if err := json.Unmarshal(args, &m); err != nil { + var fields shellCommandArgs + if err := DecodeToolArgs(args, &fields); err != nil { return "", false } - for _, key := range []string{"command", "cmd"} { - rawVal, present := m[key] - if !present { + for _, raw := range []json.RawMessage{fields.Command, fields.Cmd} { + if raw == nil { continue } var s string - if err := json.Unmarshal(rawVal, &s); err != nil { + if err := json.Unmarshal(raw, &s); err != nil { continue } if strings.TrimSpace(s) != "" { diff --git a/engine/session/toolcall.go b/engine/session/toolcall.go index 7d3798ae88..886d50577a 100644 --- a/engine/session/toolcall.go +++ b/engine/session/toolcall.go @@ -3,6 +3,8 @@ package session import ( "encoding/json" "fmt" + + "github.com/stacklok/mecatl/engine/governance" ) // ToolCallID uniquely identifies a tool invocation within a session. It is @@ -49,6 +51,11 @@ func NewToolCall(id ToolCallID, name string, args json.RawMessage) ToolCall { // both repeat. It lives on the domain ToolCall (which both layers already import) // so neither has to depend on the other. // +// It decodes through governance.DecodeToolArgs, the same decode permission +// evaluation uses, so a tool executes the values the policy evaluated. An +// ambiguous argument object (a repeated key, keys equal under case folding, or +// a key that names a tagged field of dst only under case folding) is refused. +// // An empty payload (no args) is treated as an empty object: dst is left at its // zero value and ok=true, so a tool with all-optional arguments works without the // model having to send an explicit "{}". On malformed JSON it returns a @@ -59,7 +66,7 @@ func ParseArgs(call ToolCall, dst any) (msg string, ok bool) { if len(call.Args) == 0 { return "", true } - if err := json.Unmarshal(call.Args, dst); err != nil { + if err := governance.DecodeToolArgs(call.Args, dst); err != nil { return fmt.Sprintf("invalid arguments: %v", err), false } return "", true diff --git a/engine/session/toolcall_test.go b/engine/session/toolcall_test.go index 465536f3a8..6a8be7bb49 100644 --- a/engine/session/toolcall_test.go +++ b/engine/session/toolcall_test.go @@ -3,6 +3,7 @@ package session import ( "encoding/json" "reflect" + "strings" "testing" ) @@ -48,6 +49,25 @@ func TestParseArgs(t *testing.T) { t.Fatal("malformed JSON produced no model-facing message") } }) + + t.Run("ambiguous objects are refused", func(t *testing.T) { + for _, args := range []string{ + `{"key":"a","key":"b"}`, + `{"key":"a","Key":"b"}`, + `{"KEY":"b"}`, + `{"\u212aey":"b"}`, + `{"key":"a","nested":{"x":1,"x":2}}`, + } { + var p payload + msg, ok := ParseArgs(ToolCall{Args: json.RawMessage(args)}, &p) + if ok || !strings.Contains(msg, "ambiguous tool arguments") { + t.Errorf("ParseArgs(%s) ok=%v msg=%q, want an ambiguous-arguments refusal", args, ok, msg) + } + if p != (payload{}) { + t.Errorf("ParseArgs(%s) decoded %+v before refusing", args, p) + } + } + }) } // TestToolResultPartsRoundTrip locks the JSON shape of ToolResult.Parts: a Parts diff --git a/engine/tool/toolsearch.go b/engine/tool/toolsearch.go index 2747823d76..5517f999b5 100644 --- a/engine/tool/toolsearch.go +++ b/engine/tool/toolsearch.go @@ -78,10 +78,8 @@ type matchedSpec struct { // tools. ToolSearch never returns itself. Results are ordered by name. func (s *Search) Execute(_ context.Context, in session.ToolCall, _ Environment) (session.ToolResult, error) { var args toolSearchArgs - if len(in.Args) > 0 { - if err := json.Unmarshal(in.Args, &args); err != nil { - return session.NewToolError(in.ID, fmt.Sprintf("ToolSearch: invalid args: %v", err)), nil - } + if msg, ok := session.ParseArgs(in, &args); !ok { + return session.NewToolError(in.ID, "ToolSearch: "+msg), nil } q := strings.ToLower(strings.TrimSpace(args.Query)) diff --git a/internal/app/contextual_build_evidence_test.go b/internal/app/contextual_build_evidence_test.go index 426f825474..cadb5797a6 100644 --- a/internal/app/contextual_build_evidence_test.go +++ b/internal/app/contextual_build_evidence_test.go @@ -592,6 +592,7 @@ func TestReviewEvidencePathsSkipsListDirDirectoryOperand(t *testing.T) { } type conflictingReadEvidenceProvider struct { + readArgs json.RawMessage mu sync.Mutex readResult *session.ToolResult mainCalls, reviewCalls int @@ -631,7 +632,7 @@ func (p *conflictingReadEvidenceProvider) Stream(_ context.Context, req port.LLM } } else if p.mainCalls == 0 { p.mainCalls++ - chunks = toolCallChunks(session.NewToolCall("read-conflict", "Read", json.RawMessage(`{"path":"website/a.ts","Path":"services/a.ts"}`))) + chunks = toolCallChunks(session.NewToolCall("read-conflict", "Read", p.readArgs)) } else { p.mainCalls++ for _, message := range req.Messages { @@ -651,48 +652,72 @@ func (p *conflictingReadEvidenceProvider) Stream(_ context.Context, req port.LLM }, nil } +// TestBuildContextualEvidenceMatchesBuiltinReadPathDecoding pins that the +// guardrail reviewer's evidence and the built-in Read share one decode: the +// reviewed file is the file read, and a path given in two spellings is refused +// before review or execution. func TestBuildContextualEvidenceMatchesBuiltinReadPathDecoding(t *testing.T) { - ctx := context.Background() - cfg := guardrailE2ECfg(t, false, PostureYolo, "") - cfg.GuardrailsRules = []GuardrailRule{{Match: "Read", Phases: []string{"pre"}, Mode: "block"}} - if err := os.MkdirAll(cfg.Workspace+"/website", 0o700); err != nil { - t.Fatal(err) - } - if err := os.MkdirAll(cfg.Workspace+"/services", 0o700); err != nil { - t.Fatal(err) - } - for path, marker := range map[string]string{ - "website/a.ts": "WEBSITE_READ_MARKER", - "services/a.ts": "SERVICES_READ_MARKER", + for _, tc := range []struct { + name string + args string + ambiguous bool + }{ + {name: "canonical path", args: `{"path":"services/a.ts"}`}, + {name: "path in two spellings", args: `{"path":"website/a.ts","Path":"services/a.ts"}`, ambiguous: true}, } { - if err := os.WriteFile(cfg.Workspace+"/"+path, []byte(marker), 0o600); err != nil { - t.Fatal(err) - } - } - provider := &conflictingReadEvidenceProvider{} - cfg.providerConstructor = func(_ Config, _, _, _ string) port.LLMProvider { return provider } - built, err := buildIsolated(t, ctx, cfg) - if err != nil { - t.Fatalf("Build: %v", err) - } - defer built.Close() - sess, err := built.Service.CreateSession(ctx, session.ModeDefault, session.Limits{MaxTurns: 2}) - if err != nil { - t.Fatal(err) - } - run, err := built.Service.StartRunContent(ctx, sess.ID, "read the service", nil) - if err != nil { - t.Fatal(err) - } - for range run.Events() { - } - built.Service.FinishRun(sess.ID, run) - provider.mu.Lock() - defer provider.mu.Unlock() - if provider.readResult == nil || provider.readResult.IsError || !strings.Contains(provider.readResult.Content, "SERVICES_READ_MARKER") || strings.Contains(provider.readResult.Content, "WEBSITE_READ_MARKER") { - t.Fatalf("main Read result=%+v", provider.readResult) - } - if provider.reviewCalls < 2 || !strings.Contains(provider.evidence, "SERVICES_READ_MARKER") || strings.Contains(provider.evidence, "WEBSITE_READ_MARKER") { - t.Fatalf("review calls=%d evidence=%q", provider.reviewCalls, provider.evidence) + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + cfg := guardrailE2ECfg(t, false, PostureYolo, "") + cfg.GuardrailsRules = []GuardrailRule{{Match: "Read", Phases: []string{"pre"}, Mode: "block"}} + if err := os.MkdirAll(cfg.Workspace+"/website", 0o700); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(cfg.Workspace+"/services", 0o700); err != nil { + t.Fatal(err) + } + for path, marker := range map[string]string{ + "website/a.ts": "WEBSITE_READ_MARKER", + "services/a.ts": "SERVICES_READ_MARKER", + } { + if err := os.WriteFile(cfg.Workspace+"/"+path, []byte(marker), 0o600); err != nil { + t.Fatal(err) + } + } + provider := &conflictingReadEvidenceProvider{readArgs: json.RawMessage(tc.args)} + cfg.providerConstructor = func(_ Config, _, _, _ string) port.LLMProvider { return provider } + built, err := buildIsolated(t, ctx, cfg) + if err != nil { + t.Fatalf("Build: %v", err) + } + defer built.Close() + sess, err := built.Service.CreateSession(ctx, session.ModeDefault, session.Limits{MaxTurns: 2}) + if err != nil { + t.Fatal(err) + } + run, err := built.Service.StartRunContent(ctx, sess.ID, "read the service", nil) + if err != nil { + t.Fatal(err) + } + for range run.Events() { + } + built.Service.FinishRun(sess.ID, run) + provider.mu.Lock() + defer provider.mu.Unlock() + if tc.ambiguous { + if provider.readResult == nil || !provider.readResult.IsError || !strings.Contains(provider.readResult.Content, "ambiguous tool arguments") { + t.Fatalf("main Read result=%+v, want an ambiguous-arguments error", provider.readResult) + } + if provider.reviewCalls != 0 || provider.evidence != "" { + t.Fatalf("ambiguous Read reached review: calls=%d evidence=%q", provider.reviewCalls, provider.evidence) + } + return + } + if provider.readResult == nil || provider.readResult.IsError || !strings.Contains(provider.readResult.Content, "SERVICES_READ_MARKER") || strings.Contains(provider.readResult.Content, "WEBSITE_READ_MARKER") { + t.Fatalf("main Read result=%+v", provider.readResult) + } + if provider.reviewCalls < 2 || !strings.Contains(provider.evidence, "SERVICES_READ_MARKER") || strings.Contains(provider.evidence, "WEBSITE_READ_MARKER") { + t.Fatalf("review calls=%d evidence=%q", provider.reviewCalls, provider.evidence) + } + }) } } diff --git a/internal/app/escapeclassifier.go b/internal/app/escapeclassifier.go index 2025eb0d1f..6a4e1de2e8 100644 --- a/internal/app/escapeclassifier.go +++ b/internal/app/escapeclassifier.go @@ -5,6 +5,7 @@ import ( "path/filepath" "strings" + "github.com/stacklok/mecatl/engine/governance" "github.com/stacklok/mecatl/internal/adapter/osfs" ) @@ -132,7 +133,7 @@ func (c *escapeClassifier) classify(toolName string, args json.RawMessage) escap return escapeInRoot } var a fsPathArg - if err := json.Unmarshal(args, &a); err != nil || a.Path == "" { + if err := governance.DecodeToolArgs(args, &a); err != nil || a.Path == "" { return escapeInRoot } path := a.Path diff --git a/internal/app/escapepolicy.go b/internal/app/escapepolicy.go index 856081969d..18028576f6 100644 --- a/internal/app/escapepolicy.go +++ b/internal/app/escapepolicy.go @@ -319,7 +319,7 @@ func (*escapeGuardrailRoute) failClosedDecision(err error) governance.Permission // in-root then, so this is only ever reached with a well-formed path). func escapePath(args json.RawMessage) string { var a fsPathArg - if err := json.Unmarshal(args, &a); err != nil { + if err := governance.DecodeToolArgs(args, &a); err != nil { return "" } return a.Path diff --git a/user-docs/features/security-and-execution/permissions-and-posture.md b/user-docs/features/security-and-execution/permissions-and-posture.md index 5d93ecd07d..ca1467a812 100644 --- a/user-docs/features/security-and-execution/permissions-and-posture.md +++ b/user-docs/features/security-and-execution/permissions-and-posture.md @@ -167,6 +167,12 @@ patterns use shell-style glob matching. The `permissions:` section parses strictly, so a misspelled key causes the file to be skipped rather than silently changing the policy. +A pattern matches the argument value that the tool runs with. Mecatl rejects a +tool call whose arguments repeat a key, or spell an argument in a different +letter case, before any rule, hook, or approval request sees the call. The model +receives an error and can resend the call with each argument spelled as in the +tool schema. + The configuration scopes, from highest to lowest precedence, are: 1. managed policy; From 3909455b5c9263d9d8d88ae6d6eb12af5d9e124b Mon Sep 17 00:00:00 2001 From: Giuseppe Scuglia Date: Sun, 11 Oct 2026 19:38:53 +0200 Subject: [PATCH 2/8] perf(engine): check tool arguments without allocating CheckToolArgs runs on every dispatched call and every policy evaluation. Its map-based key scan cost five to nine allocations for ordinary arguments, and DecodeToolArgs repeated it, which pushed the evaluator and engine-turn benchmarks over the allocs/op gate. The scan now keeps keys as subslices of the arguments in stack-held arrays, finds repeated keys and case-fold collisions by pairwise comparison, and decodes a key only when it carries an escape or invalid UTF-8. An object with more than sixteen keys switches to map indexes, so large objects stay linear. DecodeToolArgs checks tagged field spellings on the same keys instead of building fold strings. CheckToolArgs now allocates nothing for ordinary arguments, and DecodeToolArgs allocates exactly what its encoding/json decode does. The verdicts are unchanged: a differential test and fuzz target compare them, and the error messages, with the previous implementation. Co-Authored-By: Claude Opus 5.5 (1M context) --- engine/governance/args.go | 240 +++++++++++----- engine/governance/args_scan_test.go | 423 ++++++++++++++++++++++++++++ engine/governance/race_off_test.go | 6 + engine/governance/race_on_test.go | 8 + 4 files changed, 604 insertions(+), 73 deletions(-) create mode 100644 engine/governance/args_scan_test.go create mode 100644 engine/governance/race_off_test.go create mode 100644 engine/governance/race_on_test.go diff --git a/engine/governance/args.go b/engine/governance/args.go index 0a258931a1..994c4e1345 100644 --- a/engine/governance/args.go +++ b/engine/governance/args.go @@ -23,17 +23,35 @@ var ErrAmbiguousToolArgs = errors.New("ambiguous tool arguments") // the Copy/Move operands, and the CallMcpWithQuery target. A top-level key that // matches one of them only under case folding is ambiguous for every tool, // because the policy reads the exact name while a case-insensitive decoder -// would read the variant. -var policyArgNames = []string{ +// would read the variant. They are bytes so a key compares without conversion. +var policyArgNames = argNames( "command", "cmd", "temp_scope", "path", "file_path", "pattern", "url", "query", "source", "destination", "server", "tool", +) + +func argNames(names ...string) [][]byte { + out := make([][]byte, len(names)) + for i, name := range names { + out[i] = []byte(name) + } + return out } // maxArgNameInError bounds how much of a model-supplied key an error quotes. const maxArgNameInError = 64 +// The key scan holds up to argKeysOnStack keys and argFramesOnStack levels of +// nesting in stack arrays, so it checks an ordinary tool call's arguments +// without allocating. An object with more than maxPairwiseKeys keys is indexed +// in a map rather than compared pairwise, so a large object stays linear. +const ( + argKeysOnStack = 16 + argFramesOnStack = 16 + maxPairwiseKeys = 16 +) + // CheckToolArgs reports whether a tool call's raw arguments are in the // canonical form that permission evaluation, approval display and execution // all read identically. It returns an error wrapping ErrAmbiguousToolArgs when: @@ -47,10 +65,10 @@ const maxArgNameInError = 64 // // Empty, non-object and syntactically invalid payloads pass: no reader can // extract a value from them, and the decoders that consume them already fail -// closed. Its errors quote key names only, never argument values. +// closed. Its errors quote key names only, never argument values. It does not +// allocate for a valid object whose keys carry no JSON escapes. func CheckToolArgs(args json.RawMessage) error { - _, err := scanToolArgs(args) - return err + return checkToolArgs(args, nil) } // DecodeToolArgs is the single decode of a tool call's arguments shared by @@ -67,64 +85,70 @@ func DecodeToolArgs(args json.RawMessage, dst any) error { if len(args) == 0 { return nil } - keys, err := scanToolArgs(args) - if err != nil { + if err := checkToolArgs(args, taggedFieldNames(reflect.TypeOf(dst))); err != nil { return err } - if err := checkFieldSpelling(keys, dst); err != nil { + return json.Unmarshal(args, dst) +} + +// checkToolArgs applies CheckToolArgs and then, when fields is non-nil, rejects +// a top-level key that names one of fields only under case folding. +func checkToolArgs(args json.RawMessage, fields *taggedFields) error { + var buf [argKeysOnStack][]byte + keys, err := scanToolArgKeys(args, buf[:0]) + if err != nil { return err } - return json.Unmarshal(args, dst) + return fields.checkSpelling(keys) } -// scanToolArgs validates args (see CheckToolArgs) and returns its top-level -// keys when it is an object. -func scanToolArgs(args json.RawMessage) ([]string, error) { +// scanToolArgKeys validates args (see CheckToolArgs) and appends the top-level +// keys of an object payload to keys. +func scanToolArgKeys(args json.RawMessage, keys [][]byte) ([][]byte, error) { trimmed := bytes.TrimSpace(args) if len(trimmed) == 0 || trimmed[0] != '{' || !json.Valid(trimmed) { - return nil, nil + return keys, nil } - keys, err := scanObjectKeys(trimmed) + keys, err := scanObjectKeys(trimmed, keys) if err != nil { return nil, err } - folded := make(map[string]string, len(keys)) - for _, key := range keys { - fold := foldArgName(key) - if prior, seen := folded[fold]; seen { - return nil, fmt.Errorf("%w: %s and %s name the same argument", ErrAmbiguousToolArgs, quoteArgName(prior), quoteArgName(key)) - } - folded[fold] = key - for _, name := range policyArgNames { - if key != name && strings.EqualFold(key, name) { - return nil, fmt.Errorf("%w: %s must be spelled %q", ErrAmbiguousToolArgs, quoteArgName(key), name) - } - } + if err := checkTopLevelKeys(keys); err != nil { + return nil, err } return keys, nil } +// argFrame is an object or array the key scan is inside. +type argFrame struct { + object bool + expectKey bool + // start is where the object's keys begin in the scan's key list. + start int + // seen indexes the object's keys once it has more than maxPairwiseKeys. + seen map[string]struct{} +} + // scanObjectKeys walks a JSON document that json.Valid has accepted, rejecting a -// repeated key in any object, and returns the keys of the top-level object. It -// decodes only keys, so large string values cost no allocation. -func scanObjectKeys(doc []byte) ([]string, error) { - type frame struct { - object bool - expectKey bool - seen map[string]struct{} - } - var ( - stack []frame - top []string - ) +// repeated key in any object, and appends the keys of the top-level object to +// keys. A key is a subslice of doc unless it carries an escape or invalid UTF-8, +// and a nested object's keys are dropped when it closes, so values and plain +// keys cost no allocation. +func scanObjectKeys(doc []byte, keys [][]byte) ([][]byte, error) { + var frames [argFramesOnStack]argFrame + stack := frames[:0] for i := 0; i < len(doc); i++ { - switch c := doc[i]; c { + switch doc[i] { case '{': - stack = append(stack, frame{object: true, expectKey: true}) + stack = append(stack, argFrame{object: true, expectKey: true, start: len(keys)}) case '[': - stack = append(stack, frame{}) + stack = append(stack, argFrame{}) case '}', ']': + closed := stack[len(stack)-1] stack = stack[:len(stack)-1] + if closed.object && len(stack) > 0 { + keys = keys[:closed.start] + } case ',': if f := &stack[len(stack)-1]; f.object { f.expectKey = true @@ -137,21 +161,40 @@ func scanObjectKeys(doc []byte) ([]string, error) { if err != nil { return nil, err } - if _, repeated := f.seen[key]; repeated { - return nil, fmt.Errorf("%w: %s is given more than once", ErrAmbiguousToolArgs, quoteArgName(key)) - } - if f.seen == nil { - f.seen = make(map[string]struct{}) - } - f.seen[key] = struct{}{} - if len(stack) == 1 { - top = append(top, key) + if f.repeats(keys[f.start:], key) { + return nil, fmt.Errorf("%w: %s is given more than once", ErrAmbiguousToolArgs, quoteArgName(string(key))) } + keys = append(keys, key) } i = end } } - return top, nil + return keys, nil +} + +// repeats reports whether key repeats one of own, the keys the object has given +// so far. It compares pairwise until the object has more than maxPairwiseKeys +// keys, then indexes them in f.seen. +func (f *argFrame) repeats(own [][]byte, key []byte) bool { + if f.seen == nil && len(own) < maxPairwiseKeys { + for _, prior := range own { + if bytes.Equal(prior, key) { + return true + } + } + return false + } + if f.seen == nil { + f.seen = make(map[string]struct{}, 2*len(own)) + for _, prior := range own { + f.seen[string(prior)] = struct{}{} + } + } + if _, repeated := f.seen[string(key)]; repeated { + return true + } + f.seen[string(key)] = struct{}{} + return false } // endOfJSONString returns the index of the closing quote of the JSON string @@ -169,17 +212,60 @@ func endOfJSONString(doc []byte, start int) int { } // decodeJSONKey returns a quoted JSON key as encoding/json decodes it, resolving -// escapes and replacing invalid UTF-8. -func decodeJSONKey(quoted []byte) (string, error) { +// escapes and replacing invalid UTF-8. A key with neither is returned as a +// subslice of quoted. +func decodeJSONKey(quoted []byte) ([]byte, error) { inner := quoted[1 : len(quoted)-1] if bytes.IndexByte(inner, '\\') < 0 && utf8.Valid(inner) { - return string(inner), nil + return inner, nil } var key string if err := json.Unmarshal(quoted, &key); err != nil { - return "", err + return nil, err + } + return []byte(key), nil +} + +// checkTopLevelKeys rejects two top-level keys that are equal under case +// folding, and a key that matches a policy argument name only under case +// folding. +func checkTopLevelKeys(keys [][]byte) error { + var folded map[string][]byte + if len(keys) > maxPairwiseKeys { + folded = make(map[string][]byte, len(keys)) + } + for i, key := range keys { + if prior, ok := foldedEarlierKey(keys[:i], key, folded); ok { + return fmt.Errorf("%w: %s and %s name the same argument", ErrAmbiguousToolArgs, quoteArgName(string(prior)), quoteArgName(string(key))) + } + for _, name := range policyArgNames { + if !bytes.Equal(key, name) && bytes.EqualFold(key, name) { + return fmt.Errorf("%w: %s must be spelled %q", ErrAmbiguousToolArgs, quoteArgName(string(key)), name) + } + } } - return key, nil + return nil +} + +// foldedEarlierKey returns the key in earlier that equals key under case +// folding. With folded nil it compares pairwise with bytes.EqualFold, the +// simple folding foldArgName represents; otherwise it looks key up in folded, +// the index of earlier, and records it there. +func foldedEarlierKey(earlier [][]byte, key []byte, folded map[string][]byte) ([]byte, bool) { + if folded == nil { + for _, prior := range earlier { + if bytes.EqualFold(prior, key) { + return prior, true + } + } + return nil, false + } + fold := foldArgName(string(key)) + if prior, seen := folded[fold]; seen { + return prior, true + } + folded[fold] = key + return nil, false } // foldArgName maps name to a representative of its case-folding class: two @@ -221,30 +307,39 @@ func quoteArgName(name string) string { return fmt.Sprintf("%q", name) } -// checkFieldSpelling rejects a key that names one of dst's explicitly tagged -// struct fields only under case folding. Untagged fields keep encoding/json's -// case-insensitive matching, since their Go name is not a wire spelling. -func checkFieldSpelling(keys []string, dst any) error { - fields := taggedFieldNames(reflect.TypeOf(dst)) - if fields == nil { +// taggedFields holds a struct's explicitly tagged JSON field names. +type taggedFields struct { + names [][]byte +} + +// checkSpelling rejects a key that names one of the tagged fields only under +// case folding. Untagged fields keep encoding/json's case-insensitive matching, +// since their Go name is not a wire spelling. A nil f checks nothing. +func (f *taggedFields) checkSpelling(keys [][]byte) error { + if f == nil { return nil } for _, key := range keys { - if _, exact := fields.exact[key]; exact { + if f.has(key) { continue } - if name, ok := fields.folded[foldArgName(key)]; ok { - return fmt.Errorf("%w: %s must be spelled %q", ErrAmbiguousToolArgs, quoteArgName(key), name) + for _, name := range f.names { + if bytes.EqualFold(key, name) { + return fmt.Errorf("%w: %s must be spelled %q", ErrAmbiguousToolArgs, quoteArgName(string(key)), name) + } } } return nil } -// taggedFields indexes a struct's explicitly tagged JSON field names exactly and -// by their case-folding class. -type taggedFields struct { - exact map[string]struct{} - folded map[string]string +// has reports whether key is exactly one of the tagged names. +func (f *taggedFields) has(key []byte) bool { + for _, name := range f.names { + if bytes.Equal(key, name) { + return true + } + } + return false } var taggedFieldCache sync.Map // reflect.Type -> *taggedFields @@ -259,7 +354,7 @@ func taggedFieldNames(t reflect.Type) *taggedFields { if cached, ok := taggedFieldCache.Load(t); ok { return cached.(*taggedFields) } - fields := &taggedFields{exact: map[string]struct{}{}, folded: map[string]string{}} + fields := &taggedFields{} collectTaggedFields(t.Elem(), fields, 0) taggedFieldCache.Store(t, fields) return fields @@ -292,7 +387,6 @@ func collectTaggedFields(t reflect.Type, fields *taggedFields, depth int) { if !field.IsExported() || name == "" { continue } - fields.exact[name] = struct{}{} - fields.folded[foldArgName(name)] = name + fields.names = append(fields.names, []byte(name)) } } diff --git a/engine/governance/args_scan_test.go b/engine/governance/args_scan_test.go new file mode 100644 index 0000000000..b2b115fed1 --- /dev/null +++ b/engine/governance/args_scan_test.go @@ -0,0 +1,423 @@ +package governance + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "math/rand/v2" + "reflect" + "slices" + "strings" + "testing" + "unicode" +) + +// scanToolArgs returns the top-level keys the production scan collects for args, +// as strings, after the checks CheckToolArgs applies. It lets the key-agreement +// assertion compare the scan with encoding/json. +func scanToolArgs(args json.RawMessage) ([]string, error) { + keys, err := scanToolArgKeys(args, nil) + if err != nil || len(keys) == 0 { + return nil, err + } + out := make([]string, len(keys)) + for i, key := range keys { + out[i] = string(key) + } + return out, nil +} + +// The reference* functions are the map-based scan the allocation-free scan +// replaced. They are the oracle for the differential tests below: both must +// reach the same verdict with the same error for every payload. + +func referenceCheckToolArgs(args json.RawMessage) error { + _, err := referenceScanToolArgs(args) + return err +} + +func referenceDecodeToolArgs(args json.RawMessage, dst any) error { + if len(args) == 0 { + return nil + } + keys, err := referenceScanToolArgs(args) + if err != nil { + return err + } + if err := referenceCheckFieldSpelling(keys, dst); err != nil { + return err + } + return json.Unmarshal(args, dst) +} + +func referenceScanToolArgs(args json.RawMessage) ([]string, error) { + trimmed := bytes.TrimSpace(args) + if len(trimmed) == 0 || trimmed[0] != '{' || !json.Valid(trimmed) { + return nil, nil + } + keys, err := referenceScanObjectKeys(trimmed) + if err != nil { + return nil, err + } + folded := make(map[string]string, len(keys)) + for _, key := range keys { + fold := referenceFold(key) + if prior, seen := folded[fold]; seen { + return nil, fmt.Errorf("%w: %s and %s name the same argument", ErrAmbiguousToolArgs, quoteArgName(prior), quoteArgName(key)) + } + folded[fold] = key + for _, policyName := range policyArgNames { + name := string(policyName) + if key != name && strings.EqualFold(key, name) { + return nil, fmt.Errorf("%w: %s must be spelled %q", ErrAmbiguousToolArgs, quoteArgName(key), name) + } + } + } + return keys, nil +} + +func referenceScanObjectKeys(doc []byte) ([]string, error) { + type frame struct { + object bool + expectKey bool + seen map[string]struct{} + } + var ( + stack []frame + top []string + ) + for i := 0; i < len(doc); i++ { + switch c := doc[i]; c { + case '{': + stack = append(stack, frame{object: true, expectKey: true}) + case '[': + stack = append(stack, frame{}) + case '}', ']': + stack = stack[:len(stack)-1] + case ',': + if f := &stack[len(stack)-1]; f.object { + f.expectKey = true + } + case '"': + end := endOfJSONString(doc, i) + if f := &stack[len(stack)-1]; f.object && f.expectKey { + f.expectKey = false + var key string + if err := json.Unmarshal(doc[i:end+1], &key); err != nil { + return nil, err + } + if _, repeated := f.seen[key]; repeated { + return nil, fmt.Errorf("%w: %s is given more than once", ErrAmbiguousToolArgs, quoteArgName(key)) + } + if f.seen == nil { + f.seen = make(map[string]struct{}) + } + f.seen[key] = struct{}{} + if len(stack) == 1 { + top = append(top, key) + } + } + i = end + } + } + return top, nil +} + +func referenceFold(name string) string { + var b strings.Builder + for _, r := range name { + least := r + for next := unicode.SimpleFold(r); next != r; next = unicode.SimpleFold(next) { + least = min(least, next) + } + b.WriteRune(least) + } + return b.String() +} + +func referenceCheckFieldSpelling(keys []string, dst any) error { + t := reflect.TypeOf(dst) + if t == nil || t.Kind() != reflect.Pointer || t.Elem().Kind() != reflect.Struct { + return nil + } + exact := map[string]struct{}{} + folded := map[string]string{} + var collect func(reflect.Type, int) + collect = func(t reflect.Type, depth int) { + if depth > maxEmbeddingDepth { + return + } + for i := range t.NumField() { + field := t.Field(i) + tag := field.Tag.Get("json") + if tag == "-" { + continue + } + name, _, _ := strings.Cut(tag, ",") + if field.Anonymous && name == "" { + embedded := field.Type + if embedded.Kind() == reflect.Pointer { + embedded = embedded.Elem() + } + if embedded.Kind() == reflect.Struct { + collect(embedded, depth+1) + } + continue + } + if !field.IsExported() || name == "" { + continue + } + exact[name] = struct{}{} + folded[referenceFold(name)] = name + } + } + collect(t.Elem(), 0) + for _, key := range keys { + if _, ok := exact[key]; ok { + continue + } + if name, ok := folded[referenceFold(key)]; ok { + return fmt.Errorf("%w: %s must be spelled %q", ErrAmbiguousToolArgs, quoteArgName(key), name) + } + } + return nil +} + +func errText(err error) string { + if err == nil { + return "" + } + return err.Error() +} + +// assertScanMatchesReference checks that the production scan and the reference +// scan agree on one payload: the CheckToolArgs verdict and error, the top-level +// keys, and DecodeToolArgs' error and decoded value for the argument structs +// the policy and the built-in tools decode into. +func assertScanMatchesReference(t *testing.T, args string) { + t.Helper() + raw := json.RawMessage(args) + got, want := CheckToolArgs(raw), referenceCheckToolArgs(raw) + if errText(got) != errText(want) || errors.Is(got, ErrAmbiguousToolArgs) != errors.Is(want, ErrAmbiguousToolArgs) { + t.Errorf("CheckToolArgs(%q) = %v, reference %v", args, got, want) + } + keys, err := scanToolArgs(raw) + wantKeys, wantErr := referenceScanToolArgs(raw) + if !slices.Equal(keys, wantKeys) || errText(err) != errText(wantErr) { + t.Errorf("scan keys of %q = %q (%v), reference %q (%v)", args, keys, err, wantKeys, wantErr) + } + for _, newDst := range []func() any{ + func() any { return new(shellExecArgs) }, + func() any { return new(pathExecArgs) }, + func() any { return new(copyExecArgs) }, + func() any { return new(shellCommandArgs) }, + func() any { return new(resourceArgs) }, + func() any { return new(map[string]json.RawMessage) }, + } { + gotDst, wantDst := newDst(), newDst() + got, want := DecodeToolArgs(raw, gotDst), referenceDecodeToolArgs(raw, wantDst) + if errText(got) != errText(want) || !reflect.DeepEqual(gotDst, wantDst) { + t.Errorf("DecodeToolArgs(%q, %T) = %v %+v, reference %v %+v", args, gotDst, got, gotDst, want, wantDst) + } + } +} + +// argKeyPool mixes the policy names with case, Unicode-fold and escaped +// spellings of them, plain keys that fold onto each other, non-ASCII and +// invalid UTF-8 keys, and a key long enough to be truncated in an error. +var argKeyPool = []string{ + `"command"`, `"cmd"`, `"temp_scope"`, `"timeout_ms"`, `"background"`, + `"path"`, `"file_path"`, `"pattern"`, `"url"`, `"query"`, + `"source"`, `"destination"`, `"server"`, `"tool"`, `"content"`, + `"Command"`, `"COMMAND"`, `"CMD"`, `"Temp_Scope"`, `"PATH"`, `"pAth"`, `"File_Path"`, + `"URL"`, `"Query"`, `"Tool"`, `"Server"`, `"Destination"`, `"Timeout_MS"`, + `"ſource"`, `"de\u017ftination"`, `"\u0063ommand"`, `"\u0043ommand"`, `"pat\u0068"`, + `"key"`, `"Key"`, `"Key"`, `"\u212aey"`, `"x"`, `"X"`, `"a\"b"`, `"a\\\"b"`, + `"ü"`, `"Ü"`, `"straße"`, `"STRASSE"`, `"bad` + "\xff" + `key"`, `"bad` + "\xfe" + `key"`, + `"\ud800"`, `"\udc00"`, `"\ufffd"`, `"�"`, `""`, + `"` + strings.Repeat("k", maxArgNameInError+8) + `"`, +} + +// argPayload builds a random argument document from argKeyPool. Objects reach +// past the stack-held key and frame counts and past maxPairwiseKeys, so the +// heap and map fallbacks are compared too. +func argPayload(r *rand.Rand, depth int) string { + var b strings.Builder + writeArgValue(&b, r, depth, true) + return b.String() +} + +func writeArgValue(b *strings.Builder, r *rand.Rand, depth int, object bool) { + switch { + case object || (depth > 0 && r.IntN(4) == 0): + b.WriteByte('{') + n := r.IntN(6) + if r.IntN(5) == 0 { + n = maxPairwiseKeys + r.IntN(2*maxPairwiseKeys) + } + // A large object mostly carries generated keys: distinct ones, so the + // map-indexed checks run to the end, or random ones, which repeat. + distinct := r.IntN(2) == 0 + for i := range n { + if i > 0 { + b.WriteByte(',') + } + switch { + case n > 8 && r.IntN(24) == 0: + fmt.Fprintf(b, `"K%d"`, r.IntN(n)) + case n > 8 && distinct && r.IntN(8) > 0: + fmt.Fprintf(b, `"k%d"`, i) + case n > 8 && !distinct && r.IntN(3) > 0: + fmt.Fprintf(b, `"k%d"`, r.IntN(4*n)) + default: + b.WriteString(argKeyPool[r.IntN(len(argKeyPool))]) + } + b.WriteByte(':') + writeArgValue(b, r, depth-1, false) + } + b.WriteByte('}') + case depth > 0 && r.IntN(5) == 0: + b.WriteByte('[') + for i := range r.IntN(4) { + if i > 0 { + b.WriteByte(',') + } + writeArgValue(b, r, depth-1, false) + } + b.WriteByte(']') + default: + b.WriteString([]string{`"ls"`, `1`, `null`, `true`, `"{\"command\":1}"`, `"a\\\"b"`, `[]`, `{}`}[r.IntN(8)]) + } +} + +// TestCheckToolArgsMatchesReference compares the production scan with the +// reference scan over the seeds, hand-picked edge cases and generated payloads. +func TestCheckToolArgsMatchesReference(t *testing.T) { + t.Parallel() + nested := func(depth int, inner string) string { + return strings.Repeat(`{"a":[`, depth) + inner + strings.Repeat(`]}`, depth) + } + manyKeys := func(n int, extra string) string { + var b strings.Builder + b.WriteByte('{') + for i := range n { + fmt.Fprintf(&b, `"k%d":%d,`, i, i) + } + b.WriteString(extra) + b.WriteByte('}') + return b.String() + } + cases := slices.Clone(toolArgsSeeds) + cases = append(cases, + " \t\r\n{\"command\":\"ls\",\"Command\":\"pwd\"} \n", + "\u00a0{\"command\":\"ls\",\"Command\":\"pwd\"}", + "\u0085{\"Path\":\"a\"}\u0085", + `{"command":"ls"}x`, + `{"command":"ls","Command"`, + `{"a":{"b":{"c":1,"c":2}},"a2":1}`, + `{"a":[{"x":1},{"x":2}],"b":{"x":1},"x":1}`, + `{"a":{"x":1,"y":{"x":1}},"x":{"x":{"x":1}}}`, + `{"\u0063ommand":"ls","command":"pwd"}`, + `{"command":"ls","c\u006fmmand":"pwd"}`, + `{"a\"b":1,"a\u0022b":2}`, + `{"Key":1,"key":2}`, + `{"KEY":1,"Key":2}`, + `{"\u212aey":1,"key":2}`, + `{"KEY":1,"\u212aey":2}`, + `{"s":1,"\u017f":2}`, + `{"straße":1,"STRASSE":2}`, + `{"ü":1,"Ü":2}`, + "{\"bad\xffkey\":1,\"bad\xfekey\":2}", + "{\"bad\xffkey\":1,\"bad\\ufffdkey\":2}", + `{"\ud800":1,"\udc00":2}`, + `{"":1,"":2}`, + nested(argFramesOnStack+4, `{"x":1,"x":2}`), + nested(argFramesOnStack+4, `{"x":1,"X":2}`), + nested(3*argFramesOnStack, `{"command":"ls"}`), + manyKeys(maxPairwiseKeys, `"k3":0`), + manyKeys(maxPairwiseKeys+1, `"k3":0`), + manyKeys(maxPairwiseKeys-1, `"K3":0`), + manyKeys(maxPairwiseKeys, `"K3":0`), + manyKeys(3*maxPairwiseKeys, `"K40":0`), + manyKeys(3*maxPairwiseKeys, `"Command":0`), + manyKeys(3*maxPairwiseKeys, `"x":{"k1":1,"k1":2}`), + manyKeys(3*maxPairwiseKeys, `"x":`+manyKeys(3*maxPairwiseKeys, `"k7":7`)), + manyKeys(3*maxPairwiseKeys, `"x":`+manyKeys(3*maxPairwiseKeys, `"K7":7`)), + `{"`+strings.Repeat("ſ", maxArgNameInError+2)+`":1,"`+strings.Repeat("S", maxArgNameInError+2)+`":2}`, + ) + r := rand.New(rand.NewPCG(2265, 1)) + for range 20_000 { + args := argPayload(r, 1+r.IntN(4)) + switch r.IntN(10) { + case 0: + args = " \n" + args + "\t" + case 1: + args = args[:r.IntN(len(args)+1)] + case 2: + args = "\u00a0" + args + } + cases = append(cases, args) + } + for _, args := range cases { + assertScanMatchesReference(t, args) + } +} + +// FuzzCheckToolArgsMatchesReference compares the production scan with the +// reference scan for arbitrary payloads. +func FuzzCheckToolArgsMatchesReference(f *testing.F) { + for _, seed := range toolArgsSeeds { + f.Add(seed) + } + r := rand.New(rand.NewPCG(2265, 2)) + for range 64 { + f.Add(argPayload(r, 3)) + } + f.Fuzz(func(t *testing.T, args string) { + assertScanMatchesReference(t, args) + }) +} + +// TestCheckToolArgsDoesNotAllocate pins that the canonical-form check, which +// every dispatched call and every policy evaluation runs, costs no allocation +// for the argument objects real tool calls carry. +func TestCheckToolArgsDoesNotAllocate(t *testing.T) { + if raceEnabled { + t.Skip("allocation counts are not meaningful under the race detector") + } + for _, tc := range []struct{ name, args string }{ + {"shell", `{"command":"go test ./... && git status --short","timeout_ms":120000,"background":false,"temp_scope":"managed"}`}, + {"file tool", `{"file_path":"/proj/internal/app/main.go","old_string":"return nil","new_string":"return err","replace_all":false}`}, + {"nested", `{"path":"src","edits":[{"old_string":"a","new_string":"b"},{"old_string":"c","new_string":"d"}],"meta":{"tag":"x"}}`}, + {"non-ASCII", `{"path":"docs/straße.md","query":"größe"}`}, + } { + args := json.RawMessage(tc.args) + if err := CheckToolArgs(args); err != nil { + t.Fatalf("%s: CheckToolArgs(%s) = %v, want nil", tc.name, tc.args, err) + } + if allocs := testing.AllocsPerRun(1000, func() { _ = CheckToolArgs(args) }); allocs != 0 { + t.Errorf("%s: CheckToolArgs allocations = %v, want 0", tc.name, allocs) + } + } +} + +// TestDecodeToolArgsAllocatesLikeUnmarshal pins that DecodeToolArgs costs no +// allocation beyond the encoding/json decode it wraps. +func TestDecodeToolArgsAllocatesLikeUnmarshal(t *testing.T) { + if raceEnabled { + t.Skip("allocation counts are not meaningful under the race detector") + } + args := json.RawMessage(`{"command":"go test ./...","timeout_ms":5000,"temp_scope":"managed"}`) + decode := testing.AllocsPerRun(1000, func() { + var dst shellExecArgs + _ = DecodeToolArgs(args, &dst) + }) + unmarshal := testing.AllocsPerRun(1000, func() { + var dst shellExecArgs + _ = json.Unmarshal(args, &dst) + }) + if decode > unmarshal { + t.Errorf("DecodeToolArgs allocations = %v, json.Unmarshal %v", decode, unmarshal) + } +} diff --git a/engine/governance/race_off_test.go b/engine/governance/race_off_test.go new file mode 100644 index 0000000000..7ca0735e33 --- /dev/null +++ b/engine/governance/race_off_test.go @@ -0,0 +1,6 @@ +//go:build !race + +package governance + +// raceEnabled is false in a normal test build; see race_on_test.go. +const raceEnabled = false diff --git a/engine/governance/race_on_test.go b/engine/governance/race_on_test.go new file mode 100644 index 0000000000..d626ce8189 --- /dev/null +++ b/engine/governance/race_on_test.go @@ -0,0 +1,8 @@ +//go:build race + +package governance + +// raceEnabled reports whether the test binary was built with the race detector. +// Allocation counts mean nothing then: sync.Pool drops a share of the items put +// back, so json.Valid's pooled scanner is reallocated at random. +const raceEnabled = true From 1f2e20f7c9152a167abf8fd3f750f8438c7e1358 Mon Sep 17 00:00:00 2001 From: Giuseppe Scuglia Date: Sun, 11 Oct 2026 20:19:06 +0200 Subject: [PATCH 3/8] test(engine): restore the escaped-key cases in the argument checks Three cases in args_test.go meant to spell a key with a JSON escape (`command`) had become literal copies of their neighbours, so the fuzz seeds and the classification table covered a plain duplicate twice and never covered an escaped one. They now carry the escape again: - a key repeated once plainly and once escaped is ambiguous; - a key spelled only with an escape is the canonical name, not ambiguous. Refs #2264 Co-Authored-By: Claude Opus 5.5 (1M context) --- engine/governance/args_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/engine/governance/args_test.go b/engine/governance/args_test.go index 5e12a0ebda..1e5e1ca095 100644 --- a/engine/governance/args_test.go +++ b/engine/governance/args_test.go @@ -27,7 +27,7 @@ var toolArgsSeeds = []string{ `{"command":7,"cmd":"pwd"}`, `{"command":null}`, `{"command":"ls","command":"pwd"}`, - `{"command":"ls","command":"pwd"}`, + `{"command":"ls","\u0063ommand":"pwd"}`, `{"Command":"pwd"}`, `{"command":"ls","COMMAND":"pwd"}`, `{"COMMAND":"pwd","command":"ls"}`, @@ -68,12 +68,12 @@ func TestCheckToolArgsClassification(t *testing.T) { {`[]`, false}, {`{bad`, false}, {`{"command":"ls"}`, false}, - {`{"command":"ls"}`, false}, + {`{"\u0063ommand":"ls"}`, false}, {`{"command":"ls","cmd":"pwd"}`, false}, {`{"filePath":"a"}`, false}, {`{"meta":{"x":1,"X":2}}`, false}, {`{"command":"ls","command":"pwd"}`, true}, - {`{"command":"ls","command":"pwd"}`, true}, + {`{"command":"ls","\u0063ommand":"pwd"}`, true}, {`{"Command":"pwd"}`, true}, {`{"command":"ls","COMMAND":"pwd"}`, true}, {`{"command":"ls","Temp_Scope":"system"}`, true}, From 5755a51637e3b947d723f17dcf441655dbd4c137 Mon Sep 17 00:00:00 2001 From: Giuseppe Scuglia Date: Thu, 15 Oct 2026 15:54:45 +0200 Subject: [PATCH 4/8] fix(engine): match a built-in tool's rules against the argument it runs The single-resource extractor picked the first of path, file_path, pattern, url and query regardless of the tool, so a call could carry an allowed decoy in a field the tool ignores: WebFetch evaluated "path" but fetched "url", and Glob evaluated "path" but listed "pattern". Built-in tools now match the argument they act on. Tools whose contract governance does not know (MCP, custom) keep the generic precedence, and a pattern Deny or Ask on any other candidate argument also applies. Co-Authored-By: Claude Opus 5.5 (1M context) --- engine/CHANGELOG.md | 15 +- engine/adapter/permpolicy/permpolicy_test.go | 3 +- engine/agent/policy_args_decode_test.go | 158 ++++++++++++++++++ engine/governance/evaluator.go | 126 ++++++++++++-- engine/governance/resource_operand_test.go | 111 ++++++++++++ .../permissions-and-posture.md | 26 ++- 6 files changed, 414 insertions(+), 25 deletions(-) create mode 100644 engine/governance/resource_operand_test.go diff --git a/engine/CHANGELOG.md b/engine/CHANGELOG.md index b9f4394730..b11e17e3eb 100644 --- a/engine/CHANGELOG.md +++ b/engine/CHANGELOG.md @@ -444,8 +444,19 @@ The covered surface is the eight core packages (`session`, `governance`, `learni returns `("", false)` for them. This replaces the earlier case-insensitive, last-key-wins reading of `tool.LocalFileOperands` (issue #2094) for those shapes. Callers that sent case-variant or repeated keys must send each - argument once, spelled as in the tool schema. Changed (pre-v1 minor); no - exported signatures changed. + argument once, spelled as in the tool schema. A key that exactly names an + untagged destination field is still accepted. Plan-mode `PresentPlan` calls + apply the same check before the plan approval is surfaced. + + A built-in tool's rules now match the argument the tool acts on: `path` for + `Read`, `Write`, `Edit`, `Remove`, `ListDir` and `Grep`, `pattern` for `Glob`, + `url` for `WebFetch`, and `query` for `WebSearch`. An extra policy-looking + argument the tool ignores no longer selects the matched value, and a `Grep` + call without `path` matches only tool-wide rules instead of its regular + expression. Other tools are still matched on the first of `path`, + `file_path`, `pattern`, `url`, and `query`, and a pattern `Deny` or `Ask` on + any of the others now also applies. Changed (pre-v1 minor); no exported + signatures changed. - **Target-scoped project guidance and ephemeral snapshot** — `prompt.InstructionAssembler` now requires `Assemble(context.Context, []string, *session.InstructionSnapshot, int)` diff --git a/engine/adapter/permpolicy/permpolicy_test.go b/engine/adapter/permpolicy/permpolicy_test.go index cb5ecf5d83..2caf4d7026 100644 --- a/engine/adapter/permpolicy/permpolicy_test.go +++ b/engine/adapter/permpolicy/permpolicy_test.go @@ -23,7 +23,8 @@ func shellCall(cmd string) session.ToolCall { } func fileCall(toolName, p string) session.ToolCall { - args, _ := json.Marshal(map[string]string{"file_path": p}) + // The built-in file tools act on "path", the argument their rules match. + args, _ := json.Marshal(map[string]string{"path": p}) return session.NewToolCall("c1", toolName, args) } diff --git a/engine/agent/policy_args_decode_test.go b/engine/agent/policy_args_decode_test.go index b9d6591fc7..5f2ed1680e 100644 --- a/engine/agent/policy_args_decode_test.go +++ b/engine/agent/policy_args_decode_test.go @@ -12,6 +12,8 @@ import ( "github.com/stacklok/mecatl/engine/adapter/memfs" "github.com/stacklok/mecatl/engine/adapter/mockllm" "github.com/stacklok/mecatl/engine/adapter/permpolicy" + "github.com/stacklok/mecatl/engine/adapter/search" + "github.com/stacklok/mecatl/engine/adapter/webfetch" "github.com/stacklok/mecatl/engine/governance" "github.com/stacklok/mecatl/engine/port" "github.com/stacklok/mecatl/engine/session" @@ -291,3 +293,159 @@ func (s *argsRecordingTool) calls() int { defer s.mu.Unlock() return s.n } + +// runDenyingAsks runs one call, refusing every approval request, and reports the +// call's result and whether approval was requested. +func runDenyingAsks(t *testing.T, catalog *tool.Catalog, policy port.PermissionPolicy, env tool.Environment, call session.ToolCall) (result session.ToolResult, asked bool) { + t.Helper() + eng := NewEngine(Deps{LLM: mockllm.New(mockllm.ToolCallTurn(call), mockllm.TextTurn("done")), Catalog: catalog, Policy: policy}) + sess := session.New("policy-args", session.ModeDefault, env.Ref(), session.Limits{MaxTurns: 3}, time.Now()) + run := eng.Run(context.Background(), sess, env, RunRequest{Text: "run"}) + seen := false + for event := range run.Events() { + if event.Type == session.EvPermissionAsk && event.Ask != nil { + asked = true + run.Approve(event.Ask.AskID, session.VerdictDeny) + } + if event.Type == session.EvToolResult && event.ToolResult != nil && event.ToolResult.CallID == call.ID { + result, seen = *event.ToolResult, true + } + } + if !seen { + t.Fatalf("no tool result for %s", call.ID) + } + return result, asked +} + +// recordingSearchProvider records the queries WebSearch sends it. +type recordingSearchProvider struct { + mu sync.Mutex + queries []string +} + +func (p *recordingSearchProvider) Search(_ context.Context, q tool.SearchQuery) ([]tool.SearchResult, error) { + p.mu.Lock() + defer p.mu.Unlock() + p.queries = append(p.queries, q.Query) + return []tool.SearchResult{{Title: "result", URL: "https://example.test/", Snippet: "snippet"}}, nil +} + +func (p *recordingSearchProvider) seen() []string { + p.mu.Lock() + defer p.mu.Unlock() + return append([]string(nil), p.queries...) +} + +// TestBuiltinToolDenyMatchesTheExecutedOperand runs real built-in tools whose +// calls carry an allowed decoy in an argument the tool ignores and another value +// in the argument it acts on. A Deny written for the executed operand must stop +// the call before the tool runs, and with no rule for the executed operand the +// Allow written for the decoy must not admit it. The canonical call is the +// positive control that the rules admit the allowed operand. +func TestBuiltinToolDenyMatchesTheExecutedOperand(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + tool func(*recordingSearchProvider) tool.Tool + allow string + deny string + decoyArgs string + canonical string + // executed reports what the tool acted on, from the workspace result or + // the provider. + executed func(session.ToolResult, *recordingSearchProvider) string + }{ + { + name: "Glob", + tool: func(*recordingSearchProvider) tool.Tool { return fstools.GlobTool{} }, + allow: "public/**", + deny: "secret/**", + decoyArgs: `{"path":"public/**","pattern":"secret/**"}`, + canonical: `{"pattern":"public/**"}`, + executed: func(res session.ToolResult, _ *recordingSearchProvider) string { + if strings.Contains(res.Content, "secret/key.txt") { + return "secret/**" + } + if strings.Contains(res.Content, "public/readme.txt") { + return "public/**" + } + return "" + }, + }, + { + name: "WebSearch", + tool: func(p *recordingSearchProvider) tool.Tool { return search.NewWebSearchTool(p) }, + allow: "public *", + deny: "secret *", + decoyArgs: `{"pattern":"public docs","query":"secret docs"}`, + canonical: `{"query":"public docs"}`, + executed: func(_ session.ToolResult, p *recordingSearchProvider) string { + return strings.Join(p.seen(), ",") + }, + }, + { + // The denied call never reaches the network; the canonical call is not + // run because it would. + name: "WebFetch", + tool: func(*recordingSearchProvider) tool.Tool { return webfetch.New() }, + allow: "https://public.example/*", + deny: "https://secret.example/*", + decoyArgs: `{"path":"https://public.example/","url":"https://secret.example/"}`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + run := func(args string, rules ...governance.Rule) (session.ToolResult, bool, *recordingSearchProvider) { + ws := memfs.NewWorkspace("/ws") + for path, body := range map[string]string{"public/readme.txt": "hello", "secret/key.txt": "token"} { + if err := ws.Write(t.Context(), path, []byte(body)); err != nil { + t.Fatal(err) + } + } + provider := &recordingSearchProvider{} + catalog := tool.NewCatalog() + catalog.MustRegister(tc.tool(provider)) + result, asked := runDenyingAsks(t, catalog, permpolicy.NewPolicy(rules, nil), testEnvironment(ws, nil), session.NewToolCall("c", tc.name, []byte(args))) + return result, asked, provider + } + + allow := governance.Rule{Scope: governance.ScopeCLI, Tool: tc.name, Pattern: tc.allow, Effect: governance.Allow} + deny := governance.Rule{Scope: governance.ScopeCLI, Tool: tc.name, Pattern: tc.deny, Effect: governance.Deny} + for _, rc := range []struct { + rules []governance.Rule + wantAsk bool + }{ + {rules: []governance.Rule{allow, deny}}, + // No rule covers the executed operand, so the call needs approval; + // the decoy's Allow must not admit it. The approval is refused. + {rules: []governance.Rule{allow}, wantAsk: true}, + } { + result, asked, provider := run(tc.decoyArgs, rc.rules...) + if asked != rc.wantAsk { + t.Errorf("decoy call under %d rule(s): asked = %v, want %v", len(rc.rules), asked, rc.wantAsk) + } + if !result.IsError || !strings.Contains(result.Content, "permission denied") { + t.Fatalf("decoy call under %d rule(s): result = %+v, want a permission denial", len(rc.rules), result) + } + if tc.executed != nil { + if got := tc.executed(result, provider); got != "" { + t.Fatalf("decoy call under %d rule(s) acted on %q", len(rc.rules), got) + } + } + } + if tc.canonical == "" { + return + } + result, asked, provider := run(tc.canonical, allow, deny) + if asked { + t.Fatal("the canonical call asked for approval") + } + if result.IsError { + t.Fatalf("canonical call failed: %+v", result) + } + if got := tc.executed(result, provider); got == "" { + t.Fatalf("canonical call did not act on the allowed operand: %+v", result) + } + }) + } +} diff --git a/engine/governance/evaluator.go b/engine/governance/evaluator.go index c1163b9ac0..cdc4299701 100644 --- a/engine/governance/evaluator.go +++ b/engine/governance/evaluator.go @@ -2,6 +2,7 @@ package governance import ( "encoding/json" + "slices" "strings" ) @@ -194,7 +195,7 @@ func (*Evaluator) LearnableRule(tool string, args json.RawMessage) (Rule, bool) // learnablePattern derives the exact canonical pattern a learned rule would carry // for (tool, args), or ok=false when the call must not be learned. For Shell it // requires EXACTLY one canonicalized segment with no substitution/grouping; for -// every other tool it reuses nonShellPattern (the path/target field). An empty +// every other tool it reuses nonShellPattern (the operand rules match). An empty // derived pattern is returned as-is so the caller refuses a tool-wide grant. func learnablePattern(tool string, args json.RawMessage) (string, bool) { if CheckToolArgs(args) != nil { @@ -258,8 +259,25 @@ func (e *Evaluator) resolve(rules []Rule, tool string, args json.RawMessage) Per return e.resolvePatterns(rules, tool, patterns) } } - pattern := nonShellPattern(tool, args) - return e.resolveSimple(rules, tool, pattern) + primary, others := resourcePatterns(tool, args) + return e.resolveResource(rules, tool, primary, others) +} + +// resolveResource evaluates a single-resource call on its primary operand, then +// lets a rule written for one of its other candidate operands make the decision +// stricter. Governance cannot tell which of those arguments a tool without a +// declared operand acts on, so a pattern Deny or Ask on any of them holds. Only +// a rule with a pattern escalates: a tool-wide rule and the no-match default +// already applied to the primary operand. +func (e *Evaluator) resolveResource(rules []Rule, tool, primary string, others []string) PermissionDecision { + d := e.resolveSimple(rules, tool, primary) + for _, pattern := range others { + other, rule := e.resolveSimpleRule(rules, tool, pattern) + if rule != nil && rule.Pattern != "" && effectRank(other.Effect) > effectRank(d.Effect) { + d = other + } + } + return d } // resolvePatterns evaluates every resource named by one tool call independently. @@ -608,28 +626,102 @@ type resourceArgs struct { Query json.RawMessage `json:"query"` } +// The resource argument names, as resourceArgs tags them. +const ( + argPath = "path" + argFilePath = "file_path" + argPattern = "pattern" + argURL = "url" + argQuery = "query" +) + +// field returns the raw value of the argument name, one of resourceArgNames. +func (a *resourceArgs) field(name string) json.RawMessage { + switch name { + case argPath: + return a.Path + case argFilePath: + return a.FilePath + case argPattern: + return a.Pattern + case argURL: + return a.URL + case argQuery: + return a.Query + } + return nil +} + +// resourceArgNames are the resource arguments a tool without a declared operand +// is matched on, in precedence order. +var resourceArgNames = []string{argPath, argFilePath, argPattern, argURL, argQuery} + +// builtinResourceArgs names, for each built-in single-resource tool, the +// argument the tool acts on. Its rules are matched against that argument only, +// so an extra argument the tool ignores cannot choose the value a rule sees. +var builtinResourceArgs = map[string]string{ + "Read": argPath, + "Write": argPath, + "Edit": argPath, + "Remove": argPath, + "ListDir": argPath, + "Grep": argPath, + "Glob": argPattern, + "WebFetch": argURL, + "WebSearch": argQuery, +} + // nonShellPattern derives the pattern string to match a single-resource non-Shell tool call -// against. It uses the most common path/target field of the built-in tools so -// rules can target specific files; unknown or ambiguous shapes fall back to the -// empty string (which only matches tool-wide rules). -func nonShellPattern(_ string, args json.RawMessage) string { +// against: the primary operand resourcePatterns returns. +func nonShellPattern(tool string, args json.RawMessage) string { + primary, _ := resourcePatterns(tool, args) + return primary +} + +// resourcePatterns derives the values a single-resource non-Shell tool call is +// matched against. A built-in tool is matched on the argument it acts on (see +// builtinResourceArgs) and has no other operand; a missing or non-string +// argument derives the empty pattern, which only tool-wide rules match. Any +// other tool's argument contract is unknown to governance, so primary is the +// first string-valued resource argument in resourceArgNames order and others +// holds the remaining ones; resolveResource lets a rule written for any of them +// restrict the call. Ambiguous or invalid arguments derive no pattern. +func resourcePatterns(tool string, args json.RawMessage) (primary string, others []string) { if len(args) == 0 { - return "" + return "", nil } var fields resourceArgs if err := DecodeToolArgs(args, &fields); err != nil { - return "" + return "", nil } - for _, raw := range []json.RawMessage{fields.Path, fields.FilePath, fields.Pattern, fields.URL, fields.Query} { - if raw == nil { - continue - } - var s string - if json.Unmarshal(raw, &s) == nil { - return s + if name, ok := builtinResourceArgs[tool]; ok { + s, _ := stringArg(fields.field(name)) + return s, nil + } + found := false + for _, name := range resourceArgNames { + s, ok := stringArg(fields.field(name)) + switch { + case !ok: + case !found: + primary, found = s, true + case s != primary && !slices.Contains(others, s): + others = append(others, s) } } - return "" + return primary, others +} + +// stringArg reports raw's value when it is a JSON string. +func stringArg(raw json.RawMessage) (string, bool) { + if raw == nil { + return "", false + } + var s string + if json.Unmarshal(raw, &s) != nil { + return "", false + } + return s, true } // shellCommandArgs is the policy view of a Shell call's arguments, decoded diff --git a/engine/governance/resource_operand_test.go b/engine/governance/resource_operand_test.go new file mode 100644 index 0000000000..7feb81f8d3 --- /dev/null +++ b/engine/governance/resource_operand_test.go @@ -0,0 +1,111 @@ +package governance + +import ( + "encoding/json" + "testing" +) + +// TestBuiltinToolRulesMatchTheExecutedOperand pins that a built-in tool's rules +// see the argument the tool acts on, never another policy-looking argument the +// tool ignores. Each call names an allowed decoy in a field that the generic +// precedence would have read first, and the executed operand in the field the +// tool runs: a Deny on the executed operand must win, and an Allow written for +// the decoy must not admit the call. +// +// MUTATION-VERIFY: drop a tool from builtinResourceArgs and its row fails. +func TestBuiltinToolRulesMatchTheExecutedOperand(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + tool, args, executed, decoy string + }{ + {tool: "WebFetch", args: `{"path":"https://allowed.example/","url":"https://blocked.example/"}`, executed: "https://blocked.example/", decoy: "https://allowed.example/"}, + {tool: "WebSearch", args: `{"pattern":"allowed","query":"blocked"}`, executed: "blocked", decoy: "allowed"}, + {tool: "Glob", args: `{"path":"allowed/**","pattern":"blocked/**"}`, executed: "blocked/**", decoy: "allowed/**"}, + {tool: "Read", args: `{"file_path":"allowed.txt","path":"blocked.txt"}`, executed: "blocked.txt", decoy: "allowed.txt"}, + {tool: "Grep", args: `{"pattern":"allowed","path":"blocked/**"}`, executed: "blocked/**", decoy: "allowed"}, + } { + t.Run(tc.tool, func(t *testing.T) { + t.Parallel() + deny := NewEvaluator([]Rule{ + {Scope: ScopeUser, Tool: tc.tool, Pattern: tc.decoy, Effect: Allow}, + {Scope: ScopeUser, Tool: tc.tool, Pattern: tc.executed, Effect: Deny}, + }) + if got := deny.Evaluate(tc.tool, json.RawMessage(tc.args), false); got.Effect != Deny { + t.Errorf("Deny on the executed operand: got %v (%s), want Deny", got.Effect, got.Reason) + } + decoyOnly := NewEvaluator([]Rule{{Scope: ScopeUser, Tool: tc.tool, Pattern: tc.decoy, Effect: Allow}}) + if got := decoyOnly.Evaluate(tc.tool, json.RawMessage(tc.args), false); got.Effect == Allow { + t.Errorf("Allow written for the ignored decoy admitted the call: %s", got.Reason) + } + rule, ok := deny.LearnableRule(tc.tool, json.RawMessage(tc.args)) + if !ok || rule.Pattern != tc.executed { + t.Errorf("LearnableRule = %+v, %v; want the executed operand %q", rule, ok, tc.executed) + } + }) + } +} + +// TestGrepWithoutPathMatchesToolWideRulesOnly pins that a Grep call without a +// path, which searches the whole workspace, is not matched as if its regular +// expression were a path: only tool-wide rules apply. +func TestGrepWithoutPathMatchesToolWideRulesOnly(t *testing.T) { + t.Parallel() + e := NewEvaluator([]Rule{{Scope: ScopeUser, Tool: "Grep", Pattern: "TODO", Effect: Allow}}) + if got := e.Evaluate("Grep", json.RawMessage(`{"pattern":"TODO"}`), false); got.Effect == Allow { + t.Fatalf("a path rule matched Grep's regular expression: %s", got.Reason) + } + if _, ok := e.LearnableRule("Grep", json.RawMessage(`{"pattern":"TODO"}`)); ok { + t.Fatal("a Grep call without a path must not be learnable as a tool-wide allow") + } +} + +// TestUndeclaredToolRulesCoverEveryResourceOperand pins the deny-dominant +// matching for a tool whose argument contract governance does not know, such as +// an MCP tool: the call is evaluated on its first resource argument, and a +// pattern rule on any other resource argument can only make it stricter. +func TestUndeclaredToolRulesCoverEveryResourceOperand(t *testing.T) { + t.Parallel() + const name = "mcp__web__fetch" + args := json.RawMessage(`{"path":"https://allowed.example/","url":"https://blocked.example/"}`) + for _, tc := range []struct { + name string + rules []Rule + want Effect + }{ + { + name: "deny on another operand", + rules: []Rule{ + {Scope: ScopeUser, Tool: name, Pattern: "https://allowed.example/*", Effect: Allow}, + {Scope: ScopeUser, Tool: name, Pattern: "https://blocked.example/*", Effect: Deny}, + }, + want: Deny, + }, + { + name: "ask on another operand", + rules: []Rule{ + {Scope: ScopeUser, Tool: name, Pattern: "https://allowed.example/*", Effect: Allow}, + {Scope: ScopeUser, Tool: name, Pattern: "https://blocked.example/*", Effect: Ask}, + }, + want: Ask, + }, + { + name: "no rule on another operand", + rules: []Rule{{Scope: ScopeUser, Tool: name, Pattern: "https://allowed.example/*", Effect: Allow}}, + want: Allow, + }, + { + name: "allow on another operand", + rules: []Rule{ + {Scope: ScopeUser, Tool: name, Pattern: "https://blocked.example/*", Effect: Allow}, + }, + want: Ask, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := NewEvaluator(tc.rules).Evaluate(name, args, false); got.Effect != tc.want { + t.Fatalf("Evaluate = %v (%s), want %v", got.Effect, got.Reason, tc.want) + } + }) + } +} diff --git a/user-docs/features/security-and-execution/permissions-and-posture.md b/user-docs/features/security-and-execution/permissions-and-posture.md index ca1467a812..30801b2a9a 100644 --- a/user-docs/features/security-and-execution/permissions-and-posture.md +++ b/user-docs/features/security-and-execution/permissions-and-posture.md @@ -167,11 +167,27 @@ patterns use shell-style glob matching. The `permissions:` section parses strictly, so a misspelled key causes the file to be skipped rather than silently changing the policy. -A pattern matches the argument value that the tool runs with. Mecatl rejects a -tool call whose arguments repeat a key, or spell an argument in a different -letter case, before any rule, hook, or approval request sees the call. The model -receives an error and can resend the call with each argument spelled as in the -tool schema. +A pattern matches the argument value that the tool runs with: + +| Tool | Argument the pattern matches | +|-|-| +| `Shell` | `command` | +| `Read`, `Write`, `Edit`, `Remove`, `ListDir`, `Grep` | `path` | +| `Glob` | `pattern` | +| `WebFetch` | `url` | +| `WebSearch` | `query` | +| `Copy`, `Move` | `source` and `destination`, each evaluated separately; the stricter result applies | + +A call without that argument, such as a `Grep` call without a `path`, matches +only rules without a pattern. For MCP and other tools, a pattern matches the +first of `path`, `file_path`, `pattern`, `url`, and `query` that the call +carries. A `deny` or `ask` pattern that matches any of the others also applies +to the call. + +Mecatl rejects a tool call whose arguments repeat a key, or spell an argument in +a different letter case, before any rule, hook, or approval request sees the +call. The model receives an error and can resend the call with each argument +spelled as in the tool schema. The configuration scopes, from highest to lowest precedence, are: From 3bd14136731020298005d4da01574dc75ded49be Mon Sep 17 00:00:00 2001 From: Giuseppe Scuglia Date: Thu, 15 Oct 2026 15:54:45 +0200 Subject: [PATCH 5/8] fix(engine): check PresentPlan arguments before surfacing the plan approval PresentPlan is intercepted before permissionDecision, so a plan with a repeated or case-variant key reached the approval surface. surfacePlanAsk, shared by the batched and serial paths, now applies CheckToolArgs first. Co-Authored-By: Claude Opus 5.5 (1M context) --- engine/agent/dispatch.go | 8 ++++ engine/agent/presentplan_ask_test.go | 62 ++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/engine/agent/dispatch.go b/engine/agent/dispatch.go index e8d38c1b11..88c57ea6cb 100644 --- a/engine/agent/dispatch.go +++ b/engine/agent/dispatch.go @@ -1609,6 +1609,14 @@ func (e *Engine) askHookApproval(ctx context.Context, r *Run, sess *session.Sess // askHookApproval relies on its caller's openCard. It emits NO diagnostics line // (the loop's "exactly THREE lines" invariant holds). func (e *Engine) surfacePlanAsk(ctx context.Context, r *Run, sess *session.Session, turnIdx int, c session.ToolCall, resultEvent session.EventType) (session.ToolResult, bool) { + // PresentPlan is intercepted before permissionDecision, so it applies the + // same argument check here: an approver must never see a plan whose fields + // different readers would decode differently. + if err := governance.CheckToolArgs(c.Args); err != nil { + res := denyResult(c, governance.AmbiguousArgsReason(err)) + e.publishToolResult(r, session.Event{Type: resultEvent, Turn: turnIdx, ToolResult: ptr(res)}) + return res, false + } // Headless degrade (mirrors preHook's askable-block degrade): a // non-interactive engine has NO human to approve a plan, so it must NOT surface // an ask (a headless run never emits EvPermissionAsk). Fail safe: synthesize a diff --git a/engine/agent/presentplan_ask_test.go b/engine/agent/presentplan_ask_test.go index c01fa318d2..84bcb8ea91 100644 --- a/engine/agent/presentplan_ask_test.go +++ b/engine/agent/presentplan_ask_test.go @@ -540,3 +540,65 @@ func TestPresentPlanHeadlessNoAsk(t *testing.T) { t.Fatal("a headless plan run must NOT terminate with StopPlanApproved (no approval, no flip)") } } + +// TestPresentPlanAmbiguousArgsAreRejectedBeforeApproval pins that PresentPlan, +// which is intercepted before the permission gate, applies the same argument +// check: a plan whose argument object repeats a key or spells one in another +// case is refused without surfacing an approval, alone (the serial path), in a +// read batch, on the serial path, and on a headless engine. +func TestPresentPlanAmbiguousArgsAreRejectedBeforeApproval(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + interactive bool + serial bool + siblings []session.ToolCall + }{ + {name: "alone", interactive: true}, + {name: "in a read batch", interactive: true, siblings: []session.ToolCall{toolCall("c2", "PresentPlan", `{"plan":"canonical"}`)}}, + {name: "serial path", interactive: true, serial: true}, + {name: "headless", interactive: false}, + } { + for _, args := range []string{ + `{"plan":"shown","plan":"recorded"}`, + `{"plan":"shown","plan":"recorded"}`, + `{"plan":"shown","Plan":"recorded"}`, + } { + t.Run(tc.name+"/"+args, func(t *testing.T) { + t.Parallel() + calls := append([]session.ToolCall{toolCall("c1", "PresentPlan", args)}, tc.siblings...) + llm := mockllm.New(mockllm.ToolCallTurn(calls...), mockllm.TextTurn("done")) + cat := planCatalog(t) + if tc.serial { + // A PresentPlan that is not read-only flushes alone through + // the serial dispatch path rather than a read batch. + cat = catalogWith(t, serialPresentPlan{agent.NewPresentPlanTool()}) + } + e := newEngine(agent.Deps{LLM: llm, Catalog: cat, Interactive: tc.interactive}) + r := e.Run(context.Background(), newPlanSession(t), agent.MemEnv("/ws"), agent.RunRequest{Text: "plan"}) + var rejected bool + for ev := range r.Events() { + if ev.Type == session.EvPermissionAsk && ev.Ask != nil { + if ev.Ask.Call == "c1" { + t.Errorf("ambiguous plan surfaced an approval: %s", ev.Ask.Args) + } + r.Approve(ev.Ask.AskID, session.VerdictDeny) + } + if ev.ToolResult != nil && ev.ToolResult.CallID == "c1" && ev.ToolResult.IsError && + strings.Contains(ev.ToolResult.Content, "ambiguous tool arguments") { + rejected = true + } + } + if !rejected { + t.Fatal("the ambiguous PresentPlan call was not rejected with an ambiguous-arguments error") + } + }) + } + } +} + +// serialPresentPlan is a PresentPlan that is not read-only, so dispatch routes it +// through the serial path instead of a read batch. +type serialPresentPlan struct{ tool.Tool } + +func (serialPresentPlan) ReadOnly() bool { return false } From ed8f2cde8dc9d77fefb3f29740ed0b3ddb4c6ea6 Mon Sep 17 00:00:00 2001 From: Giuseppe Scuglia Date: Thu, 15 Oct 2026 15:54:45 +0200 Subject: [PATCH 6/8] fix(engine): accept keys that exactly name an untagged argument field The tagged-field spelling check refused {"Limit":7} for a struct with an untagged Limit field and a field tagged "limit", although encoding/json decodes it exactly. Field collection now follows encoding/json's field dominance and accepts an exact match to any effective field. Co-Authored-By: Claude Opus 5.5 (1M context) --- engine/governance/args.go | 82 +++++++++++++++++++++++------ engine/governance/args_scan_test.go | 7 ++- engine/governance/args_test.go | 57 ++++++++++++++++++++ 3 files changed, 130 insertions(+), 16 deletions(-) diff --git a/engine/governance/args.go b/engine/governance/args.go index 994c4e1345..1014c2d68e 100644 --- a/engine/governance/args.go +++ b/engine/governance/args.go @@ -307,20 +307,25 @@ func quoteArgName(name string) string { return fmt.Sprintf("%q", name) } -// taggedFields holds a struct's explicitly tagged JSON field names. +// taggedFields holds the JSON field names encoding/json decodes into a struct: +// the explicitly tagged names, which a key must spell exactly, and the names of +// untagged fields, which a key may also spell in another case. type taggedFields struct { - names [][]byte + names [][]byte + untagged [][]byte } // checkSpelling rejects a key that names one of the tagged fields only under -// case folding. Untagged fields keep encoding/json's case-insensitive matching, +// case folding. A key that exactly names any field, tagged or untagged, is +// accepted, since encoding/json decodes an exact match before trying case +// folding. Untagged fields keep encoding/json's case-insensitive matching, // since their Go name is not a wire spelling. A nil f checks nothing. func (f *taggedFields) checkSpelling(keys [][]byte) error { if f == nil { return nil } for _, key := range keys { - if f.has(key) { + if hasArgName(f.names, key) || hasArgName(f.untagged, key) { continue } for _, name := range f.names { @@ -332,9 +337,9 @@ func (f *taggedFields) checkSpelling(keys [][]byte) error { return nil } -// has reports whether key is exactly one of the tagged names. -func (f *taggedFields) has(key []byte) bool { - for _, name := range f.names { +// hasArgName reports whether key is exactly one of names. +func hasArgName(names [][]byte, key []byte) bool { + for _, name := range names { if bytes.Equal(key, name) { return true } @@ -344,9 +349,9 @@ func (f *taggedFields) has(key []byte) bool { var taggedFieldCache sync.Map // reflect.Type -> *taggedFields -// taggedFieldNames returns the explicitly tagged JSON field names of the struct -// t points to, including those promoted from untagged embedded structs, or nil -// when t is not a pointer to a struct. +// taggedFieldNames returns the JSON field names encoding/json decodes into the +// struct t points to, including those promoted from untagged embedded structs, +// or nil when t is not a pointer to a struct. func taggedFieldNames(t reflect.Type) *taggedFields { if t == nil || t.Kind() != reflect.Pointer || t.Elem().Kind() != reflect.Struct { return nil @@ -354,16 +359,32 @@ func taggedFieldNames(t reflect.Type) *taggedFields { if cached, ok := taggedFieldCache.Load(t); ok { return cached.(*taggedFields) } + var candidates []jsonField + collectJSONFields(t.Elem(), &candidates, 0) fields := &taggedFields{} - collectTaggedFields(t.Elem(), fields, 0) + for _, field := range dominantJSONFields(candidates) { + if field.tagged { + fields.names = append(fields.names, []byte(field.name)) + } else { + fields.untagged = append(fields.untagged, []byte(field.name)) + } + } taggedFieldCache.Store(t, fields) return fields } +// jsonField is a struct field encoding/json may decode into: its JSON name, +// whether a tag gave that name, and how deep in embedded structs it sits. +type jsonField struct { + name string + tagged bool + depth int +} + // maxEmbeddingDepth bounds the walk through embedded structs. const maxEmbeddingDepth = 8 -func collectTaggedFields(t reflect.Type, fields *taggedFields, depth int) { +func collectJSONFields(t reflect.Type, fields *[]jsonField, depth int) { if depth > maxEmbeddingDepth { return } @@ -380,13 +401,44 @@ func collectTaggedFields(t reflect.Type, fields *taggedFields, depth int) { embedded = embedded.Elem() } if embedded.Kind() == reflect.Struct { - collectTaggedFields(embedded, fields, depth+1) + collectJSONFields(embedded, fields, depth+1) + continue } + } + if !field.IsExported() { continue } - if !field.IsExported() || name == "" { + if name == "" { + *fields = append(*fields, jsonField{name: field.Name, depth: depth}) continue } - fields.names = append(fields.names, []byte(name)) + *fields = append(*fields, jsonField{name: name, tagged: true, depth: depth}) } } + +// dominantJSONFields applies encoding/json's rule for fields that share a name: +// the shallowest wins, a tagged field wins a tie at that depth, and a tie that +// remains hides the name. +func dominantJSONFields(fields []jsonField) []jsonField { + var out []jsonField + for i, field := range fields { + dominant, hidden := true, false + for j, other := range fields { + if i == j || other.name != field.name { + continue + } + switch { + case other.depth < field.depth: + dominant = false + case other.depth == field.depth && other.tagged && !field.tagged: + dominant = false + case other.depth == field.depth && other.tagged == field.tagged: + hidden = true + } + } + if dominant && !hidden { + out = append(out, field) + } + } + return out +} diff --git a/engine/governance/args_scan_test.go b/engine/governance/args_scan_test.go index b2b115fed1..53977d47d9 100644 --- a/engine/governance/args_scan_test.go +++ b/engine/governance/args_scan_test.go @@ -165,7 +165,12 @@ func referenceCheckFieldSpelling(keys []string, dst any) error { } continue } - if !field.IsExported() || name == "" { + if !field.IsExported() { + continue + } + if name == "" { + // An exact key for an untagged field is not a case variant. + exact[field.Name] = struct{}{} continue } exact[name] = struct{}{} diff --git a/engine/governance/args_test.go b/engine/governance/args_test.go index 1e5e1ca095..e57b1084d5 100644 --- a/engine/governance/args_test.go +++ b/engine/governance/args_test.go @@ -3,6 +3,7 @@ package governance import ( "encoding/json" "errors" + "reflect" "strings" "testing" ) @@ -131,6 +132,62 @@ func TestDecodeToolArgsRequiresExactFieldSpelling(t *testing.T) { } } +// TestDecodeToolArgsAcceptsExactUntaggedFields pins that a key exactly naming an +// untagged field is not mistaken for a case variant of a tagged one, following +// encoding/json's field dominance: an untagged field hidden by a tie at the +// same embedding depth is not a match, so its name stays a case variant of the +// tagged field encoding/json would decode it into. +func TestDecodeToolArgsAcceptsExactUntaggedFields(t *testing.T) { + t.Parallel() + type collision struct { + Lower int `json:"limit"` + Limit int + } + type promoted struct{ Limit int } + type promotedCollision struct { + promoted + Lower int `json:"limit"` + } + type left struct{ Name string } + type right struct{ Name string } + type hidden struct { + left + right + Tagged string `json:"name"` + } + for _, tc := range []struct { + name string + args string + dst func() any + ok bool + want any + }{ + {name: "untagged exact", args: `{"Limit":7}`, dst: func() any { return new(collision) }, ok: true, want: &collision{Limit: 7}}, + {name: "tagged exact", args: `{"limit":7}`, dst: func() any { return new(collision) }, ok: true, want: &collision{Lower: 7}}, + {name: "fold variant of both", args: `{"LIMIT":7}`, dst: func() any { return new(collision) }}, + {name: "promoted untagged exact", args: `{"Limit":7}`, dst: func() any { return new(promotedCollision) }, ok: true, want: &promotedCollision{promoted: promoted{Limit: 7}}}, + {name: "hidden untagged", args: `{"Name":"x"}`, dst: func() any { return new(hidden) }}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := tc.dst() + err := DecodeToolArgs(json.RawMessage(tc.args), got) + if !tc.ok { + if !errors.Is(err, ErrAmbiguousToolArgs) { + t.Fatalf("DecodeToolArgs(%s) = %v, want an ambiguous-arguments error", tc.args, err) + } + return + } + if err != nil { + t.Fatalf("DecodeToolArgs(%s) = %v", tc.args, err) + } + if !reflect.DeepEqual(got, tc.want) { + t.Fatalf("DecodeToolArgs(%s) decoded %+v, want %+v", tc.args, got, tc.want) + } + }) + } +} + // TestEvaluatorDeniesAmbiguousArgs pins the evaluator's own check: an ambiguous // argument object is denied before any rule, plan mode or learned allow, and is // never learnable. From 1a2fa81e9bd3fdb03e5194e898d67e4eeb2ca4cd Mon Sep 17 00:00:00 2001 From: Giuseppe Scuglia Date: Thu, 15 Oct 2026 15:54:45 +0200 Subject: [PATCH 7/8] test(engine): pin which ambiguous Shell arguments reach policy evaluation A case variant of an option no rule reads, such as Timeout_MS, passes the generic check and is refused by the Shell decode instead. Record policy evaluation so the test states the real ordering. Co-Authored-By: Claude Opus 5.5 (1M context) --- engine/agent/temp_scope_permission_test.go | 28 ++++++++++++++-------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/engine/agent/temp_scope_permission_test.go b/engine/agent/temp_scope_permission_test.go index a986b5c5b4..c5c89bf092 100644 --- a/engine/agent/temp_scope_permission_test.go +++ b/engine/agent/temp_scope_permission_test.go @@ -199,25 +199,33 @@ func TestSystemTempApprovalShowsFullCommand(t *testing.T) { // TestSystemTempAmbiguousArgsAreRejectedBeforeApproval pins that a system-scope // Shell call whose argument object repeats a key or spells a field in another -// case is refused before policy evaluation: no approval is surfaced and nothing -// reaches the runner. +// case never surfaces an approval or reaches the runner. An ambiguity in an +// argument the policy reads is refused before policy evaluation. A case variant +// of an option no rule reads, such as Timeout_MS, passes that generic check and +// is evaluated as an ordinary Shell call; the Shell's own decode then refuses +// it, so the system-scope approval is never surfaced. func TestSystemTempAmbiguousArgsAreRejectedBeforeApproval(t *testing.T) { t.Parallel() - for _, tc := range []struct{ name, args string }{ + for _, tc := range []struct { + name string + args string + reachesPolicy bool + }{ {name: "duplicate key", args: `{"command":"git status","temp_scope":"system","command":"git status && echo done"}`}, {name: "case-variant keys", args: `{"command":"git status","COMMAND":"git status && echo done","Temp_Scope":"system","Timeout_MS":7}`}, {name: "case-variant scope", args: `{"command":"git status","TEMP_SCOPE":"system"}`}, - {name: "case-variant option", args: `{"command":"git status","temp_scope":"system","Timeout_MS":7}`}, + {name: "case-variant option", args: `{"command":"git status","temp_scope":"system","Timeout_MS":7}`, reachesPolicy: true}, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() runner := &fakeShellRunner{res: tool.CommandResult{Stdout: "ok"}} - call := session.NewToolCall("c", tool.ShellToolName, []byte(tc.args)) - events, result := runScopedShell(t, []governance.Rule{{Tool: tool.ShellToolName, Effect: governance.Allow}}, call, runner) - for _, event := range events { - if event.Type == session.EvPermissionAsk { - t.Fatalf("ambiguous arguments surfaced an approval: %s", event.Ask.Args) - } + catalog := tool.NewCatalog() + catalog.MustRegister(NewShellTool()) + policy := newPolicyArgsRecorder() + // runPolicyArgsCall fails the test on any approval request. + result := runPolicyArgsCall(t, catalog, policy, shellEnvRunner(runner), session.NewToolCall("c", tool.ShellToolName, []byte(tc.args))) + if evaluated := len(policy.observations()) > 0; evaluated != tc.reachesPolicy { + t.Errorf("policy evaluated the call = %v, want %v", evaluated, tc.reachesPolicy) } if runner.command != "" { t.Fatalf("ambiguous arguments executed %q", runner.command) From 40cb393d0c94532430c0747c2b7c51800c4971d0 Mon Sep 17 00:00:00 2001 From: Giuseppe Scuglia Date: Thu, 15 Oct 2026 16:34:03 +0200 Subject: [PATCH 8/8] test(engine): pin the argument check on every re-authorization path Cover the paths that authorize a call after its first evaluation: a PreToolUse rewrite on the serial path and in a read batch, an approval resumed from a paused session (an ambiguous pending call, and a hook rewrite on resume), and a read batch mixing ambiguous and canonical calls. Ambiguous arguments reach neither the policy nor the tool; the canonical controls are evaluated and run. Co-Authored-By: Claude Opus 5.5 (1M context) --- engine/agent/policy_args_decode_test.go | 9 +- engine/agent/policy_args_lifecycle_test.go | 234 +++++++++++++++++++++ 2 files changed, 239 insertions(+), 4 deletions(-) create mode 100644 engine/agent/policy_args_lifecycle_test.go diff --git a/engine/agent/policy_args_decode_test.go b/engine/agent/policy_args_decode_test.go index 5f2ed1680e..641a6226ed 100644 --- a/engine/agent/policy_args_decode_test.go +++ b/engine/agent/policy_args_decode_test.go @@ -21,9 +21,9 @@ import ( ) // policyArgsRecorder is an allow-all PermissionPolicy that records, for every -// evaluated call, the Shell command and the resource pattern the governance -// package derives from the call's arguments: the values a configured rule would -// be matched against. +// evaluated call, its raw arguments plus the Shell command and the resource +// pattern the governance package derives from them: the values a configured +// rule would be matched against. type policyArgsRecorder struct { inner port.PermissionPolicy @@ -33,6 +33,7 @@ type policyArgsRecorder struct { type policyArgsObservation struct { tool string + args string command string pattern string } @@ -45,7 +46,7 @@ func (p *policyArgsRecorder) Evaluate(ctx context.Context, id session.SessionID, command, _ := governance.ShellCommandFromArgs(c.Args) rule, _ := governance.NewEvaluator(nil).LearnableRule(c.Name, c.Args) p.mu.Lock() - p.evaluated = append(p.evaluated, policyArgsObservation{tool: c.Name, command: command, pattern: rule.Pattern}) + p.evaluated = append(p.evaluated, policyArgsObservation{tool: c.Name, args: string(c.Args), command: command, pattern: rule.Pattern}) p.mu.Unlock() return p.inner.Evaluate(ctx, id, mode, c, ws) } diff --git a/engine/agent/policy_args_lifecycle_test.go b/engine/agent/policy_args_lifecycle_test.go new file mode 100644 index 0000000000..7c4d36221c --- /dev/null +++ b/engine/agent/policy_args_lifecycle_test.go @@ -0,0 +1,234 @@ +package agent + +import ( + "context" + "encoding/json" + "strings" + "testing" + "time" + + "github.com/stacklok/mecatl/engine/adapter/fstools" + "github.com/stacklok/mecatl/engine/adapter/memfs" + "github.com/stacklok/mecatl/engine/adapter/mockllm" + "github.com/stacklok/mecatl/engine/governance" + "github.com/stacklok/mecatl/engine/session" + "github.com/stacklok/mecatl/engine/tool" +) + +// The tests in this file pin the argument check on each path that authorizes a +// call after its first evaluation: a PreToolUse rewrite, an approval resumed +// from a paused session, and a read batch that mixes ambiguous and canonical +// calls. Each asserts that ambiguous arguments reach neither the permission +// policy nor the tool, with a canonical positive control. + +// argsRewriteHook is a PreToolUse hook that replaces every call's arguments. +type argsRewriteHook struct{ mutated string } + +func (h argsRewriteHook) Run(_ context.Context, ev governance.HookEvent) (governance.HookOutcome, error) { + if ev.Phase == governance.PhasePreToolUse { + return governance.HookOutcome{Mutated: json.RawMessage(h.mutated)}, nil + } + return governance.HookOutcome{}, nil +} + +// policyArgsWorkspace returns a workspace holding a.txt ("alpha") and b.txt ("beta"). +func policyArgsWorkspace(t *testing.T) tool.Workspace { + t.Helper() + ws := memfs.NewWorkspace("/ws") + for path, body := range map[string]string{"a.txt": "alpha", "b.txt": "beta"} { + if err := ws.Write(t.Context(), path, []byte(body)); err != nil { + t.Fatal(err) + } + } + return ws +} + +// assertNotEvaluated fails when the policy evaluated a call with args. +func assertNotEvaluated(t *testing.T, policy *policyArgsRecorder, args string) { + t.Helper() + for _, o := range policy.observations() { + if o.args == args { + t.Errorf("ambiguous arguments reached policy evaluation: %s", args) + } + } +} + +func assertAmbiguousResult(t *testing.T, result session.ToolResult) { + t.Helper() + if !result.IsError || !strings.Contains(result.Content, "ambiguous tool arguments") { + t.Errorf("result = %+v, want an ambiguous-arguments tool error", result) + } +} + +// TestHookMutatedAmbiguousArgsAreRejected pins that a PreToolUse rewrite into an +// ambiguous argument object is refused before the effective call is evaluated +// or executed, on the serial path (Shell) and in a read batch (Read). +func TestHookMutatedAmbiguousArgsAreRejected(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + tool tool.Tool + original string + mutated string + ambiguous bool + // want is what the canonical rewrite runs: the Shell command or the + // content Read returns. + want string + }{ + {name: "shell repeated key", tool: NewShellTool(), original: `{"command":"echo original"}`, mutated: `{"command":"echo shown","command":"echo hidden"}`, ambiguous: true}, + {name: "shell case variant", tool: NewShellTool(), original: `{"command":"echo original"}`, mutated: `{"command":"echo shown","COMMAND":"echo hidden"}`, ambiguous: true}, + {name: "shell canonical rewrite", tool: NewShellTool(), original: `{"command":"echo original"}`, mutated: `{"command":"echo rewritten"}`, want: "echo rewritten"}, + {name: "read repeated key", tool: fstools.ReadTool{}, original: `{"path":"a.txt"}`, mutated: `{"path":"a.txt","path":"b.txt"}`, ambiguous: true}, + {name: "read case variant", tool: fstools.ReadTool{}, original: `{"path":"a.txt"}`, mutated: `{"path":"a.txt","PATH":"b.txt"}`, ambiguous: true}, + {name: "read canonical rewrite", tool: fstools.ReadTool{}, original: `{"path":"a.txt"}`, mutated: `{"path":"b.txt"}`, want: "beta"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + runner := &fakeShellRunner{res: tool.CommandResult{Stdout: "ok"}} + catalog := tool.NewCatalog() + catalog.MustRegister(tc.tool) + policy := newPolicyArgsRecorder() + call := session.NewToolCall("c", tc.tool.Spec().Name, []byte(tc.original)) + eng := NewEngine(Deps{LLM: mockllm.New(mockllm.ToolCallTurn(call), mockllm.TextTurn("done")), Catalog: catalog, Policy: policy, Hooks: argsRewriteHook{mutated: tc.mutated}}) + env := testEnvironment(policyArgsWorkspace(t), runner) + sess := session.New("hook-mutation", session.ModeDefault, env.Ref(), session.Limits{MaxTurns: 3}, time.Now()) + var result session.ToolResult + for event := range eng.Run(context.Background(), sess, env, RunRequest{Text: "run"}).Events() { + if event.Type == session.EvPermissionAsk { + t.Fatalf("unexpected permission ask: %+v", event.Ask) + } + if event.Type == session.EvToolResult && event.ToolResult != nil && event.ToolResult.CallID == call.ID { + result = *event.ToolResult + } + } + runner.mu.Lock() + executed := runner.command + runner.mu.Unlock() + + if tc.ambiguous { + assertNotEvaluated(t, policy, tc.mutated) + assertAmbiguousResult(t, result) + if executed != "" || strings.Contains(result.Content, "alpha") || strings.Contains(result.Content, "beta") { + t.Fatalf("ambiguous rewrite executed: command=%q result=%q", executed, result.Content) + } + return + } + var evaluated bool + for _, o := range policy.observations() { + evaluated = evaluated || o.args == tc.mutated + } + if !evaluated { + t.Errorf("the canonical rewrite %s was not evaluated", tc.mutated) + } + if result.IsError || (executed != tc.want && !strings.Contains(result.Content, tc.want)) { + t.Fatalf("canonical rewrite: command=%q result=%+v, want %q", executed, result, tc.want) + } + }) + } +} + +// TestResumedApprovalWithAmbiguousArgsIsRejected pins that approving a paused +// permission ask re-checks the arguments: a pending call with an ambiguous +// argument object, as an earlier version could have persisted, and a canonical +// pending call that a PreToolUse hook rewrites into an ambiguous one are both +// refused after the approval without running. +func TestResumedApprovalWithAmbiguousArgsIsRejected(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + pending string + mutated string + ambiguous string + }{ + {name: "ambiguous pending call", pending: `{"command":"echo shown","command":"echo hidden"}`, ambiguous: `{"command":"echo shown","command":"echo hidden"}`}, + {name: "hook rewrite on resume", pending: `{"command":"echo approved"}`, mutated: `{"command":"echo approved","Command":"echo hidden"}`, ambiguous: `{"command":"echo approved","Command":"echo hidden"}`}, + {name: "canonical pending call", pending: `{"command":"echo approved"}`}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + call := session.NewToolCall("pending", tool.ShellToolName, []byte(tc.pending)) + sess := session.New("resume", session.ModeDefault, session.EnvironmentRef{Kind: session.EnvKindMem, ID: "/ws", Revision: "v1"}, session.Limits{MaxTurns: 3}, time.Now()) + if err := sess.BeginTurn(); err != nil { + t.Fatal(err) + } + if err := sess.RecordAssistant(session.NewAssistantMessage("", "", []session.ToolCall{call})); err != nil { + t.Fatal(err) + } + const askID = "resume:0:pending:r1" + if err := sess.PauseForApproval(session.PendingAsk{AskID: askID, Call: call.ID, Tool: call.Name, Args: call.Args, Origin: session.ApprovalOriginPermission}); err != nil { + t.Fatal(err) + } + runner := &fakeShellRunner{res: tool.CommandResult{Stdout: "ok"}} + catalog := tool.NewCatalog() + catalog.MustRegister(NewShellTool()) + policy := newPolicyArgsRecorder() + deps := Deps{LLM: mockllm.New(mockllm.TextTurn("done")), Catalog: catalog, Policy: policy} + if tc.mutated != "" { + deps.Hooks = argsRewriteHook{mutated: tc.mutated} + } + var result session.ToolResult + for event := range NewEngine(deps).ResumeApproval(context.Background(), sess, shellEnvRunner(runner), askID, session.VerdictAllowOnce).Events() { + if event.Type == session.EvToolResult && event.ToolResult != nil && event.ToolResult.CallID == call.ID { + result = *event.ToolResult + } + } + runner.mu.Lock() + executed := runner.command + runner.mu.Unlock() + + if tc.ambiguous != "" { + assertNotEvaluated(t, policy, tc.ambiguous) + assertAmbiguousResult(t, result) + if executed != "" { + t.Fatalf("approved ambiguous call executed %q", executed) + } + return + } + if result.IsError || executed != "echo approved" { + t.Fatalf("approved canonical call: command=%q result=%+v", executed, result) + } + }) + } +} + +// TestReadBatchRejectsOnlyTheAmbiguousCall pins that in a read batch mixing +// canonical and ambiguous calls, only the ambiguous call is refused: it is never +// evaluated or read, and its canonical siblings are evaluated and run. +func TestReadBatchRejectsOnlyTheAmbiguousCall(t *testing.T) { + t.Parallel() + const ambiguous = `{"path":"a.txt","Path":"b.txt"}` + calls := []session.ToolCall{ + session.NewToolCall("first", "Read", []byte(`{"path":"a.txt"}`)), + session.NewToolCall("ambiguous", "Read", []byte(ambiguous)), + session.NewToolCall("last", "Read", []byte(`{"path":"b.txt"}`)), + } + catalog := tool.NewCatalog() + catalog.MustRegister(fstools.ReadTool{}) + policy := newPolicyArgsRecorder() + eng := NewEngine(Deps{LLM: mockllm.New(mockllm.ToolCallTurn(calls...), mockllm.TextTurn("done")), Catalog: catalog, Policy: policy}) + env := testEnvironment(policyArgsWorkspace(t), nil) + sess := session.New("read-batch", session.ModeDefault, env.Ref(), session.Limits{MaxTurns: 3}, time.Now()) + results := map[session.ToolCallID]session.ToolResult{} + for event := range eng.Run(context.Background(), sess, env, RunRequest{Text: "run"}).Events() { + if event.Type == session.EvPermissionAsk { + t.Fatalf("unexpected permission ask: %+v", event.Ask) + } + if event.Type == session.EvToolResult && event.ToolResult != nil { + results[event.ToolResult.CallID] = *event.ToolResult + } + } + + assertNotEvaluated(t, policy, ambiguous) + assertAmbiguousResult(t, results["ambiguous"]) + if got := results["ambiguous"].Content; strings.Contains(got, "alpha") || strings.Contains(got, "beta") { + t.Fatalf("ambiguous call read a file: %q", got) + } + if len(policy.observations()) != 2 { + t.Errorf("policy evaluated %d calls, want the 2 canonical ones: %+v", len(policy.observations()), policy.observations()) + } + for id, want := range map[session.ToolCallID]string{"first": "alpha", "last": "beta"} { + if res := results[id]; res.IsError || !strings.Contains(res.Content, want) { + t.Errorf("canonical sibling %s = %+v, want content %q", id, res, want) + } + } +}