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..ae13c282 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. +# 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): - """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,15 @@ 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"), + # 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"), ]