Repository navigation
Merged
Conversation
…Name (#3350) 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 <noreply@anthropic.com>
## 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 201cbe7 with offline seams. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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. Comment |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The Windows probe currently risks masking unrelated Git failures, which must be restricted to the intended unsupported-filename case.
1 open issue
What changed in this PR
This pull request improves Windows audit self-test compatibility and refines write-guard handling of backslash-newline sequences according to shell quoting context.
Changes:
- Handles unsupported invalid-byte filenames during the Windows audit self-test.
- Adds tests for Windows-specific behavior and propagation of unrelated Git failures.
- Updates write-guard parsing and documentation for quoted data, heredocs, comments, and shell continuations.
| File | Summary |
|---|---|
tests/test_git_env.py |
Updates Git environment self-test coverage for Windows. |
tests/test_audit.py |
Adds coverage for invalid-byte probe behavior and failure propagation. |
spec/audit.py |
Handles Windows Git failures during the invalid-byte filename probe. Moderate [P1] finding, 2 votes: unrelated git add failures must not be converted into skips. |
host-setup/agent-safety/README.md |
Documents the refined line-continuation behavior. |
host-setup/agent-safety/claude/gh-write-guard.py |
Adds context-aware line-continuation parsing and regression cases. Nit, 1 vote: share scanning logic with _context_lex to avoid divergent parsers. |
Review effort: Lite
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3359 +/- ##
===========================================
+ Coverage 65.53% 81.32% +15.79%
===========================================
Files 18 18
Lines 10036 10047 +11
===========================================
+ Hits 6577 8171 +1594
+ Misses 3459 1876 -1583
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
) ## 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 <noreply@anthropic.com>
…3366) ## 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 8c57341 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 721f395. - 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) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…3368) ## 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\\<newline>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 main<run><newline>echo hi'`, `bash -c 'git push origin main<run><newline>'`, 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<run><newline>'` (5 flips). The destination is `main\`, which git refuses for the same reason. - `bash -c 'gh<run><newline>pr 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 <noreply@anthropic.com>
ptr727
changed the title
Owner
Author
|
Triage of the Copilot round on Declined, with evidence: the headline counts findings the round does not hold. The same review body states "No open issues", opens no thread, holds no collapsed low-confidence or previously-missed section, and lists a single finding, "Only skip git add failures caused by invalid filenames", under "resolved since last review". That thread was fixed by #3360 and is resolved. Every review thread on this pull request is resolved, so the headline's count names nothing left to act on. Coverage for this promotion, under the Merge Gate's promotion case, since the round at Lite effort states none. Each constituent pull request had full coverage at its merged head:
|
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.
Summary
CODESTYLE.mdGeneral rule: classify a subprocess failure by which command failed and the status code it reports, never by the wording of its message (Classify a Subprocess Failure by Its Command, Not Its Message Text #3366).Closes #3338
Closes #3358
Closes #3363
Refs #3344, which stays open because only part of it is fixed here. #3358's Markdown-body finding moves to #3354.
🤖 Generated with Claude Code