disk-hygiene: _engine_gate_relevant samefile-scans every token of every command, not just separator-carrying words as documented

[kyle-sexton]

Summary

_engine_gate_relevant's marker-free fallback calls os.path.samefile on every whitespace token of every Bash/PowerShell command in every session. Its own docstring says it scans separator-carrying words; the implementation applies no such filter. This is both a doc/code mismatch and the identified source of the unbounded stall that #1423 investigated and that PR #3523 works around rather than fixes.

Evidence

plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py, marker-free branch of _engine_gate_relevant:

candidates = list(marker_candidates)
words = _literal_shell_words(command, allow_backslash=allow_backslash)
candidates += (
    [token.strip("'\"") for token in command.split()]
    if words is None
    else list(words)
)
return any(_same_file_as_bundled(candidate) for candidate in candidates)

Every candidate reaches _same_file_as_bundled, which calls os.path.samefile(word, bundled) and, for relative words, a second samefile against the engine's own directory. So an ordinary git log --oneline --graph --decorate origin/main performs a filesystem identity check on each of its words.

The function's docstring describes something narrower:

A path-like word (containing a separator) that is the SAME FILE as the bundled engine — a symlink or hard link under any name — gates regardless of its filename.

and the branch's own inline comment says:

scan their whitespace tokens for separator-carrying words and identity-check those

Neither the whitespace-token path nor the _literal_shell_words path filters on a separator.

Why it matters

The module docstring already names this as the strongest candidate for the one observed 17-second hang:

the strongest identified candidate for the 17s itself is _engine_gate_relevant's marker-free fallback, which calls os.path.samefile on every separator-containing word of every Bash/PowerShell command in every session

Note that sentence says "every separator-containing word", which is what the code was believed to do. It scans every word.

The latency is unbounded, not merely large: samefile on a dead drive letter, a disconnected UNC path, or a stale network mount blocks for as long as the OS takes to fail. No deadline value fixes that — it is why PR #3523 changed the watchdog's expiry action instead of tuning its timeout. The stall itself is still here.

Two distinct costs:

1. Latency, paid by every shell tool call in every session, proportional to token count.
2. Reach. A guard that only governs disk-hygiene engine invocations performs filesystem calls against arbitrary user-supplied path tokens from entirely unrelated commands.

Suggested fix

Filter the marker-free candidate set to separator-carrying words, which is what both the docstring and the inline comment already describe.

This is a semantics change to the security core and should not ride along in a performance PR. It needs its own review of the residual set, because it narrows what the gate sees. The relevant question for a reviewer: the function's contract already lists

a PATH-installed alias with no separator, an alias inside a command the literal parser rejects when the marker is absent, and a copied engine

as accepted residuals of the copy-evasion class. A separator filter appears to add nothing beyond the first of those, which is already accepted — but that needs confirming, not assuming, and the marker-carrying branches must stay untouched.

Verification expected of a fix

• A differential over a command corpus proving allow/ask/deny parity on every non-alias shape (PR perf(disk-hygiene): cut the destructive guard's per-call spawns 4 to 1, and stop its watchdog blocking commands it never judged #3523 added a harness of this shape that can be reused).
• Explicit cases for a hard-linked engine under a non-marker name, invoked with and without a separator, since that is the class the filter could narrow. Note os.link cannot cross volumes on Windows; a cross-volume failure silently degrades such a test into a copy test, which exercises a different, already-accepted residual and reports no gap.
• A measurement of samefile calls per invocation, before and after, on a representative command.

Related

• disk-hygiene: destructive guard exits 1 with empty stderr after 17s — a live fail-open distinct from #1242 #1423 — the fail-closed exit-code work whose investigation note identified this fallback.
• fix(disk-hygiene): engine gate denies the plugin's own test file via substring marker match #1611 — why _carries_marker compares basenames rather than substrings.
• disk-hygiene: the "provably a different file" escape added in #1640 lets the plugin's own stale engines bypass the kill switch #1805 — why a path inside the plugin's own cached-version tree never earns the different-file escape.
• PR perf(disk-hygiene): cut the destructive guard's per-call spawns 4 to 1, and stop its watchdog blocking commands it never judged #3523 — bounds the consequence of the stall (expiry no longer blocks a marker-free command) without touching the stall itself.

[kyle-sexton]

This was generated by AI during triage.

Claiming for triage-only evaluation (no execution).

---

Generated by Claude Code

[kyle-sexton]

This was generated by AI during triage.

Cross-reference: #3349 measures the symptom this issue root-causes

Related, not duplicate. Recommend linking, not consolidating.

#3349 ("PreToolUse gate runs on every Bash/PowerShell call in every session, ~2.4 s median") is the symptom-side filing of the same cost. This issue is the mechanism: _engine_gate_relevant's marker-free fallback calling os.path.samefile on every whitespace token, not just separator-carrying words as its own docstring and inline comment claim. #3349's own triage comment already points at this fallback as a surviving residual behind the Bash predicate, but it does not carry the doc/code mismatch or the "every token, not every separator-carrying word" finding, which is what makes the latency unbounded rather than merely large.

Why they should not be merged:

1. disk-hygiene: PreToolUse gate runs on every Bash/PowerShell call in every session (~2.4 s median) #3349 uniquely holds a threat-model option this issue does not cover. Its fourth suggestion, making the always-on gate conditional on the plugin having been invoked at least once in the session, changes what the gate protects against. Its triage comment explicitly rules that out of the delegable scope and flags it as a maintainer decision. Folding disk-hygiene: PreToolUse gate runs on every Bash/PowerShell call in every session (~2.4 s median) #3349 into this issue would lose that thread.
2. disk-hygiene: PreToolUse gate runs on every Bash/PowerShell call in every session (~2.4 s median) #3349's remaining scoped work is the PowerShell matcher predicate, plus a fresh measurement with the plugin-disabled control the original lacked. That is registration-level work in hooks/hooks.json.
3. This issue is a semantics change to the security core, narrowing what the gate sees, and its own body says it should not ride along in a performance PR. Different review bar, different acceptance criteria.

Suggested shape: #3527 as the root-cause child of #3349, with #3349 retained as the parent measurement-and-registration item. A human should set the edge; no labels, state, or links were changed by this pass.

No comment was posted on #3349 itself: it showed activity inside the last 30 minutes, which this lane skips.

---

Generated by Claude Code

[kyle-sexton]

This was generated by AI during triage.

Triage: verified, routed human-gated

The report is confirmed, the cost is larger than reported, and the suggested fix as written opens a hole. That last point is why this is human-gated rather than delegable.

Verification

Instrumented os.path.samefile and called _engine_gate_relevant directly on ordinary commands:

Command	Words	samefile calls	Gate
echo hello	2	8	False
git log --oneline --graph --decorate origin/main	6	24	False
npm run build -- --watch --verbose	6	24	False
rg -n pattern src/ tests/ docs/ --glob !node_modules	8	32	False

Confirmed: no separator filter is applied on either the _literal_shell_words path or the command.split() path, while the docstring and the inline comment both describe one. Probed values include --oneline, --verbose, -n, and -- — flags, not paths.

The multiplier is 4x the word count, not 1x. Two independent doublings the report does not mention:

1. The candidate list is marker_candidates + words, and for a normally-parsing command those two sets are the same tokens. Nothing dedupes them, so every token is probed twice.
2. _same_file_as_bundled performs two samefile calls per candidate: the word as written, then the word joined onto the engine's own directory.

The suggested fix loses a shape the guard deliberately covers

Hard-linked the engine under a non-marker name and probed both readings:

Invocation	Link location	Head has separator	Gates today
<tmp>/cleanup apply	outside engine dir	yes	True
<tmp>/cleanup;echo done	outside engine dir	yes	True
cleanup apply	outside engine dir	no	False
./alias apply	inside engine dir	yes	True
alias apply	inside engine dir	no	True

The last row is the one a separator filter removes. It is not the already-accepted PATH-installed alias with no separator residual: it is the shape _same_file_as_bundled's second reading exists to catch, and the docstring names the reason directly, that the engine's directory is "the directory such a command must cd into for the alias to run." On a shell that searches the current directory before PATH (Windows cmd), cd <scripts> && alias apply executes the engine and would no longer gate. So the report's closing assumption, that a separator filter "appears to add nothing beyond" an accepted residual, does not hold, and the answer is the one the report asked someone to confirm rather than assume.

What the latency actually tracks

The two readings have different risk profiles, and only one of them is unbounded:

• As-written (samefile(word, bundled)) takes an arbitrary user-supplied token. This is the reading that can block on a dead drive letter, a disconnected UNC path, or a stale mount. Unbounded.
• Engine-dir-relative (samefile(<engine dir>/word, bundled)) always resolves under the plugin's own directory. Local, bounded, and it is the reading that carries the coverage in the table above.

A fix that filters the two readings independently can drop nearly all of the unbounded cost without narrowing the gate. A single separator filter over both readings cannot.

Agent Brief

Type: Bug
Summary: The engine gate's marker-free fallback performs unbounded filesystem identity checks against every token of every shell command; cut that cost without narrowing what the gate detects.

Current behavior:
In engine-gate mode, for any command that carries no engine marker, the gate builds a candidate list from both the metacharacter-split token set and the literal-shell-word set, does not dedupe them, and submits every candidate to a two-reading filesystem identity check. Cost is four samefile calls per command word, paid on every shell tool call in every session. Each as-written reading takes a user-supplied token and can block for as long as the OS takes to fail on an unreachable path.

Desired behavior:
Marker-free relevance costs a bounded, small number of filesystem probes per command, and no probe is issued against an arbitrary user-supplied path token that could be unreachable. The set of commands the gate deems relevant is unchanged, including the case where a link to the engine sits in the engine's own directory under a non-marker name and is invoked as a bare word with no separator.

Key interfaces:

• The marker-free branch of the engine-gate relevance predicate: candidate construction and the identity check it feeds.
• The two-reading identity helper. Its as-written reading and its engine-directory-relative reading have different cost and different coverage and should be filterable independently rather than as one unit.
• The marker-carrying branches are not part of this change.

Acceptance criteria:

• A differential over a command corpus shows allow/ask/deny parity with current behavior on every non-alias shape. The harness added by PR perf(disk-hygiene): cut the destructive guard's per-call spawns 4 to 1, and stop its watchdog blocking commands it never judged #3523 is the intended starting point.
• A regression test covers a hard link to the engine placed in the engine's own directory under a non-marker name, invoked both bare and with a leading ./, and asserts both still gate. The test skips explicitly, rather than silently degrading into a copy test, when os.link cannot create the link, since a cross-volume failure on Windows turns it into a different and already-accepted residual.
• A regression test covers a hard link outside the engine directory invoked by absolute path, both alone and beside a shell operator, and asserts both still gate.
• A counted measurement of identity probes per invocation, before and after, on a representative multi-token command, recorded in the PR.
• No probe is issued against a token that is neither separator-carrying nor resolvable under the engine's own directory. Demonstrated by instrumenting the probe on a command whose tokens are all flags.
• The plugin CHANGELOG records the semantics decision and its rationale.

Out of scope:

• The marker-carrying branches of the relevance predicate.
• The watchdog expiry behavior PR perf(disk-hygiene): cut the destructive guard's per-call spawns 4 to 1, and stop its watchdog blocking commands it never judged #3523 changed. This item removes the stall; it does not revisit what happens when the deadline fires.
• The copy-evasion residual class. A byte copy is a different file and remains out of reach.

Why human-gated

Not a capability blocker, a genuinely open decision on a security surface:

1. The fix named in the report is demonstrably not parity-preserving, so someone has to choose the actual approach. The independent-readings split above is a candidate, not a decision.
2. Deciding it means ruling on whether the bare-word-inside-engine-directory shape is worth keeping. That is a judgment about the guard's accepted-residual set, which is maintainer territory.

Separable, if the maintainer wants partial relief without settling the above: deduping the candidate list is provably semantics-free, because the check is a pure predicate under any(), so duplicate candidates cannot change the result. That alone halves the probe count. It still edits the security core, so it is recorded here rather than split into its own item; one decision can authorize both stages.

Labels: keeping priority: high, area: security, work-class: structural; adding needs-human.

Triage claim released.

---

Generated by Claude Code