diff --git a/engine/CHANGELOG.md b/engine/CHANGELOG.md index 2e8fd643b5..b11e17e3eb 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,35 @@ 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. 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)` 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/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/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..88c57ea6cb 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 { @@ -1599,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 @@ -1866,7 +1884,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 +1892,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..641a6226ed --- /dev/null +++ b/engine/agent/policy_args_decode_test.go @@ -0,0 +1,452 @@ +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/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" + "github.com/stacklok/mecatl/engine/tool" +) + +// policyArgsRecorder is an allow-all PermissionPolicy that records, for every +// 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 + + mu sync.Mutex + evaluated []policyArgsObservation +} + +type policyArgsObservation struct { + tool string + args 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, args: string(c.Args), 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 +} + +// 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/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) + } + } +} 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 } 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..c5c89bf092 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,53 @@ 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 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 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}`, reachesPolicy: 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() + // 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) + } + 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 = "<< 0 { + keys = keys[:closed.start] + } + 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 + key, err := decodeJSONKey(doc[i : end+1]) + if err != nil { + return nil, err + } + 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 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 +// whose opening quote is at doc[start]. +func endOfJSONString(doc []byte, start int) int { + for i := start + 1; i < len(doc); i++ { + switch doc[i] { + case '\\': + i++ + case '"': + return i + } + } + return len(doc) - 1 +} + +// decodeJSONKey returns a quoted JSON key as encoding/json decodes it, resolving +// 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 inner, nil + } + var key string + if err := json.Unmarshal(quoted, &key); err != nil { + 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 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 +// names fold to the same string exactly when strings.EqualFold reports them +// equal, which is the key matching encoding/json applies to struct fields. +func foldArgName(name string) string { + var b strings.Builder + b.Grow(len(name)) + for _, r := range name { + b.WriteRune(minFoldRune(r)) + } + return b.String() +} + +// minFoldRune returns the smallest rune in r's simple case-folding orbit. For an +// ASCII letter that is its upper case, so U+212A KELVIN SIGN and 'k' both map +// to 'K'. +func minFoldRune(r rune) rune { + if r < utf8.RuneSelf { + if 'a' <= r && r <= 'z' { + return r - ('a' - 'A') + } + return r + } + least := r + for next := unicode.SimpleFold(r); next != r; next = unicode.SimpleFold(next) { + if next < least { + least = next + } + } + return least +} + +func quoteArgName(name string) string { + if utf8.RuneCountInString(name) > maxArgNameInError { + runes := []rune(name) + name = string(runes[:maxArgNameInError]) + "…" + } + return fmt.Sprintf("%q", name) +} + +// 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 + untagged [][]byte +} + +// checkSpelling rejects a key that names one of the tagged fields only under +// 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 hasArgName(f.names, key) || hasArgName(f.untagged, key) { + continue + } + 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 +} + +// 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 + } + } + return false +} + +var taggedFieldCache sync.Map // reflect.Type -> *taggedFields + +// 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 + } + if cached, ok := taggedFieldCache.Load(t); ok { + return cached.(*taggedFields) + } + var candidates []jsonField + collectJSONFields(t.Elem(), &candidates, 0) + fields := &taggedFields{} + 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 collectJSONFields(t reflect.Type, fields *[]jsonField, 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 { + collectJSONFields(embedded, fields, depth+1) + continue + } + } + if !field.IsExported() { + continue + } + if name == "" { + *fields = append(*fields, jsonField{name: field.Name, depth: depth}) + continue + } + *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 new file mode 100644 index 0000000000..53977d47d9 --- /dev/null +++ b/engine/governance/args_scan_test.go @@ -0,0 +1,428 @@ +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() { + continue + } + if name == "" { + // An exact key for an untagged field is not a case variant. + exact[field.Name] = struct{}{} + 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/args_test.go b/engine/governance/args_test.go new file mode 100644 index 0000000000..e57b1084d5 --- /dev/null +++ b/engine/governance/args_test.go @@ -0,0 +1,306 @@ +package governance + +import ( + "encoding/json" + "errors" + "reflect" + "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","\u0063ommand":"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}, + {`{"\u0063ommand":"ls"}`, false}, + {`{"command":"ls","cmd":"pwd"}`, false}, + {`{"filePath":"a"}`, false}, + {`{"meta":{"x":1,"X":2}}`, false}, + {`{"command":"ls","command":"pwd"}`, true}, + {`{"command":"ls","\u0063ommand":"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) + } + } +} + +// 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. +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..cdc4299701 100644 --- a/engine/governance/evaluator.go +++ b/engine/governance/evaluator.go @@ -2,6 +2,7 @@ package governance import ( "encoding/json" + "slices" "strings" ) @@ -118,6 +119,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 +136,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 @@ -173,9 +195,12 @@ 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 { + return "", false + } if tool == "Shell" { cmd, _ := ShellCommandFromArgs(args) if cmd == "" { @@ -234,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. @@ -556,9 +598,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 +610,153 @@ 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"` +} + +// 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 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 m map[string]json.RawMessage - if err := json.Unmarshal(args, &m); err != nil { - return "" + var fields resourceArgs + if err := DecodeToolArgs(args, &fields); err != nil { + return "", nil } - 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 - } + 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 +// 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/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 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/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..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,6 +167,28 @@ 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: + +| 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: 1. managed policy;