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

[peppescg]

Summary

Permission evaluation and tool execution now share one decode of a tool call's arguments, and argument objects that do not decode to a single unambiguous value are rejected before policy evaluation.

Targets main. #2263 has merged, so this branch is rebased onto main and the PR targets main directly.

Design

• governance.DecodeToolArgs is the single decode. Its callers are:

  • session.ParseArgs, which every built-in tool, parseShellArgs, the system-scope approval projection, and tool.LocalFileOperands already use;
  • the policy extractors (ShellCommandFromArgs, the single-resource pattern, the Copy/Move operands).

  The function first applies governance.CheckToolArgs. It then refuses a key that names a tagged field of the destination only in another case. Finally it decodes with encoding/json.

• governance.CheckToolArgs refuses three kinds of argument object:

  • an object that repeats a key at any depth, including a repeat written with JSON escapes;
  • two top-level keys that are equal under Unicode case folding;
  • a top-level key that spells a policy-read argument in another case. The policy-read arguments are command, cmd, temp_scope, path, file_path, pattern, url, query, source, destination, server and tool.

  Empty, non-object and invalid JSON payloads pass unchanged, because no reader extracts a value from them.

  After this check, encoding/json's case-insensitive matching and its last-duplicate-wins rule can no longer pick a value. Every reader that accepts the payload sees the same fields: Go structs, exact-key maps, hooks, and MCP servers.

• Where rejection happens.

  • Dispatch pre-check. Engine.permissionDecision is the single entry to policy evaluation. It covers fresh calls, read batches, hook-mutated effective calls, and approvals that resume after a pause. It runs CheckToolArgs before Policy.Evaluate. This is the primary gate: nothing downstream sees the call, including the policy, guardrails, posture wrappers, approval surfaces, child auto-approval, the headless reviewer, PreToolUse hooks, and the tool itself.
  • Inside the evaluator. Evaluator.EvaluateWith denies the same shapes for policy wrappers and direct callers, and LearnableRule refuses them.
  • In session.ParseArgs. This layer protects execution. It also catches lone case variants of any tagged field, such as Timeout_MS, that the generic pre-check does not know.

• The rejection is a permission denied: ambiguous tool arguments: …; send each argument once, spelled exactly as in the tool schema tool error. No ask is surfaced and nothing executes.

Review follow-up

This revision addresses the panel review:

• Policy now matches the argument each built-in tool executes. The single-resource extractor took the first of path, file_path, pattern, url and query whatever the tool, so an allowed decoy in an ignored field chose the matched value. Built-in tools now match their own operand: path for Read, Write, Edit, Remove, ListDir and Grep, pattern for Glob, url for WebFetch, and query for WebSearch. A Grep call without path matches only tool-wide rules. MCP and custom tools keep the generic precedence, because governance does not know their argument contract, and a pattern Deny or Ask on any other candidate argument also applies.
• PresentPlan applies the argument check. surfacePlanAsk, shared by the batched and serial dispatch paths, runs CheckToolArgs before surfacing the plan approval or the headless refusal.
• Exact keys for untagged fields are accepted. Field collection follows encoding/json dominance, so {"Limit":7} decodes into an untagged Limit next to a field tagged limit. A name that encoding/json hides is still treated as a case variant.
• The Timeout_MS test states the real ordering. A case variant of an option no rule reads passes the generic check and reaches policy as an ordinary Shell call. The Shell decode then refuses it, before any system-scope approval. The test records policy evaluation to pin this.

Before | After

Rules: Allow on the decoy value, Deny on the executed value. "Before" is the previous head e2fe87517.

Call	Before	After
WebFetch {"path":"https://allowed.example/","url":"https://blocked.example/"}	allow, fetches blocked.example	deny
Glob {"path":"src/**","pattern":"secrets/**"}	allow, lists secrets/**	deny
WebSearch {"pattern":"go docs","query":"internal secrets"}	allow, searches internal secrets	deny
PresentPlan {"plan":"shown","plan":"recorded"} in plan mode	plan approval surfaced	permission denied: ambiguous tool arguments
DecodeToolArgs({"Limit":7}), untagged Limit plus a field tagged limit	"Limit" must be spelled "limit"	decodes Limit: 7

Policy paths and tools audited

Reader	Status
Shell: Allow/Ask/Deny rules, prefix and glob matching, substitution-aware splitting, cmd alias, plan mode, learned rules (ShellCommandFromArgs)	Uses the shared decode, after the pre-check
Shell system temp-scope evaluation and approval projection (shellSystemScope, systemScopeApprovalArgs, from #2263)	parseShellArgs → session.ParseArgs (shared decode)
Child auto-approval, isolation gate, surfaced child preview, headless ask reviewer (askadjudicator, autoApproveChildAsk)	Read the ask args, which exist only after the pre-check; Shell command extraction is shared
Read / Write / Edit / Remove / ListDir / Glob / Grep resource pattern (resourcePatterns)	Uses the shared decode, after the pre-check; matches the argument the tool acts on (path, or pattern for Glob)
Copy / Move operands (copyMovePatterns)	Uses the shared decode, after the pre-check
WebFetch / WebSearch pattern	Matches only url / query, the arguments the tools execute; the tools decode through session.ParseArgs
MCP tools (mcp__*, raw arguments forwarded)	Pre-check applies to every tool; the pattern uses the shared decode, matches the first resource argument, and a pattern Deny or Ask on any other one also applies
PresentPlan plan approval (surfacePlanAsk, intercepted before permissionDecision)	Applies CheckToolArgs before surfacing the approval
CallMcpWithQuery / MCP resource authority target (authorityTarget)	Switched to session.ParseArgs
Bound-authority local resources (authorityPathNamed)	Already exact-key and duplicate-refusing; consistent once the pre-check has passed
Action review target and evidence (reviewTarget, tool.LocalFileOperands)	reviewTarget's WebFetch branch switched to session.ParseArgs
Subagent MutatesParent (dispatch serial or parallel classification)	Switched to session.ParseArgs, the decode Execute uses (Parallel already did)
Out-of-workspace posture classifier (internal/app escape policy)	Switched to governance.DecodeToolArgs
Guardrail Shell matcher (modelhook)	Uses ShellCommandFromArgs (shared)
PreToolUse hooks, permission classifier, contextual reviewer	Receive raw arguments only after the pre-check
ToolSearch (engine and authority-filtered)	Switched to session.ParseArgs
Parked CallMcpWithQuery replay (internal/adapter/server)	Arguments passed the pre-check before parking

Two readers are display-only and stay as they are. The ACP projector shows tool-call locations, and compaction extracts a path for its summary. Neither one decides permission.

Compatibility

• Normal JSON tool calls are unchanged, and the provider adapters, mockllm, mockscript and the conformance suites need no changes. Provider fixtures contain no non-canonical keys.
• Existing tests that pinned the old case-insensitive, last-key-wins reading have moved to the new contract (Align shared file operand extraction with built-in tool argument decoding #2094 operand tests, review-target and contextual-evidence tests, and the ambiguous cases from fix(engine): show the full command in system temp-scope approval asks #2263). Those cases now assert rejection, with no ask and nothing executed. The non-ambiguous fix(engine): show the full command in system temp-scope approval asks #2263 cases (compound command, timeout and background) are kept as they were.
• An MCP tool whose schema declares a property that is a case variant of a policy-read argument is now rejected. One example is a property named URL or Query.
• engine/CHANGELOG.md has an Added entry for the new governance identifiers and a Changed entry for the behavior. The API snapshot is updated.
• The permissions user-docs page now lists the argument each built-in tool's patterns match, and states that ambiguous arguments are rejected.
• A Grep call without path now matches only tool-wide rules, rather than matching its regular expression as if it were a path. A call to a built-in tool that carries file_path instead of the tool's own argument now derives no pattern; the tool refuses that call anyway. The permpolicy accept-edits fixtures sent Edit and Write file_path and now send path.

Tests

• Red on c6d733888, green on this branch:
  • engine/agent/policy_args_decode_test.go. It drives the engine across argument encodings for Shell, the file tools, WebFetch, WebSearch and an MCP-named tool. It asserts that the value policy evaluates equals the value executed, or that ambiguous arguments never reach policy evaluation or execution.
  • Before the fix, 19 of 26 subtests failed. The 7 canonical and non-ambiguous cases passed both before and after.
• New unit tests:
  • engine/governance/args_test.go: classification, exact field spelling, evaluator deny, and refusal to learn.
  • FuzzPolicyExtractionMatchesExecutionDecode. It checks that whenever the execution decode accepts a payload, the policy extraction reads the same command, path and operands. It also checks the key scanner against encoding/json.
  • session.TestParseArgs: ambiguous objects are refused.
• Review follow-up tests, each checked against a mutation of its fix:
  • engine/governance/resource_operand_test.go: a Deny on the executed operand wins over an allowed decoy for WebFetch, WebSearch, Glob, Read and Grep; a decoy-only Allow does not admit the call; learned rules use the executed operand; Grep without path; deny-dominant matching for MCP tools.
  • TestBuiltinToolDenyMatchesTheExecutedOperand (engine/agent): real Glob over memfs, real WebSearch with a recording provider, and real WebFetch. The denied and decoy-only calls never reach the tool, and the canonical call runs.
  • TestPresentPlanAmbiguousArgsAreRejectedBeforeApproval: alone, in a read batch, on the serial path, and headless.
  • TestDecodeToolArgsAcceptsExactUntaggedFields: tagged and untagged collisions, a promoted untagged field, and a name hidden by a dominance tie.
  • engine/agent/policy_args_lifecycle_test.go: a PreToolUse rewrite into ambiguous arguments (serial path and read batch), an approval resumed with an ambiguous pending call or a hook rewrite on resume, and a mixed read batch where only the ambiguous call is refused. Ambiguous arguments reach neither the policy nor the tool, and the canonical controls run.
• Converted tests: fstools/local_operands_test.go, agent/repair2b_test.go, agent/temp_scope_permission_test.go, internal/app/contextual_build_evidence_test.go.

Gates

Each failure listed below fails the same way on a clean c6d733888 checkout on this macOS host. All of them are in packages this change does not touch.

• task lint: 0 issues in every module except environment/microvm, which has one pre-existing finding (gosec G703, launch_ownership_darwin.go:754).
• task test:race: the engine module passes, including the GOWORK=off standalone run, and so do the provider, adapters and authn/oidc modules. In the root module, every package that this change touches passes: internal/app contextual and escape policy, modelhook, permclassify, mcp, server approvals, and cmd/mecatui/ui. Pre-existing failures elsewhere in the root module:
  • internal/app:
    • TestCanonicalConfiguredDir_AllowsMissingDefaultParents
    • TestYoloRelativeEscape* (2 tests)
    • TestSubagentRunsGitInWorktreeEndToEnd
    • TestBuildSubagentToolRealWiringForksChildShellWhenShell
    • TestReadOnlyMemberRunsGitInWorktreeEndToEnd
  • internal/adapter/server: TestACPUsesHostGovernanceRoot… and TestLoadACPSessionOwnsExactBinding…
  • internal/adapter/forker: TestFork* (4 tests)
  • internal/executionexecutor: 5 tests
  • deploy/mecatl-execution-kind: 2 tests
  • cmd/mecated: TestMecatedCLICompositionValidatesMicroVMDefaultAfterReadiness
  • cmd/mecatui: TestConnectUsesSavedServerCAForLazyTLSRPC
  • internal/adapter/microvmmanager: TestDefaultOperationsStartScrubsDaemonEnvironmentAndDetachesFromRequest timed out once under load. It passes with -race -count=3 on this branch.
• task docs:check passes (matlatl strict: 0 broken links, anchors or orphans). sdk:docs was skipped because pnpm is not installed on this host.
• go run ./cmd/mecademo passes and shows the tool call, the permission ask and approval, and the result.
• Fuzzing: FuzzPolicyExtractionMatchesExecutionDecode ran for about 90 seconds with no failures. On the allocation-free scan below, it ran two more times (3 minutes and 2.5 minutes, about 51 million executions) with no failures.
• Allocation gate: CheckToolArgs runs on every dispatched call and every policy evaluation, so its key scan does not allocate on ordinary arguments.

  • Keys stay subslices of the argument bytes, held in stack arrays. Repeated keys and case-fold collisions are found by pairwise comparison (bytes.Equal, bytes.EqualFold), and a key is decoded only when it carries an escape or invalid UTF-8. An object with more than 16 keys switches to map indexes, so large objects stay linear.

  • DecodeToolArgs checks tagged field spellings on the same keys, so it allocates exactly what its encoding/json decode does. The policy extractors allocate fewer times than on main, for example 3 instead of 7 for ShellCommandFromArgs.

  • TestCheckToolArgsDoesNotAllocate pins zero allocations for typical Shell and file-tool arguments. TestDecodeToolArgsAllocatesLikeUnmarshal pins the decode against json.Unmarshal. Both skip under -race, where sync.Pool drops items at random.

  • TestCheckToolArgsMatchesReference and FuzzCheckToolArgsMatchesReference keep the previous map-based scan as an oracle and compare verdicts, error messages, top-level keys and decoded values. They agree on the seeds, edge cases past the stack and map thresholds, 20,000 generated payloads, and the 848-entry fuzz corpus. The fuzz target ran for 3 minutes (7.5 million executions) with no differences.

  • task bench BENCHCOUNT=10, compared with go run ./perf/cmd/allocsgate, reports no regression against a clean c6d733888 checkout. It also reports none against the gh-pages baseline, with benchmark names normalized to the CI runner's -4 suffix. The gated allocs/op against the gh-pages baseline:

    Benchmark	Baseline	This branch
    EvaluatorEvaluate/simple-tool	8	4
    EvaluatorEvaluate/plain-bash	12	8
    EvaluatorEvaluate/compound-bash	23	19
    RunReadOnlyTurn	167	163
    RunReadParallelTurn	217	201
    RunMutatingTurn	158	153
    SteadyStateReadOnlyTurn	156	152
    SteadyStateReadParallelTurn	205	189
    SteadyStateMutatingTurn	147	142

    Every other benchmark is unchanged.

• Review follow-up head, rebased on current main:
  • task lint: 0 issues except the same pre-existing environment/microvm G703 finding.
  • task test:race: the engine module passes. In the root module, the failures are the pre-existing ones listed above, plus cmd/mecatui TestStartupCompatibilityEmbeddedRejected (embedded socket discovery) and a 20-minute timeout in deploy/mecatl-execution-kind. Neither package is touched. TestYoloRelativeEscape* fails the same way on a clean current origin/main.
  • task docs passes. go run ./cmd/mecademo shows the tool call, the permission ask and approval, and the result.
  • BenchmarkEvaluatorEvaluate/simple-tool drops from 4 to 3 allocs/op; the Shell cases are unchanged.
  • Re-run on the previous head: task lint (same result as above), go test -race for engine/governance, engine/session, engine/agent and engine/adapter/fstools, the internal/app escape-policy and contextual-evidence tests, and go run ./cmd/mecademo.

Closes #2264

🤖 Generated with Claude Code

[JAORMX]

Panel review

Reviewed e2fe87517 against 3145b607f across Spec (#2264), Standards, independent Test adequacy, and Domain (security, architecture, library reuse, duplication).

Requesting changes for one concrete blocker: nonShellPattern can still authorize a resource the tool does not execute. Security and architecture independently confirmed this. It is pre-existing, but the rewritten extractor preserves it despite #2264 explicitly requiring policy/execution equality. See the inline example and proposed fix.

Spec

• Blocker: policy selects a generic resource field rather than the tool's actual operand.
• Important: agreement coverage omits Edit, tests Write only for rejection, and uses name-only stubs for network/MCP tools (engine/agent/policy_args_decode_test.go:155–178,246–265).

Standards

• Important: PresentPlan bypasses the argument precheck and can surface duplicate-key arguments for approval, contrary to the updated documentation. See inline comment.

Test adequacy

• Important: temp_scope_permission_test.go:200–226 claims rejection before policy evaluation but asserts only no approval, no runner call, and an eventual error. The Timeout_MS fixture passes the generic precheck and reaches ordinary policy evaluation; subsequent Shell parsing rejects it. Add a recording-policy assertion and reconcile the claimed ordering contract.
• Important: the network/MCP stubs at policy_args_decode_test.go:246 prove generic rejection, not target agreement. Use offline real-adapter fixtures with recording transports/providers, resource-specific rules, accepted escaped keys, and competing resource fields.
• Advisory: ambiguity-specific hook-mutation, pending-approval resume, and mixed read-batch regressions remain unpinned. Add these lifecycle cases with no-policy/no-execution assertions for ambiguous calls and positive controls.

The scanner's differential, fuzz, boundary, and allocation coverage is substantial.

Domain

• Blocker / High: resource-scoped permission bypass, cross-confirmed by security and architecture.
• Important / Medium: tagged-field validation rejects valid exact matches to untagged fields. See inline comment.
• No library-reuse or duplication findings. A proposed MCP-normalization concern was checked and excluded because the evidence did not establish a policy/execution disagreement.

CI checks were successful or skipped. No local tests or linters were run. The request for changes is based on the code defect, not CI.

Counts preserve independent axes: the two blocker findings describe one underlying defect.

PANEL: ship_blockers=2 important=5 advisory=1 reviewer_failures=0

[peppescg]

• Timeout_MS: the test now records policy evaluation and states the real ordering (1a2fa81). That case reaches policy as an ordinary Shell call, the Shell decode refuses it, and no system-scope approval is surfaced.
• Network/MCP agreement: covered by the real-tool test above. The name-only stub test stays, as a check of tool-agnostic rejection.
• Advisory lifecycle cases: added in 40cb393 (engine/agent/policy_args_lifecycle_test.go). TestHookMutatedAmbiguousArgsAreRejected covers a PreToolUse rewrite into a repeated or case-variant key, on the serial path (Shell) and in a read batch (Read). TestResumedApprovalWithAmbiguousArgsIsRejected covers approving a paused ask whose pending call is ambiguous, and a hook that rewrites a canonical pending call on resume. TestReadBatchRejectsOnlyTheAmbiguousCall covers a batch where only the ambiguous call is refused. Each asserts that the ambiguous arguments reach neither the recording policy nor the tool, with a canonical control that is evaluated and runs. Removing the check from permissionDecision fails every ambiguous case.