fix(agents): key review markers to the reviewed commit and worktree by EricAndrechek · Pull Request #755 · Wave-RF/WaveHouse

EricAndrechek · 2026-10-07T17:02:30Z

Summary

The pre-push review gate could attest to code no reviewer read.

Marker keyed to the wrong commit. review-marker.sh used whatever HEAD the launch directory had when the reviewer stopped, so a commit made while a reviewer ran inherited its ship_it, and a review of a sibling worktree marked the launch directory's branch instead. Reviewers now end with REVIEWED: <sha>, the commit they pinned before reading anything (an unambiguous prefix will do). The hook writes tmp/<name>-passed-<sha> into each worktree on that commit, and only when that worktree's HEAD reflog shows it on the commit from the start of the review (the reviewer transcript's first timestamp) to its end. Same-commit entries such as git reset or git stash don't count as moves.
Hand-back reports wrote no marker. When last_assistant_message lacks either line, as it does for a report delivered through the hand-back, the hook reads the hand-back message from the transcript. Every decision goes to stderr and to tmp/review-marker.log, and a blocked push prints the latest decisions from every worktree's log.
The push gate checked the wrong worktree. agent-bash-gate.sh checked the launch directory's HEAD whatever was pushed, so once that branch had markers a push from any sibling worktree went through, and a push through git -C or from a detached HEAD was never checked. The gate now runs the command through a quote-aware tokenizer and follows the push to where it runs (the tool call's cwd, a preceding cd/pushd, git -C, inside if/while/for … do or a process substitution). It requires markers for the commit each refspec publishes (its tip, or HEAD with no refspec), and a marker counts from any worktree of the repository. A command that only mentions git push, in a heredoc, a quoted string or a comment, is not a push.
What the gate blocks. It blocks when it can't tell what a push does: a computed path or refspec, GIT_DIR/GIT_WORK_TREE (inline, assigned or exported earlier on the line; a later cd or git -C doesn't undo it, unset or leaving the subshell does), --all/--mirror or the matching refspec :, a glob refspec, a push behind a wrapper or in a command substitution, a push in code handed to a shell (bash -c, sh -c, eval, a heredoc or here-string read by a shell), or a payload without cwd. It also blocks a push that follows a HEAD-moving git command on the same line (git commit … && git push), because it would otherwise judge the commit before the move. It reads only the command line, so it guards against accidents, not a deliberate workaround. A push into another repository with no reviewer manifest is outside the policy.
Long commands were skipped. Every check fed the command to grep -q through a pipe under pipefail; past 64 KB the writer got SIGPIPE when grep matched early, which read as no match, so a long gh pr create without --draft, or a push ahead of a long heredoc body, went unchecked. The checks now use here-strings.
Smaller changes. skip-pre-push-review.sh writes to the worktree root when run from a subdirectory. The reviewer definitions, /prepush, /docs-review, the docs-review rubric, AGENTS.md and the hooks docs describe the REVIEWED: line, the "don't move HEAD while reviewers run" rule and "push in a separate command". pr-review-locally names the PR worktree's path in its reviewer prompts. make verify now needs jq, which the development prerequisites list.

Rolling this out to an open Claude Code window

Hook scripts are read from disk on every call and .claude/settings.json doesn't change, so a window rooted on a checkout with this commit uses the new hooks immediately.
Reviewer definitions (.claude/agents/*.md) load when Claude Code starts. A window opened before this change runs reviewers that don't emit REVIEWED:, so their ship_it writes no marker; tmp/review-marker.log and the gate's block message say so.
Either restart it, or add to each reviewer prompt: "Before reading anything, run git -C <worktree> rev-parse HEAD, and end your report with REVIEWED: <that sha> on the line before VERDICT:".
Push in its own command, not after git commit … &&.

Test plan

make ci green locally.
make test-review-gate (new make verify leaf, .claude/hooks/review-gate.test.sh): 133/133 pass under bash 5.3 and macOS /bin/bash 3.2. It feeds both hooks synthetic SubagentStop/PreToolUse JSON in a scratch repo with sibling worktrees, with pinned reflog and transcript clocks. Markers come only from the hook or the skip script.
The same suite against the main hooks fails 85 of 133, covering the commit-during-review, sibling-worktree, hand-back, refspec, fail-closed, shell-code and long-command cases.
The test clears git's repository variables, names every repository with -C, and refuses to write unless its scratch repos land where asked. Under an inherited GIT_DIR pointing at a scratch decoy, it passes and leaves the decoy untouched.
make verify passes in the pre-commit hook; shellcheck and markdownlint are clean.
Pre-push review: code ship_it; docs review ship_it.
The first real /prepush round after merge should show 📝 marker written lines in tmp/review-marker.log for both reviewers, including reports delivered through the hand-back.

Remaining findings: none.

Related Issues

Closes #454
Closes #565
Closes #659
Part of #740

🤖 Generated with Claude Code

The pre-push review gate could attest to code no reviewer read. review-marker.sh keyed the marker to whatever HEAD the session's launch directory had when the reviewer stopped, so a commit made while a reviewer ran inherited its ship_it, and a review of a sibling worktree marked the launch directory's branch instead. Reviewers now end with `REVIEWED: <sha>` naming the commit they pinned before reading anything. The hook writes the marker into each worktree on that commit, and only when that worktree's HEAD reflog shows it on the commit from the start of the review (the subagent transcript's first timestamp) until now. Same-commit entries such as `git reset` or `git stash` don't count as moves. A report delivered through the subagent hand-back leaves only closing text in last_assistant_message, so the hook reads the hand-back's message from the transcript. Every decision goes to stderr and to tmp/review-marker.log, which the push gate prints when it blocks. agent-bash-gate.sh checked HEAD of the session's launch directory for every push. It now follows the push to where it runs (the tool call's cwd, a preceding cd/pushd, git -C), resolves each refspec source to the commit it publishes, and looks for that commit's markers in every worktree of the repository. It blocks when it can't tell: a computed directory or refspec, GIT_DIR, --all/--mirror, a glob refspec, a wrapped push, or a payload without cwd. A push into another repository with no reviewer manifest is outside the policy. Pushes through `git -C` and from a detached HEAD were not gated at all before. skip-pre-push-review.sh writes to the worktree root when run from a subdirectory. .claude/hooks/review-gate.test.sh drives both hooks with synthetic events in a scratch repository with sibling worktrees; it is a make verify leaf (test-review-gate). Because the pre-commit hook runs it with GIT_DIR exported, it clears git's repository variables, names every repository with -C, and refuses to write unless git sees the scratch directory as outside any repository and creates each scratch repository where asked. /prepush, the reviewer agents, AGENTS.md and the Claude Code docs page describe the REVIEWED line and the rule not to move HEAD while reviewers run. Closes #454 Closes #565 Closes #659 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The cheap filter in agent-bash-gate.sh strips quotes one line at a time, so a heredoc body, a multi-line quoted string or a comment that mentions `git push` reached the tokenizer, which correctly found no push, and the gate then refused the command as one it couldn't follow. It now refuses only when the tokens hold a `git … push` the walk didn't gate: behind an unknown wrapper, or inside a command substitution. The gate runs before the command line does, so in `git commit … && git push` it judged the commit before the new one. A push that follows a HEAD-moving git command (commit, merge, rebase, reset, checkout, switch, cherry-pick, revert, am, pull) on the same line is now blocked with "push in a separate command". Pushes inside if/while/until/for … do are followed instead of refused, and the matching refspec `:` (or `+:`) is blocked like --all instead of being read as a delete. review-marker.sh uses the hand-back message whenever the final message lacks either the VERDICT or the REVIEWED line, retries the transcript read once, and resolves an abbreviated REVIEWED sha that names exactly one commit. A ship_it without a REVIEWED line, which is what a session started before this change produces because reviewer definitions load at session start, logs how to recover, and the gate's block message says the same and now shows the latest decisions from every worktree's log. pre-push-reviewer pins its commit in step 1 with `git -C <path> rev-parse HEAD`. The docs say the gate checks the commit each refspec publishes (its tip, or HEAD with no refspec), and the remaining copies that described a VERDICT-only, HEAD-keyed marker now describe REVIEWED plus VERDICT and the reviewed commit. The test covers each change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

agent-bash-gate.sh looked for an unfollowed push only when it had followed none, so a second push on the same line behind a wrapper or in a command substitution went unchecked once the first was gated. The walk now records each simple command it gates, and every other one is checked. GIT_DIR or GIT_WORK_TREE exported, declared or assigned earlier on the line now makes the push's repository unknown, the same as the inline form. The test requires jq up front, and development.md lists jq among the prerequisites, since make verify now runs it. pr-review-locally names the PR worktree's path in its reviewer prompts, because reviewers pin the worktree their prompt names, else their current directory. The reviewer agents say an unresolvable REVIEWED sha means no marker, and the remaining "marker for HEAD" phrasings say "for the commit being pushed". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Four gaps in the push gate, each now covered by review-gate.test.sh: - Every check fed the command to `grep -q` through a pipe under pipefail. Past a pipe buffer (64 KB), grep's early exit sent SIGPIPE to the writer and the pipeline read as no match, so a long `gh pr create` without --draft, or a push ahead of a long heredoc body, went unchecked. The checks now read here-strings. - An exported GIT_DIR or GIT_WORK_TREE shared a variable with "directory unknown", so a later absolute cd, pushd or git -C cleared it. It is tracked on its own now; only unset or the end of a subshell clears it. - A HEAD-moving git command counted only when run directly, so `timeout 60 git commit … && git push` or `out=$(git commit …) && git push` had the push judged on the commit before the move. A wrapped or substituted one counts too. - A push in code handed to a shell (bash -c, sh -c, eval, a heredoc or here-string read by a shell) was never seen, and one in a process substitution was dropped as a redirect target. The first now blocks as a push the gate can't follow; the second is followed like any other push. The Claude Code docs now say what a gating reviewer must end its report with for its ship_it to write a marker, matching AGENTS.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Three more gaps in the push gate, each covered by review-gate.test.sh: - A substitution in a here-string or a redirect target was dropped with its word, so `grep -q x <<< "$(git -C ../wt-b push)"` or `echo hi > "$(git push …)"` went unchecked, and a commit in one didn't count as a HEAD move. The tokenizer now keeps the text of every $(…) and `…` wherever its word goes, and the push and HEAD-move checks scan all of it. A quoted substitution no longer slips past the cheap pre-filter either. - Inside $(…), a here-string's text was glued to `<<<`, so a push in `$(sh <<< 'git push')` didn't read as one. - Code handed to a shell was scanned without joining backslash-newline continuations, so a push split across lines in `bash -c` or a heredoc read by bash went through. AGENTS.md, the Claude Code docs and the changelog now say the gate reads only the command line, list what it blocks, and name the forms it doesn't follow. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The last change scanned the text of every $(…) for a push, but with its quotes already stripped, so `n=$(grep -c 'git push' AGENTS.md)`, `git commit -m "$(echo 'explain the git push gate')"` and the like were refused as pushes the gate can't follow. The tokenizer now marks quoted text inside a $(…), and each substitution is checked with that text replaced by a placeholder; only a substitution that hands it to a shell (`$(bash -c '…')`, `$(sh <<<'…')`, `$(eval '…')`) is checked in full. Two forms the wider pre-filter now tokenizes failed to parse: a heredoc inside $(…) closed with `EOF)"` on one line, and an `$'…'` string holding `\'`. Both parse now; an apostrophe in a comment inside $(…) still doesn't, and the docs say such a line is refused. The test for the here-string spacing inside $(…) now uses `<<<'…'` with no space, so it fails without that fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Two code paths from the last change had no test: stripping the quote markers from a word's value, and reading an $'…' string inside $(…). Each now has cases that fail when it is removed: a substitution's output handed to eval or bash -c, and an $'…' string in a $(…), pushed or not. The marker strip also runs in one pass with tr. Under bash 3.2, ${_w//…} took 47 s on a 200 KB substitution holding 200 quoted strings, past the hook's time limit; it now takes about a second. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Conflicts: - docs/src/content/docs/development.md: kept origin/main's reworded make vulncheck row and this branch's make verify row (adds review-gate hook tests). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

EricAndrechek · 2026-10-07T17:03:10Z

@coderabbitai review

coderabbitai · 2026-10-07T17:04:14Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 281643de-43d2-4799-9e4f-b0d7709f1915

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

📝 Summary

Summary by CodeRabbit

New Features
- Push checks now validate review approvals against the specific commits being pushed, including across worktrees. Ambiguous push commands are blocked rather than treated as approved.
- Review results are tied to the commit actually inspected; approvals are recorded only if that commit remains checked out throughout the review.
Documentation
- Updated review and setup guidance, including the jq prerequisite.
Tests
- Added automated coverage for review approvals and push checks.

Walkthrough

Review markers now identify the commit reviewed and are written only when worktree checks pass. The push gate resolves commits from supported push commands and checks for required reviewer markers. Behavioral tests and documentation cover the updated process.

Changes

Commit-scoped pre-push review gate

Layer / File(s)	Summary
Pin review commits and write markers `.claude/agents/*reviewer.md`, `.claude/hooks/review-marker.sh`, `.claude/commands/docs-review.md`, `.claude/skills/pr-review-locally/SKILL.md`, `.github/prompts/docs-review.md`, `AGENTS.md`, `docs/src/content/docs/claude-code.md`, `scripts/pre-push-reviewers.sh`	Reviewer prompts require a `REVIEWED` commit SHA. The marker hook uses the reported commit, checks worktree history, and supports reports from transcript hand-backs.
Resolve pushed commits and validate markers `.claude/commands/prepush.md`, `.claude/hooks/agent-bash-gate.sh`, `scripts/skip-pre-push-review.sh`	The gate analyzes supported push commands in their execution worktree, resolves published commits, and checks required markers across repository worktrees. The pre-push instructions and skip script use the worktree being pushed.
Test and document the review gate `.claude/hooks/review-gate.test.sh`, `Makefile`, `CHANGELOG.md`, `docs/src/content/docs/development.md`	Behavioral tests cover marker creation and push-gate command handling. `make verify` runs the new test target. The changelog and development docs describe the related changes and prerequisites.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  actor Developer
  participant Gate as agent-bash-gate.sh
  participant Git
  participant Markers as Worktree review markers
  Developer->>Gate: Submit push command and execution directory
  Gate->>Git: Resolve pushed refs and commit tips
  Git-->>Gate: Return repository and commit details
  Gate->>Git: Compare each commit with main or origin/main
  Git-->>Gate: Return review-delta status
  Gate->>Markers: Check reviewer markers for each commit
  Markers-->>Gate: Return marker status
  Gate-->>Developer: Allow or block the push

Suggested reviewers: taitelee

Merge Risk: 🔵 Low · up to 179db

The changes tighten pre-push review markers to the reviewed commit. Two edge cases can still let unreviewed work through. A reviewer's later non-pass verdict can be overridden by an earlier pass. A branch created from unreviewed local main commits can skip review. Both are small fixes and worth addressing before relying on the gate.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 55.26% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 5 files. (11 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely identifies the main change: review markers now reference the reviewed commit and worktree.
Description check	✅ Passed	The description is directly related to the changes. It explains the marker, push-gate, worktree, transcript, testing, and documentation updates.
Linked Issues check	✅ Passed	The changes satisfy the coding requirements in `#454`, `#565`, and `#659`. `review-marker.sh` uses the reported `REVIEWED:` commit, resolves repository worktrees, checks reflog continuity from the transcrip…
Out of Scope Changes check	✅ Passed	The changes remain within the linked issue scope. The tokenizer, push-gate rules, decision logging, long-command handling, skip-script path fix, reviewer prompt updates, documentation, prerequisites, …

Full details: Docstring Coverage

Explanation

Docstring coverage is 55.26% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 5 files. (11 skipped: 11 unsupported.)

✨ 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

Autopilot · 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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai · 2026-10-07T17:04:45Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

github-actions · 2026-10-07T17:05:58Z

📚 Docs preview is live → https://9ac60ca7-wavehouse-docs.wave-rf.workers.dev

Commit — 26e4f07: Merge remote-tracking branch 'origin/main' into fix/prepush-gate-holes
Author — @EricAndrechek, Claude Sonnet 5.5
Committed — 2026-10-07 13:59 (UTC-04:00)
Deployed — 2026-10-07 14:38 EDT

github-code-quality · 2026-10-07T17:10:42Z

Code Coverage Overview

Languages: Go

Go

The overall line coverage in commit 26e4f07 in the fix/prepush-gate-hol... branch remains at 93%, unchanged from commit 7e4b65d in the main branch.

Show a line coverage summary of the most impacted files.

File	`main` 7e4b65d	`fix/prepush-gate-hol...` 26e4f07	+/-
`internal/ingest/worker.go`	97%	97%	0%
`internal/mq/nats_topology.go`	91%	92%	+1%
`internal/mq/external.go`	84%	85%	+1%
`internal/mq/ext...nal_consumer.go`	88%	89%	+1%

_{Updated October 07, 2026 18:41 UTC}

coderabbitai

Actionable comments posted: 3

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: eb01fa15-ff41-4f87-b864-6d63c0d55c2a

📥 Commits

Reviewing files that changed from the base of the PR and between 86c000e and 179dbba.

📒 Files selected for processing (16)

.claude/agents/docs-reviewer.md
.claude/agents/pre-push-reviewer.md
.claude/commands/docs-review.md
.claude/commands/prepush.md
.claude/hooks/agent-bash-gate.sh
.claude/hooks/review-gate.test.sh
.claude/hooks/review-marker.sh
.claude/skills/pr-review-locally/SKILL.md
.github/prompts/docs-review.md
AGENTS.md
CHANGELOG.md
Makefile
docs/src/content/docs/claude-code.md
docs/src/content/docs/development.md
scripts/pre-push-reviewers.sh
scripts/skip-pre-push-review.sh

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

⏰ Context from checks skipped due to timeout. (4)

GitHub Check: Docs preview
GitHub Check: E2E tests
GitHub Check: Coverage
GitHub Check: Integration tests

🧰 Additional context used

📚 Code guidelines (5)

docs/src/content/docs/claude-code.md — auto-discovered
AGENTS.md — auto-discovered
.github/copilot-instructions.md — auto-discovered
CONTRIBUTING.md — auto-discovered
docs/src/content/docs/development.md — auto-discovered

📓 Path-based instructions (6)

Source excerpt: To add a command: drop a `.md` file in `.claude/commands/`.

📄 CodeRabbit inference engine (docs/src/content/docs/claude-code.md)

Files:

docs/src/content/docs/claude-code.md

Source excerpt: **Never hard-wrap prose.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

docs/src/content/docs/development.md
CHANGELOG.md
AGENTS.md
docs/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:

docs/src/content/docs/development.md
CHANGELOG.md
AGENTS.md
docs/src/content/docs/claude-code.md

Source excerpt: Run `make fix` to apply them.

📄 CodeRabbit inference engine (docs/src/content/docs/development.md)

Files:

docs/src/content/docs/development.md
CHANGELOG.md
AGENTS.md
docs/src/content/docs/claude-code.md

Source excerpt: **Editors see WH001 in `.md` only.**

📄 CodeRabbit inference engine (AGENTS.md)

Files:

docs/src/content/docs/development.md
CHANGELOG.md
AGENTS.md
docs/src/content/docs/claude-code.md

🪛 ast-grep (0.45.3)

.claude/hooks/review-marker.sh

[warning] 83-83: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: "$1/tmp/review-marker.log"
Note: [CWE-377] Insecure Temporary File.

(predictable-tmp-file-bash)

[warning] 183-183: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: "📝 marker written: ${wt}/tmp/${agent_type}-passed-${short}"
Note: [CWE-377] Insecure Temporary File.

(predictable-tmp-file-bash)

[warning] 185-185: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: "failed to write ${wt}/tmp/${agent_type}-passed-${short} — no marker written."
Note: [CWE-377] Insecure Temporary File.

(predictable-tmp-file-bash)

.claude/hooks/review-gate.test.sh

[warning] 227-227: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: "$repo/tmp/review-marker.log"
Note: [CWE-377] Insecure Temporary File.

(predictable-tmp-file-bash)

[warning] 229-229: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: "$repo/tmp/review-marker.log"
Note: [CWE-377] Insecure Temporary File.

(predictable-tmp-file-bash)

[warning] 431-431: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: "$wtc/tmp/$R2-passed-"
Note: [CWE-377] Insecure Temporary File.

(predictable-tmp-file-bash)

.claude/hooks/agent-bash-gate.sh

[warning] 654-654: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: "$wt/tmp/review-marker.log"
Note: [CWE-377] Insecure Temporary File.

(predictable-tmp-file-bash)

[warning] 685-685: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: "$wt/tmp/review-skips-${sha}.log"
Note: [CWE-377] Insecure Temporary File.

(predictable-tmp-file-bash)

🪛 LanguageTool

.claude/commands/docs-review.md

[typographical] ~12-~12: Consider using an em dash in dialogues and enumerations.
Context: - empty → gating mode: the branch...

(DASH_RULE)

.claude/skills/pr-review-locally/SKILL.md

[style] ~43-~43: Since ownership is already implied, this phrasing may be redundant.
Context: ...r current directory, which may still be your own checkout. For example: ```js // all in...

(PRP_OWN)

.claude/agents/docs-reviewer.md

[typographical] ~14-~14: Consider using an em dash in dialogues and enumerations.
Context: - Gating mode — scope is **empty / de...

(DASH_RULE)

[style] ~36-~36: Consider using the typographical ellipsis character here instead.
Context: ...s another path). Diff against that sha (main...<sha>) and report exactly it on the `RE...

(ELLIPSIS)

[style] ~39-~39: Consider using the typographical ellipsis character here instead.
Context: ... but didn't" check)* — diff the branch (git diff --name-only main...<sha>) and walk the changed **code/conf...

(ELLIPSIS)

[style] ~67-~67: Since ownership is already implied, this phrasing may be redundant.
Context: ...y by the two parseable lines**, each on its own line: ```text REVIEWED: <the full 40-c...

(PRP_OWN)

CHANGELOG.md

[typographical] ~119-~119: Consider using an em dash in dialogues and enumerations.
Context: - **A pre-push review marker now attests ...

(DASH_RULE)

[uncategorized] ~119-~119: The official name of this software platform is spelled with a capital “H”.
Context: ...ude/skills/pr-review-locally/SKILL.md, .github/prompts/docs-review.md, scripts/{skip...

(GITHUB)

[style] ~119-~119: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...thub.com//issues/659). review-marker.sh keyed a reviewer's marker to whatever HEAD the session's launch directory had when the reviewer stopped, so a commit made while a reviewer ran inherited its ship_it, and a review of a sibling worktree marked the launch directory's branch instead. Reviewers now end with `REVIEWED: ...

(TOO_LONG_SENTENCE)

[style] ~119-~119: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ... the launch directory's branch instead. Reviewers now end with REVIEWED: <sha>, the commit they pinned before reading anything (an unambiguous prefix will do), and the hook writes the marker into each worktree on that commit, and only when that worktree's HEAD reflog shows it on the commit from the start of the review to its end (git reset or git stash on the same commit doesn't count as a move). When last_assistant_message lacks e...

(TOO_LONG_SENTENCE)

[typographical] ~119-~119: The word ‘When’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...no longer has to be recorded as a skip. Each decision goes totmp/review-marke...

(WRB_QUESTION_MARK)

.claude/agents/pre-push-reviewer.md

[style] ~14-~14: Consider using the typographical ellipsis character here instead.
Context: ...is the local working state, computed as git diff main...<sha> for the commit you pin in step 1 ...

(ELLIPSIS)

[uncategorized] ~26-~26: The official name of this software platform is spelled with a capital “H”.
Context: ...d so it can refuse the marker. 2. Read .github/prompts/pr-review.md and AGENTS.md (...

(GITHUB)

[style] ~57-~57: Since ownership is already implied, this phrasing may be redundant.
Context: ...y by the two parseable lines**, each on its own line: ```text REVIEWED: <the ful...

(PRP_OWN)

.claude/commands/prepush.md

[style] ~33-~33: Consider using a more formal/concise alternative here.
Context: ...the reviewer name. To review a worktree other than your current directory, name its path i...

(OTHER_THAN)

AGENTS.md

[style] ~10-~10: 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: ...t ran returns ship_it. Every reviewer needs a marker for the commit being pushed — ...

(EN_REPEATEDWORDS_NEED)

[style] ~261-~261: Consider an alternative for the overused word “exactly”.
Context: ...e first push, before gh pr create, is exactly when the diff most needs review.) When...

(EXACTLY_PRECISELY)

[style] ~270-~270: 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)

docs/src/content/docs/claude-code.md

[uncategorized] ~83-~83: The official name of this software platform is spelled with a capital “H”.
Context: ... wt switch pr:<N>. Runs the canonical .github/prompts/pr-review.md workflow against ...

(GITHUB)

[uncategorized] ~84-~84: The official name of this software platform is spelled with a capital “H”.
Context: ...t changed but whose docs didn't), using .github/prompts/docs-review.md over the `scrip...

(GITHUB)

🪛 SkillSpector (2.11.2)

.claude/skills/pr-review-locally/SKILL.md

[warning] 75: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.

Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.

(Excessive Agency (EA2))

🔇 Additional comments (16)

.claude/agents/docs-reviewer.md (1)

14-14: LGTM!

Also applies to: 36-39, 67-74, 84-84

.claude/agents/pre-push-reviewer.md (1)

14-29: LGTM!

Also applies to: 39-39, 57-64, 90-90

.claude/commands/docs-review.md (1)

12-12: LGTM!

.claude/hooks/review-marker.sh (1)

140-193: LGTM!

.claude/skills/pr-review-locally/SKILL.md (1)

31-31: LGTM!

Also applies to: 43-50

.github/prompts/docs-review.md (1)

54-54: LGTM!

AGENTS.md (1)

10-10: LGTM!

Also applies to: 261-261, 270-270, 283-283, 308-308

docs/src/content/docs/claude-code.md (1)

27-28: LGTM!

Also applies to: 53-60, 83-84, 109-109, 213-213

scripts/pre-push-reviewers.sh (1)

6-9: LGTM!

.claude/commands/prepush.md (1)

6-10: LGTM!

Also applies to: 33-35, 39-39, 52-57

.claude/hooks/agent-bash-gate.sh (1)

44-96: LGTM!

Also applies to: 141-608, 690-781

scripts/skip-pre-push-review.sh (1)

31-36: LGTM!

.claude/hooks/review-gate.test.sh (1)

1-454: LGTM!

CHANGELOG.md (1)

119-119: LGTM!

Makefile (1)

449-457: LGTM!

Also applies to: 548-552, 566-566

docs/src/content/docs/development.md (1)

22-22: LGTM!

Also applies to: 42-42, 531-531

Two review findings in the push gate and the marker hook, each with a test that fails without the fix: - The gate took local main as the base when it existed, so commits made on local main by mistake, then branched from (`git switch -c feat`), counted as already on the base and were pushed with no review. The base is now origin/main, and local main only when there's no remote-tracking ref; a stale origin/main only gates more. - The marker hook fell back to the hand-back report whenever the final message lacked either parseable line, so a final `VERDICT: iterate` without a REVIEWED line was replaced by an earlier hand-back's ship_it. It now falls back only when the final message has no verdict or says ship_it without naming the commit. The new-reviewer recipe in AGENTS.md and the Claude Code docs now says to pin with `git -C <worktree> rev-parse HEAD`, as the shipped reviewers do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The header of review-marker.sh still said the final message is the report only when it carries both parseable lines. Since the last change it is the report whenever it has a verdict, unless that verdict is ship_it with no REVIEWED line, and a final iterate or block stands. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Conflict resolution in docs/src/content/docs/development.md: the git/curl prerequisites row keeps both sides, so it is the git + curl + jq row with main's shellcheck and oldest-Node curl uses and this branch's jq rationale. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

EricAndrechek · 2026-10-07T18:10:28Z

@coderabbitai approve

All three findings from your review are fixed in ff0a491 (header comment in 36da460), and you confirmed each in its thread; 26e4f07 only merges main.

— Posted by Claude Code on behalf of @EricAndrechek

coderabbitai · 2026-10-07T18:11:03Z

@EricAndrechek I’ll submit approval for #755. Thanks for fixing the three findings.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Action performed

Changes approved. Review threads were not changed.

…756) ## Summary The Integration job grew from about 3 to about 8 minutes with the distributed-backend tests and became CI's critical path (#701). This splits the suite into parts that CI runs as parallel jobs, runs the slow self-contained tests in parallel, and selects `internal/mq`'s integration tests by build tag instead of by name (#692). - **Parts, not shards.** `make test-integration` runs `test-integration-app` (`tests/integration`) and `test-integration-backends` (`internal/cache`, `internal/mq/natsspike`, `internal/mq`'s tagged tests) together. The `integration` job is a matrix over the Makefile's `INTEGRATION_PARTS`, which the `changes` job reads (`scripts/ci/integration-parts.sh` refuses anything but one `INTEGRATION_PARTS := …` line, and every part checks it against make's own list) and also turns into the fragment and producer lists the coverage job waits for. CI runs exactly the commands a local run does, a part added to the Makefile gets its own job with no workflow edit, and each part uploads `coverage-integration-<part>` for the coverage job to merge. Every Go job passes its own build-cache suffix (`-integration-<part>`, `-unit`, …), since two jobs on one key race to save it and a part would otherwise save a cache holding only its own test binaries. - **Parallel tests.** Ten slow tests in `tests/integration` that bring up their own servers or containers (shard claims, the NATS backend, the shared cache, boot resilience, the ClickHouse outage, the query errors against a stopped ClickHouse) call `t.Parallel()`, and the app part runs with `-parallel 8` since they mostly wait. `TestShardClaims_ScaleOneThreeTwo`, the dedupe DynamoDB tests and the tests that share the wired app are untouched. - **Nothing skipped silently.** `scripts/ci/tagged-tests.sh run` prints the tests, fuzz targets and examples `internal/mq`'s `integration`-tagged files declare, replacing the `-run '^Test(ExternalNATS|NewNATS|NATSPermissions_Refuse|Leases)'` pattern, and `tagged-tests.sh check`, which every part runs first, fails while a package with tagged tests is in no part. `scripts/ci/tagged-tests.test.sh` checks the selection against every entry `go test -list` prints on a throwaway module, and `scripts/ci/integration-parts.test.sh` checks the parts reading (both `make verify` leaves). - `internal/mq/natsspike` stays in the suite: it pins the nats-server behavior the external-NATS topology rests on, and it runs beside `internal/cache`, so it adds no wall-clock. ## Test plan Timings on a 4-vCPU amd64 Linux box (GitHub's runners have 4 vCPUs too), warm build cache: - [x] `make ci` green locally. - [x] Baseline on `main`: the two gotestsum calls took 339s + 58s = 397s (`tests/integration` 336.7s). - [x] `make test-integration COV_DEFER=1`: 221s, all pass. `tests/integration` 143.9s with the same 276 tests (test set diffed against the baseline run); backends part 76s (cache 14s alongside natsspike 17s, then `internal/mq` 56s, the same 217 + 95 tests). - [x] `test-integration-app` four more times: 143.9s to 154.9s. One run failed in `TestShardClaims_ScaleOneThreeTwo` at the one-writer check (#737); it fails the same way on `main` (the baseline run hit it), and it runs in the sequential phase, before any parallel test starts. - [x] Coverage: the integration profile covers 64.60% of statements against 64.59% for the baseline, and every block the baseline covered (9883 statements in both). - [x] #692: with a throwaway `//go:build integration` `TestMQNATSSomethingNew` in `internal/mq`, the old pattern lists 60 tests and misses it; the new selection lists 61. Without it, both select the same 60. - [x] Six more `test-integration-app` runs on a fresh 4-vCPU box: all pass (138–160s warm, 283s for the first, cold build). `-count=10` of the four parallel shard-claim tests together: 40 of 40 pass. - [x] `tagged-tests.test.sh` (it fails against the old name-only pattern), `integration-parts.test.sh`, `make check-integration-parts`, shellcheck and actionlint pass. - [x] Pre-push review: code `ship_it`; docs review `ship_it`. - [ ] CI: `Integration tests (app)` and `Integration tests (backends)` replace `Integration tests`, the coverage job finds both fragments, and the run takes about 3.5–4 minutes (it was 8–9). The first run on each new cache suffix compiles cold. Remaining findings: none. ## Related Issues Closes #701 Closes #692 Part of #740 Stacked on #755 (base `fix/prepush-gate-holes`) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

…782) ## Summary `make test-review-gate` failed intermittently in CI (four of the six `main` runs since #755, and one pull-request run) on two cases: `skip-pre-push-review.sh writes to the worktree root from a subdirectory` and `skipped reviewers satisfy the gate and are echoed`. The bug is in `scripts/skip-pre-push-review.sh`, not in the test or the hooks, and nothing about pushes to `main` causes it. The script checked the reviewer name with `bash scripts/pre-push-reviewers.sh | grep -Fxq -- "$name"` under `pipefail`. Bash writes each name in its own `write()` (one line per call under `strace` on bash 5.2), so `grep -q` can match `pre-push-reviewer` on the first line and exit before the second write. That write then dies of `SIGPIPE`, `pipefail` reports exit 141 as no match, and the script refuses: "'pre-push-reviewer' is not a reviewer in scripts/pre-push-reviewers.sh — refusing." Only the first-listed reviewer is affected. The test sent the script's stderr to `/dev/null`, so the CI log showed only a missing marker. - `scripts/skip-pre-push-review.sh` captures the list, then matches it from a here-string. Its advisory `git diff --name-only | grep -qv` check gets the same treatment. That check now resolves its base as the push gate has since #755 (`origin/main`, else `main`), which its comment already claimed. - `.claude/hooks/review-gate.test.sh` prints the skip script's stderr when a skip fails and requires both skipped reviewers' markers. It also adds a case whose manifest pauses after each name, so the reader has always exited before the next write. The old script fails that case on every run, on any machine. `review-marker.sh` has the same pipeline shape but puts `grep -E` in the middle. That `grep -E` buffers its output and writes it all at end of input, so its last `grep -q` never exits early. Run by hand against the same pausing manifest, that pipeline exited 0 every time on macOS and Linux, so the hook needs no change. The gate reads the whole list without `-q`. ## Test plan - [x] Reproduced on unchanged `main` (423f117) in `ubuntu:24.04` (bash 5.2) after a depth-1 clone with `main` checked out as a local branch, as `actions/checkout` does on a push. Idle, the suite passes. With 8 copies running at once on 4 pinned CPUs, 2 of 32 runs failed exactly the two CI cases. With the fix in place, 0 of 32 failed. - [x] Ran `skip-pre-push-review.sh pre-push-reviewer` 2000 times in 8 parallel loops on 4 CPUs (bash 5.2). The old script refused 35 times, every time with the message above. The fixed script refused 0 times. - [x] The new test case fails against the old script on bash 5.3, bash 3.2 and Linux bash 5.2 (136 ok, 1 FAIL that shows the refusal), and passes with the fix. - [x] `.claude/hooks/review-gate.test.sh`: all 137 cases pass under Homebrew bash 5.3 and under `/bin/bash` 3.2 with `/bin` first on `PATH`. - [x] `make lint-sh`, `make test-review-gate`, and `make verify` (pre-commit). ## Related Issues Closes #767. Part of #740. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

EricAndrechek and others added 8 commits October 7, 2026 06:33

github-project-automation Bot added this to WaveHouse Task Board Oct 7, 2026

EricAndrechek mentioned this pull request Oct 7, 2026

ci(integration): split the suite into parallel parts, select by tag #756

Merged

9 of 10 tasks

github-actions Bot added documentation Improvements or additions to documentation area/docs Documentation, site/, README area/infra CI, build, deploy, Docker, release labels Oct 7, 2026

EricAndrechek mentioned this pull request Oct 7, 2026

epic(ci): flaky tests, CI speed and test-infrastructure reliability #740

Open

coderabbitai Bot requested changes Oct 7, 2026

View reviewed changes

Comment thread .claude/hooks/agent-bash-gate.sh Outdated

Comment thread .claude/hooks/review-marker.sh Outdated

Comment thread AGENTS.md Outdated

EricAndrechek and others added 2 commits October 7, 2026 13:27

EricAndrechek marked this pull request as ready for review October 7, 2026 17:56

EricAndrechek requested review from a team and taitelee October 7, 2026 17:56

coderabbitai Bot approved these changes Oct 7, 2026

View reviewed changes

EricAndrechek merged commit cd4192e into main Oct 7, 2026
26 of 31 checks passed

EricAndrechek deleted the fix/prepush-gate-holes branch October 7, 2026 18:47

This was referenced Oct 8, 2026

CI red on main since #755: make test-review-gate fails 2 cases in Lint #767

Closed

fix(agents): stop the reviewer skip from refusing the first reviewer #782

Merged

Navigation Menu

Repository navigation

fix(agents): key review markers to the reviewed commit and worktree - #755

fix(agents): key review markers to the reviewed commit and worktree#755
EricAndrechek merged 11 commits into
mainfrom
fix/prepush-gate-holes

EricAndrechek commented Oct 7, 2026

Uh oh!

EricAndrechek commented Oct 7, 2026

Uh oh!

coderabbitai Bot commented Oct 7, 2026 •

edited

Loading

Review skipped

Summary by CodeRabbit

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot commented Oct 7, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 7, 2026 •

edited

Loading

Uh oh!

github-code-quality Bot commented Oct 7, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EricAndrechek commented Oct 7, 2026

Uh oh!

coderabbitai Bot commented Oct 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

EricAndrechek commented Oct 7, 2026

Summary

Rolling this out to an open Claude Code window

Test plan

Related Issues

Uh oh!

EricAndrechek commented Oct 7, 2026

Uh oh!

coderabbitai Bot commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Summary by CodeRabbit

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-code-quality Bot commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Overview

Go

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EricAndrechek commented Oct 7, 2026

Uh oh!

coderabbitai Bot commented Oct 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Oct 7, 2026 •

edited

Loading

coderabbitai Bot commented Oct 7, 2026 •

edited

Loading

github-actions Bot commented Oct 7, 2026 •

edited

Loading

github-code-quality Bot commented Oct 7, 2026 •

edited

Loading