fix(ci): reconcile PR from head SHA when the commit-PR index lags - #1445
Merged
Conversation
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe status-event resolver validates commit-to-PR associations, falls back to live open-PR head-SHA matching, and fails closed for ambiguous or failed lookups. Tests execute the workflow resolver and verify outputs, diagnostics, and API calls. ChangesStatus-event PR resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant StatusEvent
participant ResolvePR
participant GitHubAPI
StatusEvent->>ResolvePR: Resolve commit SHA
ResolvePR->>GitHubAPI: Query associated PRs
GitHubAPI-->>ResolvePR: Return association results
alt Empty or ambiguous result
ResolvePR->>GitHubAPI: List open PRs
GitHubAPI-->>ResolvePR: Return live head-SHA candidates
end
ResolvePR-->>StatusEvent: Emit unique PR output
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/ci-workflows.test.ts`:
- Around line 2941-2956: Add a regression test alongside “the resolver skips
when the fallback finds no unique open head match” with two openPulls entries
sharing the same head.sha as the statusSha. Run the resolver with the same
status event setup, assert result.outputs is empty, and verify it logs the
ambiguous/stale revalidation skip behavior.
- Around line 2974-2988: Update the test “the resolver fails closed when both
resolution paths error” to include “repos.listPullRequestsAssociatedWithCommit”
in failOn, causing both resolution calls to fail. Add an assertion that logs
contain the association-index warning alongside the existing “Could not list
open PRs” warning, then retain the no-output assertion.
🪄 Autofix
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: 5cd35232-0c9f-4673-9e1f-3b58a91b8a7e
📒 Files selected for processing (3)
.github/workflows/enforce-pr-target.ymltests/ci-workflows.test.tstests/helpers/enforce-pr-target-harness.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/ci-workflows.test.ts`:
- Around line 2958-2975: Update the test “the resolver fails closed when two
open PRs share the head SHA” to assert that the fallback lookup was invoked via
the harness’s callsTo recording, specifically verifying the expected pulls.list
call and its evaluation of both matching PRs. Keep the existing empty-output and
ambiguous/stale log assertions unchanged.
🪄 Autofix
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: f8b3c10d-643f-4930-9f83-3da00f80e2e1
📒 Files selected for processing (1)
tests/ci-workflows.test.ts
The resolve-pr job mapped a trusted CodeRabbit status SHA back to a PR via the commit-to-PR association index. That index can lag a fresh head push (seen on PR lidge-jun#1441), returning zero matches for a genuine current head, so revalidation was silently skipped and new unresolved findings never re-drafted the PR. When the index yields no unique match, fall back to reconciling directly against the live open-PR list by head.sha — the same authoritative identity the write gate uses. Resolution still requires exactly one open match, so ambiguity remains fail-closed. The fallback is a read and stays within the resolver's pull-requests:read permission. Add behavioural harness tests for the resolver: empty-index fallback, no-match skip, fresh-index direct resolve, and fail-closed on error.
Address CodeRabbit feedback on the resolver tests: - Add a regression test for the ambiguous case: two open PRs sharing the same head SHA must fail closed (no pull-number output), instead of only covering zero and one matches. - Correct the fail-closed test so both resolution calls reject. It previously only failed pulls.list, so the association-index error path was never exercised; assert both warning messages now.
Address CodeRabbit feedback on the ambiguous fallback test: assert that the resolver actually consults the live open-PR list (one pulls.list call with the open-state filter) so the test cannot pass without the head-SHA reconciliation path. The empty-output and ambiguous/stale log assertions are unchanged.
Wibias
force-pushed
the
ci/resolve-pr-fallback
branch
from
August 11, 2026 02:52
0426b67 to
1b1d7d9
Compare
The ambiguous-head regression (two open PRs sharing the same head.sha) only asserted empty output and the skip log. Without a pulls.list assertion it would pass even if the head-SHA fallback were removed, because the empty association index by itself already causes the skip. Assert the live fallback ran: exactly one pulls.list call with the open-state filter, alongside the retained empty-output and skip-log assertions. The neighbouring zero-match test keeps its own independent fallback assertion.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
resolve-prjob previously mapped a trusted CodeRabbitstatusSHA back to a PR using GitHub's commit-to-PR association index. That index can lag a fresh head push, returning zero matches for a genuine current head, so revalidation was silently skipped and newly posted CodeRabbit findings never re-drafted the PR (observed on fix(codex): stop recursive dynamic-launcher shims #1441).head.sha— the same authoritative identity the write gate already uses. Resolution still requires exactly one open PR match, so a genuine ambiguity stays fail-closed.pulls.list) and remains within the resolver job'spull-requests: readpermission; no write scope is broadened.Validation
bun test tests/ci-workflows.test.ts— pass (136/136, includes 4 new resolver scenarios)node .github/scripts/enforce-pr-target.test.cjs— pass (18/18)bun run typecheck— cleanbun run privacy:scan— passReview notes
bun run testsuite crashed inside the Bun runtime (internalindex out of boundspanic) under heavy local runner contention; that is a Bun bug, not a test failure. Tests covering this change pass in isolation.Limitations
Refs #1441
Summary by CodeRabbit