Repository navigation
ci(hooks): warn when either disk this project fills is running low - #339
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
|
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its |
A fan-out of eleven per-language demo agents took the host volume from ample to
8.8 GiB free of 926 GiB in about an hour, and took the Docker VM's virtual disk
with it. One agent's build died outright; two others pruned under each other.
Nothing warned, because the session had started with plenty of room - and a
SessionStart check would have reported all clear for exactly that reason.
The only instant that can see what the last command left behind is just before
the next one. So this is a PreToolUse hook on Bash, and it never blocks: a disk
warner that blocked Bash on a full disk would remove the very commands needed to
clear the disk. The worst case is asserted directly.
No `if` filter, for the same reason as the squash guard - `Bash(docker *)` would
miss `cd demo && docker compose up` and every wrapper script, which is most of
how containers actually get built here. It earns always-on by costing seven forks
at 8-10ms with no docker CLI on the fast path, and by saying nothing unless a
threshold trips, then at most once per ten minutes unless the band worsens.
Docker Desktop's disk image is a high-water mark: a sparse file that grows and
never shrinks, so pruning 17 GB does not shrink it by a byte. A cheap always-on
trigger is confirmed against a cached `docker system df` before anything is said,
and that correction applies only to the sparse-image reading, never to Linux's
live filesystem one.
Review hardened it substantially. Those fixes are folded in here rather than left
as later commits, so no point in the history carries the vulnerable version:
- The throttle stamp lives in shared /tmp and was read back into
$((now - last_at)). Bash arithmetic resolves a non-numeric operand as a
variable name, recursively, so a crafted stamp ran a command as the user.
- "Never exits non-zero" was false: any non-numeric first token aborted under
set -u with exit 1, taking the Bash call away - likeliest exactly when the
disk is full.
- mkdir -p succeeds against a directory another user owns, so the state dir was
never really checked, and a symlinked last-warning gave arbitrary file
truncation. Now umask 077 plus an ownership test, both builtins.
- A device name containing a space - macOS autofs `map auto_home` - shifted
df's columns so $4 was Used, reported as free: 94 GiB "free" on a disk with
1. Now anchored on the POSIX capacity column.
- A fully pruned Docker could never clear its own alarm: the correction fired
only when the total was above zero, conflating "holds nothing" with "could
not read".
- A torn docker-df cache read undercounts, producing exactly the false "fine"
this hook exists to prevent. Now a temp file and an atomic rename.
- A future-dated stamp - an NTP step, a resumed VM - made the age negative and
silenced the warner until the clock caught up.
The self-test is the interesting part, because this hook has the failure mode
every warn-only hook shares: its correct behaviour on a healthy machine is to
print nothing, which is byte-identical to being broken, unregistered, or absent.
So almost every case forces it to speak, and the one case asserting silence is
paired with forced cases proving the same call path can be made to talk. Every
case pins all four thresholds rather than inheriting them - an earlier version
left the host thresholds at their defaults and three cases flipped to failing
mid-session when the machine dropped below the 25 GiB warn line. A self-test for
a disk warner must not be a function of the disk.
Each fix above is paired with the mutant that kills it: twelve mutants injected,
the hook restored byte-identical each time, every fix killed by exactly the case
that should kill it.
docs/agent-harness.md now asserts the registered-hook count rather than stating
it. It said six against eight registered, having drifted twice.
Pre-existing, and nothing to do with the disk warner: the push-reminder section
cleaned up throttle stamps with `rm -f "${TMPDIR:-/tmp}"/pc-push-reminder-*` and
let the hook write them to that same shared location. Two copies of the suite
running at once therefore deleted each other's stamps mid-run.
Measured on a pristine master worktree: run serially it is green - 154 ok, 0
failures, five runs out of five. Run four at a time in one directory and all four
fail. That is why the flake stayed invisible, and it is why this PR's own
description claims 14 failures "byte-identical to the set a pristine master
worktree reports": two contaminated runs were being compared to each other. The
description needs correcting before merge.
The hook already honours TMPDIR, so this is a test-only change - the section gets
its own directory and stops competing for a machine-global name. Four concurrent
runs: 4/4 failed before, 0/4 after.
A second concurrency cause remains, in the merge-outstanding section, which
writes a note into the real working tree. That one is fixed on
#350 and is deliberately left alone here rather than
have two branches edit the same lines.
The review found and fixed a P0 - a throttle stamp in shared /tmp read back into bash arithmetic, which resolves a non-numeric operand as a variable name recursively, so a crafted stamp ran a command as the user. What is left needs decisions rather than fixes. The one that matters: the throttle is keyed per-UID and therefore shared across sessions, so in this PR's own motivating incident only the first of eleven agents would have been warned. A disk warner that goes quiet for everyone else is the failure it was written to prevent. Also records that the body's 14-failure verification claim is false - pristine master is green when measured serially - and that the BSD paths have still never run on BSD.
f029bf7 to
8a50303
Compare
|
@claude review this |
|
Claude finished @astubbs's task in 4m 7s —— View job Review
Environment note (not a PR defect)My checkout has FindingsRead the hook, the diff against
No correctness issues found in the parts I could statically verify. The design is unusually |
…merge `bin/check-branch-self-reference.sh`, added to master in da91f3f after this branch was cut, failed the `inflight: tags` lane on two self-references in `docs/inflight/pr-339-review-findings.md`. The gate's contract is not "do not cite your own PR" - PRs are permanent - but "write the mention as it will read AFTER this lands", then attest to that judgement with a marker. Rewriting for that turned up a section that was already stale, which is the gate earning its keep rather than a formality: - "The PR body's verification claim is false ... Correct the body before merge" was an instruction to the author that had already been carried out. It is now a durable caveat about the measurement instead: the suite's failure count is only meaningful measured serially, and an earlier 14-failures comparison was wrong because both of its runs were contaminated by concurrent builds. - "on this branch" / "fixed here" become #339 in the past tense, so the sentences survive the branch being deleted. - The BSD section pointed at a session scratchpad for the staged macOS commands. That scratchpad no longer exists, so the note now says a macOS run starts from scratch rather than implying the work is half-done somewhere. Four `post-merge: checked` markers, verified load-bearing by a red control: stripped, the gate reports 4 unconfirmed self-references and exits 1; restored, it exits 0. `bin/check-inflight-tags.sh` (90 notes valid), `bin/check-issue-refs.sh` and `bin/test-check-agent-hooks.sh` (189 ok, 0 failed, run serially) all stay green. The note keeps its `pr-` prefix and its #339 title, matching `pr-323-docs-outstanding.md` and `pr-324-tooling-outstanding.md`, which sit on master with their PRs merged.
|
@claude review this Head is now Worth steering at, since the last pass could not execute anything in its sandbox:
|
|
Claude finished @astubbs's task in 2m 29s —— View job Review
FindingsHead 1. The four rewritten self-references — checked against
No unmarked self-reference slipped through either: a grep for generic phrasing ( 2. Nothing else stale. The only instruction-shaped sentence in the old note was "Correct the body before merge" under the false-verification-claim section; the PR body's own "Verification, with a control" section already carries that correction (the "An earlier version of this description claimed... That was wrong" paragraph), so the instruction was satisfied before this commit turned it into a durable caveat. The "Never verified against BSD userland" section changed too (scratchpad no longer exists) but never named the branch/PR, so it wasn't gated — and it reads as a standing fact either way. The remaining sections ("Smaller, still open", "Worth keeping") are findings, not instructions, and needed no rewrite. 3. No new issues. Agrees with the prior pass's "lgtm" and with the steer's own conclusions on all four points — this is a confirmation pass, not a correction. |
|
Babysit pass on One correction to the review above
That quote is the header of
The conclusion still holds, but for a different reason: The contention evidence itself is unchanged and reproducible: on the previous head, Verification the review sandbox could not runThe last two passes both noted they could not execute the self-test. Run here, serially:
Same-defect-class sweepDefect class: untrusted content read from a file reaching bash arithmetic, where a non-numeric operand resolves as a variable name recursively.
So: none found beyond the one fixed. Reading a file's contents into arithmetic happened only at the Unrelated and pre-existing, not touched here: Left for youThe PR is still a draft, so it cannot be merged as-is — marking it ready is your call, not something I should flip. |
The findings note recorded the shared-across-sessions throttle as open but not who was blocked on what, so it read as unowned work rather than a pending call. Keying the stamp on `session_id` instead of the UID is a product decision about how far a per-session warner should isolate itself - Antony's, not a review pass's. Says so now, so the next reader does not re-litigate it or quietly implement one answer. No behaviour change; the note is the only file touched.
Only ONE textual conflict, and three things that merged CLEANLY while becoming false - which is the part worth reading, because nothing goes red for those. CONFLICT: `.claude/settings.json`. Both sides appended a `PreToolUse` hook into the same array slot - this branch's `warn-low-disk.sh` and master's `check-shallow-history.sh` (3d51667). Resolved by keeping BOTH, as two separate matcher entries, master's first so its slot is unmoved. Neither carries an `if` filter, which is deliberate on both sides. CLEANLY MERGED BUT WRONG, all in `docs/agent-harness.md`, all fixed here: - It said the file registers "eight hooks". Master's ninth landed in a region this branch never touched, so the count merged without a murmur. It is now "nine", and `bin/test-check-agent-hooks.sh` was already asserting that number against `settings.json` - so this one WOULD have gone red, which is exactly why that assertion exists. - It called `check-history-rewrite.sh` "the one guard here that refuses". `check-shallow-history.sh` also refuses, so the sentence became false on arrival. Reworded to name both. - Master registered its hook without adding it to that doc's hook list, leaving the list one short of the count it states. Documented, pointing at the hook's own header for the reasoning, per the convention the neighbouring entries use. THE MERGE FOUND A REAL GAP IN THIS BRANCH'S META-TEST. This PR added "every registered hook is self-tested", which read coverage as "has a `$HOOKS/<name>` section in `bin/test-check-agent-hooks.sh`". Master's hook is self-tested by a DEDICATED `bin/test-check-shallow-history.sh`, so a genuinely tested hook was reported untested. That is the assertion being narrower than the property: since 1078acd, `bin/check-all.sh --with-tests` globs `bin/test-*.sh`, so a standalone self-test runs in CI with no wiring and is equally real coverage. It now accepts both shapes - and deliberately NOT a leading-comment mention, because `# Self-test for .claude/hooks/foo.sh` is prose and accepting it would let a hook buy coverage with a sentence. Two red controls, both fire: registering a hook nothing tests reports it untested (and the count arm fires too), and pointing the standalone self-test at another path re-reds the exact case that started this. Both fixtures restored byte-identically, verified by sha256. MASTER'S NEW `check-shell-hazards.sh` GATE CAUGHT THIS BRANCH'S DELIVERABLE. `.claude/hooks/warn-low-disk.sh` uses `stat -f` and `stat -c` in a platform `case`, which the gate flags because `stat -f` on GNU exits 1 while PRINTING filesystem prose to stdout. Both uses are correct - the `case` has already dispatched on `REAL_UNAME`, which is the un-injectable reading kept separate from `OS_NAME` for precisely this - so they carry the `hazard-ok:` declaration the gate prescribes, on the immediately preceding line, which is the only place it reads. `bin/check-shell-lint.sh` (ShellCheck, 406dc7d) had never run against this branch. Clean at the gated `error` floor. At `warning` it found one SC2155 in the new hook, which `docs/inflight/static-shell-lint-severity-tiers.md` ranks #1 on its work order as the cheapest floor to raise next - so it is split here rather than added to that pile. The hook is now clean at `warning`. `docs/inflight/pr-339-review-findings.md` renamed to `ci-low-disk-warner-open-items.md`. It kept the `pr-` prefix on the stated grounds that `pr-323-docs-outstanding.md` and `pr-324-tooling-outstanding.md` sat on master with their PRs merged - and master has since deleted BOTH (65cf9ec), relocating their survivors to area prefixes for the reason recorded there. The precedent that decision rested on is gone, and the four sibling PRs all renamed, so this follows. Title now names the subject rather than the PR number, matching `test-lincheck-lane-open-items.md`. Nothing cited the old path. STILL OPEN AND UNCHANGED: the throttle stamp is keyed per-UID, so it is shared across sessions and only the first of eleven agents would have been warned in this hook's own motivating incident. Fixing it means keying on `session_id`, which is Antony's call, and the note still records it as open and blocked. VERIFICATION. `bin/check-all.sh --with-tests`: 38 ran, 38 passed, 0 failed, 0 could not run (5 PR-scoped gates skipped by design). Hook suite serially: 194 ok, 0 failures. The private-`TMPDIR` fix survived - master never touched `bin/test-check-agent-hooks.sh`, so there was no conflict - and it is re-proven against a pristine-master control worktree at f7e58d7, four concurrent copies each side. Master: 4/4 runs red, with push-reminder-section failures in three of them. This branch: ZERO push-reminder failures in 4/4 runs. The residual red on two of four is `the open item is quoted into the deny reason`, which is the merge-outstanding section writing its fixture into the real working tree - a different, pre-existing cause owned by #350, outside every hunk this branch wrote, and one master fails on too. Re-run serially it is green, which is how it was confirmed as contention rather than a defect. NOTE FOR WHOEVER MERGES NEXT: the shared clone re-shallowed itself three times during this work - the `shallow` file lives in `--git-common-dir`, so one sibling agent's depth-limited fetch re-shallows every worktree. While shallow, `git merge-base` returns EMPTY and ahead-counts read in the hundreds. All figures here were taken with `git rev-parse --is-shallow-repository` asserted false on both sides of the measurement, and match ground truth: 827 commits reachable from master, behind 22, ahead 6, merge-base cf0007d. Master's new `check-shallow-history.sh` is the guard for exactly this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eleven commits, merge-base f7e58d7. One textual conflict, plus two things master brought down that change what this branch can claim. CONFLICT: docs/agent-harness.md, in the settings.json hook list. Both sides rewrote the same bullet's tail. Master's #353 corrected a false claim about check-squash-subject.sh - the file said the hook matches `gh` in COMMAND POSITION, and running it showed it matches a regex over the raw command, so an `echo` naming a subject-overriding merge is denied. This branch had appended its own bullet for warn-low-disk.sh immediately after that tail. Resolved by taking BOTH: master's corrected paragraph replaces the wrong claim, and this branch's warn-low-disk.sh bullet follows it unchanged. Taking either side alone would have dropped the other's whole contribution. bin/test-check-agent-hooks.sh auto-merged, and the auto-merge is correct: master added 35 cases to the pre-commit-gate section (keywords, multi-line payloads, the `function` spelling), this branch added its cases further down, and the private-TMPDIR fix in the push-reminder section is untouched. Verified by running it rather than by reading the diff. The hook-count assertion did not fire. #353 registered no new hook, so settings.json still holds nine and the prose still says nine. WHAT MASTER CHANGED ABOUT THIS BRANCH'S CLAIMS 11c6551 added a macOS lane that runs every bin/test-*.sh under BSD userland and bash 3.2. This branch's deliverable is a shell hook with `stat -f`/`stat -c` platform branching whose BSD arm the inflight note records as never having run, so that gap now has a CI answer for the first time. Recorded in the note. 3a98b8d/957d08423 added a shared-git-state hazard row and taught check-shell-hazards.sh to join backslash continuations and to see wrapper command words. The two `hazard-ok:` declarations on warn-low-disk.sh's platform `case` still satisfy it - the gate passes. VERIFICATION ON THE MERGED TREE bin/check-all.sh --with-tests 39 ran, 37 passed, 0 failed, 2 could not run (check-shell-lint.sh and its self-test: ShellCheck is not installed here) shellcheck run through the cached koalaman/shellcheck image with the lane's own command over all 76 tracked scripts: clean at the `error` floor, and clean at `warning` for this branch's files check-branch-self-reference.sh green with PR_NUMBER=339 and GITHUB_HEAD_REF=ci/warn-low-disk-hook, which is the only way it checks anything hook suite, serial 208 ok, 0 failures (master alone: 173 ok, 0 failures - the same 35-case delta as before) CONCURRENCY, MEASURED AGAINST A PRISTINE-MASTER CONTROL Four concurrent copies, this branch: 3/4 red, all on one arm. Four concurrent copies of a detached origin/master worktree: 4/4 red, on that same arm AND on five push-reminder assertions. So the private-TMPDIR fix survived the merge and still works - zero push-reminder failures on this branch against five on master. The arm that remains red is check-merge-outstanding-work.sh's note fixture, which writes docs/inflight/pr-90001-selftest.md into the real working tree at a fixed path; it is pre-existing on master, untouched by this branch, and already fixed on #350, which this branch's own inflight note and PR body both name as its owner. Deliberately not fixed here: two branches editing those lines is the conflict the note exists to prevent.
… of it Master's `shell: macos` job runs every `bin/test-*.sh` on macos-latest under BSD userland and a pinned bash 3.2, discovered by glob. That sweeps in the warner's self-test with no wiring, so `stat -f` and the `df -Pk` column read execute on a real BSD box for the first time - `REAL_UNAME` is deliberately un-injectable, so those are the genuine spellings rather than the Linux ones under a `PC_DISK_UNAME` costume. Recorded with its limit rather than as a tick: a hosted macOS runner has no `Docker.raw`, so the sparse-image high-water-mark correction is still unexercised. An open item whose status changes from "no coverage exists" to "partial coverage exists" is worth less than nothing if the note lets the next reader read it as closed.
…ships Master's new `shell: macos` lane went red on this branch the moment the merge landed, and the failure is this branch's, not the lane's. It is the first time this suite has been parsed by bash 3.2. WHAT BROKE. The registration meta-test ran python from a heredoc inside a command substitution: `$(python3 - <<REGCHECK ... )`. bash 3.2 scans a substitution for its closing paren treating the heredoc body as SHELL TEXT and does not recognise `#` comments while doing it, so an ordinary apostrophe in a python comment - a possessive, in a sentence about an earlier hook count - was the fifth and unbalancing single quote in that body. 3.2 then read to the end of the file looking for the close, and reported an unexpected EOF against the last line of a 1420-line file. WHY NOTHING CAUGHT IT. bash 5 parses the same text correctly, so every local run and every ubuntu lane was green. And the lane could not name the case: the suite died with exit 2 after some sixty cases had already passed, which check-all.sh and the macOS job both classify as CANNOT RUN. A gate that could not measure, wearing the message of a missing tool. THE FIX. Heredoc at top level into a file, then run the file. The body is never scanned as shell at all, so no later edit to the python can bring this back - a narrower fix that merely balanced the quotes would have left the next author one apostrophe away from the same red. The python itself is byte-for-byte unchanged. NOT A COUNTER-EXAMPLE: three python heredocs inside `$( ... )` survive on master in check-cve-exclusions.sh, check-ossindex-audit.sh and check-shell-hazards.sh. Their bodies happen to balance when read as shell. That is luck, not a contract, which is why bin/check-shell-hazards.sh is the right long-term home for the class - silent, version-dependent, invisible to ShellCheck, its own stated admission test. Not added here: a row would report those three, each in a shared file another branch may be editing, so it is queued in the inflight note instead. Verified: 208 ok, 0 failures serially; check-all.sh --with-tests 37 passed, 0 failed; ShellCheck clean at the error floor over all 76 tracked scripts and at the warning floor over this file, run through the cached image with the lane command. No heredoc operator remains inside any command substitution in the file.
…all to the operator The throttle stamp was one file per UID, shared by every concurrent session, so the first agent to notice a low disk silenced all the others for the full ten-minute window. In the incident this hook was written for - eleven per-language demo agents taking the host volume from ample to 8.8 GiB free in about an hour - ten of the eleven could not have been told, while they were the ten still filling the disk. Two reviewers flagged it independently and it was left open as a product call, because the obvious fix trades one failure for another: warn everybody and eleven agents may each start pruning. Settled the other way, and the message is what makes it safe. Every session is warned, and each agent is told to report the situation and suggest a reclaim rather than run one, so the operator is the gate against duplicate effort instead of the throttle. That cost is not hypothetical - the same incident had two agents pruning under each other while only one of them had been warned - and an operator can tell duplicate cleanups apart, which a ten-minute timer cannot. Keying is best-effort by design. `session_id` comes from the PreToolUse payload, read past every fast-path exit so the healthy case still forks nothing extra, and with no python3 on that path: the header's standing rule is that an interpreter must never become a precondition of the message. An absent, unparseable or session-less payload leaves the key empty and falls back to the shared stamp, which is precisely the old behaviour - the failure this file refuses is "warned" becoming "silent", and a coarser throttle is not that. The value reaches a file path, so `tr -cd` deletes anything outside `[A-Za-z0-9_-]` rather than escaping it, and the length is capped. Also fixed, the smaller half of the same finding: the stamp was written BEFORE the message was emitted, so a kill between the two swallowed that warning for the whole window - likeliest exactly when the disk is full, which is this hook's entire subject. Speaking first can at worst repeat a warning. No test can kill the hook mid-flight, so the order is asserted in the source, the way the suite already asserts structural properties whose behaviour has no observable difference until the moment it matters. Every new assertion was run against the unfixed hook first. The keying case reports the defect numerically - the second session received zero bytes - and both message cases and the ordering case fail too. The traversal case is NOT a red control and is marked as such: the old hook never read `session_id`, so nothing could have escaped; it guards only the new path. Closes the open item in the warner's inflight note, and one more that the rewording answers on its way past: the critical-band advice used to tell an agent to look for named volumes without saying what to do on finding one, which is now "tell the operator". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5LgTetnPkQciHFNaQWwSN
Three conflicts, all from #350 landing the branch-context hook into the same three files this branch touches. Resolutions, and why: `.claude/settings.json` - both sides appended a PreToolUse entry at the same position. Kept both, so `warn-low-disk.sh` (matcher `Bash`) and `inject-branch-context.sh` (matcher `*`) are each registered. Validated as JSON, and the registration self-test confirms all ten scripts are wired and covered. `bin/test-check-agent-hooks.sh` - both sides had independently given the push-reminder cases a private TMPDIR for the same reason, under different names. Took master's `PUSH_TMPDIR`, which is what the surviving declaration outside the conflict uses; this branch's `push_tmp` would have been an undefined reference under `set -u`. That left master's block and this branch's near-identical explanatory comment both present - one fact stated twice, which AGENTS.md forbids because the copies drift - so the duplicate is deleted and the one datum only it carried is grafted into the survivor: the specific case that failed, "an immediate second push is throttled" reporting `repeated`. `docs/agent-harness.md` - a genuine cross-branch interaction rather than a textual clash. This branch added a gate pinning the doc's hook count; master then reworded that same sentence to distinguish hook SCRIPTS from REGISTRATIONS, which the gate's regex could not match. Kept master's wording, since it explains why the count is stated at all and the two numbers legitimately differ - one script can be registered against several events - and widened the gate to parse both and verify both against `settings.json`. Verified by making each number wrong in turn and watching the gate name it. The doc now reads ten scripts across eleven registrations, counted from the merged file rather than by incrementing master's nine and ten. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5LgTetnPkQciHFNaQWwSN
#357 landed the push-drift hook and the shared `hook-common.sh`, touching the two files this branch also changes. Two conflicts, both additive: `docs/agent-harness.md` - master added bullets for `remind-master-drift-on-push.sh` and the shared library; this branch had added one for `warn-low-disk.sh`. Kept both sets, and placed master's push bullets immediately after `remind-inflight-on-push.sh` so its "same self-filtering shape" back-reference still lands on a push hook, and so the shared-library note follows both push hooks it describes. The count sentence is now eleven scripts across twelve registrations, recounted from the merged `settings.json` rather than incremented. `bin/test-check-agent-hooks.sh` - the header said "the eight hooks"; master dropped the number. Took master's wording. A count in unchecked prose is exactly what rots, and the counts that remain are the ones the registration gate verifies. KNOWN RED, and not from this merge: the registration gate this branch added reports `remind-master-drift-on-push.sh` as registered but not self-tested, and that is accurate - nothing under `bin/` references it. #357 tested the shared push detection through `remind-inflight-on-push.sh`'s fixture, deliberately and with a negative control, but the drift hook's own contract - fetch before reading, SHA-keyed throttle, subject listing, file-overlap reporting - has no coverage. Master's own copy of the suite even cites "the drift section" that was never added. Recorded here rather than worked around; the gate is doing the job it was added for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5LgTetnPkQciHFNaQWwSN
…r self-tested The registration check this branch added reports every hook `settings.json` registers that nothing under `bin/` references, and `remind-master-drift-on-push.sh` was the first thing it caught - 205 lines, landed by #357, with no coverage of its own. #357 did test the SHARED push detection thoroughly, through the sibling reminder's fixture and with a negative control, so what was missing was this hook's own contract; master's copy of this suite even cites "the drift section" that was never added. Fixed here rather than exempted, because a gate with a recorded hole is a gate nobody trusts, and this is the same trade the copyright gate took in #338: widen the check and fix what it finds. Twelve cases, covering what the hook promises: it names the SUBJECTS of the commits the base ref gained (the reason it exists at all - a divergence count is what `rev-list --left-right --count` already answers), it names the files changed on both sides while not reporting base-only files as overlap, it throttles on the base SHA so one tip is reported once, it reports again the moment the base moves, it counts UNCOMMITTED work as this branch's side of the overlap, it takes the other arm of the report when nothing overlaps, and it stays silent when there is nothing to inherit - level with the base, pushing the base branch itself, or a command that is not a push. Offline by construction: the hook only fetches when `MASTER_DRIFT_REF` contains a slash, so a local branch stands in for `origin/master` and no case touches the network. Two things this suite caught in its own cases, both worth stating because they are the failure mode this file exists to prevent. The level-with-base case first PASSED while the fixture sat in a broken merge state, so it was asserting nothing; the catch-up is now a `reset --hard`, since both sides had appended to the same file and the merge conflicted. And every silence case now gets a FRESH stamp directory: by that point the stamp already holds the current base SHA, so the throttle alone would explain the silence, and a case that passes whether or not the exit it names exists is decorative. Mutation-checked rather than assumed. Dropping the SHA throttle, reporting base-only files as overlap, reading `HEAD` instead of the working tree, and dropping the behind-count guard each fail exactly the case that names it; the hook was restored byte-identical afterwards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5LgTetnPkQciHFNaQWwSN
…ing to the repo root The new drift section ended with `cd "$REPO_ROOT"`, and the section immediately after it documents that it runs from the push fixture's scratch repo reached ONLY by inheriting the cwd - its enriched refusal renders only when a PR lookup finds a named branch, and the CI checkout is detached, so from the real tree it fails. Jumping to the repo root therefore turned "a PR with nothing outstanding still states the risk" red on every runner while the suite stayed green locally, where the checkout is on a branch. Both `repo: hygiene` and `shell: macos` failed on that one case. Saves `$PWD` at the top of the section and restores it at the bottom, rather than naming the push fixture's directory, so the coupling survives another section being inserted between the two. Reproduced before fixing and verified after, since a local run cannot see this: a detached worktree at the same commit fails with CI's exact message under the old line and passes under the new one. Left alone deliberately: the inherited-cwd coupling itself. It is a deliberate, documented choice in the section that depends on it - the fixture is what gives those cases a named branch - so the fix is to stop breaking it, not to relocate someone else's fixture in this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5LgTetnPkQciHFNaQWwSN
|
@claude review this The two review passes on this PR were against
Worth a close look at two things in particular. First, whether the per-session key is right where it Known and deliberately still open, tracked in |
|
Claude finished @astubbs's task in 4m 27s —— View job Review
Environment note (not a PR defect, same as the last two passes)My checkout again has What's new since
|
|
The one finding is fixed: the PR description's "Known open" section no longer claims the per-UID keying awaits a decision, since these commits settle and implement it. Two related staleness items went with it, both found while checking that one. The BSD bullet contradicted the section above it - "has still never run against BSD userland" while the macOS-lane section already recorded that the The description also now carries a section for the work that landed after your two earlier passes, so the body no longer describes a PR that stops at |
1 participant
Independent of any feature work: a dev-machine guard. It serves no single issue, so it cites none.
Description
A fan-out of eleven per-language demo agents took the host volume from ample to 8.8 GiB free of 926 GiB in about an hour, and took the Docker VM's virtual disk with it. One agent's build died outright; two others pruned under each other. Nothing warned, because the session had started with plenty of room - and a
SessionStartcheck would have reported all clear for exactly that reason.The only instant that can see what the last command left behind is just before the next one. So this is a
PreToolUsehook onBash, and it never blocks.Why no
iffilterSame reasoning as the squash guard:
Bash(docker *)would misscd demo && docker compose upand every wrapper script, which is most of how containers actually get built here. It earns the right to run on every call by costing two stat-class syscalls - measured at 7ms, nodockerCLI on the fast path - and by saying nothing unless a threshold trips, then at most once per ten minutes unless the band worsens.Two properties that are load-bearing
It must never exit non-zero. A disk warner that blocked
Bashon a full disk would remove the very commands needed to clear the disk. The worst case is asserted directly: even at the critical band it still allows the call.Docker Desktop's disk image is a high-water mark - a sparse file that grows and never shrinks, so pruning 17 GB does not shrink it by a byte. A cheap always-on trigger is confirmed against a cached
docker system dfbefore anything is said, and that correction applies only to the sparse-image reading, never to Linux's live filesystem one.The self-test is the interesting part
This hook has the failure mode every warn-only hook shares: its correct behaviour on a healthy machine is to print nothing, which is byte-identical to being broken, unregistered, or absent. So almost every case forces it to speak, and the one case asserting silence is paired with forced cases proving the same call path can be made to talk.
Every case pins all four thresholds rather than inheriting them. An earlier version left the host thresholds at their defaults and three cases flipped to failing mid-session when the machine dropped below the 25 GiB default warn line - a self-test for a disk warner must not be a function of the disk.
The
df-unreadable cases exist for the same reason: a hook that fails to measure and therefore says nothing is indistinguishable from one reporting a healthy disk, so the guard is the absence of a reading rather than a default of "fine". Three cases shadowdfonPATH; a fourth uses the same stub with a plausible answer, to prove the stub did not simply break the invocation.What review found, and why it is folded in rather than stacked
Review hardened this substantially, and those fixes are folded into the feature commit so that no point in the history carries the vulnerable version for a bisect to land on:
/tmpand was read back into$((now - last_at)). Bash arithmetic resolves a non-numeric operand as a variable name, recursively, so a crafted stamp ran a command as the user. Demonstrated, not theorised. The value is now shape-checked before it reaches the evaluator.set -uwith exit 1, taking theBashcall away. Likeliest exactly when the disk is full, which is the one moment this must not block.mkdir -psucceeds against a directory another user owns, so the state dir was never really checked; a symlinkedlast-warninggave arbitrary file truncation. Nowumask 077plus an ownership test, both builtins.map auto_home- shifteddf's columns so$4was Used, reported as free: 94 GiB "free" on a disk with 1. Now anchored on the POSIX capacity column.docker system dfcache read undercounts, producing exactly the false "fine" this hook exists to prevent. Now a temp file and an atomic rename.Each is paired with the mutant that kills it: twelve mutants injected, the hook restored byte-identical each time, every fix killed by exactly the case that should kill it.
One of the new cases was itself decorative and had to be repaired: the injection case used a payload containing a space, and
readsplits on whitespace, so it could never have executed - it passed with every guard deleted. A test asserting a security property has to be run against the unguarded code before it is believed.Verification, with a control
The hook self-test goes from 173 passing cases to 208 on this branch, and both numbers are green: 0 failures, measured serially on this branch and on a detached
masterworktree. (The figures were 154 and 189 before master's own new cases arrived; the branch's contribution is the same 35 either way.)An earlier version of this description claimed the suite reports 14 failures "byte-identical to the set a pristine
masterworktree reports". That was wrong, and the way it was wrong is worth stating: pristine master is green when run serially, and the 14 appeared only because both sides were measured while other work ran concurrently on the same box. Two contaminated runs were being compared to each other.The concurrency failures are real, and one of them is fixed here in its own commit: the push-reminder section cleaned up throttle stamps in a machine-global
${TMPDIR:-/tmp}location that the hook also wrote to, so two copies of the suite deleted each other's stamps mid-run. Four concurrent runs: 4/4 failed before, 0/4 after.Re-measured after the master merge, against a detached
origin/masterworktree as the control. Master: 4/4 red, five push-reminder assertions among the failures. This branch: 3/4 red, zero push-reminder failures - so the fix survived the merge and still works. Every remaining failure is one arm,check-merge-outstanding-work.sh's note fixture, which writesdocs/inflight/pr-90001-selftest.mdinto the real working tree at a fixed path so concurrent copies race between one run's write and another's cleanup. It is pre-existing on master, untouched here, and already fixed on #350 - deliberately left alone rather than have two branches edit the same lines.What the new macOS lane found
Master added a
shell: macosjob that runs everybin/test-*.shonmacos-latestunder BSD userland and a bash pinned to the system 3.2. Merging master therefore parsed this suite with bash 3.2 for the first time, and it went red - on this branch's own code, not on the lane.The registration meta-test ran python from a heredoc inside a command substitution,
$(python3 - <<REGCHECK ... ). Bash 3.2 scans a substitution for its closing paren treating the heredoc body as shell text, and does not recognise#comments while doing it - so an ordinary apostrophe in a python comment was the fifth and unbalancing single quote in that body, and 3.2 read to the end of the file looking for the close. Bash 5 parses the same text correctly, which is why every local run and every ubuntu lane was green.The way it presented is worth as much as the defect: the suite exited 2 after some sixty cases had already passed, and both
bin/check-all.shand the macOS job classify exit 2 as CANNOT RUN. A gate that could not measure, wearing the message of a missing tool.Fixed by writing the heredoc to a file at top level and running the file, so the body is never scanned as shell at all - a narrower fix that merely balanced the quotes would leave the next author one apostrophe away from the same red. The python is byte-for-byte unchanged. Three python heredocs inside
$( ... )survive on master; their bodies happen to balance when read as shell, which is luck rather than a contract, so abin/check-shell-hazards.shrow for the class is queued in the inflight note rather than added here.This also moves the "never verified against BSD userland" item: the lane now exercises the real
stat -farm and thedf -Pkcolumn read on a genuine BSD box. It still does not reach the Docker Desktop sparse-image path, since a hosted macOS runner has noDocker.raw.What changed after the review passes
Two review passes signed off at
8a50303andaa1635e7. Everything below landed after them, and wasreviewed at
57c333cbf.The per-UID throttle was the finding those passes left open, and it is now settled. Two reviewers
independently flagged that one stamp per Unix user means the first agent to notice silences every
other concurrent session for the window - in this PR's own motivating incident, ten of the eleven
agents could not have been told, while they were the ten still filling the disk. It stayed open
because the obvious fix trades one failure for another: warn everybody and eleven agents may each
start pruning, which the same incident shows is a real cost rather than a hypothetical - two of them
pruned under each other.
Settled by warning every session, with the message carrying the safety: each agent is told to
report the situation and suggest a reclaim rather than run one, so the operator is the gate
against duplicate effort instead of the throttle. An operator can tell duplicate cleanups apart,
which a ten-minute timer cannot. The keying is best-effort by design -
session_idis read pastevery fast-path exit so a healthy disk still forks nothing extra, with no
python3on that path, andan absent or unparseable payload falls back to the shared stamp, which is precisely the old
behaviour. The failure this file refuses is "warned" becoming "silent", and a coarser throttle is not
that. The value reaches a file path, so it is filtered with
tr -cdrather than escaped, and capped.The stamp is now written after the message rather than before it, so a kill between the two
cannot swallow a warning for the whole window - likeliest exactly when the disk is full, which is
this hook's entire subject.
The registration assertion caught a hook that was registered and never self-tested, and it was
right:
remind-master-drift-on-push.sh, 205 lines from #357, had no coverage of its own.Fixed here rather than exempted - the same trade #338 took, widen the check and fix what it
finds - because a gate with a recorded hole is a gate nobody trusts. Twelve cases, each
mutation-checked: dropping the SHA throttle, reporting base-only files as overlap, reading
HEADinstead of the working tree, and dropping the behind-count guard each fail exactly the case that
names it.
That new section then caused a CI-only failure, which is worth recording rather than just fixing.
It ended by returning to the repo root, and the section after it reaches the push fixture's scratch
repo by inheriting the cwd - its enriched-refusal cases need a findable PR, which needs a named
branch. A local checkout is on a branch, so it passed; every runner's checkout is detached, so it
failed. It now restores whatever cwd it found, which is correct whatever the following sections need
rather than correct only for today's arrangement. It also explains a dangling reference in master's
own suite - "see the note at the top of the drift section", pointing at a section #357 never
added.
The count check now pins both numbers. Master reworded the
docs/agent-harness.mdsentence todistinguish hook scripts from registrations while this branch's check still pinned the older
single count, so merging the two went red on the doc rather than on either change - the check
working, not failing. It now verifies eleven scripts and twelve registrations independently, each
confirmed by making it wrong in turn.
Where it came from
Extracted from the language-proxy stack, where it was written. Nothing in it is feature-specific:
.claude/binds Claude Code sessions only, so it can never run in CI, and CI runners are reaped anyway. Included on its own because the problem it catches is not specific to that work.Checklist
docs/agent-harness.mdgains the hook's entry, the no-ifreasoning, and the paragraph on self-testing a warn-only hookdocs/features/-N/A - a local development guard for Claude Code sessions; it changes nothing a user of the library can observe and never runs in CI.bin/test-check-agent-hooks.sh(35 for this hook, plus 12 coveringremind-master-drift-on-push.sh, which the new registration assertion found registered and untested), including the never-block assertion, the fourdf-unreadable cases with their positive control, both halves of the high-water-mark correction, the throttle escalation, a case per security fix paired with the mutant it kills, and a registration assertion (every registered hook is self-tested and vice versa)ce-simplifyandce-code-reviewlocally - both; that pass is what produced the P0 security fixes folded into the feature commit, the twelve-mutant verification, and the still-open findings noteKnown open, tracked in
docs/inflight/ci-low-disk-warner-open-items.mdstat -farm and thedf -Pkcolumn read now do, on master'sshell: macoslane, but a hosted macOS runner has noDocker.raw, so the sparse-file branch itself remains unexercised.dfon the fast path has no timeout, so a hung NFS mount would freeze the session; portable bounding is awkward, sincetimeoutis GNU coreutils.docker_rootis hardcoded to/var/lib/docker, ignoringdaemon.json'sdata-root.PC_DISK_*variables, whose own comment frames them as test-only.Commits are atomic units rather than a development log: the hook and its self-test, the pre-existing push-reminder flake fix, the per-session keying decision, the drift-hook coverage, the cwd repair, and two master merges whose conflict resolutions are explained in the merge commits themselves.