From b489718f758299fc31e203a0961cca3563c34eca Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 7 Aug 2026 06:41:01 -0700 Subject: [PATCH 1/2] End a git push argument list at a newline, not only at && 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) --- .github/workflows/validate-task.yml | 2 + OPERATIONS.md | 1 + TODO.md | 17 +-------- host-setup/agent-safety/gh-write-guard.py | 45 +++++++++++++++++------ 4 files changed, 37 insertions(+), 28 deletions(-) diff --git a/.github/workflows/validate-task.yml b/.github/workflows/validate-task.yml index ea729253..816ad566 100644 --- a/.github/workflows/validate-task.yml +++ b/.github/workflows/validate-task.yml @@ -55,6 +55,7 @@ jobs: # Each gate in scripts/ is proven by a case that reintroduces the fault it catches. # Standard library only, so no install step and no dependency to pin. # The audit engine self-test is offline, so it runs here rather than only on an owner sweep. + # The write-guard self-test is offline too, and it otherwise runs only when a host installs the hook, which is where a regression in it would surface as a broken machine. - name: Run script self-tests step run: | set -Eeuo pipefail @@ -62,6 +63,7 @@ jobs: python3 scripts/test_repo_gate.py python3 scripts/test_pr_review.py python3 spec/audit.py --selftest + python3 host-setup/agent-safety/gh-write-guard.py --selftest - name: Check repo gates step run: python3 scripts/repo_gate.py diff --git a/OPERATIONS.md b/OPERATIONS.md index 456f4d06..b09383f4 100644 --- a/OPERATIONS.md +++ b/OPERATIONS.md @@ -15,6 +15,7 @@ python3 scripts/test_prose_lint.py python3 scripts/test_repo_gate.py python3 scripts/test_pr_review.py python3 spec/audit.py --selftest +python3 host-setup/agent-safety/gh-write-guard.py --selftest python3 scripts/repo_gate.py python3 scripts/prose_lint.py . --check charset --check dupword --check spelling python3 scripts/prose_lint.py . --check charset-unknown --check semicolon --check dash --check comment-wrap --check comment-case --summary diff --git a/TODO.md b/TODO.md index f3f0e555..b4b363c4 100644 --- a/TODO.md +++ b/TODO.md @@ -62,21 +62,6 @@ One pull request adding two gates of the same shape, each confirming that a refe - **Settled** - Three stale descriptions in one session generated six review findings between them, each a reviewer noticing that the body named a commit, a branch, or a behavior the branch no longer carried. - **Settled** - Prose claims stay out of scope, since judging those needs a similarity heuristic, which [`spec/section-model.md`][section-model] rejects for the reason it would fail here. -### The Write-Guard Newline Defect - -One pull request fixing the argument-list split in the installed hook, plus the self-test case that locks it. - -**State** `ready`. **Touches** [`host-setup/agent-safety/gh-write-guard.py`][write-guard] and its self-tests. **Cost** one hub edit, and it reaches a machine only when the installer is re-run, so it rides the host visit in "Fleet Sweeps". - -- **End a `git push` argument list at a newline, not only at `&&`.** Every token on a later line of the same command is otherwise read as a refspec. - - **Blocked by** - Nothing. - - **Issue** - None filed, and [#365][issue-365] carries the rollout that delivers it. - - **Checked** - `develop` at `1ed0cc8` on 2026-08-03, measured against the installed hook. - - **Open** - Nothing. - - **Settled** - A lone `git push -u origin revendor/x` resolves to the one branch, while the same push followed by a newline and a `gh pr create` with a base of `develop` resolves to five, meaning `revendor/x`, `gh`, `pr`, `create`, and `develop`. - - **Settled** - The `&&` form resolves correctly, which is what isolates the defect to the newline case, and the existing suite covers only that form. - - **Settled** - The error direction is over-blocking rather than under-blocking, so it is a usability defect rather than a safety hole, and that is why it is worth fixing: the denial claims a direct push to a protected branch when the push targets an ordinary feature branch, and teaching a safety hook to cry wolf is how it stops being read. - ### Three Rules That Leave the Recurring Case Unstated One pull request widening three carried [`GOVERNANCE.md`][governance] rules that each state their common case and go quiet on the case that recurs, filed together because they share that shape and land in one re-vendor. @@ -428,7 +413,7 @@ Regenerate [reports/divergences.md][divergences-report] before using it as the w - **Hub state** - Done for the documentary half, verified `develop` at `1ed0cc8` on 2026-08-03. - **Outstanding** - Four machines, WSL2 Ubuntu, the MacBook Air and both ThinkPads, plus any headless or cron environment running with the token. macOS needs someone on that platform, the Proxmox question is whether that host also runs containers which decides whether Docker is required there, and the engine-inside-the-distro variant of the WSL2 Docker cell is unverified. - **Issue** - [#365][issue-365] and [#483][issue-483]. - - **Rides with** - The write-guard newline fix, since only re-running the installer deploys it. + - **Rides with** - Nothing on the hub, since the write-guard newline fix has landed on `develop` and a machine keeps running the old hook until the installer is re-run there. - **Detail** - A ticked row means the host-wide rules text and not the hook, since only running the installer deploys both layers, and the proxmox host proved that distinction by carrying the documentary half alone for eight days on the machine where the incident originated. - **Detail** - Honor the issue's own rule when filling a cell, that an unverified install command is worse than a blank, because a blank prompts a question while a wrong command produces a broken host and a false sense that setup succeeded. - **Detail** - The superseded safety section from [#364][issue-364] still sits above the canonical block in this host's rules file, so the two overlap. Removing it is a judgment call on a per-machine file, which is why it is surfaced rather than applied. diff --git a/host-setup/agent-safety/gh-write-guard.py b/host-setup/agent-safety/gh-write-guard.py index 8f2e341d..5e142dfd 100644 --- a/host-setup/agent-safety/gh-write-guard.py +++ b/host-setup/agent-safety/gh-write-guard.py @@ -199,23 +199,36 @@ def _current_push_branch(cwd): _GIT_GLOBAL_VALUE_OPTS = {"-C", "-c", "--git-dir", "--work-tree", "--namespace", "--exec-path", "--config-env"} -_SHELL_OP_CHARS = set("();<>|&") +# A newline ends a command exactly as `;` does, so it is an operator character here rather than whitespace. +# Read as whitespace it vanishes when tokenizing, and every token on a later line of a multi-line command is then read as one more argument of the first line's command. +# A backslash-newline continuation is folded to a space in `classify` before any of this runs, so every newline reaching the tokenizer is a real command separator. +_SHELL_OP_CHARS = set("();<>|&\n") +# The same operator set as a string, which is the form shlex takes it in. +_PUNCTUATION_CHARS = "();<>|&\n" def _shell_tokens(cmd): - """Tokenize like a shell, isolating operator runs (`|`, `&&`, `;`, `>`, `2>&1`, ...) as their own - tokens even when glued to a word - so a `>` inside a quoted value stays part of that token while a - real redirection is separated. Degrades gracefully if the quoting cannot be parsed. + """Tokenize like a shell, isolating operator runs (`|`, `&&`, `;`, newline, `>`, `2>&1`, ...) as + their own tokens even when glued to a word - so a `>` or a newline inside a quoted value stays part + of that token while a real redirection or line break is separated. Degrades gracefully if the + quoting cannot be parsed. """ try: - lex = shlex.shlex(cmd, posix=True, punctuation_chars=True) + lex = shlex.shlex(cmd, posix=True, punctuation_chars=_PUNCTUATION_CHARS) lex.whitespace_split = True + lex.whitespace = lex.whitespace.replace("\n", "") # A newline is an operator above rather than a gap between words. return list(lex) except (ValueError, TypeError): # bad quoting, or punctuation_chars unsupported on old Python - try: - return shlex.split(cmd, posix=True) - except ValueError: - return cmd.split() + # Neither fallback isolates an operator, so the lines are split here to keep the one thing this path must not lose, that a newline ends the command before it. + toks = [] + for i, line in enumerate(cmd.split("\n")): + if i: + toks.append("\n") + try: + toks.extend(shlex.split(line, posix=True)) + except ValueError: + toks.extend(line.split()) + return toks def _is_shell_op(tok): @@ -227,7 +240,7 @@ def _is_redir_op(tok): def _is_separator(tok): - return _is_shell_op(tok) and ">" not in tok and "<" not in tok # |, ||, &, &&, ;, (, ) + return _is_shell_op(tok) and ">" not in tok and "<" not in tok # |, ||, &, &&, ;, (, ), newline def _is_git_exe(tok): @@ -243,7 +256,8 @@ def _git_subcommand_arglists(cmd, sub): Keying off a real `git`->`` token sequence (git's value-taking global options skipped, an absolute-path or .exe git recognized) means the same invocation named inside a quoted --body forms no - such sequence, and a compound ` A && B` yields two independent arg lists so both are seen. + such sequence, and a compound ` A && B` yields two independent arg lists so both are seen, + whether the two are joined by `&&` or written on their own lines. """ toks = _shell_tokens(cmd) n = len(toks) @@ -265,7 +279,7 @@ def _git_subcommand_arglists(cmd, sub): while k < n: t = toks[k] if _is_separator(t): - break # a command separator (|, &&, ;) ends this git invocation + break # a command separator (|, &&, ;, newline) ends this git invocation if t.isdigit() and k + 1 < n and _is_redir_op(toks[k + 1]): k += 1 # a file-descriptor number before a redirection is shell syntax, not git argv continue @@ -622,6 +636,13 @@ def classify(cmd, cwd=None, origin=None, current_branch=None, rules_lookup=None, ("gh issue comment 5 --body \"first git push\" && git push origin develop", None, {"develop": _CODE_RULES}, "deny", "a quoted mention before a real push does not hide the real target"), ("git push >push.log 2>&1", "develop", {"develop": _CODE_RULES}, "deny", "redirection tokens are not a branch: bare push to develop still denies"), ("git push origin develop >push.log 2>&1", None, {"develop": _CODE_RULES}, "deny", "redirect after a real refspec does not hide the develop target"), + # A newline ends a command as `&&` does, and reading it as whitespace made every token on a later line an argument of the push. + # A feature-branch push followed by a `gh pr create` then denied as a direct push to the base branch that command named. + ("git push -u origin feature/x\ngh pr create --base develop --title x --body y", None, {"feature/x": set(), "develop": _CODE_RULES}, "allow", "a newline ends the push argv: the pr-create base is not a push target"), + ("cd /repo\ngit push origin develop", None, {"develop": _CODE_RULES}, "deny", "a push on a later line is still parsed as a push"), + ("git push origin feature/x\ngit push origin develop", None, {"feature/x": set(), "develop": _CODE_RULES}, "deny", "a second push on the next line is checked: develop denies"), + ("git push \\\n origin develop", None, {"develop": _CODE_RULES}, "deny", "a backslash-newline is a continuation, not a separator: develop still parsed"), + ("gh issue comment 5 --body \"one line\ngit push origin develop\"", None, {"develop": _CODE_RULES}, "allow", "a newline inside a quoted body does not start a new command"), ] From a2665db93ec051ad7819aa33d3853062486018dd Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 7 Aug 2026 06:50:11 -0700 Subject: [PATCH 2/2] Derive the operator set from its string, and lock the degraded path 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) --- host-setup/agent-safety/gh-write-guard.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/host-setup/agent-safety/gh-write-guard.py b/host-setup/agent-safety/gh-write-guard.py index 5e142dfd..ae13c282 100644 --- a/host-setup/agent-safety/gh-write-guard.py +++ b/host-setup/agent-safety/gh-write-guard.py @@ -202,9 +202,9 @@ def _current_push_branch(cwd): # A newline ends a command exactly as `;` does, so it is an operator character here rather than whitespace. # Read as whitespace it vanishes when tokenizing, and every token on a later line of a multi-line command is then read as one more argument of the first line's command. # A backslash-newline continuation is folded to a space in `classify` before any of this runs, so every newline reaching the tokenizer is a real command separator. -_SHELL_OP_CHARS = set("();<>|&\n") -# The same operator set as a string, which is the form shlex takes it in. +# The string is the form shlex takes the set in, and the set is derived from it so the two cannot drift apart. _PUNCTUATION_CHARS = "();<>|&\n" +_SHELL_OP_CHARS = set(_PUNCTUATION_CHARS) def _shell_tokens(cmd): @@ -643,6 +643,8 @@ def classify(cmd, cwd=None, origin=None, current_branch=None, rules_lookup=None, ("git push origin feature/x\ngit push origin develop", None, {"feature/x": set(), "develop": _CODE_RULES}, "deny", "a second push on the next line is checked: develop denies"), ("git push \\\n origin develop", None, {"develop": _CODE_RULES}, "deny", "a backslash-newline is a continuation, not a separator: develop still parsed"), ("gh issue comment 5 --body \"one line\ngit push origin develop\"", None, {"develop": _CODE_RULES}, "allow", "a newline inside a quoted body does not start a new command"), + # Unbalanced quoting is what actually reaches the degraded path, and the separator has to survive there too. + ("git push origin feature/x\ngit push origin develop 'unclosed", None, {"feature/x": set(), "develop": _CODE_RULES}, "deny", "the degraded path keeps the newline: a push on the next line is still read"), ]