fix(advisor): wait for required PR checks#3977
Merged
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an optional deterministic wait for repository-required GitHub status checks before PR Review Advisor runs analysis: ruleset discovery with env fallback, polling of statusCheckRollup, required-context-aware CI gate derivation, workflow timeout/env updates, tests, and README updates. ChangesRequired Status Check Waiting Integration
Sequence Diagram(s)sequenceDiagram
participant main as main()
participant waitFunc as waitForRequiredChecksBeforeAnalysis()
participant ruleset as extractRequiredStatusChecksFromRulesets()
participant github as GitHub GraphQL
participant derive as deriveGateStatus()
main->>waitFunc: headSha
waitFunc->>ruleset: fetch rulesets(baseBranch)
ruleset-->>waitFunc: requiredContexts
loop poll until timeout or all terminal
waitFunc->>github: query statusCheckRollup(headSha)
github-->>waitFunc: status summaries
waitFunc->>waitFunc: compute pendingRequiredContexts()
end
waitFunc-->>main: required/additional contexts
main->>derive: pass requiredStatusCheckContexts
derive-->>main: gateStatus with required-context evidence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
PR Review AdvisorRecommendation: blocked This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: Automated advisory review only; a human maintainer must make the final merge decision.; CI was still pending for head SHA 9a47df0 in the trusted context.; GitHub reports mergeStateStatus=BLOCKED.; No linked issues were present, so acceptance coverage is based on PR-stated clauses and trusted PR/E2E Advisor comments only.; Review relied on the provided trusted deterministic context and visible diff; no commands or tests were executed. Full advisor summaryPR Review AdvisorBase: The implementation appears directionally sound and improves stale-run handling, but merge readiness is blocked by pending head-SHA CI, BLOCKED merge state, and unsettled E2E Advisor status for a privileged workflow/enforcement change. Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
![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: 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 `@tools/pr-review-advisor/analyze.mts`:
- Around line 413-416: The code sets baseBranch from env defaults which makes
fetchRequiredStatusChecks(repo, token, baseBranch) use "main" on manual runs;
update baseBranch to prefer the analyzed/parsed CLI base value (the variable
computed earlier in the analysis flow, e.g. the parsed "base" or
"analyzedBaseBranch" value) and only fall back to
process.env.PR_REVIEW_ADVISOR_REQUIRED_CHECK_BASE / process.env.GITHUB_BASE_REF
/ "main" if that analyzed base is undefined; apply the same change where
baseBranch is computed again (the similar block around lines 552-555) so both
fetchRequiredStatusChecks calls use the actual analyzed base branch.
🪄 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: CHILL
Plan: Enterprise
Run ID: 985698f8-272a-42e6-aaf4-d00a39a458d0
📒 Files selected for processing (4)
.github/workflows/pr-review-advisor.yamltest/pr-review-advisor.test.tstools/pr-review-advisor/README.mdtools/pr-review-advisor/analyze.mts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tools/pr-review-advisor/analyze.mts (1)
407-416:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the analyzed base ref for required-context discovery.
This helper still derives
baseBranchfrom env /"main"only, so non-default-base runs can wait on and gate against the wrong ruleset contexts.💡 Minimal fix
- await waitForRequiredChecksBeforeAnalysis(headSha); + await waitForRequiredChecksBeforeAnalysis(headSha, baseRef); -async function waitForRequiredChecksBeforeAnalysis(headSha: string): Promise<void> { +async function waitForRequiredChecksBeforeAnalysis(headSha: string, baseRef: string): Promise<void> { ... - ...(await discoverRequiredStatusCheckContexts()), + ...(await discoverRequiredStatusCheckContexts(baseRef)), ... } -export async function discoverRequiredStatusCheckContexts(): Promise<string[]> { +export async function discoverRequiredStatusCheckContexts(baseRef?: string): Promise<string[]> { ... - const baseBranch = process.env.PR_REVIEW_ADVISOR_REQUIRED_CHECK_BASE || process.env.GITHUB_BASE_REF || "main"; + const baseBranch = normalizeBaseBranch( + process.env.PR_REVIEW_ADVISOR_REQUIRED_CHECK_BASE || + process.env.GITHUB_BASE_REF || + baseRef || + "main", + );function normalizeBaseBranch(ref: string): string { return ref.replace(/^refs\/heads\//, "").replace(/^origin\//, ""); }🤖 Prompt for 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. In `@tools/pr-review-advisor/analyze.mts` around lines 407 - 416, discoverRequiredStatusCheckContexts currently builds baseBranch only from env vars and "main", which ignores the analyzed PR base ref; update it to normalize and use the actual analyzed base ref before calling fetchRequiredStatusChecks. Add or reuse a normalizeBaseBranch helper (e.g., normalizeBaseBranch(ref: string) that strips "refs/heads/" and "origin/") and compute baseBranch from process.env.PR_REVIEW_ADVISOR_REQUIRED_CHECK_BASE || process.env.GITHUB_BASE_REF || analyzedBaseRef || "main" (where analyzedBaseRef is the ref determined earlier by the analyzer), then pass that normalized baseBranch into fetchRequiredStatusChecks so required-context discovery uses the correct branch; reference discoverRequiredStatusCheckContexts, normalizeBaseBranch, and fetchRequiredStatusChecks when making the change.
🤖 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.
Duplicate comments:
In `@tools/pr-review-advisor/analyze.mts`:
- Around line 407-416: discoverRequiredStatusCheckContexts currently builds
baseBranch only from env vars and "main", which ignores the analyzed PR base
ref; update it to normalize and use the actual analyzed base ref before calling
fetchRequiredStatusChecks. Add or reuse a normalizeBaseBranch helper (e.g.,
normalizeBaseBranch(ref: string) that strips "refs/heads/" and "origin/") and
compute baseBranch from process.env.PR_REVIEW_ADVISOR_REQUIRED_CHECK_BASE ||
process.env.GITHUB_BASE_REF || analyzedBaseRef || "main" (where analyzedBaseRef
is the ref determined earlier by the analyzer), then pass that normalized
baseBranch into fetchRequiredStatusChecks so required-context discovery uses the
correct branch; reference discoverRequiredStatusCheckContexts,
normalizeBaseBranch, and fetchRequiredStatusChecks when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 923ec4e6-d1aa-4ac8-849f-9fec6d1ecef9
📒 Files selected for processing (2)
test/pr-review-advisor.test.tstools/pr-review-advisor/analyze.mts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/pr-review-advisor/analyze.mts (2)
336-385:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAbort this run if the PR head advances while you are waiting.
When
headRefOidchanges, the loop only logs the mismatch and then keeps going. That means this job can wait on checks for the newer commit and still analyze the older checkout, which can post a stale review against code that is no longer the PR head.Suggested fix
lastState = await fetchRequiredCheckWaitState({ repo, token, prNumber, requiredContexts }); - const shaNote = lastState.headRefOid && lastState.headRefOid !== headSha - ? ` latest PR head is ${lastState.headRefOid.slice(0, 12)}, workflow head is ${headSha.slice(0, 12)}.` - : ""; + if (lastState.headRefOid && lastState.headRefOid !== headSha) { + throw new Error( + `PR head advanced from ${headSha.slice(0, 12)} to ${lastState.headRefOid.slice(0, 12)}; rerun advisor on the latest commit.`, + ); + } if (lastState.pendingContexts.length === 0) { - logProgress(`Required-check wait complete.${shaNote}`); + logProgress("Required-check wait complete."); return; } - logProgress(`Required-check wait pending: ${lastState.pendingContexts.join(", ")}.${shaNote}`); + logProgress(`Required-check wait pending: ${lastState.pendingContexts.join(", ")}.`);🤖 Prompt for 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. In `@tools/pr-review-advisor/analyze.mts` around lines 336 - 385, The loop in waitForRequiredChecksBeforeAnalysis currently only logs when lastState.headRefOid differs from headSha, allowing analysis to continue against a stale checkout; change this so the run aborts when the PR head advances by detecting lastState.headRefOid !== headSha and then immediately stop further work (e.g., log a clear message and throw an Error or call process.exit with a non-zero code) instead of continuing; update the branch around where shaNote is computed (referencing waitForRequiredChecksBeforeAnalysis, lastState.headRefOid, and headSha) to perform this abort early in the loop so stale analysis cannot be posted.
552-563:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep empty required-check rollups in
pending, notunknown.Now that required contexts are passed into
deriveGateStatus, the zero-node path still collapses tounknown. For fresh PRs or wait timeouts, that drops the new “pending or missing” evidence instead of reporting the required contexts as pending.Suggested fix
function deriveCiGateStatus(statuses: CheckStatusSummary[], requiredContexts: string[]): GateStatus { - if (statuses.length === 0) return { status: "unknown", evidence: "No statusCheckRollup data was available." }; + if (statuses.length === 0) { + return requiredContexts.length > 0 + ? { + status: "pending", + evidence: `Required status context(s) pending or missing: ${requiredContexts.join(", ")}. Non-required contexts still pending: 0; failed: 0.`, + } + : { status: "unknown", evidence: "No statusCheckRollup data was available." }; + }🤖 Prompt for 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. In `@tools/pr-review-advisor/analyze.mts` around lines 552 - 563, The gate computation currently treats an empty requiredStatusCheckContexts rollup as `unknown`; change the logic so an empty rollup is treated as `pending` (so fresh PRs / timeouts report "pending or missing" evidence). Update deriveGateStatus to detect an empty requiredStatusCheckContexts (or adjust the caller in collectDeterministicContext) and return a gate status that marks required checks as pending (include a pending evidence entry for requiredStatusCheckContexts) rather than returning `unknown`; reference the deriveGateStatus function and the requiredStatusCheckContexts value passed from collectDeterministicContext to locate where to implement this behavior.
🤖 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.
Outside diff comments:
In `@tools/pr-review-advisor/analyze.mts`:
- Around line 336-385: The loop in waitForRequiredChecksBeforeAnalysis currently
only logs when lastState.headRefOid differs from headSha, allowing analysis to
continue against a stale checkout; change this so the run aborts when the PR
head advances by detecting lastState.headRefOid !== headSha and then immediately
stop further work (e.g., log a clear message and throw an Error or call
process.exit with a non-zero code) instead of continuing; update the branch
around where shaNote is computed (referencing
waitForRequiredChecksBeforeAnalysis, lastState.headRefOid, and headSha) to
perform this abort early in the loop so stale analysis cannot be posted.
- Around line 552-563: The gate computation currently treats an empty
requiredStatusCheckContexts rollup as `unknown`; change the logic so an empty
rollup is treated as `pending` (so fresh PRs / timeouts report "pending or
missing" evidence). Update deriveGateStatus to detect an empty
requiredStatusCheckContexts (or adjust the caller in
collectDeterministicContext) and return a gate status that marks required checks
as pending (include a pending evidence entry for requiredStatusCheckContexts)
rather than returning `unknown`; reference the deriveGateStatus function and the
requiredStatusCheckContexts value passed from collectDeterministicContext to
locate where to implement this behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b938b6a6-f4d0-42fa-b169-af96b4a169e6
📒 Files selected for processing (2)
test/pr-review-advisor.test.tstools/pr-review-advisor/analyze.mts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
wscurran
added
CI/CD
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
2 participants
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
Make the PR Review Advisor wait for required PR checks and the E2E recommendation before launching model analysis. This should reduce early, low-context reviews where the advisor reports blocked primarily because CI is still pending.
Changes
tools/pr-review-advisor/analyze.mtsthat reads required contexts from repository rulesets, falls back to configured contexts, and excludes the advisor's own check to avoid self-deadlock..github/workflows/pr-review-advisor.yamlwith wait timeout/poll settings, the E2E Advisor context, and documentation that the workflow must remain advisory/non-required.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Documentation
Tests
Chores