Harden the Unbounded-Wait Guard: Delete the Machinery Its Escapes Kept Outrunning by ptr727 · Pull Request #1632 · ptr727/ProjectTemplate

ptr727 · 2026-09-15T14:55:21Z

Answers the five CodeRabbit findings on promotion PR #1629, and the five rounds of local strict review those fixes then drew. Requirement 7 shipped in #1622 and this is the hardening pass over it.

What was wrong

CodeRabbit found two bypasses, a decoy registration, and an install that could disable its own guard. Fixing those exposed more of the same class, so the branch is five commits, each answering the round before it.

The load-bearing change is the last one, and it is a deletion. Four rounds narrowed a token scan that tried to decide which & backgrounds which compound, and each narrowing was defeated by a new spelling: a statement between the loop and its group's closer, a disown before a wait, a subshell the sequencing had already reaped, a setsid inside the payload rather than beside the timeout. That is a parse question and the scan does not have a parse, so _waits_after, _escapes_timeout_group and the group-closer walk are gone. One predicate replaces them: a background operator anywhere in the command, a coproc, or a setsid anywhere, and nothing in that command bounds anything.

The cost is a false deny on <loop> & wait, a bound nothing in the command text can verify. timeout 900 <command> &, the shape the fleet rules actually ask agents to write, still passes, because a backgrounded timeout outlives the shell and still enforces. That case is tested.

Closed in the guard

A crash that failed all seven rules open. str.isdigit() is true for a superscript and int() then raises, and a non-zero exit lets the tool call through exactly as an allow does.
A group backgrounding the loop inside it, in every spelling including a fused closer and a trailing statement.
A redirect that rebinds the pipe the loop already reads: <&0, /dev/stdin, /proc/self/fd/0, /dev/full, /dev/./zero, //dev/zero. The whole of /dev and /proc is read as a category, since a list of the streams that never end always had another entry.
The last descriptor-0 redirect is what binds, so < in.txt < /dev/zero no longer lets the file vouch for the stream.
read -u 3 draws on a descriptor the redirect never bound, so it is no bound.
wait $p, timeout --foreground, setsid, and coproc each leave a loop running past the timeout.
False denies removed: done 2>&1 < f and four siblings, make build |& tee log, and a case arm ending ;&.

Closed in the installer

The registration check was recomputing the launcher at check time, so a correctly installed machine reported STALE.
The decoy hole was closed on the sweep and left open on the guard, which is the hook that actually denies.
A matcher had to be exactly Bash; absent, "", * and Bash|Task all receive Bash calls.
A path had to be double-quoted; bare, single-quoted, ~ and $HOME all run the deployed file.
A rejected entry skipped its own count, so every real problem came with a second line falsely saying the hook is not registered.
A sweep timeout larger than the installer writes was reported as a defect.
The staged-copy path had no error handling and two concurrent installs shared one filename.

Known and open

Six findings from the final review round are recorded and not fixed here, per the local-strict-review budget. Two are genuinely unbounded waits the guard allows, both pre-existing: an until read loop over an exhausted input, where exhaustion is what makes it never exit, and timeout -s 0, which delivers a signal the payload ignores. The rest are a stale diagram node, a stale count in one sentence, a SessionEnd matcher check that rejects "" and *, and a test that does not assert the defects it plants. Each is filed.

Closes nothing on its own: #1589 is closed by #1629, which this unblocks.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JeRfDdqc5Ua2Q1Ui5K7XJS

…That Broke Its Own Guard CodeRabbit raised five on the promotion pull request, blocking it. All five were real and the installer one is the reason that block was worth having. ## The installer replaced a live hook before testing it `main()` copied each hook straight onto its registered path and ran the self-test afterwards, returning early on failure without restoring anything. A hook that failed its own self-test therefore ended up installed, with the existing registration still pointing at it, so an install refusing to register would have disabled the guard it refused to register. Every hook is staged beside its live path now, every staged copy is tested, and the live files are replaced with `os.replace` only once all of them pass. Measured against a deliberately broken sweep over a good install: exit 1, the live hook byte-identical and still self-testing, and no staged file left behind. ## A decoy counted as a registration `registration_problems` accepted any command containing the sweep's name, so `echo stray-process-sweep` made `--report` call a machine current with no sweep installed. The installer and the check now spell the command through one helper, and the entry is accepted only when its type, command and timeout all match what was written. ## Two rule 7 bypasses A redirect on any descriptor read as a bound, so `yes | while read line; do sleep 30; done 2<errors` passed while the `read` went on consuming the pipe on descriptor 0. Only an implicit redirect or an explicit `0<` binds what the loop reads. An inherited timeout reached a wrapper the payload backgrounds, so `timeout 600 bash -c "bash -c '<loop>' &"` passed. The inner shell outlives the one the timeout controls, exactly as a backgrounded loop does. The ampersand is found past the invocation's whole argument list, which is where the argument collector stopped rather than at the wrapper token. ## And the summary that redefined the rule The kit README restated the denied shape as a `while`/`until` loop with two bounds, which the rule outgrew twice on this branch. It refers to requirement 7 rather than redefining it.

Six findings from the local review of the previous commit. Two of them are defects that commit introduced, one is the half of its own fix it left undone, and the rest are the same root causes one step further out. ## The false stale, introduced by the last commit The new registration check recomputed `hook_launcher()` at check time and compared the whole command string, so a machine installed under a PATH without `python3` and checked under one with it reported STALE with two false messages, one of them claiming the sweep was not registered at all. It compares the deployed path now, which is what the installer writes and what never drifts, and leaves the launcher spelling alone. A launcher drift reports clean again, while every decoy shape still reports. ## The half of that fix that was left undone The decoy hole was closed on the SessionEnd sweep and left open on the PreToolUse guard, which is the hook that actually denies a dangerous write. `echo gh-write-guard`, a `prompt` type, and a command pointing at a path that does not exist all reported no problem. Both halves go through one test now, and the guard's group matcher is checked for the first time: registered under anything but `Bash` it never sees a Bash call, and nothing said so. ## A fused ampersand defeated the backgrounding check `_backgrounded_after` tested `tok == "&"`, and the tokenizer fuses a run of operator characters, so `&` followed by a newline arrives as one token and read as not backgrounded. Measured: the newline form really leaks, and a Bash tool call is routinely multi-line. It tests for a token that begins a background operator now, which also fixes the pre-existing loop-level call that had the same blindness. ## The redirect scan, finished A redirect on a later command counted as this loop's bound, so `yes | while read l; do sleep 30; done; cat < f` passed. The scan stops at the loop's own invocation. The descriptor is compared as a number rather than as text, since bash resolves a padded zero to descriptor 0, and a `{name}` form names a variable that is never descriptor 0. ## And two smaller ones The replace loop was the one step that was not all-or-nothing and had no error handling, so a failure part way left a split state, an orphan staged file, and a traceback. It now says which hooks were replaced and removes what it staged. The new installer test leaked a full kit copy into the temporary directory on every run, including in CI. The spec the kit README now defers to said neither of the things this branch taught the code, so requirement 7 names the descriptor and the backgrounded intermediate wrapper.

…omeone Else Wrote Twelve findings from a second local review, over the content the last commit left. Four of them are the same class the last round fixed, one step further out, and one is a crash the last round introduced. ## A group backgrounds the loop inside it `( <loop> ) &` and `{ <loop>; } &` leak exactly as `done &` does, and the scan for the ampersand stopped at the closing bracket a token before it, so an outer `timeout` was credited with a bound it does not have. The spec claims this class in three places and lists what it deliberately misses, and this was not among them. The scan reads past a closer now, fused to the ampersand or not. A `wait` after the ampersand is the exception and was denied. It holds the shell open until the loop ends, so the timeout fires on a live process group and takes the loop with it. That is a real idiom and a correctly bounded one. ## A redirect that rebinds the pipe it is already reading The new descriptor test read which descriptor a redirect binds and never what it binds it to, so `< /dev/stdin` and `<&0` re-opened the same never-ending producer and passed as bounds, as did `/dev/zero`. A redirect now has to name a source rather than duplicate a descriptor, and a `/dev/` stream is not a source that ends. The same function read the token before a `<` as a descriptor, which it often is not: `done 2>&1 < f` put the previous redirect's target there, and five legal bounded loops were denied. It walks the redirections in order now rather than indexing backwards. ## A crash that failed all seven rules open `fd.isdigit()` is true for a superscript, which `int()` then refuses, and the hook's own contract is that a non-zero exit lets the call through exactly as an allow does. One such character disabled every rule, GitHub-write and primary-checkout included. `isdecimal()` is the predicate that matches `int()`. ## The installer reported working machines as broken Three ways, each from the last commit. A rejected entry skipped the count that follows it, so every real problem came with a second, false line saying the hook is not registered at all. The matcher had to be the exact string `Bash`, while an absent matcher, `*`, and `Bash|Task` all receive Bash calls. And the deployed path had to be double-quoted, which is this installer's spelling and not the one the `/hooks` UI or a hand-written entry uses. A sweep timeout larger than the installer writes was reported as a defect too, where it is strictly better than the default it exists to raise. ## And three smaller ones The deployed guard path was spelled twice, a literal for the writer and the constant for the reader, which is the drift `hook_command` was added to end. Two concurrent installs shared one staged filename and clobbered each other. And the new decoy test asserted that something was reported rather than what, so the false line above satisfied it and it would have passed with the branch it tests deleted.

…imeout's Reach A third review found fourteen, nine of them against text the previous commit wrote. Three shared one root cause, and it was the fix that commit made: a list of the streams that never end. ## The list does not close, so it is gone `/proc/self/fd/0` re-opens the same pipe `/dev/stdin` does. `/dev/full` reads like `/dev/zero`. And `/dev/./zero` is neither, against a literal compare. Each was allowed, each measurably never ends, and each would have been answered by one more entry and one more spelling after it. The target is normalized now and the whole of `/dev` and `/proc` reads as no bound at all, `/dev/null` included rather than carved out. That is decidable from the command text, where the list never was. A redirect from `/dev/fd/3` is denied with them, which is the cost of taking the category. ## Three ways out of a timeout's process group The `wait` exception the previous commit added holds only for the shape it was written against. A `wait` carrying an operand waits for the job it names and returns while the loop runs on. `timeout --foreground`, abbreviated to `--f` or not, signals its direct child rather than the group. And `setsid` forks into a session of its own, so nothing the timeout signals reaches what it started. Each was allowed, each leaves the loop running past the timeout, and the spec asserted the opposite of all three. The timeout's own command run is read whole now instead of stopping at the duration, which is where the last two were hiding. ## The installer, and the prose A SessionEnd entry carrying no timeout was reported as carrying one whose value is None. One defect belonging to a group was reported once per entry under it, disagreeing with the dedupe the same function already does for a wrong settings shape. The guard was spelled by hand on the search side and by position on the deploy side, which is the drift the sweep's own two constants exist to prevent, so the guard has them too. And `runs_hook` claimed to decide whether a command runs the hook, which a command string does not decide, so it says what it does decide. Requirement 8 still pointed at "the three shapes" requirement 7 does not reach, which the previous commit made five.

…s Fatal A fourth review found eight, four against the third round's own text. Four rounds have now each closed the shapes they were shown and left the next one, which is what a scan deciding a parse question does. This round removes the scan rather than narrowing it again. ## What is gone `_waits_after` and `_escapes_timeout_group` are deleted outright, and the group-closer walk in `_backgrounded_after` with them. Each existed to carve an exception out of "a backgrounded loop is unbounded", and each exception had another spelling: a statement between the loop and its group's closer, a `disown` before the `wait`, a subshell the sequencing had already reaped, a `setsid` written inside the payload rather than beside the timeout. All four were measured leaking, all four passed. In their place, one predicate over the whole command. A background operator anywhere, or a `setsid` anywhere, means nothing in that command bounds anything. There is no next spelling, because nothing is being told apart any more. The cost is a false deny on `<loop> & wait`, which is a bound no reading of the command text can verify, and on nothing else that matters: a `timeout` the command backgrounds as a whole still bounds what it runs, since that process outlives the shell, so `timeout 900 <command> &`, the shape the fleet rules actually ask for, is untouched and tested. The declared limitations drop from five to four. A loop in a function body backgrounded at its call site was one of them, and the whole-command read reaches it now without being asked to. ## Three ways the read bound was still wrong POSIX requires exactly two leading slashes to survive normalization, so `//dev/zero` passed the tree test that `/dev/zero` fails. Leading slashes are collapsed before the test now. A shell applies redirections in order and each replaces the last, so `< in.txt < /dev/zero` reads the stream while the scan stopped at the file. The last binding on descriptor 0 is what is judged. And `read -u 3` draws on the descriptor it names rather than on the one the redirect bound, so a loop could be handed a bound it never reads from. A `read` naming its own descriptor is no longer a bound. ## Two smaller ones The matcher message claimed the guard never sees a Bash call, which is a machine-wide claim from a per-group fact: a second group with a wrong matcher says nothing about the correct group beside it. And a new assertion compared `DEPLOYED_HOOKS[0]` against the constant that defines it, eight lines apart, which cannot fail.

…st Does Not Reach Five findings from a fifth review, every one of them against what the previous commit wrote or claimed. Four others it found are older than this branch and are filed rather than fixed here. ## An ampersand that backgrounds nothing The new whole-command fork scan skipped `&&` and nothing else, so bash's `|&`, which is `2>&1 |`, and the `case` terminators `;&` and `;;&` all read as backgrounding. `timeout 600 bash -c 'make build |& tee log; <loop>'` was denied, and that loop is bounded. An ampersand preceded by a pipe or a semicolon is part of that operator rather than a background operator. ## A fork with no operator to find `coproc` backgrounds without writing an ampersand at all, so a scan for one never sees it, and a `timeout` around the shell was credited with a bound. Measured: a coprocess kept writing for six seconds after a one second timeout returned. It is recognized now, and the claim beside it is corrected with it. The previous commit said two shapes fork out of reach and that nothing further was left to miss. Three are recognized, a launcher that starts a session some other way is not, and what that costs is a leak requirement 8 reports rather than a deny requirement 7 makes. ## Two comments describing deleted code One said the ampersand is found past the loop's own redirections, and one said it follows the wrapper's whole argument list. Neither is true of a whole-command scan, which has no notion of where the loop closes. The index the second pointed at was captured and never read. ## The installer, twice A registration spelled with `~` or `$HOME`, which is how a person writes that path and how the `/hooks` UI may write it, was reported as naming the guard without running it. Both spellings resolve to the deployed file, and a trailing `;` or `|` after the path is no longer read as a different path either. And the staging copy sat outside the block's own error handling, so a full or read-only hooks directory left a `.staged` file behind and exited on a traceback, which the comment beside it said only a kill could do. ## One claim narrowed The `/dev` and `/proc` category test reads an absolute target. A relative one resolves against a working directory this rule does not model, and the spec says so rather than implying the trees are closed.

coderabbitai · 2026-09-15T14:55:27Z

Warning

Review limit reached

Next included review available in 31 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: f9326a04-7dc1-4726-93de-7a501360c77b

📥 Commits

Reviewing files that changed from the base of the PR and between d8c65cc and 8aa40e8.

📒 Files selected for processing (5)

host-setup/agent-safety/README.md
host-setup/agent-safety/claude/README.md
host-setup/agent-safety/claude/gh-write-guard.py
host-setup/agent-safety/claude/install.py
host-setup/agent-safety/claude/test_install.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Copilot

🟡 Changes recommended

_names_a_stream() currently does not treat /dev and /proc roots as streams (only /dev/ and /proc/), contradicting the documented “whole tree” behavior and risking misclassification of redirects.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR hardens the host-safety unbounded-wait guard (requirement 7) and the Claude Code installer by removing brittle token-walk machinery, tightening what counts as a “bounded read”, and making hook deployment/registration checks more robust against decoys and drift.

Changes:

Simplifies the unbounded-wait detection by treating any backgrounding/coproc/setsid in the command as defeating timeout, and strengthens stdin-redirect bounding checks (descriptor-0 binding, last-redirect-wins, and /dev//proc category handling).
Hardens the Claude Code installer’s registration auditing (detects decoy/nonsensical registrations, avoids double-reporting, and validates matchers/paths more realistically).
Makes hook deployment safer by staging + self-testing before replacing the live hooks, and adds/extends installer test coverage for the new behaviors.

File summaries

File	Description
host-setup/agent-safety/README.md	Updates spec prose to match the new, coarser “forks out of timeout reach” model and the refined stdin-redirect bounding definition.
host-setup/agent-safety/claude/gh-write-guard.py	Implements the new fork-detection predicate and expanded stdin-redirect analysis; adds selftest cases for new bypass shapes.
host-setup/agent-safety/claude/install.py	Fixes registration checks (decoy/stale detection), improves defect reporting semantics, and stages hooks before replacing live copies.
host-setup/agent-safety/claude/test_install.py	Adds targeted tests covering decoy registration, matcher semantics, quoting variations, timeout rules, and staged deploy safety.
host-setup/agent-safety/claude/README.md	Aligns Claude Code kit documentation with the spec’s requirement-7 definition and the new “forks defeat timeout” behavior.

Review details

Files reviewed: 5/5 changed files
Comments generated: 1
Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

ptr727 · 2026-09-15T15:01:09Z

@coderabbitai review

coderabbitai · 2026-09-15T15:01:14Z

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

ptr727 added 6 commits September 15, 2026 06:58

Copilot AI lite review requested due to automatic review settings September 15, 2026 14:55

Copilot started reviewing on behalf of ptr727 September 15, 2026 14:56 View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

Comment thread host-setup/agent-safety/claude/gh-write-guard.py

ptr727 merged commit dbbb28f into develop Sep 15, 2026
9 checks passed

ptr727 mentioned this pull request Sep 15, 2026

Requirement 6 Denies the Remote Branch Delete That merge-and-release Step 8 Prescribes #1642

Open

ptr727 deleted the feature/promotion-findings-1629 branch September 17, 2026 01:48

Navigation Menu

Harden the Unbounded-Wait Guard: Delete the Machinery Its Escapes Kept Outrunning - #1632

Harden the Unbounded-Wait Guard: Delete the Machinery Its Escapes Kept Outrunning#1632
ptr727 merged 6 commits into
developfrom
feature/promotion-findings-1629

ptr727 commented Sep 15, 2026

Uh oh!

coderabbitai Bot commented Sep 15, 2026 •

edited

Loading

Review limit reached

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

ptr727 commented Sep 15, 2026

Uh oh!

coderabbitai Bot commented Sep 15, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ptr727 commented Sep 15, 2026

What was wrong

Closed in the guard

Closed in the installer

Known and open

Uh oh!

coderabbitai Bot commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

ptr727 commented Sep 15, 2026

Uh oh!

coderabbitai Bot commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Sep 15, 2026 •

edited

Loading

coderabbitai Bot commented Sep 15, 2026 •

edited

Loading