Repository navigation
review-gate.sh: review what the branch ships and pass only on a PASS verdict - #1330
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Review of #1330 (review-gate.sh verdict gating) No blocking correctness issues found. The diff now covers commits, tracked edits and untracked files. Failures are closed on a non-zero claude exit or a missing verdict. The "|| true" guard on the grep avoids a silent pipefail exit. Non-blocking notes:
🤖 Generated with Claude Code |
…verdict Three ways the gate could pass without reviewing anything: - the diff covered commits only, so a branch whose work was still uncommitted (or untracked) sent an empty diff to the reviewer; - `mktemp ….XXXXXX.patch` is not randomised by BSD mktemp (the X's must be trailing), so every run used one fixed file and a second run collided; - the gate returned claude's exit code, which says the CLI ran, not what the review found. The diff is now everything since the merge base plus untracked files; the temp files have proper templates; claude runs non-interactively with the diff on stdin and must end with `REVIEW-VERDICT: PASS|FAIL`. A FAIL, a run that did not complete, or a missing verdict is exit 1, each with a message (the new tests also caught pipefail ending the no-verdict case silently). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
566bd37 to
056fdff
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 566bd37316
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| trap 'rm -f "$diff_file" ${output_file:+"$output_file"}' EXIT | ||
| git diff --find-renames "$base" > "$diff_file" | ||
| while IFS= read -r -d '' untracked; do | ||
| git diff --no-index -- /dev/null "$untracked" >> "$diff_file" || true |
There was a problem hiding this comment.
When git diff --no-index fails rather than returning its expected “differences found” status of 1—for example, because a diff/textconv driver fails or an untracked file is unreadable—the unconditional || true suppresses the error. The resulting patch can omit the file contents or be empty, allowing the gate to report PASS or “no branch or working-tree diff” without reviewing everything that would ship; distinguish status 1 from actual error statuses and fail the gate for the latter.
AGENTS.md reference: AGENTS.md:L137-L140
Useful? React with 👍 / 👎.
| # The exit code says the CLI ran, not what the review found: gate on the verdict. | ||
| # `|| true`: under pipefail a grep that finds no verdict would otherwise end | ||
| # the script with status 1 and no message — a silent failure. | ||
| verdict=$(grep -E '^REVIEW-VERDICT: (PASS|FAIL)[[:space:]]*$' "$output_file" | tail -1 | awk '{print $2}' || true) |
There was a problem hiding this comment.
If Claude prints a matching REVIEW-VERDICT: PASS line and then appends prose, an error, or no actual final verdict, this grep still extracts PASS and opens the gate. That contradicts the prompt's exact-final-line contract and can turn malformed or incomplete review output into success; validate the final output line itself rather than accepting a matching line anywhere in the response.
AGENTS.md reference: AGENTS.md:L137-L140
Useful? React with 👍 / 👎.
| var env = ["PATH": "\(stubs.path):/usr/bin:/bin:/opt/homebrew/bin", "HOME": NSTemporaryDirectory(), | ||
| "TMPDIR": NSTemporaryDirectory(), "GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@t", | ||
| "GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@t", "GIT_CONFIG_NOSYSTEM": "1"] |
There was a problem hiding this comment.
On developer machines where thermo-nuclear-swift-review is installed in /opt/homebrew/bin, this PATH exposes the real executable; the script selects it before the stubbed claude, so the new tests can invoke an expensive real review, hang, or fail without creating stdin.txt instead of testing the intended Claude path. Restrict PATH to the required system tools or provide a deterministic stub for the higher-priority reviewer.
Useful? React with 👍 / 👎.
|
Review of the diff: no blocking issues. The gate logic is sound (exit status checked via PIPESTATUS, explicit verdict required, and the working tree and untracked files are now included). Three minor points:
🤖 Generated with Claude Code |
Scripts/review-gate.shcould pass without reviewing anything, three ways:origin/master...HEADonly, so a branch whose work was uncommitted or untracked sent an empty diff. Now: everything since the merge base (committed + uncommitted tracked) plus untracked files.mktemp ….XXXXXX.patch— BSD mktemp only randomises trailing X's, so every run used one fixed filename and the second run collided. Templates fixed.claude's exit code, which says the CLI ran, not what the review found. Nowclaude -pruns non-interactively with the diff on stdin and must end withREVIEW-VERDICT: PASS|FAIL; FAIL, a run that didn't complete, or a missing verdict is exit 1, each with a message.The new
ReviewGateScriptTests(real script, scratch repo, stubclaude) also caught a fourth: underpipefailthe no-verdict case exited 1 silently — fixed with an explicit fallback. Exit codes 0/1/2 keep their meaning; usage anddocs/process/agent-pr-workflow.mdupdated.KEYPATH_TEST_ENFORCE_CLEAN_SUMMARY=1 Scripts/run-tests-safe.sh: 5,799 passed, 0 app warnings/errors, 0 build warnings.🤖 Generated with Claude Code