Repository navigation
Merged
Conversation
Closes #3373. The prose gate gains a fixed tier 4 of emoji callout markers, per the maintainer's recorded decision. Each marker is admitted alone or directly followed by VARIATION SELECTOR-16 (U+FE0F). Every other emoji and a U+FE0F not directly after a marker still report `charset-unknown`. Downstream needs no markup. Markers chosen (code point, name): - U+26A0 WARNING SIGN - U+2139 INFORMATION SOURCE - U+2705 WHITE HEAVY CHECK MARK - U+274C CROSS MARK - U+1F4DD MEMO (the note marker) - U+1F4A1 ELECTRIC LIGHT BULB Also: U+2139 is category Ll, so a new `starts_lowercase` helper keeps a marker opening a comment or line from tripping `comment-case`, `comment-wrap`, or `sentence-split`. Docs updated: the `comment-and-doc-style` Skill "Character set" section (source in `.agents/skills/`, generated trees regenerated with `build_dist.py`), `scripts/README.md`, and the gate's own docstring. Open for the maintainer, outside this change's file set: - `GOVERNANCE.md` line 174 lists the tiers as "never legitimate, legitimate next to a number, always legitimate, developer-typed Unicode". Proposed wording: "never legitimate, legitimate next to a number, always legitimate, a fixed set of emoji callout markers, developer-typed Unicode". - `spec/readme-structure.md` line 55 says "the three tiers in GOVERNANCE.md"; proposed: "the tiers". Local review findings left open after the two-round budget (for the review loop): - `CHARSET_CLASSIFY` and the assertion at `tests/test_prose_lint.py` still name only TIER1, TIER2, TIER3. - Tier 4 is not coupled to the Skill text in `TestCommentAndDocStyleSkillCoupling`, nor in the tier-overlap test. - The three `starts_lowercase` call sites are not each pinned by an end-to-end bait. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
## Summary Resolves the three open review findings on the `CODESTYLE.md` "Operations and Failure Messages" subprocess-failure bullet. The bullet stays a rule plus its reason, with no list of tools or cases. - **Finding 1, the apparent conflict with "Command-Line Arguments".** A closing sentence states that a test asserting a script's output, such as the refusal "Command-Line Arguments" requires, verifies that output rather than classifying a failure. It names a test, so a runtime fallback branching on a refusal's prose cannot claim it. The head rule now forbids classifying by the *prose* of a message, so the notation that marks a printed code is not read as forbidden wording. - **Finding 2, a printed status code located by the text around it.** Maintainer ruling on #3367 (option B): a printed code is read by the notation that marks it, and a match finding none counts as an unexpected failure. The reason sentence's stability claim now names the code's value. - **Finding 3, the pronoun and the quantifier.** "split it into real operations, one per failure" becomes: where one command's status code cannot separate the failures its caller handles differently, give each expected failure its own real operation, and where no such operation exists, treat the failure as unexpected. That names what gets an operation, covers two expected failures sharing one code, and gives the case with no operation the same fail-closed outcome as a printed code with no match. No generated include carries this bullet. `python-codestyle` and `shell-codestyle` include only `CODESTYLE.md > Command-Line Arguments`, so `scripts/build_dist.py --check` passes with nothing to regenerate. ## Rulings Recorded - Maintainer ruling on #3367 finding 2: option B, the notation sentence above. - Maintainer ruling on #3365: line 34, "Let the real operation be the check", stays unchanged. A capability check answered by its own exit status counts as a real operation of its own, which the follow-up change for #3365 applies. - Declined, a local pass's finding that "a status code's value does not [vary]" overstates, since tools sometimes change exit codes. That trade-off was already accepted on #3366, and the clause stays. - Dropped "and status code" from the remedy, as a local pass found. Separate operations are already told apart by which command failed, so a distinct code is not required. ## Local Review Findings Still Open The local pass's edit budget for this push is spent, so this goes to the review loop for an outcome before merge. 1. `introduced`: "give each expected failure its own real operation" could be read as allowing an existence test such as `test -e src` before a copy, which line 34 forbids. Sites that still classify by prose are filed as #3378 (`spec/audit.py`, `scripts/skills_install.py`), beside #3365 for `scripts/pr_review.py`. Closes #3367 🤖 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 subprocess failures are classified, including how status codes and missing codes are handled. * Specified when commands should be split based on whether their status codes distinguish failures requiring different handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Closes #3379. The GOVERNANCE.md "Character Set" pointer now lists the fixed set of emoji callout markers between the always-legitimate tier and developer-typed Unicode, matching the `comment-and-doc-style` Skill. The `spec/readme-structure.md` bullet now points at that Skill's "Character set" section instead of a GOVERNANCE.md section that holds no tiers. Its trailing clause about the em dash and spaced hyphen ban stays true, since the Skill section states both bans. A tree grep outside reports/ found no other summary counting or listing the tiers stale. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
…al (#3386) ## Summary Moves the `scripts/pr_review.py` job-log read onto the `CODESTYLE.md` "Operations and Failure Messages" rule, as merged in #3377. A subprocess failure is classified by which command failed and the status code it reports, never by the prose of its message. - `gh_rest` used to retry a raw read without `--allow-escape-sequences` when its stderr held `unknown flag: --allow-escape-sequences`. `gh` exits 1 for an unknown flag and for an HTTP error alike, so the read's own status could not tell them apart. - New `gh_takes_escape_flag()` runs `gh api --allow-escape-sequences --help` once per process (`functools.cache`). Its exit status alone decides whether the read passes the flag. It reads nothing from GitHub. - A failed read is returned as it is, whatever its message says, with no retry. A non-log read never runs the probe. Measured on real binaries: the probe exits 0 on gh 2.103.0, and exits 1 on gh 2.90.0, which predates the flag. Neither needs the network. ## Tests `TestRawLogRead` clears the probe's cache in `setUp` and in `addCleanup`, the way `GqlCase.setUp` clears the other memoized helpers. It covers: - an older `gh` reading without the flag - a newer `gh` reading with it - the probe running once for two reads - a non-log read never asking - `test_another_failure_is_not_retried`, which keeps its intent. A failed read after the probe is returned unchanged after one attempt, including one whose stderr says `unknown flag`. `python3 -m unittest tests.test_pr_review` passes all 954 tests. ## Rulings Recorded - Maintainer ruling on #3365: option (a), a cached capability probe with its exit code as the answer. Line 34 of `CODESTYLE.md`, "Let the real operation be the check", stays unchanged, and the probe counts as a real operation of its own. The other sites of the same class are tracked in #3378. Closes #3365 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…3390) ## Summary Closes the write guard's export, xargs, env, and launcher allow gaps for #3336, #3339, #3340, #3343, #3347, the open items of #3307, and #3337 under the maintainer's ruling recorded on that issue. The closing keywords go on the `develop -> main` promotion pull request. This change only closes allow gaps: no command develop's guard denies becomes allowed. - **One export primitive (#3339, #3340, #3347).** `_export_events` reads an `export`, a `declare`, `typeset`, or `local` given `-x`, and a bare-name export. A bare-name export reads every value a plain assignment, `declare`, `typeset`, `local`, or `readonly` gave that name. A plain assignment to an exported name exports too, and so does one after `set -a` or `set -o allexport`. `_exported_states`, `_env_assignments`, `_reading_env_config_values`, and `_wrapper_env_groups` all read it. Where a loop or a function may run a git word after a later export, every export is read. - **Run-time `GIT_DIR` (#3343).** A mutating git command whose `GIT_DIR`, `GIT_WORK_TREE`, `--git-dir`, or `--work-tree` value holds a run-time word other than `$HOME` fails closed. - **Launchers (#3340, #3347).** Behind a launcher the guard does not know, an assignment after an `env` or a `sudo` is read beside the shell's state rather than over it. That holds for `GIT_DIR`, config values, and `GH_REPO`, so `uv run env GH_REPO=stranger/example gh pr comment ...` is denied, and `find ./env -name GIT_DIR=... -exec git ...` cannot hide an export. - **xargs (#3336, #3307, #3340, #3347).** These now fail closed: a gh write that `xargs` runs, a bare `gh`, a `gh api` call it appends to, and a bare `git`. So do a replace string in git's subcommand, a `-c` value, or a mutating command's global options, and a shell string xargs fills that runs git or gh. The exception is a written-out gh write fed only inert literal words, as in `printf 5 | xargs gh pr comment --body x`. The config-change scan reads a `git config` xargs appends to, a replace string in the command word, and the replace option inside a cluster such as `-0I{}` or as the BSD `-J`. - **Config-change scan (#3307, #3340).** A wrapper body is read with the same per-word predicate as the outer command, `_word_may_change_config`, and a named coproc's group is read. A push denied because an earlier command may rewrite its remote config says so, and asks for the push as its own command. - **Outer-expanded wrapper bodies (#3337).** A double-quoted wrapper body holding a `$` or backtick word the outer shell expands fails closed where the body runs git or gh. A body running neither keeps its verdict, so `bash -c "cd $DIR && ls"` is allowed, and so is the issue's own `sh -c "echo $B"` example. - READMEs updated to match. ## Verification - `python3 host-setup/agent-safety/claude/gh-write-guard.py --selftest`: `SELFTEST PASS`, 1868 checks, 86 of them new: 59 denies, 26 allow controls, and one check that the config-change deny names its cause. - ruff format and check, mypy, and `prose_lint.py --diff` are clean, and the pre-commit and pre-push hooks passed. `host-setup/agent-safety/claude/test_install.py` passed 162 of 163 in a full run. The one failure, `test_a_write_cut_short_is_not_read_back_as_a_hand_edit`, passed when rerun alone, on this branch and on develop's tree alike. ### Develop-versus-branch differential | Corpus | Checks | Flips | | --- | --- | --- | | develop's own self-test corpus | 1782 | 0, every one passes with develop's verdict | | constructed probes | 261 | 100, all allow -> deny | The probes cover each issue's own commands, near variants of each, and controls. They span export forms, `set` flags, functions and loops, launchers with `env`, `sudo`, and `./env` paths, xargs option spellings and input producers, gh reads and writes, `git config` and `git remote` reads under xargs, named coprocs, and single- and double-quoted wrapper bodies. No probe flips from deny to allow, so there is no deny-to-allow flip to explain. ### Revert proof Each new case was classified by develop's guard, which is this change reverted, with the self-test's own seams. Every one of the 55 new deny cases that develop allows comes back `allow` there, so each fails without the fix. Four further new denies are regression guards for readings a review round found broken at an intermediate head, and develop denies all four. The config-change reason check fails on develop too, whose deny names the push's repository as undetermined. Every new allow control gives the same verdict on develop. ### Local strict review Six passes at the `frontier` tier. Passes 1 and 2 each raised findings answered in their own commit: a `./sudo` config-value override, several near variants, two over-denials, a consolidation of `_runs_git`, and narrowed README claims. Pass 3 was recorded at the opening head with its edit budget spent, so its introduced findings were listed here. Each is now fixed, and passes 4 to 6 reviewed those fixes. A seventh pass reviewed the fix for CodeRabbit's `flock -c` and `watch` finding and is recorded at the current head with no findings. **Local-review findings listed at the opening push (pass 3), each now fixed** - `introduced`: `_wrapper_dir_prefixes` reads a wrapper's prefix from `_wrapper_env_groups`, where a `./sudo` path's `-name GIT_DIR=...` argument joined the group and replaced an earlier prefix. `GIT_DIR=/example/primary/.git find ./sudo -name GIT_DIR=/example/scratch/.git -exec sh -c 'git reset --hard' \;` was denied on develop and allowed at the opening head. Fixed in 4e4649f: the pre-launcher group is read beside it, as `_git_dir_rereading` does. b9bc9dd also reads each GH_REPO value into a wrapper body on its own, closing the same collapse on the gh side. - `introduced`: `_xargs_reads_inert` ignored a redirection of xargs's own stdin, as in `printf 5 | xargs gh pr comment --body x <<< '-R stranger/example 9'`, which develop allows too. Fixed in 4e4649f and b9bc9dd: any input redirection in xargs's simple command voids the exception. - `introduced`: an inert word can be a bare owner after a trailing `--org`, `--owner`, `--target-owner`, or `-o`, as in `printf stranger | xargs gh secret set TOKEN --body x --org`, which develop allows too. Fixed in 4e4649f and b9bc9dd: an owner or repo flag ending the command, or holding the replace string, voids the exception. - `introduced`: BSD `-J` splices every input word, where `-I` replaces with one line, so the endpoint-only and read-only global-option exemptions did not hold under `-J`. Develop allows the shapes too. Fixed in 4e4649f: a `-J` voids both exemptions. - `pre-existing`, filed: #3387 (a replace string as the command word in the other run-time readers), #3388 (rule 6's ref-checkout exemption under xargs), and #3389 (a git variable set by `printf -v`, `read`, `for`, or `mapfile`). Issues filed along the way: #3380 (over-denial of a quoted heredoc's data), #3381 (a coproc named `git`), #3382 (decision: a quoted run-time `-C` value), #3384 (an unquoted heredoc fed to a shell), and #3385 (decision: a shell string that is only an xargs replace string). 🤖 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
Unresolved critical and moderate write-guard issues, plus a governance terminology correction, remain before approval.
3 open issues
What changed in this PR
This pull request promotes emoji callout tier support, subprocess-failure handling fixes, and expanded write-guard protections from develop to main.
Changes:
- Adds Tier 4 emoji markers, variation-selector handling, documentation, and tests.
- Replaces message-based
ghdetection with a cached capability probe. - Expands write-guard analysis for exports, launchers,
xargs, wrappers, runtime directories, and configuration changes.
| File | Summary |
|---|---|
tests/test_prose_lint.py |
Tests emoji markers, variation selectors, lowercase handling, and tier coupling. |
tests/test_pr_review.py |
Tests cached gh capability detection and raw-log behavior. |
spec/readme-structure.md |
Updates the character-set guidance reference. |
scripts/README.md |
Documents the fourth character-set tier and gate behavior. |
scripts/pr_review.py |
Adds the cached gh escape-flag capability probe. |
host-setup/agent-safety/README.md |
Documents expanded write-guard coverage. |
host-setup/agent-safety/claude/README.md |
Updates write-guard behavior and target limitations. |
host-setup/agent-safety/claude/gh-write-guard.py |
Adds export, launcher, xargs, wrapper, runtime-directory, and configuration analysis. Critical, 4 votes: model set +a and set +o allexport so allexport can be disabled. Critical, 1 vote: recognize separated BSD xargs -J values, including the occurrences at lines 6203 and 6221. Moderate, 1 vote: do not mark readonly variables as exported. |
GOVERNANCE.md |
Updates the character-set tier summary. Nit, 3 votes: rename the expanded system so it is not still described as an “ASCII tier system.” |
CODESTYLE.md |
Clarifies subprocess-failure classification rules. |
.github/skills/comment-and-doc-style/SKILL.md |
Updates the generated comment-and-documentation Skill. |
.github/actions/prose-gate/prose_lint.py |
Implements Tier 4 marker recognition and lowercase handling. |
.claude-plugin/fleet-skills/skills/comment-and-doc-style/SKILL.md |
Updates the Claude Skill distribution. |
.claude-plugin/fleet-skills/.source-digests/comment-and-doc-style |
Records the updated Skill digest. |
.agents/skills/comment-and-doc-style/SKILL.md |
Updates the canonical comment-and-documentation Skill source. |
Review effort: Lite
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3393 +/- ##
=======================================
Coverage 81.32% 81.33%
=======================================
Files 18 18
Lines 10047 10049 +2
=======================================
+ Hits 8171 8173 +2
Misses 1876 1876
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:
|
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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
Promotes these pull requests from develop.
ghwhether it takes the escape flag instead of matching its refusal.Closes #3373
Closes #3367
Closes #3365
Closes #3379
Closes #3336
Closes #3339
Closes #3340
Closes #3343
Closes #3347
Closes #3307
Closes #3337
🤖 Generated with Claude Code