End a git push argument list at a newline, not only at && - #601
Merged
Conversation
The write-guard hook read a newline as whitespace, so it vanished when tokenizing and every token on a later line became an argument of the first line's `git push`. A push and a `gh pr create` written as two lines resolved to seven push targets rather than one, `develop` among them, and the hook denied the push as a direct push to a protected branch while the push targeted an ordinary feature branch. The direction is over-blocking, so this was a usability defect rather than a safety hole. It is worth fixing because the denial states a reason that is not true of the command, and a safety hook that cries wolf is one that stops being read. A newline ends a command exactly as `;` and `&&` already did, so it is an operator character rather than whitespace: `_SHELL_OP_CHARS` gains it, the operator set is passed to shlex as a string, and it is removed from shlex's whitespace. The degraded path splits lines itself so an unparseable command cannot lose the separator either. Backslash-newline continuations are folded to a space in `classify` before any of this runs, so they still read as one command. Five self-test cases lock it. Against the pre-fix code the incident case regresses and the other four pass unchanged, which is what makes them guardrails: a push on a later line, a second push on the next line, a continuation, and a newline inside a quoted body. A run of both versions over 179 commands, being every existing case, each of those again with a trailing newline, and eleven realistic multi-line shapes, changes five decisions, all of them deny to allow and all of them the false-positive shape, with no allow becoming deny. The self-test ran nowhere but at install time, where a regression surfaces as a broken machine, so CI runs it beside the audit self-test it matches: offline, standard library only. OPERATIONS.md carries the same line, since that list mirrors what CI runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a parsing defect in the gh-write-guard hook where newline separators in multi-line Bash commands were treated as whitespace and dropped during tokenization, causing tokens on later lines to be misclassified as additional git push arguments.
Changes:
- Treat
\nas a shell operator/separator in the hook tokenizer so multi-line commands are split into distinct command invocations. - Add self-test coverage for multi-line
git pushscenarios and run the write-guard self-test in CI and documented local verification. - Remove the now-completed TODO entry and update rollout notes accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
host-setup/agent-safety/gh-write-guard.py |
Updates shell tokenization to preserve newline as a command separator and adds self-test cases for the regression and related shapes. |
.github/workflows/validate-task.yml |
Runs the write-guard self-test in CI alongside other offline self-tests. |
OPERATIONS.md |
Documents the additional self-test command in the local/CI-aligned verification list. |
TODO.md |
Removes the completed write-guard newline defect backlog entry and updates related rollout notes. |
Review of #601 asked for two things. `_SHELL_OP_CHARS` and `_PUNCTUATION_CHARS` stated the same operator set twice, so adding an operator to one and not the other would have split them silently. The set is now derived from the string shlex takes. The second finding, that the degraded path splits a line even when the newline is inside quotes, is declined and answered on the thread. What it motivates is a case, since only unbalanced quoting reaches that path and nothing covered it: a push on the second line of a command whose quoting cannot be parsed must still be read as a push. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ptr727
added a commit
that referenced
this pull request
Aug 7, 2026
Clears the `A Home for a Disproved Finding` cluster from `TODO.md`. Its `Checked` anchor was re-verified against `develop` at `756a53e` before anything was written: the file still says the reviewer is sometimes factually wrong, still requires a decline to carry evidence, and still keeps its list of known non-working request paths, with nowhere to put the proof itself. ## The gap A decline that carries proof proves something about this tree, and [GOVERNANCE.md "Every Finding Ends in an Action"](https://github.com/ptr727/ProjectTemplate/blob/develop/GOVERNANCE.md#every-finding-ends-in-an-action) is right that the thread is where it belongs while the pull request is open. Afterwards the thread is the wrong place. The pull request merges, the next round starts with no memory of the last, and the second occurrence of the same claim reaches a maintainer with no way to tell it from a first. `Disproved Claims` is a new `###` under the runbook, so the three declared `##` sections in `spec/files.json` are unchanged. An entry names the claim, what was run or read to disprove it, the revision it was proved against, and what ends it. **It is deliberately not a list to append to**: an entry outliving the code it was proved against becomes a reason not to check, which is strictly worse than proving the claim again, so an entry whose subject moves is deleted by the change that moves it rather than edited to look current. Two guards ride with it. The record answers a repeated claim and never dismisses a new one, so a finding is judged on its merits first and matched second, and a reply carries the proof re-read rather than a pointer to a file the reviewer cannot open. And the entries are this repository's own, so a repository holding a copy carries the shape and deletes an entry whose subject it does not carry. ## The three worked examples **`keys_unsorted` requires jq 1.6.** A suppressed finding on #555 against the normalizer in `repo-config/configure.sh`, reasoning by analogy from the `walk/1` failure #553 fixed. Re-run in this session rather than quoted from the thread, on `jq-1.5-1-a5b5cbe` in `ubuntu:18.04`, the build that reproduces `walk/1`: ```console jq-1.5-1-a5b5cbe $ echo '{"b":1,"a":2}' | jq -c 'keys_unsorted' ["b","a"] $ echo '{"b":1}' | jq -c 'walk(.)' jq: error: walk/1 is not defined at <top-level>, line 1: jq: 1 compile error ``` **The write-guard's fallback parse, from #601.** Declined on the ground that the arm cannot execute, since `punctuation_chars` arrived in Python 3.6, the module uses f-strings throughout, and `install.py` refuses below 3.7. That is exactly the kind of disproof that expires, which is why the entry names the floor as what ends it. It also records that the finding earned a test case rather than a change, since only `ValueError` from unbalanced quoting reaches that path in practice and nothing covered it. **The bare-SHA design, from #602.** This one came from this repository's own backlog rather than from a reviewer, and it is here because a rejected method costs the same to re-propose as a declined finding costs to re-derive, while a backlog has a place for a claim the tree contradicts and none for a method a measurement rejects. Over the 25 most recent merged pull requests the bare-SHA arm raised four references and all four were correct prose, and a path arm flagged 54 of 215 backticked candidates. Both #601 and #602 are folded under disposition **Amends "Record what was tested and against which revision, and delete an entry whose subject changes."** They land as the second and third worked examples in the shipped section rather than as `Settled` bullets, because the entry ships in the same change. ## Verification - `python3 scripts/prose_lint.py . --diff develop` clean, `markdownlint-cli2` clean on both files. - `python3 -m unittest discover -s scripts` reports 372 tests OK and `python3 spec/audit.py --selftest` passes, neither touched by this change but both run because the file is carried. - Every factual claim in the new section was read against the tree at `756a53e`: the `keys_unsorted` call in `repo-config/configure.sh`, `_git_subcommand_arglists` and `_PUNCTUATION_CHARS` in `gh-write-guard.py`, the `(3, 7)` floor in `install.py`, and the verb alternation in `pr_review.py`. ## TODO.md The cluster is deleted per the file's own step 9. What this change does not carry becomes a new `ready` cluster: `GOVERNANCE.md` outcome 2 ends at the thread and nothing agent-agnostic points at the record, so an agent that never opens the provider runbook posts a decline and has nowhere to put the proof. That is a byte-locked carried section and a fleet re-vendor, which is why it is a separate change rather than a second surface in this one. ## Downstream `.github/copilot-instructions.md` is carried `whole` at `intent` with `appliesTo: "*"`, so this joins the re-vendor the ledger already lists. A repository taking it carries the section and its rules, and starts its own entries empty. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
ptr727
added a commit
that referenced
this pull request
Aug 8, 2026
… that blocks on what it cannot read (#609) Promotes 14 commits from `develop`. Merge with a **merge commit** (`gh pr merge --merge`), never a squash, and **without `--delete-branch`**, since this pull request's head is `develop` itself. Closes #607 through the closing keyword already carried in `530cf71`, which is why it is not repeated here. ## The prose backlog, cleared end to end `#600`, `#604`, `#605`, `#606` took the tree from **557 findings across 45 files to 0 across 0**, in four batches ordered by surface: snippets, comments, hub-only Markdown, then the carried files. Each batch measured the checker's own exemption against the live corpus *before* sweeping, and twice the measured answer was **"do not change the checker"**, which is a result of that pass rather than a skipped one. `#594` added the floor that makes those numbers trustworthy: a diff-scoped run now asserts what it actually scanned, since a check whose scan matches nothing reports zero findings and reads exactly like a pass. One finding from that work is worth carrying up: an exemption that is too **loose** produces silence rather than false positives. #519 recorded the governance files as clean; today's checker reports 38 findings against those same files as they stood at the commit that measured them. ## A review loop that fails closed `#599`, `#601`, `#602`, `#603` and `#608` are one arc on `scripts/pr_review.py`, each removing a shape in which the loop reported a clean pass over a review it had misread: - **`#599`** removed the shape a reply kept failing in, by taking the thread's *words* rather than an id, so there is no argument a hand-typed `PRRT_...` fits in. - **`#602`** made `claims` resolve what a description points at rather than what it looks like. - **`#603`** gave a disproved claim a home the next round reads. - **`#608`** reads the file-coverage line, and then generalizes: every reader keys on a structural marker, so a marker that changes spelling is a section the reader stops finding and reports as absent. The digest now vets headings, `<summary>` texts, metadata labels, coverage wordings and the reviewer login against an inventory measured from **332 review bodies**, and **blocks on anything outside it**, exit `43`, with the remedy stated as filing an issue on the hub. Whether to merge regardless is the maintainer's decision. `GOVERNANCE.md` merge gate went from four preconditions to **five** accordingly. ## Governance and tooling - **`#593`** states which checkout an agent works in and what the hub is, which is the host-wide routing the repositories that most need it cannot carry. - **`#596`** gates the pattern-detectable half of the representative-data rule, honest that no pattern closes the name-shaped case. - **`#598`** declares where a repository states what CI cannot verify. - **`#592`** regrouped `TODO.md` by what ships rather than by what it touches, so a `###` heading is one pull request. - **`#601`** ended a `gh push` argument list at a newline rather than only at `&&`, fixing a write-guard over-block. ## Verification Run on `develop` at `530cf71` immediately before opening this: `test_pr_review.py` (174), `test_prose_lint.py`, `test_repo_gate.py`, `spec/audit.py --selftest`, `gh-write-guard.py --selftest`, `spec/validate.py`, `repo_gate.py`, the prose gate in both CI invocations, markdownlint and editorconfig-checker. All clean. ## Not carried by this promotion - **#519 is complete and still open.** `TODO.md` holds its closing evidence under "Verified Complete, Awaiting Close". Closing it is the maintainer's call, so no keyword for it appears here. - **The re-vendor debt is now nine files.** `#606` queued seven, and `#608` changed `GOVERNANCE.md` "PR Review Etiquette" (`verbatim`) and `.github/copilot-instructions.md` (`intent`) on top. The `intent` half produces no hash and therefore no audit finding, which is why the Fleet Sweeps entry names those files by hand. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The write-guard hook read a newline as whitespace, so it disappeared when tokenizing and every token on a later line became an argument of the first line's
git push. A push and agh pr createwritten as two lines resolved to seven push targets rather than one:resolves to
revendor/x,gh,pr,create,develop,xandy, and the hook denies it as a direct push todevelop, a branch the command never pushes to.The direction is over-blocking, so this was a usability defect rather than a safety hole. It is worth fixing because the denial states a reason that is not true of the command, and a safety hook that cries wolf is one that stops being read.
The fix
A newline ends a command exactly as
;and&&already did, so it is an operator character rather than whitespace._SHELL_OP_CHARSgains it, the operator set is passed toshlexas a string, and it is removed fromshlex's whitespace set. The degraded path, which runs when the quoting cannot be parsed, splits lines itself, so an unparseable command cannot lose the separator either.Backslash-newline continuations were already folded to a space in
classifybefore any of this runs, so a continued command still reads as one command rather than two.Evidence
Five self-test cases lock it. Run against the pre-fix code the incident case regresses, and the other four pass both before and after, which is what makes them guardrails rather than restatements: a push on a later line, a second push on the next line, a continuation, and a newline inside a quoted body.
Both versions were run over 179 commands, being every existing self-test case, each of those again with a trailing newline, and eleven realistic multi-line shapes. Five decisions change. All five are deny to allow, all five are the false-positive shape, and no allow becomes deny.
End to end through the hook entrypoint against this repository's live branch rules, the two-line shape is denied by the pre-fix hook and allowed by this one, while
git push origin develop, agit push origin developon the second line of a multi-line command, andgit push origin HEAD:mainare all still denied.Why CI gains a line
The hook's self-test ran nowhere but at install time, where a regression surfaces as a broken machine rather than as a failed check. It is offline and standard library only, the same footing as the audit self-test it now runs beside. OPERATIONS.md carries the same line because that list mirrors what CI runs.
Backlog
TODO.mdloses the cluster this carries, and the host-rollout entry that pointed at this fix is rewritten, since a machine keeps running the old hook until the installer is re-run there.The file's warn-only prose backlog is unchanged at 52 findings. It belongs to the prose-content cluster rather than this one, and the diff-scoped prose run is clean.