dsh-tools: createExecution identity is mutable until final notification — pre-execute listeners can retarget calls (0.1.6-alpha.2 → 0.2.1-alpha.2)

[Hylouis233]

Bug report (upstream Issues are disabled, so posting here per README "Submit feedback or bug reports through GitHub Discussions").

Summary

ToolRegistry.createExecution builds the execution identity (name, callId, rootCallId, token, ...) as ordinary writable object fields. The only Object.freeze(exec) in lib/index.js runs at the start of notifyResult(exec, result) — i.e. when the final result is notified — not at creation. dispatchToolBody then resolves the tool body via this.resolveExecution(exec.name, ...), i.e. from the still-mutable exec.name.

Any tools/pre-execute listener can therefore run exec.name = '<another-allowed-tool>' and silently retarget the call to a different whitelisted tool's body. Guards/policy sitting downstream of pre-execute only ever see the rewritten name. TypeScript readonly provides no runtime protection.

Affected versions

Reproduced on every published version we tested, from 0.1.6-alpha.2 (pinned) through the latest 0.2.1-alpha.2 (2026-10-09), including 0.1.7-rc.2 and 0.2.0-rc.2. Key lines in lib/index.js (0.2.1-alpha.2): createExecution at ~3129, const name = exec.name at ~3134, execution object construction (only arguments deep-frozen) at ~3165, dispatchToolBody/resolveExecution(exec.name) at ~3292/3305, and the single Object.freeze(exec) in notifyResult at ~3408. Line numbers shift slightly per release but the structure is identical across all nine releases we audited (0.1.6-alpha.2, 0.1.7-alpha.1/.2, 0.1.7-rc.1/.2, 0.2.0-rc.1/.2, 0.2.1-alpha.1/.2).

Minimal reproduction

1. Register two tools, e.g. dengue_weekly_descriptive and brief_verifier (both allowed).
2. Register a tools/pre-execute listener that mutates the execution object and allows the call:

ctx.on('tools/pre-execute', async (exec) => {
  exec.name = 'brief_verifier'; // retarget to another whitelisted tool
  return { action: 'allow' };
});

3. Invoke dengue_weekly_descriptive.

Observed: the actual executed body is brief_verifier's; the result reports isError=false; the runtime raises no error (no mutation_rejected_by_runtime signal). The mutation is not rejected in strict mode either, because the identity fields are plain writable properties.

Expected: the executed body must remain the originally requested tool, or the call must be rejected with a structured error.

We ran this against pinned 0.1.6-alpha.2 and 0.2.1-alpha.2, plus 0.1.7-rc.2 and 0.2.0-rc.2, in isolated installs (npm install @deepseek-ai/dsh@<version>): the attack succeeded on all four, with zero model calls / zero session events.

Suggested fixes (any one closes the hole; A is preferred)

A. Freeze identity at creation (immutable identity interface). Before createExecution returns, freeze the identity fields — at minimum name/callId/rootCallId/agent/parent/token (e.g. shallow-freeze the base object before composing the execution object, or expose identity through a read-only accessor view). In dispatchToolBody, use the name captured at creation instead of reading exec.name. A listener attempting the rename then throws TypeError in strict mode, which is a sufficient signal.

B. Anti-drift assertion. If a mutable carrier must be kept, after the pre-execute waterfall and before the guard, compare exec.name against the original name recorded in the token; on mismatch, terminate with a structured error without entering dispatch. Also provide a supported read-only query for the original identity on ToolExecutionToken.

C. Sampling seed pass-through (related gap). LlmCallConfig/GenerateOptions have no seed field (absent across 0.1.6-alpha.2 → 0.2.1-alpha.2 in dsh-llm; grep -i seed over all lib sources returns nothing), and dsh-llm-deepseek's request construction (~lines 1705–1721 in 0.2.1-alpha.2) forwards only model/stream/messages/max_tokens/thinking/system/temperature/stop_sequences/tools. Please add an optional seed: number to the config types and zod schemas, forward it in the adapter when the provider supports it, and raise an explicit UNSUPPORTED_* error rather than silently dropping it.

Acceptance criteria

• With a renaming listener from the repro, the executed body is still the originally requested tool, or the call is rejected with a structured error.
• A request carrying seed makes that field observable at the adapter layer.

Impact statement

We treat this as a security/adversarial defect (EXECUTION_IDENTITY_MUTABLE) in our evaluation harness: any pre-execute plugin can silently substitute the tool that actually runs, which breaks the integrity of audited/agentic evaluation pipelines. Combined with the missing seed pass-through (no way to make sampling reproducible), we currently fail-closed: all live model entry points exit before startup (live_ready=false) and we stay pinned on 0.1.6-alpha.2 until an upstream fix or a supported hardening interface lands.

Evidence: full per-version audit (sha256 of released lib/index.js for all nine versions, line-level findings, adversarial probe outputs) is maintained in our downstream project (CTModelling-LLMdev/dsh-plugin-public-health, docs/reviews/20261009-execution-identity-upstream-audit.md). Happy to share the probe scripts or adapt them into a PR if that helps.

[argszero]

Confirmed against the source — and the hole is wider than name: it covers every field the registry resolves the call with, including the scope the lookup happens in. Re-verified below on master d7432673, packages/core/tools/src/index.ts.

What you found, in the registry's own terms

createExecution (:1397) captures the identity into locals and then composes it as a plain writable object — const execution: MutableToolRunContext = { ...base, arguments: deepFreeze(detached) } (:1451). The asymmetry sits in that one line, and it is deliberate on one half only: the arguments are deep-frozen, the identity fields are not. The only Object.freeze(exec) in the file is :1704, inside notifyResult — after a result exists to notify. (That freeze also lands late enough to be worth naming: tools/result (:1710) and tools/post-execute (:1789) are the observers that see the frozen object, which is why no observer ever gets a chance to notice the drift.)

The identity is not only name

dispatchToolBody (:1570) resolves through resolveExecution(exec.name, exec.agent, exec.parent !== undefined) (:1584), and resolveExecution (:1253) is this.get(name, scope) plus this.collapses(name, scope, nested). So three mutable fields decide what runs:

• name — which definition is dispatched. Yours.
• agent — the scope key of the lookup. Renaming it resolves the call in a different agent's scope, where a tool of the same name may carry a different body; no tool rename is needed at all.
• parent — the nested flag, which is the ptc collapse's own input (collapses (:1357): !nested && modeFor(scope) === 'ptc' && name !== RUN_CODE_NAME).

The around-dispatch layer is selected the same way: dispatchScheduledExecution computes scopeTarget(this, exec.agent) (:1610), so the tools/execute listeners the call reaches are chosen by the mutated field too.

The guard stage is behind the same mutation

Your "guards downstream of pre-execute only ever see the rewritten name" applies to the monotonic guard stage as well. prepareExecution computes const denialReason = decision.kind === 'allow' ? this.guardReason(exec) : decision.reason (:1525) from the same object, after the waterfall returns — so a guard and the dispatched body agree with each other while both disagree with what the pre-execute listeners approved. ToolGuard's doc calls its parameter "the identity-protected call" (:729) and the tools/execute doc says "call identity remains immutable"; the contract is already written down. What is missing is only the moment it is enforced.

One correction to fix B

createExecutionToken() (:1911) returns Symbol('dsh.tool.execution') — documented as "a same-process correlation token whose identity is its value". It carries no name, so "compare exec.name against the original name recorded in the token" cannot be implemented as written. The provenance has to live where the registry already keeps per-execution state — deferredContexts, contentFinalizers and cancellationStates are all WeakMaps keyed by the execution object — or in the closure that already captures it (const name = exec.name, :1402, today used only for the collapse-denial message).

On a supported hardening interface (your ask)

There is one, and it is worth knowing before you decide that pinning is the only safe posture.

tools/pre-execute is a cordis waterfall, and a waterfall runs its listeners outermost-first (vendor/cordis/src/events.ts:235), with prepend inserting at the front of the hook list (register, :255: options.prepend ? 'unshift' : 'push'). A listener registered prepend: true is therefore the outermost frame: its await next() returns only after every other listener and the built-in allow have settled, and the registry has not read the decision yet. From there it can

• restore the identity fields it captured before next(), so :1584 resolves the originally requested tool, or
• return { kind: 'deny', reason }, which :1525 honours exactly as it honours a denying inner listener.

Both are available from the public surface, and neither needs the token to carry anything. The limits, stated plainly: it cannot make the object immutable (that is fix A, and A is the right fix); a prepend listener registered after it sits outside it; and it closes only the pre-execute window — anything that mutates the execution from tools/execute onwards is out of its reach.

I maintain a published guard that sits on exactly this event (@argszero/dsh-destructive-command-guard), and this defect is why a pre-execute guard cannot be the last word on what actually runs. It deserves a hardening plugin of its own, so I am preparing one that captures the identity at the outermost frame, restores it, and counts every drift it sees — I will post it here once it is on npm, whatever you find it worth.

[argszero]

Shipping the seam — the repair sketched above is now an installable plugin.

@argszero/dsh-tool-identity-pin-guard@0.1.0

• npm: https://www.npmjs.com/package/@argszero/dsh-tool-identity-pin-guard
• source: https://github.com/argszero/dsh-tool-identity-pin-guard

npm install @argszero/dsh-tool-identity-pin-guard

Mount it as a bundle; the patch it ships (dsh.bundle.patch → cordis.patch.yml) only inserts the plugin and changes nothing else:

- insert:
    - id: tool-identity-pin-guard
      name: '@argszero/dsh-tool-identity-pin-guard'

How it relates to the report: it does exactly what the analysis above described, and nothing more. It registers on tools/pre-execute with { prepend: true }, so it owns the outermost frame; it captures name / agent / parent / callId / token / signal (and the scope a caller decides by) before awaiting next(), and writes those values back into the same object after the waterfall settles — that is, after every inner listener has spoken, and before resolveExecution(exec.name, exec.agent, exec.parent !== undefined) reads it for dispatch, before scopeTarget(this, exec.agent) picks the tools/execute carriers, before the approval request is built, and before canonicalResults.set(result, exec.token).

Four modes, because a deployment should be able to measure before it repairs:

• pin (default) — restore the captured identity, let the call run, count the drift.
• report — count and log the same drift, write nothing. This is the posture for answering "is anything in my deployment rewriting calls?" before trusting a repair.
• deny — refuse the call before any approval prompt is raised, naming the field that was rewritten.
• off — no listener at all; the report tool still registers, and its one job in that posture is to say the guard is off.

Counters — including the denominator, so "no drift" can be told apart from "never ran" — and the retained drifts (bounded window, newest kept) are read back with the tool_identity_drift_report tool.

Three properties that matter more than the feature list:

• Zero runtime dependencies. defineTool from @deepseek-ai/dsh-tools is declared as a peer, not a dependency, on purpose: a dependencies entry is how a plugin ships its own copy of the registry's package, a second evaluation whose symbols are equal to nothing the host holds — the failure this repository has already shipped more than once. The only import is a value from the host.
• Fail-open. If the plugin's own work throws (a capture, a comparison, the write-back), the call proceeds untouched and the failure is counted and reported rather than raised at the caller. A guard that can break the pipeline it guards is a worse bug than the one it fixes.
• It is a reading, not a lock. It compares the identity against the values this deployment approved (name/agent/parent/callId are all changed by a rewrite, and a property that appears or vanishes counts as drift too), and it writes back only the fields the deployment asked it to pin. It does not pretend to make the object immutable — nothing in a listener can, because the object the registry dispatches is the same object — it just makes the window between approval and dispatch not observable to the registry.

Verified against the published artifact: 68/68 tests, 34/34 injected defects caught (0 silent), and the shipped tarball was installed by name from the registry and executed from its own lib/. Source anchors for the analysis, per the reply above: packages/core/tools/src/index.ts (tools/pre-execute declaration, createExecution, resolveExecution, the guard's live read, the notifyResult freeze) and vendor/cordis/src/events.ts (outermost-first waterfall dispatch, prepend unshifting to the front).