Uh oh!
There was an error while loading. Please reload this page.
fix(github): require a substantive review at head; classify vacuous bot checks (factory#432) - #451
Conversation
…ify vacuous bot checks (factory#432) `evaluateGithubMergeGate` treated `reviewDecision === 'APPROVED'` and a non-blocking `statusCheckRollup` as evidence of review, but neither is bound to the commit that actually merges nor to content. relay#1638 merged on a repo-level APPROVED whose only review at head was an empty body; a Devin/CodeRabbit/cubic check reporting a skipped or rate-limited review reads identically to a real pass. Adds a review-at-head predicate requiring at least one review pinned to `headRefOid`, from someone other than the PR author, carrying a body or inline comments — refusing with a reason naming which condition failed (no review at head, author-only, or vacuous). Widens `statusCheckRollup` parsing to keep each entry's description and classify it REAL, VACUOUS (matching known trial-expired/rate-limited/seat-unassigned/line-limit markers), or BLOCKING, so an all-vacuous check set is refused the same as no checks at all instead of satisfying the "checks observed" guard. Out of scope for this change: the CI check for the same predicate, and the seat/config repairs (cubic seats, CodeRabbit OSS trigger, Devin trial) — tracked separately in factory#432. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S2QvRkHKvzpduLuDFRipaF Session-Id: 56185aca-7dc6-42c8-b69e-650394d744aa
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. |
Warning Review limit reachedNext included review available in 17 seconds. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
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 review Requested for exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:f9cc3dff58
ℹ️ 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".
| ['reviews', Array.isArray(reviews) ? reviews : undefined], | ||
| ['author', author], |
There was a problem hiding this comment.
Requiring reviews and author here makes the existing MUST FIRE: the default gate reads readiness from the mount test in src/orchestrator/factory.test.ts reject with a capability error, because its otherwise-ready mounted payload still contains neither field while the test expects READY. The normal full vitest run therefore fails even though the two targeted test files were updated; update that fixture (and any equivalent mounted payloads) with a third-party substantive review at the expected head.
Useful? React with 👍 / 👎.
| login, | ||
| state: state.toUpperCase(), | ||
| commitId, | ||
| bodyLength: (stringValue(record.body) ?? '').length, |
There was a problem hiding this comment.
When the only third-party review at the current head has a body such as " \n" and no inline comments, bodyLength > 0 treats it as substantive and the gate can return READY using a stale repo-level approval plus an ordinary successful check. This defeats the newly introduced substantive-review guard for bot/API reviews whose nominally non-empty bodies contain only whitespace; compute the body length after trimming or otherwise test for meaningful content.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
4 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/github/merge-gate.ts">
<violation number="1" location="src/github/merge-gate.ts:315">
P2: Update the existing mounted merge-gate fixtures to include `reviews` and `author`; otherwise `MountedGithubMergeGate` now throws a capability error and the default-gate test cannot reach its expected `READY` result.</violation>
<violation number="2" location="src/github/merge-gate.ts:379">
P1: When a blocking check description contains a vacuity marker, this branch labels it `VACUOUS` before inspecting its state. `evaluateGithubMergeGate` can then proceed with a failed check; apply these markers only to non-blocking states.</violation>
<violation number="3" location="src/github/merge-gate.ts:382">
P2: A successful rollup with lowercase `state` or `status` is classified as `BLOCKING` because `classifyCheckKind` compares the raw value with uppercase states. Normalize the state before the set lookup.</violation>
<violation number="4" location="src/github/merge-gate.ts:435">
P2: Whitespace-only review bodies satisfy the substantive-review predicate because the code checks length without trimming. Trim the body before calculating `bodyLength`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| stringValue(record.text) | ||
| const classifyCheckKind = (state: string, description: string | undefined): CheckSignal['kind'] => { | ||
| if (description && VACUOUS_REVIEW_MARKERS.some((marker) => marker.test(description))) { |
There was a problem hiding this comment.
P1: When a blocking check description contains a vacuity marker, this branch labels it VACUOUS before inspecting its state. evaluateGithubMergeGate can then proceed with a failed check; apply these markers only to non-blocking states.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/github/merge-gate.ts, line 379:
<comment>When a blocking check description contains a vacuity marker, this branch labels it `VACUOUS` before inspecting its state. `evaluateGithubMergeGate` can then proceed with a failed check; apply these markers only to non-blocking states.</comment>
<file context>
@@ -311,31 +349,127 @@ const refuse = (reason: string, live: GithubMergeGateVerdict['live']): GithubMer
+ stringValue(record.text)
+
+const classifyCheckKind = (state: string, description: string | undefined): CheckSignal['kind'] => {
+ if (description && VACUOUS_REVIEW_MARKERS.some((marker) => marker.test(description))) {
+ return 'VACUOUS'
+ }
</file context>
| if(description&&VACUOUS_REVIEW_MARKERS.some((marker)=>marker.test(description))){ | |
| if(nonBlockingCheckStates.has(state)&&description&&VACUOUS_REVIEW_MARKERS.some((marker)=>marker.test(description))){ |
| login, | ||
| state: state.toUpperCase(), | ||
| commitId, | ||
| bodyLength: (stringValue(record.body) ?? '').length, |
There was a problem hiding this comment.
P2: Whitespace-only review bodies satisfy the substantive-review predicate because the code checks length without trimming. Trim the body before calculating bodyLength.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/github/merge-gate.ts, line 435:
<comment>Whitespace-only review bodies satisfy the substantive-review predicate because the code checks length without trimming. Trim the body before calculating `bodyLength`.</comment>
<file context>
@@ -311,31 +349,127 @@ const refuse = (reason: string, live: GithubMergeGateVerdict['live']): GithubMer
+ login,
+ state: state.toUpperCase(),
+ commitId,
+ bodyLength: (stringValue(record.body) ?? '').length,
+ inlineCommentsAtHead: reviewInlineComments(record),
+ }]
</file context>
| bodyLength: (stringValue(record.body)??'').length, | |
| bodyLength: (stringValue(record.body)??'').trim().length, |
| if (description && VACUOUS_REVIEW_MARKERS.some((marker) => marker.test(description))) { | ||
| return 'VACUOUS' | ||
| } | ||
| return nonBlockingCheckStates.has(state) ? 'REAL' : 'BLOCKING' |
There was a problem hiding this comment.
P2: A successful rollup with lowercase state or status is classified as BLOCKING because classifyCheckKind compares the raw value with uppercase states. Normalize the state before the set lookup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/github/merge-gate.ts, line 382:
<comment>A successful rollup with lowercase `state` or `status` is classified as `BLOCKING` because `classifyCheckKind` compares the raw value with uppercase states. Normalize the state before the set lookup.</comment>
<file context>
@@ -311,31 +349,127 @@ const refuse = (reason: string, live: GithubMergeGateVerdict['live']): GithubMer
+ if (description && VACUOUS_REVIEW_MARKERS.some((marker) => marker.test(description))) {
+ return 'VACUOUS'
+ }
+ return nonBlockingCheckStates.has(state) ? 'REAL' : 'BLOCKING'
+}
+
</file context>
| returnnonBlockingCheckStates.has(state) ? 'REAL' : 'BLOCKING' | |
| returnnonBlockingCheckStates.has(state.trim().toUpperCase()) ? 'REAL' : 'BLOCKING' |
| ['headRefOid', headRefOid], | ||
| ['reviewDecision', reviewDecision], | ||
| ['statusCheckRollup', Array.isArray(statusCheckRollup) ? statusCheckRollup : undefined], | ||
| ['reviews', Array.isArray(reviews) ? reviews : undefined], |
There was a problem hiding this comment.
P2: Update the existing mounted merge-gate fixtures to include reviews and author; otherwise MountedGithubMergeGate now throws a capability error and the default-gate test cannot reach its expected READY result.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/github/merge-gate.ts, line 315:
<comment>Update the existing mounted merge-gate fixtures to include `reviews` and `author`; otherwise `MountedGithubMergeGate` now throws a capability error and the default-gate test cannot reach its expected `READY` result.</comment>
<file context>
@@ -270,12 +304,16 @@ const mountedMergeGateFields = (
['headRefOid', headRefOid],
['reviewDecision', reviewDecision],
['statusCheckRollup', Array.isArray(statusCheckRollup) ? statusCheckRollup : undefined],
+ ['reviews', Array.isArray(reviews) ? reviews : undefined],
+ ['author', author],
].flatMap(([name, value]) => value === undefined ? [name] : [])
</file context>
…ture CI's `package` job failed: defaultMergeGate.check() now requires reviews and author in the mounted PR projection (factory#432 part a), and this fixture was missing both. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S2QvRkHKvzpduLuDFRipaF Session-Id: 56185aca-7dc6-42c8-b69e-650394d744aa
@coderabbitai review Requested for exact head |
Summary
Scoped to parts (a) and (b) of #432 only.
(a) Review-at-head predicate:
evaluateGithubMergeGatenow requires atleast one review anchored to
headRefOid, from someone other than the PRauthor, carrying a non-empty body or inline comments — not just a
repo-level
reviewDecision === 'APPROVED', which survives stale commitsand empty-bodied approvals (relay#1638: 14 reviews, exactly one at head,
empty body). Refusal reasons name which condition failed: no review at
head, review at head is author-only, or review at head has no content.
(b) Vacuous check classification:
statusCheckRollupentries are nolonger collapsed to a bare state string. Each entry keeps its
context/descriptionand is classifiedREAL,VACUOUS(matching thedescription-string markers from merge gate returns READY on PRs nobody reviewed: reviewDecision is not bound to head, and a vacuous bot check is indistinguishable from a real one #432 — Devin trial-expired, CodeRabbit
rate-limited/OSS-skip, cubic seat-unassigned/line-limit, codex usage
limits), or
BLOCKING. A check set that is all-vacuous is refused thesame as having no checks at all, instead of satisfying the "successful
status checks observed" guard.
GithubMergeGateVerdict.livegainscheckSignals: CheckSignal[];checkStates: string[]is retained,derived, for one release.
Out of scope (per the issue and lane instructions): part (c), the new
review-at-head.ymlCI workflow, and the adjacent cubic seat / CodeRabbitOSS-trigger / Devin trial repairs — those are separate ops work.
Tests
one empty-bodied approval at head → refused, not READY.
commit.
substantive.
Full review skipped: trial expired..., CodeRabbitReview rate limited/ OSS-skip, cubic seat-unassigned / line-limit → each classifiedVACUOUS, notREAL; an all-vacuous check set refuses.SUCCESSwith an ordinary description classifiedREAL.surfaced in the reason.
MountedGithubMergeGatecoverage for the two new requiredmounted fields (
reviews,author) failing closed as a capability errorwhen absent.
Test plan
npx vitest run src/github/merge-gate.test.ts src/github/gh-identity.test.ts— 34 passednpx vitest run src/github src/hosted src/writeback— 197 passednpx tsc -p tsconfig.build.json --noEmit— clean🤖 Generated with Claude Code
https://claude.ai/code/session_01S2QvRkHKvzpduLuDFRipaF
Summary by cubic
Fixes parts (a) and (b) of factory#432 by requiring a substantive third-party review anchored to the current head instead of relying on repo-level
APPROVEDalone. Status checks now distinguish real passes from vacuous bot skips, so an all-vacuous check set no longer makes a PR ready.REAL,VACUOUS, orBLOCKING; derivedcheckStatesremains available for one release.reviewsandauthorand fails closed when either is missing.Written for commit ca472c3. Summary will update on new commits.