Repository navigation
fix(agents): judge five more forms the gh gate misread - #812
EricAndrechek wants to merge 10 commits into
Conversation
The PR-discipline gate judged five uncommon forms wrongly (#809): - `$[ … ]`, bash's older arithmetic, was tokenized as text, so the `<<` in `x=$[1<<3]` opened a heredoc that hid every later line. It is now read as arithmetic, like `$(( … ))`, and a `$(…)` in it is judged. - A GraphQL query read from a shell variable on stdin (`-F query=@- <<< "$q"`, an unquoted heredoc, `--input -`) was not matched against the line that sets the variable, as `-f query="$q"` is. Heredocs and here-strings now record whether the shell expands them, and such a query is matched against the whole command, which also covers a variable set outside the `$(…)` the call runs in. - In a double-quoted `${x:-…}` default, or an unquoted heredoc's body, a `'…'` is literal and bash runs the `$(…)` in it; the default is now queued as double-quoted code. This leaves the documented limits. - A query's GraphQL variables (`$id`) counted as shell variables, so another field's text naming a mutation refused it. A `$name` the query declares (`$name: Type`) no longer counts. - `addPullRequestReview` matched `addPullRequestReviewThreadReply`, and any field containing APPROVE counted as the review's event. The event now counts in the query or as a field's whole value, and only `addPullRequestReview` and `submitPullRequestReview` are checked. Regression cases for each form fail on the base and pass here, under bash 5.3 and /bin/bash 3.2; the 60 KB timed cases are unchanged. Closes #809 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The array-subscript case for $[ … ] now follows it with a guarded command, so a $[ that closes early (and opens a phantom heredoc) lets it through and the case fails. The GraphQL-variable case sends its query from an unquoted heredoc, so the declared-variable rule actually runs on the stdin path. A new case pins that a quoted heredoc is static: a $ in its text is not a shell variable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
📝 Summary
Merge Risk: 🔵 Low · up to Some comment reviews with inline body text are incorrectly refused. The impact is narrow, but the matching should be fixed before merge if that form is supported. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 2 files. (3 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Linked Issues check Issue #809is open and directly linked. The reviewed changes address all five coding requirements..claude/hooks/agent-bash-gate.shparses$[ ... ]arithmetic, tracks expansion for heredocs and he…Out of Scope Changes check The changes stay within issue #809's gate accident-prevention scope. The computed-event handling extends the same review-event check. The test additions verify the gate behavior. Updates to `AGENTS.md…Title check The title clearly identifies the agent gate fix and the additional forms it now judges. It is concise and directly related to the main changes. Description check The description directly explains the gate changes, added tests, documentation updates, and verification results. It is fully related to the changeset.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 2 files. (3 skipped: 3 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
-
📝 Generate docstrings 💡
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
🧪 Generate unit tests (beta)
-
-
- Commit to this branch
- Create a new PR
-
✨ Simplify code
-
-
- Commit to this branch
- Create a new PR
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
github-actions
Bot
added
documentation
|
@coderabbitai review |
|
…e-forms # Conflicts: # AGENTS.md # docs/src/content/docs/claude-code.md
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
70acb4f4-0cb4-41f4-be85-6b34233582cc
📒 Files selected for processing (5)
.claude/hooks/agent-bash-gate.sh.claude/hooks/review-gate.test.shAGENTS.mdCHANGELOG.mddocs/src/content/docs/claude-code.md
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
🧰 Additional context used
📚 Code guidelines (3)
CONTRIBUTING.md — auto-discovered
.github/copilot-instructions.md — auto-discovered
AGENTS.md — auto-discovered
📓 Path-based instructions (4)
Source excerpt: Update the documentation and `CHANGELOG.md` (under `## Unreleased`) that your change affects.
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
docs/src/content/docs/claude-code.mdCHANGELOG.md
See [AGENTS.md](../AGENTS.md) for project conventions, architecture notes, and AI agent instructions.
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
AGENTS.md
Source excerpt: **Never hard-wrap prose.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/claude-code.mdAGENTS.mdCHANGELOG.md
Source excerpt: **Docs prose**: never hard-wrap Markdown — one paragraph is one line.
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
docs/src/content/docs/claude-code.mdAGENTS.mdCHANGELOG.md
🪛 LanguageTool
docs/src/content/docs/claude-code.md
[style] ~55-~55: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ... on the same line, run directly or not. It reads only the command line, so it catches the common accidental forms, not deliberate evasion; it doesn't follow a script that pushes, a shell behind a wrapper or one it doesn't know (fish -c), code piped into a shell, git push --tags, git run by path or through an alias, or a $( split by a backslash-newline inside an unquoted heredoc body. A line it can't parse, such as one with an apostrophe in a comment inside $(…), is refused when it mentions a push or a gh pr / gh api command. It follows 64 pieces of code a shell ru...
(TOO_LONG_SENTENCE)
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...a push or a gh pr / gh api command. It follows 64 pieces of code a shell runs;...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~110-~110: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...d. Through GraphQL, createPullRequest needs draft: true, and `markPullRequestRead...
(EN_REPEATEDWORDS_NEED)
[style] ~110-~110: Since ownership is already implied, this phrasing may be redundant.
Context: ... (!). Each gh api call is judged by its own endpoint (its first argument that isn't...
(PRP_OWN)
[style] ~110-~110: Since ownership is already implied, this phrasing may be redundant.
Context: ...s first argument that isn't a flag) and its own fields, so a comment or review-thread r...
(PRP_OWN)
AGENTS.md
[style] ~249-~249: Since ownership is already implied, this phrasing may be redundant.
Context: ... reads. Each gh api call is judged by its own endpoint (its first argument that isn't...
(PRP_OWN)
[style] ~249-~249: Since ownership is already implied, this phrasing may be redundant.
Context: ...s first argument that isn't a flag) and its own fields, so a comment body that names an...
(PRP_OWN)
[style] ~249-~249: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...pieces of nested code the gate follows. gh -R <repo> pr … and gh pr -R <repo> … (or --repo) count as gh pr …, a shell given options before -c (bash -euo pipefail -c '…') still hands its code over, and a GraphQL query computed by a heredoc inside $(…) is read from that heredoc, not from the rest of the line, unless the query reads a shell variable: a $name or ${ that isn't a GraphQL variable the query declares ($name: Type). Such a query, sent as a field or fro...
(TOO_LONG_SENTENCE)
[style] ~277-~277: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...er from any worktree of the repository. It blocks a push when it can't tell where it runs or what it publishes: a computed path or refspec, GIT_DIR or GIT_WORK_TREE set earlier on the line, --all or a glob refspec, a push behind a wrapper such as timeout or in a command substitution (one in the body of an unquoted heredoc included), or code handed to a shell (bash -c, eval, a heredoc or here-string read by a shell, inside a $(…) or not). It also blocks a push that follows a ...
(TOO_LONG_SENTENCE)
[style] ~277-~277: Since ownership is already implied, this phrasing may be redundant.
Context: ...ide $(…), is refused: run the push as its own command. On VERDICT: iterate or `VERD...
(PRP_OWN)
CHANGELOG.md
[typographical] ~126-~126: To join two clauses or introduce examples, consider using an em dash.
Context: ...quoted heredoc (-F query=@- <<< "$q", --input - <<< "$b") now is too, and the line it i...
(DASH_RULE)
[style] ~126-~126: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...now counts in the query or as a field's whole value (which a GraphQL variable may hol...
(EN_REPEATEDWORDS_WHOLE)
🔇 Additional comments (4)
.claude/hooks/review-gate.test.sh (1)
855-863: LGTM!AGENTS.md (1)
249-249: LGTM!docs/src/content/docs/claude-code.md (1)
55-55: LGTM!CHANGELOG.md (1)
126-126: LGTM!
A review whose event came from a shell variable (-f event="$ev", or a GraphQL event variable whose field is computed) went unread, so an approval written that way passed. The gate now refuses it, unless a field gives the event literally, so a COMMENT review with a computed body still passes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A --input - body the shell expands was read with jq, which saw "$ev" as a plain string and, once a "body": $(jq -Rs . notes.md) made the text invalid JSON, read nothing: a computed event and a literal APPROVE both passed, and a draft PR built that way was refused as not a draft. Such a body is now read from its text, and an event there that isn't a plain string is refused as computed. A computed input[event] field and a review payload built by jq and sent through --input - are refused the same way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hat isn't A GraphQL --input - body the shell expands now has its "event" key read from its text, as a REST one does, so "event": "$ev" in an input object is refused. A test pins the title read from an expanded body. The docs now name the forms the gate refuses and the ones it doesn't follow: a body built whole from a variable or $(jq …), or a variables entry for an escaped \$e. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A query field that puts a shell variable straight into a review's event (event: $ev) was refused only when some other field happened to be computed, and an unquoted heredoc query's escaped \$e, given by a computed field, wasn't matched. A query that reads a shell variable now counts as computed, and \$e is matched in the query the fields give (not in a --input - body, whose variables may give the event literally). The docs say more plainly which escaped \$e the gate doesn't follow. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
f161dc3c-df54-46d4-94d9-503ec0f488c1
📒 Files selected for processing (5)
.claude/hooks/agent-bash-gate.sh.claude/hooks/review-gate.test.shAGENTS.mdCHANGELOG.mddocs/src/content/docs/claude-code.md
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
🧰 Additional context used
📚 Code guidelines (3)
CONTRIBUTING.md — auto-discovered
.github/copilot-instructions.md — auto-discovered
AGENTS.md — auto-discovered
📓 Path-based instructions (4)
Source excerpt: Update the documentation and `CHANGELOG.md` (under `## Unreleased`) that your change affects.
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
CHANGELOG.mddocs/src/content/docs/claude-code.md
See [AGENTS.md](../AGENTS.md) for project conventions, architecture notes, and AI agent instructions.
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
AGENTS.md
Source excerpt: **Docs prose**: never hard-wrap Markdown — one paragraph is one line.
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
AGENTS.mdCHANGELOG.mddocs/src/content/docs/claude-code.md
Source excerpt: **WH001 applies to every tracked Markdown file, with no carve-out** — `AGENTS.md`, `CHANGELOG.md`, `.github/` CI docs and `.claude/` agent prompts included.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
AGENTS.mdCHANGELOG.mddocs/src/content/docs/claude-code.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse
Timestamp: 2026-10-11T00:14:13.570Z
Learning: In Wave-RF/WaveHouse, `.claude/hooks/agent-bash-gate.sh` is a Bash accident-prevention gate, not an adversarial barrier. Its documented review-event limits include whole request bodies computed from a shell variable or command substitution such as `$(jq …)`, and computed variables entries supplying a GraphQL query's escaped `\$e`. Do not treat these documented limits as violations of the gate's intended coverage.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-10-11T01:00:53.392Z
Learning: Source excerpt:
# AGENTS.md — AI Agent Instructions for WaveHouse
## Operating Rules
1. **Validate locally before every push** — run `make ci` the documented way, here or on a configured host with `make ci-remote` ([§Running `make ci`](#running-make-ci-for-agents)). Don't use CI as your first feedback loop.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-10-11T01:00:53.392Z
Learning: Source excerpt:
# AGENTS.md — AI Agent Instructions for WaveHouse
## Operating Rules
3. **Every code change updates its docs + `CHANGELOG.md` in the same PR** — a code change without its doc update is incomplete ([§Documentation Sync](#documentation-sync)).
🪛 LanguageTool
AGENTS.md
[style] ~249-~249: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...nt built into a body any other way (the whole body from a variable or $(jq …), or, ...
(EN_REPEATEDWORDS_WHOLE)
[style] ~249-~249: Since ownership is already implied, this phrasing may be redundant.
Context: ... reads. Each gh api call is judged by its own endpoint (its first argument that isn't...
(PRP_OWN)
[style] ~249-~249: Since ownership is already implied, this phrasing may be redundant.
Context: ...s first argument that isn't a flag) and its own fields, so a comment body that names an...
(PRP_OWN)
[style] ~249-~249: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...pieces of nested code the gate follows. gh -R <repo> pr … and gh pr -R <repo> … (or --repo) count as gh pr …, a shell given options before -c (bash -euo pipefail -c '…') still hands its code over, and a GraphQL query computed by a heredoc inside $(…) is read from that heredoc, not from the rest of the line, unless the query reads a shell variable: a $name or ${ that isn't a GraphQL variable the query declares ($name: Type). Such a query, sent as a field or fro...
(TOO_LONG_SENTENCE)
CHANGELOG.md
[typographical] ~126-~126: To join two clauses or introduce examples, consider using an em dash.
Context: ...quoted heredoc (-F query=@- <<< "$q", --input - <<< "$b") now is too, and the line it i...
(DASH_RULE)
[style] ~126-~126: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...now counts in the query or as a field's whole value (which a GraphQL variable may hol...
(EN_REPEATEDWORDS_WHOLE)
docs/src/content/docs/claude-code.md
[style] ~110-~110: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...d. Through GraphQL, createPullRequest needs draft: true, and `markPullRequestRead...
(EN_REPEATEDWORDS_NEED)
[style] ~110-~110: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...nt built into a body any other way (the whole body from a variable or $(jq …), or, ...
(EN_REPEATEDWORDS_WHOLE)
[style] ~110-~110: Since ownership is already implied, this phrasing may be redundant.
Context: ... (!). Each gh api call is judged by its own endpoint (its first argument that isn't...
(PRP_OWN)
[style] ~110-~110: Since ownership is already implied, this phrasing may be redundant.
Context: ...s first argument that isn't a flag) and its own fields, so a comment or review-thread r...
(PRP_OWN)
Every field whose whole value was a review event counted as the event, so a COMMENT review whose body was the word APPROVE was refused, and a body that was the word COMMENT counted as a literal event, letting a computed event variable beside it pass. The gate now collects the fields and, once the query is known, judges only the event's own: a field named event or …[event], or the variable the query's event takes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…query The event's own field was found from the first event: $x in the query its fields gave, so a second mutation's approval in the same request passed, and a COMMENT review whose query was held in a shell variable was refused as computed. Every event: $x in the query is now collected, after a query that reads a variable has been matched against the line that sets it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Match the review event, not the full GraphQL query. · agent-bash-gate.sh:322
.claude/hooks/agent-bash-gate.sh:322
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch the review event, not the full GraphQL query.
When
addPullRequestReviewusesevent: COMMENT, a body such as"I would APPROVE after CI"still triggers the approval block because line 322 scans the complete query. Limit theAPPROVEandREQUEST_CHANGESchecks to the event argument, and add a regression test for an inline body string.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
ad151111-48eb-407a-9367-3673aae808e3
📒 Files selected for processing (5)
.claude/hooks/agent-bash-gate.sh.claude/hooks/review-gate.test.shAGENTS.mdCHANGELOG.mddocs/src/content/docs/claude-code.md
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
🧰 Additional context used
📚 Code guidelines (3)
CONTRIBUTING.md — auto-discovered
.github/copilot-instructions.md — auto-discovered
AGENTS.md — auto-discovered
📓 Path-based instructions (4)
Source excerpt: Update the documentation and `CHANGELOG.md` (under `## Unreleased`) that your change affects.
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
CHANGELOG.mddocs/src/content/docs/claude-code.md
See [AGENTS.md](../AGENTS.md) for project conventions, architecture notes, and AI agent instructions.
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
AGENTS.md
Source excerpt: **Docs prose**: never hard-wrap Markdown — one paragraph is one line.
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
CHANGELOG.mdAGENTS.mddocs/src/content/docs/claude-code.md
Source excerpt: **WH001 applies to every tracked Markdown file, with no carve-out** — `AGENTS.md`, `CHANGELOG.md`, `.github/` CI docs and `.claude/` agent prompts included.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
CHANGELOG.mdAGENTS.mddocs/src/content/docs/claude-code.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-10-11T03:02:01.590Z
Learning: Source excerpt:
# AGENTS.md — AI Agent Instructions for WaveHouse
## Documentation Sync
Every code change should update the corresponding docs in the same PR. A code change without its doc update is incomplete.
| Change | Files to update |
| ------ | --------------- |
| Add/modify API endpoint | `docs/src/content/docs/api.md`, `README.md` (if user-facing) |
| Add/modify boot config option | `docs/src/content/docs/configuration.mdx`, `config.yaml`, `deployments/compose/*` env blocks, `docs/src/content/docs/deployment.md` |
| Add/modify a settings-directory key (`config.json`, `roles.json`, `policies.json`, `pipes.json`) | `docs/src/content/docs/settings-directory.mdx` (table **and** the example block), `internal/settings/seed/*.json`, `deployments/compose/settings/*.json`, `tests/e2e/fixtures/settings/*.json`, `config.yaml` (the `settings:` comment enumerates the keys) |
| Change architecture / add a package | `docs/src/content/docs/architecture.md`, `AGENTS.md` |
| Change ingest / e...
🪛 LanguageTool
CHANGELOG.md
[typographical] ~126-~126: To join two clauses or introduce examples, consider using an em dash.
Context: ...quoted heredoc (-F query=@- <<< "$q", --input - <<< "$b") now is too, and the line it i...
(DASH_RULE)
[style] ~126-~126: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...event now counts in the query or as the whole value of the event's own field (event...
(EN_REPEATEDWORDS_WHOLE)
AGENTS.md
[style] ~249-~249: Since ownership is already implied, this phrasing may be redundant.
Context: ... reads. Each gh api call is judged by its own endpoint (its first argument that isn't...
(PRP_OWN)
[style] ~249-~249: Since ownership is already implied, this phrasing may be redundant.
Context: ...s first argument that isn't a flag) and its own fields, so a comment body that names an...
(PRP_OWN)
[style] ~249-~249: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...pieces of nested code the gate follows. gh -R <repo> pr … and gh pr -R <repo> … (or --repo) count as gh pr …, a shell given options before -c (bash -euo pipefail -c '…') still hands its code over, and a GraphQL query computed by a heredoc inside $(…) is read from that heredoc, not from the rest of the line, unless the query reads a shell variable: a $name or ${ that isn't a GraphQL variable the query declares ($name: Type). Such a query, sent as a field or fro...
(TOO_LONG_SENTENCE)
docs/src/content/docs/claude-code.md
[style] ~110-~110: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...d. Through GraphQL, createPullRequest needs draft: true, and `markPullRequestRead...
(EN_REPEATEDWORDS_NEED)
[style] ~110-~110: Since ownership is already implied, this phrasing may be redundant.
Context: ... (!). Each gh api call is judged by its own endpoint (its first argument that isn't...
(PRP_OWN)
[style] ~110-~110: Since ownership is already implied, this phrasing may be redundant.
Context: ...s first argument that isn't a flag) and its own fields, so a comment or review-thread r...
(PRP_OWN)
github-project-automation
Bot
moved this from In review
to In progress
in WaveHouse Task Board
|
Superseded by #815, which replaces the gate's shell tokenizer with plain text checks; the forms this PR handled no longer apply. |
github-project-automation
Bot
moved this from In progress
to Done
in WaveHouse Task Board
…815) Replaces the agent PR gate's bash tokenizer with plain text checks over the command string. `.claude/hooks/agent-bash-gate.sh` goes from 1,032 lines to 304, and its tests from 684 lines to 493. The gate exists to catch accidental breaches of the human-only PR actions and of the pre-push review rule. It doesn't try to stop deliberate evasion. Teaching it to read bash had become the larger problem: each review round found narrower forms it misread, harmless edits that mentioned a guarded command were blocked, and on macOS bash 3.2 a large command could outlast the hook's timeout and pass unchecked. It now catches the plain, ordinary forms. The `deny` rules in `.claude/settings.json`, AGENTS.md and Claude Code's own permission checks cover the rest. **Blocks** - a `gh pr create` / `gh pr new` without `--draft`, and a literal title the PR-title lint rejects; - adding or removing reviewers and assignees, `gh pr ready`, and `gh pr review --approve` / `--request-changes`; - the single-line `gh api` forms of those, and of a merge or auto-merge; - a `git push` that isn't a plain `git [-C <worktree>] push …` on its own (optionally with `2>&1` and one `| filter`); - a plain push whose commit lacks a marker from every reviewer in `scripts/pre-push-reviewers.sh`. That marker check (`gate_commit`) is unchanged. **Accepted false positives.** The text is searched, not parsed. So a command that only mentions a guarded name is blocked too: a `--body` naming the ready-for-review mutation, a commit message containing `git push`, or a GET of `requested_reviewers`. The workaround is to put the text in a file (`--body-file`, `git commit -F <file>`, `gh api -F body=@file`). Unusual forms aren't caught, for example a quoted `-C` path or a multi-line `gh` command. AGENTS.md and the Claude Code guide say only plain forms are checked and that every other form is still against the rules. This supersedes #810 and #812, which hardened the tokenizer this removes. **Verified** - `review-gate.test.sh` passes under Homebrew bash 5.3 and macOS `/bin/bash` 3.2. A 108 KB command gets a verdict in about 0.07 s. - `hook-commands.test.sh` passes, and shellcheck is clean. - `make verify` and `make ci` pass. Closes #797 Closes #809 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Closes #809. This builds on #810 (#797's gate rework); review it after #810.
What changed.
.claude/hooks/agent-bash-gate.shnow judges the five forms #809 lists, plus one more found in review:$[ … ]is read as arithmetic, sox=$[1<<3]no longer opens a heredoc that hides the next line, and a$(…)inside it is judged.-F query=@- <<< "$q", an unquoted heredoc,--input -) is matched against the whole command, as-f query="$q"is. That also covers a variable set outside the$(…)the call runs in.${x:-…}default expanded as in double quotes is read as double-quoted code, so a$(…)inside'…'there is judged. The documented limit is removed.$namethat the query declares as a GraphQL variable ($name: Type) no longer counts as a shell variable.addPullRequestReviewitself andsubmitPullRequestRevieware checked for an event (not…ThreadReplyor…Comment), and only the event's own field counts:event,…[event], or the variable eachevent:in the query takes. A body that mentionsAPPROVE, or is just the word, is a body.eventorinput[event]field, an"event"key in a--input -body the shell expands, and a GraphQLevent: $e(or a shell variable in that slot) whose value no field gives literally. A literal event beside a computed body still passes. A--input -body the shell expands is read from its text, so in one that is JSON only once expanded ("body": $(jq -Rs . notes.md)) a literalAPPROVEis now seen and a draft PR is no longer refused as not a draft. An event built into a body any other way (the whole body from a variable or$(jq …), or a computedvariablesentry for the query's escaped\$e) isn't followed, and the docs say so.The docs (
AGENTS.md,claude-code.md), the gate's comments andCHANGELOG.mdare updated to match.Verification.
.claude/hooks/review-gate.test.shhas 46 new cases, each run with the hooks under bash 5.3 and macOS/bin/bash3.2. Every case that refuses a form fails against the gate from before its fix, and the allow cases guard against over-blocking. The 60 KB timed cases stay at 0–1 s.make verifyandmake cipass.🤖 Generated with Claude Code