fix(engine): evaluate tool permissions on the same argument decode the tool executes

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

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}`, "", ""},
 		{`{}`, "", ""},

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

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

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)

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,15 +1884,15 @@ 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
 	case "ListMcpResources", "ReadMcpResource":
 		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