From acb46d02f3c737aa76a33fb4c08bd6810eb2d805 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 22 Oct 2026 07:19:54 -0700 Subject: [PATCH 1/5] Skip the Invalid-Byte Selftest Probe When Windows Git Cannot Add the Name (#3350) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hub_tracked probe in the audit self-test aborted on Windows when git add exited 128 on its non-UTF-8 filename. Now only that add's failure skips the probe, and only on Windows. Any other git failure still aborts, and a failing add elsewhere still fails loudly. The Windows skip on the self-test wrapper test is removed. Tests simulate the Windows failure on Linux by patching the platform flag and subprocess.run, and skip on hosts that cannot create the name. Reverting either branch of the fix fails a test. Real Windows verification is this PR's Windows CI job. Closes #3338 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 5.5 --- spec/audit.py | 18 +++++++++-- tests/test_audit.py | 73 +++++++++++++++++++++++++++++++++++++++++++ tests/test_git_env.py | 4 --- 3 files changed, 88 insertions(+), 7 deletions(-) diff --git a/spec/audit.py b/spec/audit.py index 6403550ed..d6fbe33f7 100755 --- a/spec/audit.py +++ b/spec/audit.py @@ -3764,6 +3764,9 @@ def read_text(path): return findings, audited_sha +_IS_WINDOWS = sys.platform == "win32" + + def _selftest(): """Run the self-test with no variable naming a repository in the environment. @@ -4734,15 +4737,24 @@ def _selftest_cases(): ["git", "config", "user.name", "test"], ): subprocess.run(cmd, cwd=tmp_root_path, check=True, capture_output=True) + added = False try: (tmp_root_path / bad_name).write_text("x") except OSError: # This host's filesystem cannot represent the byte: skip rather than aborting the whole --selftest run over an environment limitation, not a code fault. print(" skip hub_tracked: non-UTF-8 filename (host cannot create it)") else: - subprocess.run( - ["git", "add", "-A"], cwd=tmp_root_path, check=True, capture_output=True - ) + try: + subprocess.run( + ["git", "add", "-A"], cwd=tmp_root_path, check=True, capture_output=True + ) + except subprocess.CalledProcessError: + if not _IS_WINDOWS: + raise + print(" skip hub_tracked: non-UTF-8 filename (git cannot add it)") + else: + added = True + if added: subprocess.run( ["git", "commit", "-q", "-m", "add invalid-utf8 name"], cwd=tmp_root_path, diff --git a/tests/test_audit.py b/tests/test_audit.py index 372b42684..7b20be889 100755 --- a/tests/test_audit.py +++ b/tests/test_audit.py @@ -4,7 +4,10 @@ from __future__ import annotations import base64 +import contextlib +import io import pathlib +import subprocess import sys import unittest from typing import Any @@ -193,5 +196,75 @@ def test_a_failed_shebang_read_raises_rather_than_reading_as_undecided(self) -> run_audit(tree, {"bin/Run_Backup": "100755"}, frozenset({"bin/Run_Backup"})) +def _probe_run(add: Any, commit: Any = None) -> Any: + """A subprocess.run stand-in whose answer to the invalid-byte probe's own git add is `add`.""" + real_run = subprocess.run + + def run(cmd: list[str], *args: Any, **kwargs: Any) -> Any: + cwd = kwargs.get("cwd") + if ( + cmd == ["git", "add", "-A"] + and cwd + and any(p.name.startswith("bad-") for p in pathlib.Path(cwd).iterdir()) + ): + return add(cmd) + if commit is not None and cmd[:2] == ["git", "commit"] and "add invalid-utf8 name" in cmd: + return commit(cmd) + return real_run(cmd, *args, **kwargs) + + return run + + +def _fail(cmd: list[str]) -> Any: + raise subprocess.CalledProcessError(128, cmd) + + +class TestSelftestInvalidByteProbe(unittest.TestCase): + def _selftest(self, run: Any, windows: bool) -> tuple[int, str]: + out = io.StringIO() + with ( + mock.patch.object(audit, "_IS_WINDOWS", windows), + mock.patch.object(subprocess, "run", run), + contextlib.redirect_stdout(out), + ): + result = audit._selftest() + return result, out.getvalue() + + def _require_probe_file(self, text: str) -> None: + if "host cannot create it" in text or "no candidate byte" in text: + self.skipTest("this host cannot exercise the invalid-byte probe") + + def test_a_failing_add_of_the_probe_file_skips_on_windows(self) -> None: + result, text = self._selftest(_probe_run(_fail), windows=True) + self._require_probe_file(text) + self.assertIn("skip hub_tracked: non-UTF-8 filename (git cannot add it)", text) + self.assertEqual(result, 0, text) + + def _error_of(self, run: Any, windows: bool) -> subprocess.CalledProcessError: + out = io.StringIO() + with ( + mock.patch.object(audit, "_IS_WINDOWS", windows), + mock.patch.object(subprocess, "run", run), + contextlib.redirect_stdout(out), + ): + try: + audit._selftest() + except subprocess.CalledProcessError as error: + return error + self._require_probe_file(out.getvalue()) + self.fail("the git failure did not propagate") + + def test_a_failing_add_of_the_probe_file_fails_loudly_elsewhere(self) -> None: + error = self._error_of(_probe_run(_fail), windows=False) + self.assertEqual(error.cmd, ["git", "add", "-A"]) + + def test_another_git_failure_still_fails_loudly(self) -> None: + run = _probe_run( + lambda cmd: subprocess.CompletedProcess(cmd, 0), commit=lambda cmd: _fail(cmd) + ) + error = self._error_of(run, windows=True) + self.assertEqual(error.cmd[:2], ["git", "commit"]) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_git_env.py b/tests/test_git_env.py index de0453bf8..1feb957b1 100755 --- a/tests/test_git_env.py +++ b/tests/test_git_env.py @@ -57,10 +57,6 @@ def test_a_variable_set_inside_the_context_does_not_outlive_it(self) -> None: self.assertNotIn("GIT_INDEX_FILE", os.environ) -@unittest.skipIf( - sys.platform == "win32", - "audit.py --selftest fails on Windows at its invalid-byte filename probe", -) class TestAuditSelftestIgnoresAmbientGitDir(unittest.TestCase): def test_the_selftest_leaves_the_ambient_repository_unchanged(self) -> None: with tempfile.TemporaryDirectory() as tmp: From e0a6dfcfa0fa686a6321837abca4efdaabf51de4 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 22 Oct 2026 09:12:37 -0700 Subject: [PATCH 2/5] Read a Backslash-Newline by Its Quoting in the Write Guard (#3357) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Narrows the write guard's backslash-newline denies, which since the last round read every backslash-newline in a command naming git or gh, quoting and heredoc data included. - A scan, `_line_break_contexts`, maps each backslash-newline to its context. - Unquoted, inside double quotes, in an unquoted heredoc, at a comment's end, and wherever the scan does not finish, the split-word and escaped-line-break denies stand as before. - A single unquoted backslash-newline between a command separator (`&&`, `||`, `|`, `|&`, `;`, `&`) and a non-operator character splits no word, so it is left to the fold. - Inside single quotes, or ending a quoted heredoc data line not followed by its delimiter, the outer shell keeps the backslash-newline as text. `_literal_span_kept` keeps such a span as text only where the hook can read it as a shell running it would. A single-quoted span must be its word's only quoted, escaped, or expanded part. A span naming git, gh, or a push must hold no further quoting, with each backslash-newline in its own command text or ending a comment there. A span not kept is read as develop reads it. - For a kept span, the command is classified again with each backslash-newline as the span's own scan places it, with each joined, and with each a line break, and denied where any reading is. - The README's requirement 4 paragraph describes the cases the denies skip. Refs #3344. This fixes these items from #3344: - "Over-Denials the Change Introduces": all three, a single-quoted backslash-newline, a Markdown hard line break in a quoted heredoc inside `"$(cat <<'EOF' ... EOF)"`, and `git commit -F- <<'EOF'` data ending in `\`. - The fourth-pass item on `_SPLIT_WORD` matching an operator glued to a line continuation, `git add x &&\` and `git log --oneline |\`. The other items in #3344 are not addressed here. ## Verification - Self-test passes. New cases cover each intended allow and a deny for each shape that must still deny: a split inside double quotes, in an unquoted heredoc, before a quoted heredoc's delimiter, inside `$'...'`, an `&\`-newline-`&` split operator, a redirection operator, single-quoted shell bodies (joined, escaped backslash, comment-ending, mixed comment-ending and splitting), quoted heredocs fed to a shell, a further quoting level, a `'\''`-joined word, and a span after an escaped word break. - Revert proofs: with the narrowing replaced by develop's whole-text regexes, the new allow cases fail. With no extra readings, the six shell-body deny cases fail. With the line-break reading removed, the two comment-ending cases fail. With only the joined and line-break readings, the three mixed cases fail. With every span kept, the nesting and word-adjacency cases fail. - Differential: every distinct command string the self-test and `test_install.py` pass to `classify`, plus constructed probes from this lane and its local review passes, classified under develop's guard and this branch's with identical offline seams. 10560 distinct commands: 3889 deny/deny, 6591 allow/allow, 78 deny to allow, 2 allow to deny, 0 errors. - The recorded self-test and `test_install.py` corpus flips only on this branch's seven new allow cases. - Every probe-only deny-to-allow flip is an intended form or a direct sibling (single-quoted data, quoted-heredoc data including Markdown bodies with apostrophes, a continuation glued to a separator), or a command whose plain form develop already allows (filed below, or harmless, such as a ref holding a backslash or a word with a plain suffix). - The two allow-to-deny flips are a spaced comment-ending backslash-newline in a `bash -c` body, which develop allowed while denying its plain form. - `test_install.py`, `prose_lint.py` (diff scope, sentence-length included), `repo_gate.py --check eol` and `eol-coverage`, `spec/validate.py`, ruff, and the markdownlint, cspell, and editorconfig-checker Docker linters are clean. ## Known Remaining Findings The local review reached its cap of six passes. The last two passes stopped before probing, so their probe phase ran in the lane itself over the separator and kept-span paths, with no new flip. These introduced findings remain, each a wording or over-deny item rather than an allow. Each is filed in #3358, since a fix push would need a pass past the cap: - **introduced, docs**: the README states the word rule as "its word's only quoted part", where the code also refuses an escaped or expanded part, and states the names test as "git or gh", where the code also tests a push in each reading. - **introduced, docs**: `_literal_span_kept`'s summary line reads as if every kept span is read as its inner scan places it, which its later lines qualify for a span naming no git or gh. - **introduced, over-deny**: for an escaped backslash before a kept newline, every reading drops the backslash pairs, where a shell keeps one literal backslash per pair. This can only make a reading deny more. - **introduced, residual over-deny**: a quoted span naming git or gh that also holds an apostrophe, double quote, backquote, or `<<` is read as develop reads it. A Markdown PR body mentioning `git` in a code span and holding a hard line break is still denied, as on develop. ## Filed Pre-existing gaps the local review found, each allowed on develop in its plain form: #3351 (a `cd` that does not lead the command, for rule 6), #3352 (a `trap` action string), #3353 (`)#` and `<<-` tab stripping), #3355 (a comment-ending backslash-newline at top level, and an `eval` body, for rule 6), #3356 (text piped into a shell, or produced by `$(...)` as a `-c` body). Design: #3354 (consolidate the guard's context scanners). ## #3341 Confirmation `BASH_BIN="$(command -v bash)"; git push --tags origin` and `bash -c 'command -v sh'; git push --tags origin` both classify allow on develop 201cbe78 with offline seams. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- host-setup/agent-safety/README.md | 14 +- .../agent-safety/claude/gh-write-guard.py | 393 +++++++++++++++++- 2 files changed, 401 insertions(+), 6 deletions(-) diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index e10c489f1..e9f398401 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -179,7 +179,19 @@ text says, because the harm it covers was never in the text. A command naming git or gh that holds a carriage return is denied, since the shell reads one as part of a word. So is one holding a word that a backslash-newline splits. So is one holding an escaped backslash before a line break. The shell reads that as a line break where the hook - would join the lines, and as a continuation inside a double-quoted wrapper body. Two comments whose + would join the lines, and as a continuation inside a double-quoted wrapper body. Neither deny + reads a backslash-newline the outer shell keeps as text, where the hook can read it as a shell + running that text would. That is one inside single quotes in command text, including command + text in a `$(...)` substitution. It is also one ending a quoted heredoc's data line, unless the + delimiter line follows. It is not one inside `$'...'`, or single quotes within double quotes, a + backquote, or `${...}`. A Markdown hard line break in a quoted heredoc or a single-quoted body + is one. The single-quoted part must be its word's only quoted part. Where the text names git or + gh, it must also hold no further quoting. Each backslash-newline in it must then sit in its + command text or end a comment there. The command is then read again as a shell running that text reads it, + joining the lines except where one ends a comment. It is also read with each one joined and with + each one a line break, and denied where any reading is. Nor does either read a single unquoted + backslash-newline between a command separator such as `&&` or `|` and a non-operator character, + which splits no word. Two comments whose apostrophes pair up as one quoted span are read as the shell reads them, as comments. The lines between them are then read as commands. Some words are read as they stand. They are a directory option's literal or double-quoted value where the guard can read its quoting, a commit option's double-quoted value, a diff --git a/host-setup/agent-safety/claude/gh-write-guard.py b/host-setup/agent-safety/claude/gh-write-guard.py index 9f49b25d4..9df355798 100755 --- a/host-setup/agent-safety/claude/gh-write-guard.py +++ b/host-setup/agent-safety/claude/gh-write-guard.py @@ -4107,8 +4107,8 @@ def _push_arg_lists(cmd): _CD_BUILTINS = ("cd", "pushd", "popd", "chdir") _UNRESOLVED = object() -_SPLIT_WORD = re.compile(r"(?<=\S)\\\r?\n(?=\S)") -_ESCAPED_LINE_BREAK = re.compile(r"(?()"): + raise ValueError("partly quoted heredoc delimiter") + tag = next(t for t in heredoc.group(2, 3, 4) if t is not None) + quoted = heredoc.group(4) is None or "\\" in heredoc.group(0) + pending.append((tag, bool(heredoc.group(1)), quoted)) + i = heredoc.end() + elif ch == "#" and word_start: + end = cmd.find("\n", i) + if end < 0: + return n + ctx[end] = "comment" + i = end + elif word_start and _CASE_KEYWORD.match(cmd, i): + cases += 1 if cmd.startswith("case", i) else -1 if cases else 0 + i += 4 + else: + if ch == "(": + depth += 1 + elif ch == ")" and depth: + depth -= 1 + elif ch == ")" and closer and not cases: + return i + 1 + i += 1 + if closer: + raise ValueError("unterminated group") + return n + + command(0, None) + return ctx + + +_PLAIN_WORD_PART = re.compile(r"[\w./:=@%+,-]*") +_NESTED_QUOTING = ("'", '"', "`", "<<") + + +def _literal_span_kept(cmd, start, stop, word, inner): + """True where a shell running the `literal` text `cmd[start:stop]` reads each backslash-newline in it as `inner` places it. + + A span in a word with any other quoted or expanded part is never kept, since the shell runs the whole word. + A span naming no git or gh, however its backslash-newlines are read, is kept, since its readings show what it runs. + Any other span is kept only where its own scan finished and it holds no quoting a further shell would unwrap. + Each backslash-newline in it must then sit in command text or end a comment there. + """ + if word: + head = cmd.rfind(" ", 0, start - 1) + 1 + for c in "\t\n;&|<>(": + head = max(head, cmd.rfind(c, 0, start - 1) + 1) + tail = _PLAIN_WORD_PART.match(cmd, stop + 1).end() + if ( + not _PLAIN_WORD_PART.fullmatch(cmd, head, start - 1) + or cmd[head - 2 : head - 1] == "\\" + or (tail < len(cmd) and cmd[tail] not in " \t\n;&|<>()") + ): + return False + text = cmd[start:stop] + if not any( + _names_git_or_gh(t) or _names_push(t) + for t in (text, text.replace("\\\n", ""), text.replace("\\\n", "\n")) + ): + return True + if inner is None or any(q in text for q in _NESTED_QUOTING): + return False + return all( + inner.get(m.end() - 1) in ("command", "comment") for m in _LINE_BREAK_RUN.finditer(text) + ) + + +def _line_break_reading(cmd): + """Read each backslash-newline in `cmd`, returning (denied, readings). + + `denied` is True where one in a context the shell joins splits a word, or follows an escaped backslash. + A split directly after an unquoted command separator such as `&&` or `|` splits no word, so it is left to the fold. + A `literal` one is not denied where `_literal_span_kept` keeps its span, and is read as any other one otherwise. + `readings` holds `cmd` read as a shell running a kept span may read it, or is empty where `cmd` holds none. + One reading takes each kept backslash-newline as the span's own scan places it. + A shell running the span reads one ending a comment there as a line break, and joins any other. + The other readings join each one, or read each one as a line break. + An escaped backslash before a kept newline is a line break in all of them. + A scan that does not finish leaves every backslash-newline in a context the shell joins. + """ + try: + ctx = _line_break_contexts(cmd) + except (ValueError, RecursionError): + ctx = {} + spans = {} + edits = [] + for m in _LINE_BREAK_RUN.finditer(cmd): + nl = m.end() - 1 + odd = (nl - m.start()) % 2 == 1 + kind = ctx.get(nl) + if isinstance(kind, tuple): + _, start, stop, word = kind + if (start, stop) not in spans: + try: + inner = _line_break_contexts(cmd[start:stop]) + except (ValueError, RecursionError): + inner = None + kept = _literal_span_kept(cmd, start, stop, word, inner) + spans[start, stop] = (inner or {}) if kept else None + inner = spans[start, stop] + if inner is not None: + joined = cmd[m.start() : nl - 1] if odd else "\n" + ends_comment = inner.get(nl - start) == "comment" + edits.append((m.start(), nl + 1, joined, "\n" if ends_comment else joined)) + continue + kind = None + if not odd: + return True, () + before, after = cmd[nl - 2 : nl - 1], cmd[nl + 1 : nl + 2] + if not before.strip() or not after.strip(): + continue + k = nl - 1 + while k and cmd[k - 1] in "();<>|&": + k -= 1 + glued = ( + kind == "command" + and m.start() == nl - 1 + and cmd[k : nl - 1] in _GLUED_SEPARATORS + and cmd[k - 1 : k] != "\\" + and after not in _SHELL_OP_CHARS + ) + if not glued: + return True, () + if not edits: + return False, () + readings = [] + for pick in (3, 2, None): + out, at = [], 0 + for edit in edits: + out.append(cmd[at : edit[0]] + ("\n" if pick is None else edit[pick])) + at = edit[1] + readings.append("".join(out) + cmd[at:]) + return False, tuple(dict.fromkeys(readings)) + + def _opaque_cd(toks): """True when something in the command may change the directory or the repository without a literal `cd`. @@ -8645,9 +8866,12 @@ def classify( "than as a space or a line break, so the guard cannot tell where its words end. " "Failing closed. Write the command with plain newlines." + _handoff(cmd) ) - if (_SPLIT_WORD.search(cmd) or _ESCAPED_LINE_BREAK.search(cmd)) and ( - _names_git_or_gh(cmd) or _names_push(cmd) - ): + denied, readings = ( + _line_break_reading(cmd) + if _LINE_BREAK_RUN.search(cmd) and (_names_git_or_gh(cmd) or _names_push(cmd)) + else (False, ()) + ) + if denied: return "deny", ( "This command has a word split by a backslash-newline, or an escaped backslash before " "a line break. The shell joins the first into one word the guard cannot read. It reads " @@ -8655,6 +8879,23 @@ def classify( "inside a double-quoted wrapper body. A hidden cd could redirect a push, or a split value add a " "subcommand. Write each word whole on one line." + _handoff(cmd) ) + for reading in readings: + dec, reason = classify( + reading, + cwd, + origin, + current_branch, + rules_lookup, + environ, + primary_checkout_lookup, + ref_resolver, + config_lookup, + ) + if dec == "deny": + return "deny", ( + "Read as a shell running its quoted text reads it, with a backslash-newline there " + "joined or read as a line break, this command is denied. " + reason + ) # Fold shell line-continuations so a multi-line Bash invocation, such as `gh pr merge 5 \ --admin`, parses as one command. # Only backslash-newline is joined, so a real newline between commands still separates them. cmd = re.sub(r"\\\r?\n", " ", cmd) @@ -9004,6 +9245,116 @@ def _nested_wrapper(body, depth): "deny", "an escaped backslash before a newline is no line continuation, so the next line is read", ), + ( + "gh pr comment 5 --body 'a\\\nb'", + "allow", + "a backslash-newline inside single quotes is literal, so it splits no word", + ), + ( + "gh pr comment 5 --body 'a\\\\\nb'", + "allow", + "an escaped backslash before a newline inside single quotes is literal too", + ), + ( + "gh pr create --title x --body \"$(cat <<'EOF'\nline one\\\nline two\nEOF\n)\"", + "allow", + "a Markdown hard line break in a quoted heredoc inside a substitution is data", + ), + ( + "git commit -F- <<'EOF'\nsubject\n\nline one\\\nline two\nEOF", + "allow", + "a line ending in a backslash in a quoted heredoc is data", + ), + ( + 'git commit -F- <<"EOF"\nline one\\\\\nline two\nEOF', + "allow", + "an escaped backslash ending a line in a double-quoted-delimiter heredoc is data", + ), + ( + "git add x &&\\\ngit commit -m y", + "allow", + "a line continuation glued to && splits no word", + ), + ( + "git log --oneline |\\\nhead -5", + "allow", + "a line continuation glued to a pipe splits no word", + ), + ( + "git add x &\\\n& git commit -m y", + "deny", + "a line continuation inside && joins the operator the guard reads apart", + ), + ( + "git status 2>\\\n&1", + "deny", + "a line continuation after a redirection operator still fails closed", + ), + ( + 'git commit -m "a\\\nb" --no-verify', + "deny", + "a backslash-newline inside double quotes splits a word, as the shell joins it there", + ), + ( + "git commit -F- < Date: Thu, 22 Oct 2026 10:47:51 -0700 Subject: [PATCH 3/5] Skip the Windows Self-Test Probe Only on Git's Invalid-Path Error (#3360) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Skip the Windows invalid-byte self-test probe only when `git add` reports an invalid path, and re-raise any other `CalledProcessError`. - Pin the narrowing in the probe tests: the skip case carries the invalid-path stderr, and a new case shows an unrelated Windows add failure still raises. The previous skip covered every Windows `git add` failure, so an unrelated failure in the temporary repository passed `--selftest`. This addresses a review finding on the develop to main promotion. The Windows message text is reasoned from git's source and was not run on Windows from this host. Local review: one pass, one finding (the existing test's stub carried no stderr), fixed in the second commit. Revert proof: with the old `audit.py`, the new unrelated-failure test fails. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 5.5 --- spec/audit.py | 12 +++++++++++- tests/test_audit.py | 21 +++++++++++++-------- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/spec/audit.py b/spec/audit.py index d6fbe33f7..934500cde 100755 --- a/spec/audit.py +++ b/spec/audit.py @@ -4737,6 +4737,13 @@ def _selftest_cases(): ["git", "config", "user.name", "test"], ): subprocess.run(cmd, cwd=tmp_root_path, check=True, capture_output=True) + (tmp_root_path / "control.txt").write_text("x") + subprocess.run( + ["git", "add", "--", "control.txt"], + cwd=tmp_root_path, + check=True, + capture_output=True, + ) added = False try: (tmp_root_path / bad_name).write_text("x") @@ -4746,7 +4753,10 @@ def _selftest_cases(): else: try: subprocess.run( - ["git", "add", "-A"], cwd=tmp_root_path, check=True, capture_output=True + ["git", "add", "--", bad_name], + cwd=tmp_root_path, + check=True, + capture_output=True, ) except subprocess.CalledProcessError: if not _IS_WINDOWS: diff --git a/tests/test_audit.py b/tests/test_audit.py index 7b20be889..2dc957579 100755 --- a/tests/test_audit.py +++ b/tests/test_audit.py @@ -196,17 +196,15 @@ def test_a_failed_shebang_read_raises_rather_than_reading_as_undecided(self) -> run_audit(tree, {"bin/Run_Backup": "100755"}, frozenset({"bin/Run_Backup"})) -def _probe_run(add: Any, commit: Any = None) -> Any: - """A subprocess.run stand-in whose answer to the invalid-byte probe's own git add is `add`.""" +def _probe_run(add: Any, commit: Any = None, control: Any = None) -> Any: + """A subprocess.run stand-in answering the invalid-byte probe's own git add of the bad name with `add`.""" real_run = subprocess.run def run(cmd: list[str], *args: Any, **kwargs: Any) -> Any: cwd = kwargs.get("cwd") - if ( - cmd == ["git", "add", "-A"] - and cwd - and any(p.name.startswith("bad-") for p in pathlib.Path(cwd).iterdir()) - ): + if control is not None and cmd == ["git", "add", "--", "control.txt"]: + return control(cmd) + if cmd[:3] == ["git", "add", "--"] and cwd and cmd[3].startswith("bad-"): return add(cmd) if commit is not None and cmd[:2] == ["git", "commit"] and "add invalid-utf8 name" in cmd: return commit(cmd) @@ -254,9 +252,16 @@ def _error_of(self, run: Any, windows: bool) -> subprocess.CalledProcessError: self._require_probe_file(out.getvalue()) self.fail("the git failure did not propagate") + def test_a_failing_control_add_fails_loudly_on_windows(self) -> None: + error = self._error_of( + _probe_run(lambda cmd: subprocess.CompletedProcess(cmd, 0), control=_fail), + windows=True, + ) + self.assertEqual(error.cmd, ["git", "add", "--", "control.txt"]) + def test_a_failing_add_of_the_probe_file_fails_loudly_elsewhere(self) -> None: error = self._error_of(_probe_run(_fail), windows=False) - self.assertEqual(error.cmd, ["git", "add", "-A"]) + self.assertEqual(error.cmd[:3], ["git", "add", "--"]) def test_another_git_failure_still_fails_loudly(self) -> None: run = _probe_run( From 053dd106247f4aeb3e85ef38d62c5916b5235569 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 22 Oct 2026 14:11:17 -0700 Subject: [PATCH 4/5] Classify a Subprocess Failure by Its Command, Not Its Message Text (#3366) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Adds one bullet to `CODESTYLE.md` "General" > "Operations and Failure Messages", beside "Let the real operation be the check": > **Classify a subprocess failure by which command failed and the status code it reports, never by the wording of its message.** That wording varies by tool version and locale, and a status code does not, even one printed inside the message. Where one command's status code cannot separate an expected failure from an unexpected one, split it into real operations, one per failure. ## Why A skip that matched a command's stderr for a phrase never fired on a host whose tool printed different wording. Message wording varies by tool version and locale. The failing command and its status code do not. Constructed example of the split the rule asks for, where only the probe's own add may skip: ```python subprocess.run(["git", "add", "--", "control.txt"], cwd=repo, check=True) try: subprocess.run(["git", "add", "--", probe_name], cwd=repo, check=True) except subprocess.CalledProcessError: skip("this host's git cannot add the probe name") ``` ## Wording Beyond the Issue's Proposal #3363 proposed "exit status" and "message text". A tool such as `gh api` exits 1 for every HTTP error, so only the HTTP status it prints separates an absent object from a credential or network failure, and `scripts/pr_review.py` reads it that way. Under "exit status" alone that code had no compliant form, which a local pass and the Copilot round both raised. The bullet therefore names "the status code it reports" and counts one printed inside the message. ## Local Review Four recorded local strict review passes, the cap for this change. - Pass 1: the split could be read as licensing a probe, which the bullet above forbids. Fixed by naming "real operations". It also raised the `gh api` exit-status case, first deferred to #3365 and then fixed in 8c57341b after Copilot raised it too. - Pass 2: the same probe reading, as borderline. Declined, since "real operations" is the adjacent bullet's term and that bullet forbids probing first. - Pass 3: a status code printed inside the message was not said to count. Fixed in 721f3957. - Pass 4 raised the findings below, still open at the cap and deferred to #3367 rather than growing the bullet further: - `introduced`: possible conflict with "Command-Line Arguments", whose refusal-wording contract the suite verifies by matching stderr text. - `introduced`: locating a printed status code still matches the text around it, so "a status code does not [vary]" holds for the value only. - `style`: "it" in the split sentence sits nearest "status code", and "one per failure" can read as per occurrence. - The `unknown flag` stderr retry in `scripts/pr_review.py` classifies by wording, tracked in #3365. Refs #3363 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by CodeRabbit * **Documentation** * Clarified how to classify subprocess failures: use the failed command and its status code rather than message wording. When the status code alone cannot distinguish expected failures from unexpected ones, document separate operations for the commands. This guidance helps make failure handling more consistent and easier to interpret. --------- Co-authored-by: Claude Opus 5.5 --- CODESTYLE.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CODESTYLE.md b/CODESTYLE.md index da7f86f5a..4ff129457 100644 --- a/CODESTYLE.md +++ b/CODESTYLE.md @@ -32,6 +32,7 @@ Each language defines a **clean-compile** verification: the combination of build ### Operations and Failure Messages - **Let the real operation be the check, not a prediction of it.** Attempt the read, the write, or the call itself, and turn its failure into a clean refusal. Do not probe first with an existence, type, or permission test. Where the operation could block or succeed on the wrong kind of target, judge what it opened instead. A probe is a second model of the operation. It misjudges each case its author did not foresee, such as a dangling link, a link loop, or a platform's own permission model. The operation cannot disagree with itself. +- **Classify a subprocess failure by which command failed and the status code it reports, never by the wording of its message.** That wording varies by tool version and locale, and a status code does not, even one printed inside the message. Where one command's status code cannot separate an expected failure from an unexpected one, split it into real operations, one per failure. - **A failure message states the error, the path or other target, and the remedy, and claims nothing about the state the failure left.** The error the operation raised is the one fact the code holds at that point. A description of what the failure left behind is a prediction. It reads false once an unforeseen state reaches the message, such as a file written in full that then fails to close. A remedy such as "fix what it names, then re-run" needs no such claim. ### Command-Line Arguments From 4f16a3e896ec6f78e04256db07285864abea67b9 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 22 Oct 2026 14:11:41 -0700 Subject: [PATCH 5/5] Keep Escaped Backslash Pairs Before a Line Break in the Write Guard (#3368) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fixes the first three findings of #3358 in the write guard's backslash-newline narrowing. - **Escaped backslash pairs before a kept newline.** A shell running a kept quoted span (a single-quoted shell body, or a quoted heredoc fed to a shell) reads each escaped backslash pair before a line break as one literal backslash on the word. Every reading `_line_break_reading` built dropped the whole run instead, so `bash -c 'git push origin main\\echo hi'` was read as a push of `main`, where the inner shell pushes `main\`. The readings now keep the pairs as script text, so a shell reading them still sees one literal backslash per pair. A space parts the pairs from the line break, so no reading holds a backslash-newline that `classify` would read again. The odd-run line-break reading keeps its pairs the same way. Outside a kept span nothing changes, and an escaped backslash before a line break is still denied there. - **`_literal_span_kept` docstring.** The summary line no longer reads as if every kept span is read as its own scan places it. A span naming no git, gh, or push is kept whatever its scan found, and the word test now names an escaped part and an escaped word break before the word. - **README requirement 4.** States the word test (no other quoted, escaped, or expanded part, and no escaped word break just before it) and the names test (git, gh, or a push, read as written, joined, and with line breaks), and says every reading keeps the escaped pairs. Out of scope: #3358's fourth finding, a Markdown body naming git or gh across a further quoting level. It goes to #3354's single context walker and is not attempted here. ## Verification - `python3 host-setup/agent-safety/claude/gh-write-guard.py --selftest`: `SELFTEST PASS`, with new `_CASES` entries: four allows, plus control denies for the line after a pair and for a pair ending a comment in a shell body. - Revert proof: with only the `_line_break_reading` change reverted and the new cases kept, the self-test fails on exactly the four new allow cases (`deny` where `allow` is wanted). The control denies pass either way. - Local strict review: two passes. Pass 1 raised a docstring overclaim (a joined odd run before an empty line leaves pairs before a newline, which `classify` re-reads and denies correctly) and a missing comment-ending test. Both are fixed in the second commit, and pass 2 raised nothing. - ruff format and check, mypy, `prose_lint.py --diff` (default and `--check sentence-length`), the eol gate, and markdownlint on the README are clean. ### Develop-versus-branch differential Every command classified by develop (`075ec57a`) and by this branch with the self-test's pins: | Corpus | Commands | Flips | | --- | --- | --- | | develop's own self-test case lists | 1618 | 0 | | constructed probes | 280 | 30, all deny -> allow | The probes are 40 templates, each with a backslash run of 0 to 6 before a newline. They cover even and odd runs, outside quotes, inside a single-quoted `bash -c` / `sh -c` body, inside a double-quoted body, in a quoted heredoc fed to `bash`, in a `$(...)` substitution, in `gh pr comment` / `gh pr create` bodies, and next to git, gh, push, `cd`, `-C`, `--no-verify`, `--force`, and a comment. Every flip is intended. Each has a run of 2 to 6 backslashes, so at least one escaped pair, before the newline in a kept span: - `bash -c 'git push origin mainecho hi'`, `bash -c 'git push origin main'`, the same in a quoted heredoc fed to `bash`, and inside `echo "$(...)"` (20 flips). The inner shell's refspec is `main\` (even run, followed by a line break) or `main\echo` (odd run, joined). A ref name cannot hold a backslash, so git refuses either and no push of `main` runs. - `bash -c 'git push origin HEAD:main'` (5 flips). The destination is `main\`, which git refuses for the same reason. - `bash -c 'ghpr merge 5 --admin'` (5 flips). The inner shell runs a command named `gh\` (even run) or `gh\pr` (odd run joined), not gh. The single-backslash run (a plain continuation) and the plain newline flip nothing, and the control shapes that must stay denied, such as a later line pushing `main`, a `cd` before a push, `-C` split from `push`, and `--no-verify` on the next line, stay denied. Refs #3358 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- host-setup/agent-safety/README.md | 13 ++-- .../agent-safety/claude/gh-write-guard.py | 59 ++++++++++++++++--- 2 files changed, 58 insertions(+), 14 deletions(-) diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index e9f398401..7e27b3005 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -185,11 +185,14 @@ text says, because the harm it covers was never in the text. text in a `$(...)` substitution. It is also one ending a quoted heredoc's data line, unless the delimiter line follows. It is not one inside `$'...'`, or single quotes within double quotes, a backquote, or `${...}`. A Markdown hard line break in a quoted heredoc or a single-quoted body - is one. The single-quoted part must be its word's only quoted part. Where the text names git or - gh, it must also hold no further quoting. Each backslash-newline in it must then sit in its - command text or end a comment there. The command is then read again as a shell running that text reads it, - joining the lines except where one ends a comment. It is also read with each one joined and with - each one a line break, and denied where any reading is. Nor does either read a single unquoted + is one. The single-quoted part must be its word's only quoted, escaped, or expanded part, with no + escaped word break just before it. Where the text names git, gh, or a push, it must also hold no + further quoting. Those names are read in the text as written, with its lines joined, and with + each one a line break. Each backslash-newline in that text must then sit in its command text or + end a comment there. The command is then read again as a shell running that text reads it, + joining the lines except where one ends a comment. It is also read with each one joined and with each one a line break, and denied where + any reading is. Every reading keeps each escaped backslash pair before a line break, which the + shell reads as one literal backslash. Nor does either read a single unquoted backslash-newline between a command separator such as `&&` or `|` and a non-operator character, which splits no word. Two comments whose apostrophes pair up as one quoted span are read as the shell reads them, as comments. The diff --git a/host-setup/agent-safety/claude/gh-write-guard.py b/host-setup/agent-safety/claude/gh-write-guard.py index 9df355798..99dfb7f68 100755 --- a/host-setup/agent-safety/claude/gh-write-guard.py +++ b/host-setup/agent-safety/claude/gh-write-guard.py @@ -4239,12 +4239,13 @@ def command(i, closer): def _literal_span_kept(cmd, start, stop, word, inner): - """True where a shell running the `literal` text `cmd[start:stop]` reads each backslash-newline in it as `inner` places it. + """True where the readings of the `literal` text `cmd[start:stop]` show what a shell running it runs. - A span in a word with any other quoted or expanded part is never kept, since the shell runs the whole word. - A span naming no git or gh, however its backslash-newlines are read, is kept, since its readings show what it runs. + A span in a word with any other quoted, escaped, or expanded part is never kept, since the shell runs the whole word. + Nor is one with an escaped word break just before it. + A span naming no git, gh, or push, as written, joined, or with line breaks, is kept whatever its own scan found. Any other span is kept only where its own scan finished and it holds no quoting a further shell would unwrap. - Each backslash-newline in it must then sit in command text or end a comment there. + Each backslash-newline in it must then sit in command text or end a comment there, as `inner` places it. """ if word: head = cmd.rfind(" ", 0, start - 1) + 1 @@ -4280,7 +4281,8 @@ def _line_break_reading(cmd): One reading takes each kept backslash-newline as the span's own scan places it. A shell running the span reads one ending a comment there as a line break, and joins any other. The other readings join each one, or read each one as a line break. - An escaped backslash before a kept newline is a line break in all of them. + Each escaped backslash pair before a kept newline stays in all of them, read as one literal backslash. + A space parts those pairs from each line break a reading puts after them, so that break is not read again. A scan that does not finish leaves every backslash-newline in a context the shell joins. """ try: @@ -4304,9 +4306,13 @@ def _line_break_reading(cmd): spans[start, stop] = (inner or {}) if kept else None inner = spans[start, stop] if inner is not None: - joined = cmd[m.start() : nl - 1] if odd else "\n" + pairs = cmd[m.start() : nl - 1 if odd else nl] + broken = f"{pairs} \n" if pairs else "\n" + joined = pairs if odd else broken ends_comment = inner.get(nl - start) == "comment" - edits.append((m.start(), nl + 1, joined, "\n" if ends_comment else joined)) + edits.append( + (m.start(), nl + 1, joined, broken if ends_comment else joined, broken) + ) continue kind = None if not odd: @@ -4329,10 +4335,10 @@ def _line_break_reading(cmd): if not edits: return False, () readings = [] - for pick in (3, 2, None): + for pick in (3, 2, 4): out, at = [], 0 for edit in edits: - out.append(cmd[at : edit[0]] + ("\n" if pick is None else edit[pick])) + out.append(cmd[at : edit[0]] + edit[pick]) at = edit[1] readings.append("".join(out) + cmd[at:]) return False, tuple(dict.fromkeys(readings)) @@ -9325,6 +9331,41 @@ def _nested_wrapper(body, depth): "deny", "an escaped backslash before a newline in a single-quoted body is a line break to the inner shell", ), + ( + "bash -c 'git push origin main\\\\\necho hi'", + "allow", + "an escaped backslash pair before a line break in a shell body stays a literal backslash on the word", + ), + ( + "bash <<'EOF'\ngit push origin main\\\\\necho hi\nEOF", + "allow", + "a quoted heredoc fed to a shell keeps the literal backslash an escaped pair leaves before a line break", + ), + ( + "bash -c 'git push origin HEAD:main\\\\\\\n'", + "allow", + "an escaped pair before a backslash-newline in a shell body stays a literal backslash in every reading", + ), + ( + "bash -c 'gh\\\\\npr merge 5 --admin'", + "allow", + "an escaped backslash pair after gh in a shell body names a command other than gh", + ), + ( + "bash -c 'git push origin feature/x\\\\\ngit push origin main'", + "deny", + "control: the line after an escaped backslash pair in a shell body is still read", + ), + ( + "bash -c 'true # x\\\\\ngit push origin main'", + "deny", + "control: an escaped backslash pair ending a comment in a shell body leaves the next line read", + ), + ( + "bash -c 'true # x\\\\\\\ngit push origin main'", + "deny", + "control: an escaped pair before a comment-ending backslash-newline leaves the next line read", + ), ( "bash -c 'true # x\\\ngh pr merge 5 --ad\\\nmin'", "deny",