feat(devtools): add merge-gate, a structural pre-merge safety check - #3518
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds ChangesMerge Gate
Sequence Diagram(s)sequenceDiagram
participant CLI
participant merge_gate
participant GitCheckout
participant GitHub
participant ReceiptStore
CLI->>merge_gate: record(pr, command)
merge_gate->>GitHub: fetch PR head
merge_gate->>GitCheckout: check SHA and status
merge_gate->>ReceiptStore: persist verification receipt
CLI->>merge_gate: check(pr)
merge_gate->>ReceiptStore: load current-head receipt
merge_gate->>GitHub: poll PR state and review comments
merge_gate-->>CLI: emit merge verdict
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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: 6e4bc9249a
ℹ️ 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".
| head_sha = info["headRefOid"] | ||
|
|
||
| argv = shlex.split(command) | ||
| started = time.time() | ||
| result = subprocess.run(argv, capture_output=True, text=True) |
There was a problem hiding this comment.
When a merge-train coordinator is currently on master, another PR, or a dirty worktree, the fetched head_sha is only copied into the receipt; the verification command still runs in the unchanged current directory. check then accepts that receipt because its recorded SHA matches the remote PR, even though unrelated source was tested. Check out the fetched commit in an isolated worktree, or at minimum require a clean local HEAD equal to headRefOid, before issuing the receipt.
AGENTS.md reference: AGENTS.md:L494-L500
Useful? React with 👍 / 👎.
| examples=( | ||
| 'devtools workspace merge-gate record 3517 --command "devtools verify --quick"', | ||
| "devtools workspace merge-gate check 3517", |
There was a problem hiding this comment.
Operators following this documented example record devtools verify --quick, which explicitly runs no tests and is not the required verification baseline. Consequently the gate's standard workflow can report OK for exactly the test regression it is meant to prevent; use the default devtools verify or an explicit affected-test command in the example and enforce or record the required verification profile.
AGENTS.md reference: AGENTS.md:L341-L343
Useful? React with 👍 / 👎.
| try: | ||
| review_comments = _gh_json(["api", f"repos/{{owner}}/{{repo}}/pulls/{pr}/comments"]) |
There was a problem hiding this comment.
If the review bot posts 30–60 seconds after this request—the exact motivating incident—this single API snapshot is empty, so check returns OK and the comment can arrive before the coordinator merges. There is no grace delay, repeated stable poll, or minimum head age, so this does not replace the claimed grace-period polling; require a quiet window with repeated observations before returning OK.
AGENTS.md reference: AGENTS.md:L369-L371
Useful? React with 👍 / 👎.
| if head_committed_at: | ||
| for comment in review_comments: | ||
| created_at = comment.get("created_at", "") | ||
| if created_at > head_committed_at: | ||
| verdict.late_comments.append( |
There was a problem hiding this comment.
Once any inline comment is posted after the latest commit, its created_at > head_committed_at comparison remains true forever; re-recording verification or resolving/dismissing the comment cannot make the gate pass. Thus even a reviewed false positive or question requires an empty/new commit despite the message instructing the operator merely to read and triage it. Persist an acknowledgement tied to the comment and current head, or compare against an explicit reviewed-at receipt.
Useful? React with 👍 / 👎.
CodeRabbit review on merge-gate's own PR (#3518) found real gaps: 1. record() only copied the fetched head_sha into the receipt without verifying the local checkout was actually AT that commit -- a receipt could attest to unrelated code (e.g. recording from master or a stale worktree). Now refuses (exit 2) unless `git rev-parse HEAD` matches the PR's headRefOid and the tree is clean. 2. The documented example used `devtools verify --quick`, which explicitly skips tests -- the exact profile that would have missed PR #3517's 43-test regression, this tool's own motivating incident. Fixed the example to `devtools verify`, and record() now tags a receipt with skips_tests: true when the command looks like it didn't run tests (heuristic), which check() surfaces as an advisory. 3. check() took a single comment snapshot, so a comment posted 30-60s later (the PR #3502 incident this tool exists to prevent) could still slip through if check() ran before it landed. check() now polls comments across a configurable grace window (default 3x20s) instead of one snapshot. 4. Once a comment's created_at was later than the head commit, it blocked forever with no way to mark it triaged short of an empty commit. Added `ack <PR> <comment-id> --reason "..."`, scoped to the PR's current head sha so a new push always re-requires triage. Verification: devtools test tests/unit/devtools/test_merge_gate.py -- 16 passed (added: checkout-mismatch refusal, dirty-tree refusal, skips_tests flagging, multi-round poll catching a comment that only appears on round 2, ack suppressing a late comment for its exact head sha but not a different one). devtools verify --quick exit 0.
6e4bc92 to
7b80472
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devtools/command_catalog.py`:
- Around line 690-693: Correct the `record` command description in the command
catalog so it states that the command verifies the current checkout already
matches the PR’s exact head commit and is clean, then refuses otherwise; do not
claim that `record` checks out or switches commits. Preserve the surrounding
behavior description and rendered documentation contract.
In `@devtools/merge_gate.py`:
- Line 259: In devtools/merge_gate.py at lines 259-259, 288-288, and 183-183,
add a shared guarded JSON reader that returns None for absent, unreadable,
invalid, or non-object files. Use it for the receipt at 259-259 and preserve the
existing no-receipt BLOCK reason, use None as an empty acknowledgement map at
288-288, and make cmd_ack at 183-183 fail with an explicit message instead of
overwriting unreadable acknowledgements.
- Around line 71-77: Update _LOOKS_LIKE_TESTS_MARKERS so _command_skips_tests
recognizes the bare full-verify command, “devtools verify,” as a test-running
command. Keep the existing _TEST_SKIPPING_MARKERS precedence and markers
unchanged so “devtools verify --quick” and other explicitly test-skipping
profiles remain flagged.
- Around line 146-148: Update the command execution flow around shlex.split and
subprocess.run to cleanly refuse unusable --command values: reject an empty
parsed argv and handle missing executables without allowing IndexError or
FileNotFoundError tracebacks. Emit the same explicit REFUSING message style used
by the other preconditions, and preserve the fail-closed behavior without
writing a receipt.
- Around line 247-249: Update the head commit selection in the commits handling
flow to search for the commit whose oid matches the already-known head_sha,
rather than using commits[-1]. Preserve the existing None behavior when no
matching commit is present, so head_committed_at is only derived from the exact
PR head commit.
- Around line 290-315: Update the head_committed_at fallback in the late-comment
check so that when review_comments exist but the commit timestamp is
unavailable, verdict.ok is set to False before recording the existing reason.
Preserve the current late-comment evaluation and success behavior when
head_committed_at is known.
- Around line 191-196: Update _fetch_review_comments to retrieve every page from
the GitHub review-comments endpoint rather than only the default first page,
using the existing GitHub CLI/API flow and handling pagination failures
consistently. Aggregate all page results into one flat list[dict[str, Any]]
before returning it so the late-comment scan examines comments beyond the first
30.
- Line 193: Update _fetch_review_comments() to include issue-level comments from
issues/{pr}/comments and submitted review bodies from pulls/{pr}/reviews in the
same late-comment window as pulls/{pr}/comments, so cmd_check detects all
top-level review signals; preserve the existing filtering and gate behavior for
the combined results.
In `@tests/unit/devtools/test_merge_gate.py`:
- Around line 289-296: Update test_check_blocks_when_receipt_older_than_max_age
to use the repository’s frozen_clock fixture, freeze recording and checking at a
known time, then advance the clock beyond a realistic positive max_age_s before
calling cmd_check. Assert the existing failure result while exercising the
intended receipt-age expiration branch, rather than relying on max_age_s=-1 or
host wall-clock time.
- Around line 59-71: Add coverage for the missing blocking paths in cmd_check:
create a receipt with a non-zero exit_code and assert cmd_check returns 1, then
add tests confirming it also blocks when mergeStateStatus is outside CLEAN,
UNSTABLE, or UNKNOWN and when _poll_stable_comments returns None after gh api
failure. Reuse existing helpers such as _base_pr_view and _fake_run, keeping the
tests focused on the gate’s non-zero result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5f26056a-cf69-46cc-992d-4d585f7ae2a5
📒 Files selected for processing (4)
devtools/command_catalog.pydevtools/merge_gate.pydocs/devtools.mdtests/unit/devtools/test_merge_gate.py
Problem: across a ~28-PR merge train in one coordinator session (2026-08-01), two incidents showed operator memory doesn't scale as the sole safety net before squash-merging. PR #3502 merged with 0 review comments showing at check time; CodeRabbit posted 3 real findings 30-60s later (caught only by an ad hoc grace-period poll habit adopted afterward). PR #3517 nearly merged carrying a 43-test regression that no CI check or review comment ever flagged, since per-PR CI deliberately skips the heavy test suite (CLAUDE.md) - caught only because the coordinator happened to run the broader local suite by hand before merging that specific time. What changed: `devtools workspace merge-gate record <PR> --command "..."` runs a local verification command against a PR's current head sha and persists a receipt keyed to that exact sha under .cache/verify/merge-gate/. `merge-gate check <PR>` BLOCKs unless a fresh, exit-0 receipt exists for the PR's CURRENT head (a new push invalidates the old receipt) and no review comment's created_at is newer than the head commit's timestamp - late comments are listed explicitly rather than requiring a human to compare two timestamps by hand. This doesn't replace judgment about what a late comment means; it makes an unverified late signal impossible to merge past silently. Verification: devtools test tests/unit/devtools/test_merge_gate.py (9 passed, covering fresh/stale-sha/late-comment/closed-PR/expired- receipt cases against a faked gh subprocess). Live smoke test against open PR #3517: record + check round-tripped correctly (BLOCK before recording, OK after). devtools verify --quick exit 0.
CodeRabbit review on merge-gate's own PR (#3518) found real gaps: 1. record() only copied the fetched head_sha into the receipt without verifying the local checkout was actually AT that commit -- a receipt could attest to unrelated code (e.g. recording from master or a stale worktree). Now refuses (exit 2) unless `git rev-parse HEAD` matches the PR's headRefOid and the tree is clean. 2. The documented example used `devtools verify --quick`, which explicitly skips tests -- the exact profile that would have missed PR #3517's 43-test regression, this tool's own motivating incident. Fixed the example to `devtools verify`, and record() now tags a receipt with skips_tests: true when the command looks like it didn't run tests (heuristic), which check() surfaces as an advisory. 3. check() took a single comment snapshot, so a comment posted 30-60s later (the PR #3502 incident this tool exists to prevent) could still slip through if check() ran before it landed. check() now polls comments across a configurable grace window (default 3x20s) instead of one snapshot. 4. Once a comment's created_at was later than the head commit, it blocked forever with no way to mark it triaged short of an empty commit. Added `ack <PR> <comment-id> --reason "..."`, scoped to the PR's current head sha so a new push always re-requires triage. Verification: devtools test tests/unit/devtools/test_merge_gate.py -- 16 passed (added: checkout-mismatch refusal, dirty-tree refusal, skips_tests flagging, multi-round poll catching a comment that only appears on round 2, ack suppressing a late comment for its exact head sha but not a different one). devtools verify --quick exit 0.
Real findings, all fixed: - use_when text overclaimed that `record` checks out the PR's head commit; it only verifies the current checkout already matches. - `_command_skips_tests` flagged the documented happy-path command itself (`devtools verify`) as test-skipping -- added it to the positive markers. - An empty --command or a missing executable raised an unhandled IndexError/FileNotFoundError instead of a clean refusal. - `gh api .../comments` without pagination silently hid comments past the first page (30-100 items) from the late-comment check. - The late-comment check only watched inline diff comments, missing issue-level PR comments and review summary bodies -- now merges and normalizes all three, dropping empty-bodied entries (e.g. a plain APPROVE review). - `commits[-1]` assumed positional ordering matched the PR head; switched to matching by oid, since a capped/reordered commits array could silently mispoint the late-comment timestamp. - Receipt/ack JSON reads had no guard against a truncated or corrupt file; added a shared `_read_json_object` used everywhere, and `cmd_ack` now refuses explicitly on a corrupt ack file rather than silently discarding prior acknowledgements. - Critical: when the head commit's timestamp couldn't be determined, the late-comment check silently reported OK (an `elif` that never set ok=False) -- exactly the "fails closed" contract this tool exists to guarantee. Now blocks explicitly in that case. Verification: devtools test tests/unit/devtools/test_merge_gate.py -- 20 passed (added: nonzero-receipt-exit-code block, dirty mergeStateStatus block, comment-polling-failure block, a late review body caught alongside inline comments, frozen_clock for the freshness test per repo convention). devtools verify --quick exit 0 (one transient unrelated SQLite disk-I/O error in demo-corpus- datasheet rendering reproduced as a one-off and cleared on rerun).
7b80472 to
bb3cbbd
Compare
1 participant
Summary
Adds
devtools workspace merge-gate, replacing coordinator memory (grace-periodcomment polling, remembering to run the broader local test suite per-PR CI
skips) with a check that fails closed.
Problem
Two incidents in a single ~28-PR merge-train session (2026-08-01):
CodeRabbit posted 3 real findings 30-60s later.
review comment ever flagged (per-PR CI deliberately skips the heavy test
suite — see CLAUDE.md). It was caught only because the coordinator
happened to run the broader local suite by hand before merging that time.
A coordinator merging dozens of PRs across a few hours cannot reliably repeat
either habit purely from memory every single time.
Solution
devtools workspace merge-gate record <PR> --command "..."runs a localverification command against the PR's current head sha and persists a
receipt keyed to that exact sha under
.cache/verify/merge-gate/.devtools workspace merge-gate check <PR>BLOCKs unless a fresh, exit-0receipt exists for the PR's CURRENT head (a new push invalidates the old
receipt) and no review comment's
created_atis newer than the headcommit's timestamp — late comments are listed explicitly instead of
requiring a human to compare timestamps by hand.
Does not replace judgment about what a late finding means — makes an
unverified late signal impossible to merge past silently.
Verification
devtools test tests/unit/devtools/test_merge_gate.py— 9 passed (freshreceipt, stale-sha, late-comment, closed-PR, expired-receipt cases against a
faked
ghsubprocess). Live smoke test against open PR #3517:checkBLOCKedbefore recording, then
record+checkround-tripped to OK.devtools verify --quickexit 0.Summary by CodeRabbit
New Features
Documentation
Tests