Skip to content

feat(disk-hygiene): add scan --quiet to drop the duplicated children_rollup - #3783

Merged
kyle-sexton merged 3 commits into
mainfrom
claude/3352-scan-verbosity
Sep 7, 2026
Merged

feat(disk-hygiene): add scan --quiet to drop the duplicated children_rollup#3783
kyle-sexton merged 3 commits into
mainfrom
claude/3352-scan-verbosity

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Closes #3352

Summary

scan had no output-verbosity flag: every run emitted its full children_rollup to stdout, one row per immediate child, whatever the caller needed. The rollup is the only part of the payload that grows with the frontier, and it is already written to the snapshot file on disk, so a caller that only needs the summary paid for a second copy of detail it would never read.

Trimming at the call site is not available and should not be. The Bash guard fails closed on pipes, redirects and shell operators, and that rejection is load-bearing: an exception for | head is an exception for everything spellable as | head. Nothing in this change relaxes it. The fix is in the engine.

Fix

scan --quiet omits children_rollup from stdout and replaces the long closing note with a short one naming where the rows went. Everything a keep-or-review decision rests on survives: status, target, snapshot, entries, hinted_entries, unhinted_entries, empty_directory_count, target_logical_bytes, target_reclaimable_local_bytes, truncated_paths, errors, policy_sources, os_autoclean.

Three calls worth flagging for review:

The default is unchanged, and that is deliberate. Quiet is opt-in. An existing consumer parsing children_rollup off stdout must not be quietened by an upgrade, and a verbosity flag is not worth a silent breaking change to the payload every caller already reads. test_default_scan_still_carries_the_rollup_and_the_explaining_note pins it, so a future flip of that default has to break a test to happen.

snapshot is kept in quiet output, though the issue's field list does not name it. The whole argument for the flag is "read per-child detail from the snapshot instead"; a quiet payload that does not say where the snapshot is cannot be acted on. This is the one deviation from the literal acceptance checkbox.

Root-children mode is covered too. That mode emits its own scan-complete payload with its own rollup. A flag that shapes one of the two and silently does nothing in the other is a trap for the caller who reaches for it exactly where the frontier is widest, so both go through the same shaping helper.

destructive_guard.py admits --quiet as a third valueless scan flag alongside --confirmed-large-scan and --root-children: at most one per invocation, no trailing value, everything else still fails closed. It shapes stdout only, so it reaches no path and skips no check the same invocation without it would not already reach. No pipe, redirect or shell-operator allowance is added anywhere.

handoff-verify is deliberately left alone. The issue notes its note block is re-emitted on every single-path call. That note is a deletion-safety warning ("verify ONE path per deletion... re-verify after any delay"), it is roughly 400 bytes against the rollup's thousands, and its repetition is the point of it. The 17x multiplication comes from the per-path round-trip, which is the separate --path issue. Suppressing a safety warning to save 400 bytes is the wrong trade, so it is not made here.

Verification

The issue's ~63,000-token premise does not reproduce at that magnitude on this host, and the real numbers are below. The reported figure was an estimate from output size on a user home directory, not a token count; it corresponds to a home directory with roughly 110 immediate children. The reduction ratio holds regardless of where on that curve a target sits.

Real --max-depth 1 scans, default vs --quiet, stdout bytes with a ~4-bytes-per-token estimate:

Target Rollup rows Default Quiet Saved
/root (real home dir) 20 7,247 B (~1,811 tok) 893 B (~223 tok) 88 %
/home/user (real) 7 3,489 B (~872 tok) 835 B (~208 tok) 76 %
synthetic, 40 children 41 14,780 B (~3,695 tok) 1,690 B (~422 tok) 89 %
synthetic, 80 children 81 27,900 B (~6,975 tok) 2,530 B (~632 tok) 91 %
synthetic, 200 children 201 67,262 B (~16,815 tok) 5,052 B (~1,263 tok) 93 %

So on this container the audited seven-scan run would have cost ~12,700 tokens rather than ~63,000; on a home directory wide enough to produce the reported ~9,000 tokens per scan, --quiet takes the same seven scans to roughly 5,000 tokens total. The saving grows with the frontier because the rollup is the only per-child term: quiet's own per-child growth is one truncated_paths string, an order of magnitude flatter, which test_quiet_stdout_stops_growing_with_the_child_count pins.

Default output byte-identical to origin/main, checked empirically by loading both engine revisions against one fixture: same parsed payload, same 5,751-byte stdout, and the only key --quiet removes is children_rollup. (Two note string literals were re-wrapped to fit the added indentation; they concatenate to the same text, which is what this check proves.)

Commands run, all in the foreground:

  • scripts/affected-tests.sh --run — exit 3, the success code: 151 shell suites passed or skipped, 11 Python/mjs suites selected for their own lanes.
  • python3 -m unittest test_hygiene test_guard_launch_monitor in plugins/disk-hygiene/skills/clean/scripts — 368 tests, OK.
  • python3 -m unittest test_hook_telemetry in plugins/disk-hygiene/lib — 3 tests, OK.
  • scripts/run-ruff.sh check plugins/disk-hygiene — All checks passed.
  • scripts/run-ruff.sh format --check . in the clean scripts directory — the touched test file is clean; hygiene.py reports one pre-existing blank-line finding that is present unchanged on origin/main and is not touched here.
  • scripts/check-changelog-parity.sh in all four modes (--check, --check-order, --check-bump origin/main, --check-preserved origin/main) — exit 0 each.
  • scripts/check-purged-em-dashes.sh — 105 files scanned, no em dashes.

New coverage: seven tests. Default pinned; quiet's field set proved to be the default's minus exactly children_rollup with every surviving value equal; the snapshot proved to carry the rollup in both modes (a quiet run that lost it would be data loss wearing a verbosity flag); root-children mode's quiet path; the growth ratio; the shaping helper's non-mutation of its input; and the guard's grammar, which allows --quiet once in every position and still denies --quiet --quiet, --quiet v, --quiet=1 and -q.

Pre-existing and unrelated: test_save_point.py::test_new_origin_falls_back_to_directory_name fails identically on pristine origin/main.

Related

🤖 Generated with Claude Code

https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob

Generated by Claude Code

@claude
feat(disk-hygiene): add scan --quiet to drop the duplicated children_…
819a6ee 
…rollup

Every `scan` emitted its full `children_rollup` to stdout, one row per
immediate child, with no way to ask for less. The rollup is the only part of
the payload that grows with the frontier, and it is already written to the
snapshot file on disk, so a caller that only needs the summary paid for a
second copy of detail it would never read. Trimming at the call site is not
available and should not be: the Bash guard fails closed on pipes, redirects
and shell operators, and that rejection stays untouched. So the flag belongs
in the engine.

`scan --quiet` omits `children_rollup` from stdout and replaces the long
closing note with a short one naming where the rows went. Everything a
keep-or-review decision rests on survives: `status`, `target`, `snapshot`,
the three coverage terms, `empty_directory_count`, both byte totals,
`truncated_paths`, `errors`, `policy_sources` and `os_autoclean`. The
snapshot file carries the rollup in full in both modes, asserted by test, so
the flag drops a duplicate rather than data. Root-children mode emits its own
`scan-complete`, so quiet reaches that payload too rather than silently doing
nothing exactly where the frontier is widest.

The default is unchanged and pinned by test. A caller already parsing
`children_rollup` off stdout must not be quietened by an upgrade, so quiet is
opt-in and a future flip of that default has to break a test to happen.

`destructive_guard.py` admits `--quiet` as a third valueless scan flag, at
most one per invocation and with no trailing value, alongside
`--confirmed-large-scan` and `--root-children`. It shapes stdout only, so it
reaches no path and skips no check the same invocation without it would not
already reach. No pipe, redirect or shell-operator allowance is added.

Measured on a real `--max-depth 1` home-directory scan: 7,247 bytes to 893
(88 % smaller). On a 200-child target: 67,262 to 5,052 (93 %).

Closes #3352

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob
@kyle-sexton kyle-sexton mentioned this pull request Sep 5, 2026
Open
@kyle-sexton
kyle-sexton marked this pull request as ready for review September 5, 2026 22:51
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-05T22:56:25.466455Z 819a6ee Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 57s —— View job

Code review — /review:code-review

  • Gather context (PR body, diff via gh pr diff, changed files)
  • Review hygiene.py: scan_stdout_payload, both scan-complete call sites, argparse wiring
  • Review destructive_guard.py: valueless-flag grammar change
  • Review test_hygiene.py: new coverage for quiet output, snapshot parity, guard grammar
  • Review doc changes: SKILL.md, reference/safety-model.md, CHANGELOG.md, plugin.json
  • Post findings

Scope note: this lane reviews correctness/maintainability and defers security to the separate claude-security-review lane on this repo, per REVIEW.md.

Verification note: this lane's tool allowlist only grants gh pr diff/view/comment/review plus read-only file tools, not arbitrary Bash/python3, so I could not independently execute the test suite, ruff, or the guard's own test harness. The PR body's test/ruff/changelog-parity results are author-claimed and unverified by me; I reviewed the diff by reading it directly instead.

Findings

None that clear the high-signal bar. Specifically checked and found correct:

  • scan_stdout_payload (hygiene.py#L138-L152) builds a new dict via comprehension rather than mutating its input — the non-mutation guarantee the new unit test pins actually holds.
  • Both scan-complete emission sites (root-children mode and the default walk) route through scan_stdout_payload(..., args.quiet) — the PR's claim that root-children mode is covered too checks out; the earlier root-children-selection-required early-return (a different status, no children_rollup) is correctly left unwrapped.
  • destructive_guard.py's generalized valueless-flag loop (destructive_guard.py#L142-L153) preserves the prior semantics for --confirmed-large-scan and extends them to --quiet (at most one occurrence, no trailing value, order-independent since it's count+remove); traced by hand against all four allowed/denied cases the new test_guard_scan_accepts_single_quiet_flag asserts, including combination with --root-children.
  • Default-mode payload shape is untouched when quiet is False (scan_stdout_payload returns the same object), matching the "byte-identical to origin/main" claim structurally.
  • Docs (SKILL.md, safety-model.md) and CHANGELOG.md describe the same field list the code actually keeps/drops.

Nothing here rises to a blocking or flaggable issue — the change is small, symmetric across both scan-complete call sites, and the new tests target the actual risk points (default-mode regression, snapshot data loss, root-children parity, guard grammar).

@kyle-sexton Claude

kyle-sexton commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Independent review — fresh context, producer rationale withheld — flipped to ready

Posted by the autonomous worker lane (vm-worker-202609050257). Verdict: SOUND-WITH-NITS, ready to flip. This flip is the lane's handoff; the merge lane owns it from here. This lane does not merge.

Reproduced, not read

Check Result
Default unchanged Byte-identical confirmed. Both engine revisions loaded against one shared fixture at 4/20/40/80 children: exact stdout string match and equal parsed payloads at every width (2,835 / 7,957 / 14,357 / 27,157 B). --quiet removes exactly {children_rollup} and adds nothing; only note changes (820 B → 321 B).
Default-pinning test Not vacuous. Forcing quiet = True inside scan_stdout_payload in a copied tree failed three tests, not one.
Measurement Reproduces. /root 7,333 → 977 B (87%) vs the author's 7,247 → 893 (88%); 200 children 65,561 → 4,151 B (94%) vs 67,262 → 5,052 (93%). Per-child growth 320 B default vs 17 B quiet, 18.8x flatter, so the ratio holds at any frontier width.
The issue's ~63k Does not reproduce — ~12,800 tokens for seven /root-class scans, matching the author's ~12,700.
Group fence Respected — nothing in handoff-verify (#3351), hooks.json (#3349), or denial branching (#3348).
Version bump Correct — new backward-compatible flag is a minor.

Both deviations from the brief upheld

  • Keeping snapshot in quiet output: defensible. Quiet's whole instruction is "read detail from the snapshot"; omitting the path makes the payload unactionable.
  • Leaving the handoff-verify note alone: a genuine safety warning, not avoidable work. hygiene.py:3077-3084 is a time-of-check/time-of-use caveat ("a clear verdict is valid only at emission time... verify ONE path per deletion... re-verify after any delay"), ~370 B. Its 17x multiplication is disk-hygiene: handoff-verify should accept --path inline, halving prompts on multi-path runs #3351's per-path round-trip, not the note.

One correction to the PR's own reasoning

The body says the search for an existing verbosity convention found zero hits. That premise is overstated: two first-party output-verbosity --quiet flags do exist, at plugins/claude-ops/skills/observability/scripts/clean.sh:38 and plugins/guardrails/lib/verification/verify-cli-flag.sh:13. Both spell it --quiet, so the chosen flag matches the house convention rather than conflicting with it — the conclusion holds, the stated basis was wrong.

Non-blocking findings

  1. hygiene.py root-children quiet path — quiet replaces the root-children note wholesale with the generic QUIET_SCAN_NOTE, dropping the scoping caveat that the volume root and skipped OS-owned/hidden/reparse entries "were never walked". That caveat governs entries and target_logical_bytes, both of which survive quiet. Inferable from root_children_mode/root_children_selected, but a one-sentence addition to QUIET_SCAN_NOTE on that branch would close it.
  2. ## Verification names one pre-existing ruff format --check finding in hygiene.py (confirmed byte-identical on origin/main), but guard_launch_monitor.py also fails it and is untouched — equally pre-existing, just unmentioned.
  3. destructive_guard.py:883-903 — --quiet is stripped before pair-matching, so --policy --quiet X classifies as allow then fails at argparse. Pre-existing and identical for --confirmed-large-scan; no path, data-root, or authority widening, and not introduced here.

Gates re-run green in the foreground: affected-tests.sh --run exit 3 (151 suites passed/skipped, 11 delegated); test_hygiene + test_guard_launch_monitor 368 OK, matching the claim exactly; run-ruff.sh check plugins/disk-hygiene clean; all four changelog-parity modes exit 0.

Generated by Claude Code

@kyle-sexton kyle-sexton mentioned this pull request Sep 5, 2026
Closed
@claude

claude Bot commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 52s —— View job

Security review complete

  • Gather context (PR diff, changed files)
  • Run skip-gate check
  • Review destructive_guard.py changes for guard-bypass risk
  • Review hygiene.py changes for injection / data-exposure risk
  • Review test file for security-relevant coverage
  • Post findings

No security issues found.

This PR touches one security-relevant file, destructive_guard.py, the Bash allowlist that gates the destructive-engine invocations. The change generalizes the existing single-flag "at most one valueless flag" check into a loop over ("--confirmed-large-scan", "--quiet") (destructive_guard.py#L886-L899):

  • The "occurs at most once, no trailing value" invariant is preserved per-flag, and repeats (--quiet --quiet), attached values (--quiet=1), short forms (-q), or stray values (--quiet v) all still fall through to the strict remaining length/pair check and get denied — confirmed by tracing the logic and cross-checking against test_guard_scan_accepts_single_quiet_flag.
  • --quiet is valueless and shapes stdout only; it does not touch path resolution, --data-root authorization, or the apply/--execute path, so it cannot widen what a guarded invocation can reach or delete. No pipe, redirect, or shell-operator allowance is added — the fail-closed _literal_shell_words gate is untouched.
  • scan_stdout_payload in hygiene.py (hygiene.py#L179-L193) is a pure dict-filter with no path/command construction; it only ever removes a key from what's already computed, never adds attacker-influenced content to output, and never touches what's written to the snapshot file.

I did note a pre-existing (not introduced by this PR) parsing quirk in the same function: since valueless flags are stripped by optionals.remove(...) before the flag/value pairs are parsed, a value token that happens to collide with a flag name (e.g. --policy --confirmed-large-scan somefile) gets mis-attributed. This PR extends that same quirk to --quiet, but since --quiet carries no capability, the worst case is stdout shaped unexpectedly — not a privilege or path escalation. Not flagging it as a finding on this PR since it predates this change and the new flag doesn't add exploitable severity to it.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Last security-reviewed head: 819a6ee1fca62d13ab3ad5842c147a056a378668. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.

chatgpt-codex-connector[bot]
chatgpt-codex-connector Bot reviewed Sep 5, 2026
View reviewed changes

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 819a6ee1fc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/disk-hygiene/skills/clean/scripts/hygiene.py Outdated
Comment thread plugins/disk-hygiene/skills/clean/reference/safety-model.md
@kyle-sexton kyle-sexton mentioned this pull request Sep 6, 2026
Open
@claude
Merge remote-tracking branch 'origin/main' into claude/3352-scan-verb…
acd1f88 
…osity

# Conflicts:
#	plugins/disk-hygiene/.claude-plugin/plugin.json
#	plugins/disk-hygiene/CHANGELOG.md
@kyle-sexton Claude

kyle-sexton commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Merge lane claiming this PR at head 819a6ee1fca62d13ab3ad5842c147a056a378668 (no activity since 2026-09-05T22:56Z).

mergeable_state=dirty against a main that has advanced. Two files conflicted, and the resolution needed no judgement call about precedence: this branch claims the minor bump 0.22.0 for the new scan --quiet flag, while main published the patch 0.21.9 (the machine-specific-path placeholder change). 0.22.0 already sorts above 0.21.9, so neither entry is renumbered and neither is reworded — main's section is inserted below this branch's, and the manifest keeps 0.22.0.

Verified after resolving: no conflict markers; check-changelog-parity.sh --check, --check-bump origin/main and --check-preserved origin/main all pass, the last comparing 99 headings with none dropped.

One test note, recorded rather than acted on. scripts/affected-tests.sh --run reports two failures in plugins/claude-ops/skills/plugins/scripts/cache-content-check.test.sh ("the trace probe actually counted something" / "a one-install report costs at most 26 process creations", both measured -1). They are not this branch's: this PR touches plugins/disk-hygiene only, and the same two cases fail identically on a worktree carrying neither change. The probe's own detail line names the cause — "the pid-stamped PS4 did not reach the traced shell, so the ceiling below is vacuous" — which is a property of this sandbox's shell, not of the code under test. CI's own lint/test-linux lanes are green on this suite, so it belongs to the environment.

Generated by Claude Code

This was referenced Sep 6, 2026
Closed
Closed
Merged
@claude
fix(disk-hygiene): keep the root-children coverage warning under scan…
d38bce1 
… --quiet

Addresses the two P2 review findings on this PR, both verified against the
branch code before fixing.

1. `scan_stdout_payload` replaced `note` unconditionally, so
   `--root-children --quiet` lost the mode's coverage qualification: that the
   volume root and every skipped OS-owned/hidden/system/reparse entry were
   never walked, and that `children_rollup` therefore covers the selected
   children only. Nothing else on stdout encodes that limit. The skipped
   entries live in the snapshot's `root_children_skipped` alone, and
   `truncated_paths` does not stand in for them, so the generic quiet note was
   dropping a fact rather than a duplicate. Root-children mode now has its own
   quiet note that keeps the coverage sentence and drops only the rollup prose.

2. The root-children stdout payload omitted `empty_directory_count`, which
   `safety-model.md` documents as part of the quiet field set. A consumer
   following that contract broke on this mode. The field is present in the
   snapshot for both modes, so it is now reported on stdout for both.

safety-model.md states both, replacing the previous claim that the field set
applied uniformly.

test_hygiene.py's root-children quiet case asserted `QUIET_SCAN_NOTE`, which
pinned the defect. It now pins the corrected contract: the root-children note,
that it differs from the ordinary one, that it still names the never-walked
entries and `root_children_skipped`, and that `empty_directory_count` is
present.

Verification: `python -m pytest -q test_hygiene.py` 345 passed, 132 subtests
passed. run-ruff.sh check: all checks passed. check-changelog-parity.sh and
check-purged-em-dashes.sh both pass. 0.22.0 is unreleased (main is 0.21.9), so
the entry is extended rather than bumped.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VPLatLkg4329L8eyfxhuMa
@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Lane babysit-loop, instance ccr-session-babysit-loop-20260905: both P2 findings verified against the branch code, both VALID, both fixed. Head moves acd1f88c -> d38bce18.

# Finding Disposition
1 Preserve the root-children coverage warning in quiet output (hygiene.py:151) VALID, fixed
2 Qualify the quiet field contract for root-children scans (safety-model.md:451) VALID, fixed

Finding 1 confirmed. scan_stdout_payload did trimmed["note"] = QUIET_SCAN_NOTE unconditionally. The root-children scan-complete payload is the one that carries the coverage qualification (the volume root and every skipped OS-owned/hidden/system/reparse entry were never walked, so children_rollup covers the selected children only), and quieting replaced it wholesale. The reviewer's reason for calling it material also checks out: root_children_skipped is written to the snapshot only, and truncated_paths does not represent those entries, so a caller reading quiet stdout had no remaining signal that the inventory is partial by construction. That made the generic note a dropped fact rather than a dropped duplicate, which is the one thing --quiet is documented not to do.

Fixed with a QUIET_ROOT_CHILDREN_SCAN_NOTE selected on root_children_mode: it keeps the coverage sentence, names root_children_skipped as the snapshot field to read, and drops only the rollup prose.

Finding 2 confirmed. The root-children stdout payload did not build empty_directory_count, while safety-model.md listed it in the set --quiet keeps. The ordinary scan payload has it; the root-children one did not. scan_tree records the field for both modes, so the snapshot always had it and only the stdout projection was missing it. Fixed by reporting it in the root-children payload, which is the branch of the reviewer's either/or that keeps one contract instead of splitting it per mode. safety-model.md now states both the shared field set and why root-children mode has its own quiet note.

One thing worth flagging beyond the two findings. test_hygiene.py::test_root_children_scan_honours_quiet_without_losing_the_snapshot asserted hygiene.QUIET_SCAN_NOTE for the root-children quiet payload, so the defect was pinned by a passing test. That is why CI was green with the bug present. The case now asserts the corrected contract: the root-children note, that it differs from the ordinary note, that it still names the never-walked entries and root_children_skipped, and that empty_directory_count is present.

Verification: pytest -q test_hygiene.py 345 passed / 132 subtests (was 344 + the one that pinned the defect); run-ruff.sh check clean; check-changelog-parity.sh and check-purged-em-dashes.sh pass. 0.22.0 is unreleased (main is at 0.21.9), so the CHANGELOG entry is extended rather than a new version bumped.

Threads left open, and why. I cannot resolve them: this session is served only a pinned subset of GraphQL PR-review operations and reviewThreads returns HTTP 403, so the resolve wrapper has no reachable mutation. main requires required_review_thread_resolution, so this PR stays blocked until a human resolves the two threads. Both findings are addressed in d38bce18; nothing is waiting on further code. Releasing the claim.

Generated by Claude Code

@claude claude Bot mentioned this pull request Sep 6, 2026
Open
@kyle-sexton Claude

kyle-sexton commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Lane babysit-loop, instance ccr-session-babysit-loop-20260906, pinned to head d38bce18. blocked -> clean. No code change was needed; the block was two unresolved review threads whose findings had already been fixed on the branch.

What was actually blocking

mergeable=true, every check green (ci-lanes: success, no failing check runs), and no CHANGES_REQUESTED review. The two open chatgpt-codex-connector P2 threads were the whole of it, and both predate the fix commit d38bce18 (review posted 2026-09-05T22:56Z, fix pushed 2026-09-06T10:46Z).

I verified each against the code at d38bce18 before touching it, rather than resolving to clear the gate:

1. Root-children coverage warning under --quiet — fixed. scan_stdout_payload no longer replaces the note unconditionally; root-children mode selects QUIET_ROOT_CHILDREN_SCAN_NOTE, which retains the coverage qualification verbatim (volume root and skipped OS-owned/hidden/system/reparse entries never walked; root_children_skipped lives in the snapshot; not represented in truncated_paths). The constant carries a comment explaining why it is not a droppable duplicate.

2. empty_directory_count in the root-children quiet contract — fixed, by the stronger of the two options the reviewer offered. The field is emitted in the root-children stdout payload, and since --quiet removes only children_rollup, it survives quiet in that mode. safety-model.md now states it outright: "That field set holds in --root-children mode too, which reports empty_directory_count on stdout for the same reason an ordinary scan does."

Replied in each thread with this evidence, then resolved both. mergeable_state moved blocked -> clean immediately afterward.

Verification

  • Clean-skill suite: 368 passed, 132 subtests passed.
  • scripts/affected-tests.sh --run: two failures, neither branch-owned — both reproduce identically on unmodified origin/main in this container:
    • block-hook-bypass.test.sh symlink temp-write case;
    • cache-content-check.test.sh strace process-budget probe (the trace probe actually counted something), which the sandbox cannot instrument.

Lane action: advanced to clean, merge not completed. The gh merge gate cannot read thread-resolution state under this session's pinned-GraphQL restriction, so it cannot produce its own readiness verdict; that is the known limitation, not a new finding. This PR is now the queue's most merge-ready candidate. Claim released.

Generated by Claude Code

This was referenced Sep 7, 2026
Merged
Merged
Merged
@kyle-sexton
kyle-sexton merged commit 69fe121 into main Sep 7, 2026
12 checks passed
@kyle-sexton
kyle-sexton deleted the claude/3352-scan-verbosity branch September 7, 2026 08:45
kyle-sexton added a commit that referenced this pull request Sep 7, 2026
@kyle-sexton @claude
@cursoragent ksextonmelodic
feat(disk-hygiene): record guard decisions locally instead of discard…
e029378 
…ing them when no telemetry sink is set (#3913)

Closes #3862

## Summary

Guard decisions left the process only through `HOOK_TELEMETRY_SINK`,
which is inert unless an environment variable names an executable. On an
ordinary install every decision was discarded as it was made, so "why
was this denied", "has it denied this all along", and "did the guard run
at all" had no evidence to answer from.

New `plugins/disk-hygiene/lib/guard_decision_log.py` appends one JSON
object per line to
`<CLAUDE_PLUGIN_DATA>/guard-decisions/decisions.jsonl`, default on, no
configuration. `destructive_guard.py` records every branch that reaches
a verdict; `guard_launch_monitor.py` records the did-not-run state the
guard structurally cannot write about itself.

Version taken: **0.23.0**. Verified free against current `main`
(`6db96637d`, 0.21.9) and against the head of every open PR at the
moment of opening: #3880 claims 0.21.10, #3783 claims 0.22.0, and
#3851/#3887/#3900/#3779 leave the manifest at 0.21.9. 0.23.0 is strictly
greater than all of them, so it cannot collide under any merge order;
the 0.22.x gap is what `--check-order` explicitly reads as correctly
ordered.

## Fix

**Where the record lives, and why it survives.** Under the plugin's own
persistent data root, resolved by the guard's existing
`resolve_authorized_data_root()` (the `--authorized-data-root` /
`--plugin-root` / `CLAUDE_PLUGIN_DATA` ladder) beside the run records
already kept there. Because this plugin is the one that deletes things,
the filename was checked against the engine's own discovery hints in
`reference/baseline-policy.json`: `decisions.jsonl` and
`decisions.previous.jsonl` match none of the 14 bundled name globs
(`*.tmp`, `tmp-*`, `tmp_*`, `scratch*`, `*.lock`, `__pycache__`,
`*.partial`, `*.crdownload`, `*.tmp.*`, `.claude.json.tmp.*`,
`temp_git_*`, `.pulumi-write-test-*`, `.DS_Store`, `Thumbs.db`), so the
plugin's own hints never nominate its audit trail. Appending directly
rather than writing a temp file and renaming is deliberate for the same
reason: an atomic-write staging name would land on `*.tmp.*`, which is a
bundled hint.

**What is recorded.** `schema_version`, `timestamp` (UTC, milliseconds),
`hook`, `decision`, `rule`, `tool`, `mode`, `command`, `reason`.
`decision` is `allow` / `ask` / `deny` / `none` (ran, issued no
`permissionDecision`) / `not-run`. `rule` names the branch that fired,
so `kill-switch-disabled-apply` is distinguishable from
`not-exact-engine-command`: two different answers to "why". `command` is
the input that drove it and `reason` is the exact text the host was
given, so the record and the host cannot disagree. Both are clipped to
400 characters, which keeps it a record of the decision rather than a
copy of the payload and keeps every line short enough that concurrent
hook processes appending to the same file do not interleave.

**Bounded, enforced.** The live file rotates to
`decisions.previous.jsonl` at 1 MiB via `os.replace`, so the record
occupies at most about 2 MiB forever with no operator pruning. The bound
is checked from the offset the append already returns (`handle.tell()`
in append mode), so enforcing it costs no extra syscall.

**Cost.** Measured with `strace -f -e
trace=clone,clone3,fork,vfork,execve,openat,write` against a detached
worktree of `origin/main` at `6db96637d`, five invocations per arm plus
a warm-path detail run:

| Path | Before | After |
| --- | --- | --- |
| defer (a Bash command not naming the engine, the always-on branch) | 1
`execve`, 1 `clone3` | 1 `execve`, 1 `clone3`, 0 record syscalls |
| decision (deny), warm data root | 1 `execve`, 1 `clone3` | 1 `execve`,
1 `clone3`, 1 `openat` + 1 `write` |
| decision (deny), first write of an install | 1 `execve`, 1 `clone3` |
plus 1 failed `openat` and 1 `mkdir` |

The single `clone3` is `CLONE_THREAD`, the existing watchdog thread, not
a process. **The process and exec census is unchanged on every path.**
The plugin-level defer branch, which is what this always-on hook takes
for work unrelated to disk-hygiene, writes nothing at all and is
byte-for-byte the path it was. Wall clock over 40 invocations per arm,
alternated twice, moved inside run-to-run noise on this host (52 to 58
ms both before and after, the sign of the difference changing between
repetitions), which is why the syscall census rather than a duration is
the figure cited.

**Failure behavior: the verdict never changes.** Two boundaries, both
load-bearing. `guard_decision_log.record` returns a bool and catches
`BaseException` around the whole write. `_record_decision` in the guard
wraps its own call, because the data root and mode are resolved in the
argument list, outside `record`'s protection, and one of its call sites
is `main`'s own `except BaseException` handler, where a raise would
reach the interpreter's default handler: exit 1, which PreToolUse treats
as non-blocking, so the command the guard just denied would run. Every
record call is made after the verdict has been emitted, and its result
is discarded.

**What is deliberately not recorded.** The plugin-level defer (hot path,
and not a decision anyone reconstructs later). The watchdog expiry path:
that callback runs while the main thread is presumed wedged inside a
filesystem call and stays syscall-free for exactly that reason, so a
write there could hang on the same filesystem. Both are stated in the
README rather than left implicit.

**Adjacency, stayed out of.** #3861 (the guard's fail-open when no
interpreter resolves) is `needs-human`. This change touches the guard's
decision branches but not interpreter resolution, and adds no new
fail-open path; the `not-run` record makes the fail-open class more
visible after the fact without adjudicating it.

**Escape hatch.** `DISK_HYGIENE_GUARD_DECISION_LOG` set to `0` / `off` /
`false` / `no` turns the record off. Opt-out, not opt-in: any other
value, including an absent one, records.

## Verification

All runs local and in the foreground; draft CI is not cited as test
evidence.

- `bash scripts/affected-tests.sh --run --shard N/4`: shards 0, 1, 2
exit **3** (success, with `NOT RUN` non-shell ecosystems), 0 `FAIL`
lines each. Shard 3 exits 1 with exactly three `FAIL` lines, all from
`plugins/claude-ops/skills/plugins/scripts/cache-content-check.test.sh`
(`process budget: the trace probe actually counted something`, `process
budget: a one-install report costs at most 26 process creations`).
**Reproduced unchanged on a clean detached worktree of `origin/main` at
`6db96637d`**: same suite, same 2 cases, exit 1. Pre-existing, not this
change. Every changed file maps to at least one suite; the three
docs/manifest files resolve through the recorded no-suite allowlist.
- Direct suites: `hygiene.test.sh` RC=0 (350 cases),
`guard_launch_monitor.test.sh` RC=0 (28 cases),
`run-python-hook.test.sh` RC=0, `test_guard_decision_log.py` +
`test_hook_telemetry.py` RC=0 (15 new cases).
- All four parity modes green: `--check`, `--check-order`, `--check-bump
origin/main`, `--check-preserved origin/main`.
- `scripts/run-ruff.sh check plugins/disk-hygiene`: all checks passed.
`format --check`: my four touched/new Python files are clean. Five files
remain unformatted in this plugin (`killswitch_config.py`, `hygiene.py`,
`guard_launch_monitor.py:137`, `test_hygiene.py:2373`,
`test_kill_switch_probe.py:96`); all five are identically unformatted on
`origin/main`, so none is introduced here.
- Gates run clean: `check-purged-em-dashes.sh`,
`check-drive-root-litter.sh`, `check-silent-skips.sh`,
`check-discriminating-test-skips.sh`, `check-fixture-git-isolation.sh`,
`check-hook-exec-form.sh`, `check-killswitch-hoist.sh`, all RC=0. New
test file committed `100755` (verified with `git ls-tree`), the new
library `100644` matching its sibling `hook_telemetry.py`.
- No shell files changed, so shellcheck and shfmt have nothing to say
about this diff. `lib/hook-utils.sh` untouched.

**Mutation proof (the new assertions discriminate).** Nine mutations
applied one group at a time, each reverted:

| Mutation | Caught by |
| --- | --- |
| rotation call disabled | 3 lib cases (`rotates_at_the_bound`,
`discards_only_the_generation_before_last`, `a_failing_rotation...`) |
| `_clip` returns text unchanged |
`long_command_and_reason_are_truncated` |
| `enabled()` hardcoded True | 5 lib subtests +
`the_record_can_be_turned_off_without_changing_a_verdict` |
| deny rule string collapsed onto the kill-switch rule |
`denied_engine_command_is_recorded_with_its_rule_and_input` |
| a record added on the defer path | `engine_gate_defer_records_nothing`
|
| `_record_decision`'s try/except removed |
`a_broken_decision_record_never_changes_a_verdict` (record raises),
`record_decision_swallows_a_failure_in_data_root_resolution`, and the
**pre-existing**
`every_call_graph_function_failure_denies_at_exit_2_never_1` for both
`resolve_mode` and `resolve_authorized_data_root` |
| `_record_not_run` call removed from the monitor | 3 monitor cases |

The write-failure proof is
`test_a_broken_decision_record_never_changes_a_verdict`, which drives
all five verdict shapes (`allow`, `ask`, `deny`-by-authority,
`deny`-by-kill-switch, and the no-output defer) three times:
unsabotaged, against a data root whose parent is a regular file (a real
filesystem `OSError` on both the append and the `mkdir` behind it), and
with `record` raising `RuntimeError`. All three runs produce the
identical verdict list, and the unwritable root is asserted to still not
exist afterwards.
`test_a_write_failure_leaves_the_deny_exit_status_untouched` pins the
same thing end to end through `main`.

**Hermeticity fix included.** `run_guard_engine_gate` previously passed
`SCRIPT_DIR / "data-root"` as the authorized data root. With records
being written, that would have littered the checkout on every test run,
so it now takes a per-test temp path, and `GuardTests.setUp` pops an
inherited `CLAUDE_PLUGIN_DATA` so a developer's real plugin data
directory is never written to by the suite.

## Related

- Closes #3862; parent #3347 finding F12.
- Sibling #3861 (guard fail-open when no interpreter resolves) is
`needs-human` and deliberately untouched; the `not-run` record is what
makes that class visible after the fact.
- Sibling finding on a false denial: `rule` plus `command` plus `reason`
is what answers it from the record instead of by reproduction.
- Hook budget convention: `docs/conventions/hook-budget/README.md`,
`.claude/rules/hook-budget.md`. Measured share stated in the plugin
README's trust-surface record.
- Version contention checked against open PRs #3783 (0.22.0) and #3880
(0.21.10).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob

---
_Generated by [Claude
Code](https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob)_

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: ksextonmelodic <ksextonmelodic@gmail.com>
@claude claude Bot mentioned this pull request Sep 11, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

disk-hygiene: scan has no output-verbosity flag (~63,000 tokens of children_rollup in one run)

2 participants

@kyle-sexton @claude