fix(pr-reviewer): gate the "ready" ping; stop committing VFS state#50
Conversation
The reviewer announced PRs "ready for your review" in cases where it shouldn't, and re-announced the same commit on every webhook: - verifyReadyForHumanReview only queried mergeable,statusCheckRollup — never the PR state — so a PR that merged/closed between the harness run and the check could still be announced ready. Now query state and gate on OPEN. - prReadyStateAllowsHumanReview let an *empty* statusCheckRollup pass vacuously (checks.every === true), so a PR whose checks were queued but not yet registered read as "ready" — the pending-checks symptom. Now an empty rollup is not-ready, and drafts / CHANGES_REQUESTED are held back. - announceReadyOnce records the head SHA it announced ready for and skips re-announcing the same commit; new commits still get a (threaded) note. Also: require the harness to account for every bot/reviewer comment under an "## Addressed comments" heading with the file:line where each was fixed, so the channel and comment authors can see where each was handled. And stop tracking memory/workspace/.relay/state.json — it's relay VFS runtime state rewritten every reconcile cycle, so the review harness kept committing it to PRs. Untracked + gitignored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Warning Review limit reached
More reviews will be available in 40 minutes and 52 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the PR review agent to prevent duplicate Slack notifications by tracking announced ready states per head commit, requiring agents to explicitly document addressed comments, and enhancing the PR ready-state checks (ensuring the PR is open, not a draft, has no changes requested, and has passing status checks). Feedback highlights that if pr.headSha is missing, duplicate notifications can still occur, and suggests querying headRefOid from GitHub to populate it. Additionally, reviewers noted that requiring at least one status check might block PRs in repositories without CI configured, and skipped checks might inappropriately block the ready status.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| 'state,mergeable,mergeStateStatus,reviewDecision,statusCheckRollup', | ||
| ], { cwd: ctx.sandbox.cwd, encoding: 'utf8', maxBuffer: 1024 * 1024 }); | ||
| const state = parsePrReadyState(stdout); | ||
| const ready = prReadyStateAllowsHumanReview(state); |
There was a problem hiding this comment.
If pr.headSha is missing or undefined in the webhook payload (which can happen on certain events), announceReadyOnce will not be able to track whether the PR has already been announced. This leads to duplicate Slack notifications on every subsequent webhook execution.
By querying headRefOid from gh pr view and populating pr.headSha, we ensure that we always have a reliable commit SHA to deduplicate announcements and safely perform merges.
'state,mergeable,mergeStateStatus,reviewDecision,statusCheckRollup,headRefOid',
], { cwd: ctx.sandbox.cwd, encoding: 'utf8', maxBuffer: 1024 * 1024 });
const state = parsePrReadyState(stdout);
if (typeof state.headRefOid === 'string') {
pr.headSha = state.headRefOid;
}
const ready = prReadyStateAllowsHumanReview(state);| reviewDecision?: unknown; | ||
| statusCheckRollup?: unknown; |
There was a problem hiding this comment.
Add headRefOid to the PullRequestReadyState interface to support populating pr.headSha from the gh pr view output.
reviewDecision?: unknown;
statusCheckRollup?: unknown;
headRefOid?: unknown;| // checks haven't registered yet (still pending) — not "no checks to worry | ||
| // about" — so treat it as not-ready rather than letting `every` pass vacuously. | ||
| const checks = Array.isArray(state.statusCheckRollup) ? state.statusCheckRollup : []; | ||
| if (checks.length === 0) return false; |
There was a problem hiding this comment.
Requiring checks.length > 0 introduces two potential issues:
- Repositories without CI/status checks: If a repository has no status checks configured,
checks.lengthwill always be0, permanently blocking the PR from being announced as ready. Consider making this behavior configurable (e.g., via aREQUIRE_CI_CHECKSinput option). - Skipped checks: The
checkPassedAndCompletehelper does not currently treatSKIPPEDas a successful/passing conclusion. In GitHub Actions, skipped checks are common and do not block merges, but here they will prevent the PR from being marked as ready.
|
No findings. I reviewed the PR diff and traced the changed Addressed comments
Validation run locally:
I am not printing |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…hecks, no-CI repos From cubic + gemini review of #50: - Populate pr.headSha from `gh pr view --json headRefOid` so the ready dedupe always keys on the commit. Some webhook payloads (check_run. completed) omit the head SHA; without it the dedupe was bypassed and re-pinged. (cubic P1 / gemini) - Treat SKIPPED checks as non-blocking in checkPassedAndComplete. A conditionally-skipped CI job is GitHub's "not applicable", not a failure, and was pinning PRs out of "ready" forever. Mirrors the trigger's ciFailed(), which already treats skipped as non-failing. (gemini) - Don't permanently block no-CI repos: an empty status-check rollup now counts as ready when GitHub's mergeStateStatus is CLEAN (nothing blocking), while a transient "checks queued but unregistered" window (UNSTABLE/BLOCKED/UNKNOWN) still reads as not-ready. (gemini) The concurrent-run dedupe race cubic flagged (P2) is a known best-effort limit — the memory API has no atomic check-and-set, and per-PR webhook delivery is effectively serialized. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
✅ pr-reviewer applied fixes — committed and pushed Fixed the unresolved cubic finding: Added focused coverage for repeat same-head announcements, overlapping runs, and retry after Slack failure in tests/review-agent.test.mjs. Addressed comments
Validation:
I’m not printing the READY sentinel because these local edits still need the cloud commit and GitHub-side checks on the resulting head. |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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="review/agent.ts">
<violation number="1" location="review/agent.ts:287">
P2: Ready-announcement dedupe is bypassed when reservation save returns no id, allowing duplicate Slack pings for the same head SHA.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| async function reserveReadyAnnouncement(ctx: WorkforceCtx, pr: Pr): Promise<{ id: string } | {} | undefined> { | ||
| if (await alreadyAnnouncedReady(ctx, pr)) return undefined; | ||
| const saved = await rememberReadyAnnouncementReservation(ctx, pr); | ||
| if (!saved?.id) return {}; |
There was a problem hiding this comment.
P2: Ready-announcement dedupe is bypassed when reservation save returns no id, allowing duplicate Slack pings for the same head SHA.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At review/agent.ts, line 287:
<comment>Ready-announcement dedupe is bypassed when reservation save returns no id, allowing duplicate Slack pings for the same head SHA.</comment>
<file context>
@@ -247,40 +247,106 @@ async function reviewAndFix(ctx: WorkforceCtx, pr: Pr): Promise<void> {
+async function reserveReadyAnnouncement(ctx: WorkforceCtx, pr: Pr): Promise<{ id: string } | {} | undefined> {
+ if (await alreadyAnnouncedReady(ctx, pr)) return undefined;
+ const saved = await rememberReadyAnnouncementReservation(ctx, pr);
+ if (!saved?.id) return {};
+ const [winner] = await readyAnnouncementItems(ctx, pr, 'reservation');
+ if (!winner || winner.id === saved.id) return saved;
</file context>
| if (!saved?.id) return {}; | |
| if (!saved?.id) return undefined; |
…90) (#51) * fix(pr-reviewer): gate the "ready" ping and stop committing VFS state The reviewer announced PRs "ready for your review" in cases where it shouldn't, and re-announced the same commit on every webhook: - verifyReadyForHumanReview only queried mergeable,statusCheckRollup — never the PR state — so a PR that merged/closed between the harness run and the check could still be announced ready. Now query state and gate on OPEN. - prReadyStateAllowsHumanReview let an *empty* statusCheckRollup pass vacuously (checks.every === true), so a PR whose checks were queued but not yet registered read as "ready" — the pending-checks symptom. Now an empty rollup is not-ready, and drafts / CHANGES_REQUESTED are held back. - announceReadyOnce records the head SHA it announced ready for and skips re-announcing the same commit; new commits still get a (threaded) note. Also: require the harness to account for every bot/reviewer comment under an "## Addressed comments" heading with the file:line where each was fixed, so the channel and comment authors can see where each was handled. And stop tracking memory/workspace/.relay/state.json — it's relay VFS runtime state rewritten every reconcile cycle, so the review harness kept committing it to PRs. Untracked + gitignored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(pr-reviewer): address PR #50 review — headSha fallback, skipped checks, no-CI repos From cubic + gemini review of #50: - Populate pr.headSha from `gh pr view --json headRefOid` so the ready dedupe always keys on the commit. Some webhook payloads (check_run. completed) omit the head SHA; without it the dedupe was bypassed and re-pinged. (cubic P1 / gemini) - Treat SKIPPED checks as non-blocking in checkPassedAndComplete. A conditionally-skipped CI job is GitHub's "not applicable", not a failure, and was pinning PRs out of "ready" forever. Mirrors the trigger's ciFailed(), which already treats skipped as non-failing. (gemini) - Don't permanently block no-CI repos: an empty status-check rollup now counts as ready when GitHub's mergeStateStatus is CLEAN (nothing blocking), while a transient "checks queued but unregistered" window (UNSTABLE/BLOCKED/UNKNOWN) still reads as not-ready. (gemini) The concurrent-run dedupe race cubic flagged (P2) is a known best-effort limit — the memory API has no atomic check-and-set, and per-PR webhook delivery is effectively serialized. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(hn-monitor): claim stories as seen before posting (interim double-post guard) A single cron tick can re-invoke this handler ~3 min apart: cloud re-runs a tick delivery whose 180s processing lease expires before the first run reports done (AgentWorkforce/cloud#1990). With post-before-save, the re-invocation re-read stale `seen` and posted the digest a second time — the observed double-post. Record the stories as seen BEFORE posting so a re-invocation loads them as already-seen and stays silent. Trade-off: a failed post drops that digest rather than risking a duplicate — acceptable for a low-stakes twice-daily summary. Stopgap; the durable fix is idempotent cron delivery in cloud#1990 (stable per-occurrence key + atomic claim in the runner). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: apply pr-reviewer fixes for #51 --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: agent-relay-code[bot] <agent-relay-code[bot]@users.noreply.github.com>
…-gating # Conflicts: # review/agent.ts
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
1 participant
Why
From the review channel: the pr-reviewer was sending duplicate "ready for your review" reminders, pinging PRs that were already merged, and saying ready while checks were still pending. It was also committing a relay VFS state file into every PR.
Changes (
review/agent.ts)verifyReadyForHumanReviewnow queriesstate,mergeable,mergeStateStatus,reviewDecision,statusCheckRollup(was justmergeable,statusCheckRollup) andprReadyStateAllowsHumanReviewrejects anything notOPEN.statusCheckRollupused to pass vacuously ([].every() === true) — exactly the window where checks are queued but not yet registered. Now empty → not ready. Drafts andCHANGES_REQUESTEDare also held back. So a ping requires: open, no conflicts, not draft, no changes-requested, ≥1 check, all checks complete + passing.announceReadyOncerecords the head SHA it announced ready for (workspace memory) and won't re-announce it on subsequent webhooks; a genuinely new head SHA still gets a note, threaded under the PR's first Slack message via the existingpostSlackPrUpdatethreading.## Addressed commentssection — one bullet per comment with thefile:lineof the fix (or a reason if skipped). The sandbox has nogh/git and the VFS client only exposes whole-PRcomment/review, so this lands in the posted PR comment. Per-thread in-thread replies are tracked in github writeback: support replying to an existing PR review-comment thread (in_reply_to) relayfile-adapters#160.Also
memory/workspace/.relay/state.json— relay VFS runtime state rewritten every reconcile cycle, which the review harness kept committing to PRs.git rm --cached+ gitignored.Tests
npm run test+npm run typecheckgreen. Added cases for merged/closed, empty rollup, draft, changes-requested, and the addressed-comments prompt requirement.Related issues
🤖 Generated with Claude Code
Summary by cubic
Fixes noisy “ready for review” pings by gating on PR state and CI, deduping once per head commit with a race-safe reservation, and ignoring Relay VFS state. Adds an “Addressed comments” section with exact file:line locations.
Bug Fixes
SKIPPEDis non-blocking. An empty rollup is ready only when GitHub reportsCLEAN(no-CI repos), otherwise it stays pending.gh pr view --json headRefOid. Use a workspace reservation to choose a single winner across overlapping runs and mark failures/cancellations so a later retry can post.memory/workspace/.relay/state.jsonand ignore.relay/andmemory/workspace/.relay/in.gitignore.New Features
file:lineof the fix, or a brief reason if skipped.Written for commit 2b2a994. Summary will update on new commits.