Skip to content

disk-hygiene: hint matching is misanchored and case-sensitive, byte accounting cannot express "unknown", and five smaller findings from a live audit #1806

Description

@kyle-sexton

Filed by an agent session after a live /disk-hygiene:clean run on Windows 11. Bundled deliberately — these are the non-CRITICAL findings from one audit, and a maintainer may want to split them. The two CRITICALs are filed separately.

1. Hint matching is misanchored, and case-sensitive in the unsafe direction

matching_hints() applies fnmatch.fnmatchcase(subject, pattern) against the basename for name_glob (hygiene.py:482-500). Executed against the shipped baseline:

Name Hints matched
.rate-limits.json.tmp.1363789.17391 none
settings.json.tmp.4 none
.claude.json.tmp.9552.9bfba4e83eaa claude-json-failed-atomic-write
foo.tmp common-temp-file
Thumbs.db windows-explorer-metadata
thumbs.db none
.DS_Store macos-finder-metadata
.ds_store none
tmp-build common-temp-directory
TMP-build none
scratch.md scratch-artifact
Scratch.md none

Two distinct defects:

Anchoring. *.tmp requires .tmp as a suffix; .claude.json.tmp.* requires one producer's exact prefix. Neither matches .tmp as an infix before pid and random suffixes — the standard atomic-write staging shape. The hint's own reason field claims to cover the class ("atomic-write staging remnant") while its pattern encodes a single filename.

Case discipline contradicts the protection layer. has_protected_name() casefolds; matching_hints() does not. On Windows and macOS, where the filesystem is case-insensitive so both spellings name the same file, protections are case-robust and discovery is not.

This is a same-vendor blind spot with a live consequence. The producer is melodic-software/rate-limit-guard 0.3.6, scripts/statusline-tee.sh:89local tmp="$dir/.rate-limits.json.tmp.$$.$RANDOM". A scan of that plugin's state directory returned hinted_entries: 0 across 63 entries, 61 of which were failed-atomic-write remnants. They surfaced only because a subagent read the directory positionally.

Suggested fix: match name_glob case-insensitively, and add a class-level staging-remnant hint (*.tmp.*, ceiling medium) alongside the producer-specific one. Note that changing the shared matcher also affects path_glob and additional_protected_path_globs, matched with fnmatchcase in five places (hygiene.py:492, 714, 1392, 1558, 1803), so the protection-side globs should move together deliberately rather than by accident. The leak itself is filed against rate-limit-guard; the two should cross-reference.

2. Byte accounting cannot say "unknown" or "not real bytes"

metadata() (hygiene.py:503-513) records stat_size, logical_size, mtime_ns, device, inode, mode. Everything needed to qualify a byte count is discarded or never read:

  • Hard links double-count, and st_nlink is in the same lstat result, unrecorded. Measured: .bun\bin\bun.exe and .bun\bin\bunx.exe each report st_size = 98,480,216 with st_nlink = 2 — one object, summed twice, 196,960,432 reported bytes against ~98 MB of data.
  • A truncated subtree emits logical_size: 0 (hygiene.py:727-734), byte-identical to a genuinely empty directory. The "not walked" signal lives only in the separate truncated_paths array, so any per-entry consumer — including a fan-out worker summarising its subtree — reads 0 as empty. Observed: a .cache subtree exceeding 1.35 GB reported far below its real size.
  • No allocated size. No st_blocks on POSIX, no GetCompressedFileSizeW on Windows, so compression, sparseness, and cluster slack are invisible and every total is a floor.
  • st_file_attributes is read by is_linkish() then thrown away, which is why the cloud-placeholder class filed separately leaves no trace in the snapshot.

Where it bites hardest is apply: logical_bytes_removed is summed from the same unqualified logical_size (hygiene.py:1848-1852), so a report can honestly claim gigabytes removed while the observed free-space delta is ~0 — and the section 6 caveat about "concurrent disk activity, sparse files, hard links, compression" explains that away instead of surfacing it.

Suggested fix: record nlink and attributes (free, same lstat) and allocated_size where cheap; set a truncated directory's logical_size to null rather than 0; add a per-entry size_qualifiers list (hardlinked, cloud-placeholder, sparse, not-walked); and report expected reclaimable local bytes as a figure distinct from logical bytes.

3. No structural provenance for an engine-derived summary

The snapshot itself is well-provenanced — engine, schema_version, session_nonce, created_utc (hygiene.py:767-783) — and snapshot_digest() exists (:1274-1276). But no CLI surface prints a digest or a subtree rollup, and nothing in the reporting contract requires a reported number to cite the snapshot it came from.

So a fan-out worker's improvised Get-ChildItem -Recurse and a genuine engine subtree summary are textually indistinguishable in the parent's report. That happened in this audit for five of eight subtree groups, when a template substitution failure left workers unable to reach the engine at all — and nothing in the artifacts revealed it.

Suggested fix: add a read-only summarize --snapshot <f> [--subtree <rel>] emitting totals, truncated_paths, and the snapshot digest; add it to classify_exact_engine_command's allowed shapes; and require every reported figure to carry its snapshot path plus digest. It composes with finding 2 — the summariser is the natural place to refuse to total a subtree containing not-walked entries.

4. The always-on Stop detector's cost claim does not hold on the happy path

guard_launch_monitor.py is registered at plugin level on Stop (hooks/hooks.json:24-40), so it runs once per turn in every session of every consumer, whether or not disk-hygiene is in use. Its behavioural claims check out: stdlib-only, emits systemMessage and nothing else, except BaseException: return 0 (:273-274), once-per-session marker consulted before the read (:247), tail read capped at 2 MB (:88, :165-179).

But the marker is written only when a warning fires (:266-271). Its docstring claims "the amortized per-turn cost for the rest of a long session is a single stat() of a small marker file" — true only after a failure has been reported. In the common case, where the guard never fails, no marker is ever written, so every Stop in every turn pays interpreter startup (the plugin's own ADR 0004 measures python3 -c 'pass' at 396 ms on this host) plus a seek-and-read of up to 2 MB plus a json.loads per line. The module is explicit that it exists to avoid repeating a "12-19s p50 on every single Bash call" cost class; it reduced the frequency but not the per-invocation cost, and its own amortisation does not apply where it matters.

Second, narrower: the detector matches record["type"] == "attachment" and attachment["type"] == "hook_non_blocking_error" (:193-203). Those are internal transcript-record shapes not found in any official documentation — unverified. Combined with fail-silent on every error, a schema change makes this safety-observability detector permanently and invisibly inert, with no self-test that it can still parse what it looks for.

Suggested fix: write the marker on the no-failure path too (with a turn count or TTL so a later failure still warns), or gate the read on a cheap st_size high-water mark stored in the marker. Separately, have /disk-hygiene:setup check feed the parser one synthetic known-shape record and assert it still matches, so schema drift surfaces as a check failure instead of silence.

5. The bash denial text under-reports the allow-list

SKILL.md says the guard allows the argument-free kill_switch_probe.py shape, and that is accurate: is_exact_kill_switch_probe() (destructive_guard.py:833-847) is evaluated at :1109, before classify_exact_engine_command, and is deliberately not gated on the kill switch — correct, since it is read-only. The drift is confined to one human-facing string: _bash_denial_guidance() (:953) enumerates only "scan, preview, handoff-verify, and apply."

Why it matters more than a typo: the documented bootstrap path is to submit a shape with bare python precisely so the denial teaches the grammar. A consumer who learns the allow-list from the denial never learns the probe is permitted — and the probe is the step SKILL.md:52-57 requires in order to state the kill-switch value honestly rather than assuming the default.

Related gap in the same string: it discloses the absolute interpreter and the authorised --data-root, but not the bundled engine's path. If ${CLAUDE_PLUGIN_ROOT} ever arrives unexpanded in a rendered body — the same failure class the plugin already handles for ${user_config.*} — the consumer has no disclosed route to the engine, and the exact-path identity check (:751-753) denies every guess.

Suggested fix: add the probe and the expected engine path to the enumeration, derived from one list shared with the classifier so the two cannot drift again.

6. The probe trusts an environment channel the guard refuses

killswitch_config.default_settings_path() (lib/killswitch_config.py:35-39) honours CLAUDE_CONFIG_DIR, falling back to ~/.claude. The guard refuses exactly that channel and documents why — a repo .claude/settings.json env block reaches hook subprocesses, so it locates user settings from the tamper-resistant --plugin-root instead (destructive_guard.py:636-656). SKILL.md:52-57 then tells the model to run the probe and "honor the effective value it reports."

So the model's self-enforcement reads a repo-redirectable location while the guard reads the trusted one; the two can disagree and the only tell is the report's settings_path field. Not a deletion bypass — the guard is the backstop and both fail closed to enabled — but it is a misreporting channel on a safety control, and the probe's docstring names its managed and --settings scope gaps while omitting this one.

Suggested fix: pass --plugin-root to the probe and derive the same trusted path, or have the probe label its path provenance (trusted-derived versus environment-derived).

7. No retention for the plugin's own run state, and one containment gap

Measured contents of the plugin's data root:

There is no retention or prune logic anywhere in the engine, and no README explaining what the directory is. An auditing agent in this very session classified the older run as third-party managed state. The only bound is uninstall, which deletes the data directory unless --keep-data is passed.

The stray top-level snapshot is a separable defect: SKILL.md:92-93 requires snapshots to live in the run directory, but state_output_path() (hygiene.py:83-97) only enforces "inside the data root and outside the plugin root." The skill's own convention is unenforced, and was violated in practice.

Suggested fix: tighten state_output_path() to accept only <data-root>/runs/<run-id>/…; add documented retention (keep N runs or M days, pruned at scan time, policy stated in SKILL.md); write a per-run manifest.json naming the engine version and each snapshot's digest — which also serves finding 3; and drop a short README.md in the data root so a future auditor can classify it correctly without the plugin installed.

Also worth a decision, from the recurring-concerns pass

The marker-free fallback taxes every session. _engine_gate_relevant's no-marker branch (destructive_guard.py:396-417) calls os.path.samefile on every separator-carrying word of every Bash and PowerShell command in every session, including commands unrelated to this plugin. The guard's own docstring (:22-31) names this as the strongest candidate for an observed 17-second stall and identifies a stale network drive letter in an unrelated command as a user-reachable trigger. It violates the module's own stated principle that "a plugin-level hook must never tax unrelated work," and is bounded only by the 10s watchdog — i.e. the mitigation is a fast deny of someone else's command. Worth narrowing to words that are plausible invocation targets.

Skill-hook lifetime is undocumented and load-bearing. The belt is deny-by-default for Bash: anything that is not one of four exact engine shapes (plus the probe) is denied. Its tolerability rests entirely on being short-lived. The hooks page says component hooks are "scoped to the component's lifetime and cleaned up when it finishes" but never defines a skill's lifetime, and we could not confirm the belt's duration from any official source. Issue #383's E1 records the empirical symptom — once the skill's hook was active, every non-engine Bash command in the session was denied. Worth measuring the lifetime, stating it with the Claude Code version measured, documenting the escape, and considering narrowing the belt from deny-unknown-Bash to deny-deletion-spellings-plus-engine, since the deny-unknown posture is what creates the lockout while the engine's own containment is the stated authority anyway.

One tooling gap, reported for whoever owns skill-quality: references/component-types/skill.md mandates running skill-quality:check, but that plugin's scripts/check-skill.sh:96-100 does git rev-parse --show-toplevel and exits 2 with "Error: not in a git repo". An installed plugin cache is not a git working tree, so the mandated static gate is unreachable for a post-use audit of an installed component — precisely the scenario it is mandated for. The manual lens fallback was used instead.

What is working well and should not be disturbed

Recorded because an audit that reports only defects invites a rewrite of code better than its replacement.

  • Exit-code discipline. Only 0 and 2 are reachable past main's boundary; os._exit avoids CPython's exit-120-on-flush-failure; a broken stdout converts an undelivered allow into a deny (destructive_guard.py:1215-1236). Correct against the hooks page, and reasoned from a real observed incident.
  • Identity over enumeration. _same_file_as_bundled asks the filesystem whether a spelling resolves to the engine rather than enumerating Windows filename aliases. The docstring's argument — enumeration closes one spelling per review round — is right.
  • handoff-verify's per-path verdict expiry, and the note restating the verify-one-per-deletion rule on every invocation so the warning travels with the data. Used exactly that way for 11 deletions in this session with zero skips.
  • Fail-closed direction throughout: unverifiable handle state, unreadable mountinfo, and truncated coverage all land in contested or keep, never clear.
  • Truncated-candidate short-circuits (:1367-1372, :1399-1406) skip unbounded live walks for candidates that can never become approvable — a correctness fix and a cost fix at once.
  • SSOT discipline is unusually good: MIN_PYTHON is pointed at rather than restated, and _DECLARED_HOOK_TIMEOUT_SECONDS duplicates the registrations' timeout with a named test pinning them.
  • The affirmed decision not to ship a Windows engine apply lane is defensible and honestly documented with its residual named. The separate request on issue disk-hygiene: POSIX has no support lane (Bash denies all non-engine) and macOS manual-handoff has no deletion lane #383 is for a recycle primitive, which is a much smaller surface — not for reversing that call.

Scope of verification

Single Windows 11 host. The Linux apply path — the only code where this plugin ever deletes — was read line by line and never executed, because the host returns execution-platform-unsupported. No policy overlay was exercised, so the additive-only guarantee is unverified empirically. --confirmed-large-scan, the 250,000-entry ceiling, and the kill switch actually set to false were never exercised. D: is ReFS and was never scanned; macOS untested.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions