fix(skills): use latest check-run attempts - #7024
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCI gates now bind checks to exact PR revisions, select current workflow attempts, fail closed on incomplete evidence, verify final mergeability, support Dependabot DCO bypasses, and add workflow contracts plus extensive mocked evidence tests. ChangesMerge gate validation
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant PRData
participant checkCi
participant currentCheckRollup
participant GitHubActions
participant checkFinalRevision
participant checkConflicts
PRData->>checkCi: Provide status checks and exact head/base OIDs
checkCi->>currentCheckRollup: Resolve current checks
currentCheckRollup->>GitHubActions: Fetch workflow attempts and jobs
GitHubActions-->>currentCheckRollup: Return latest-attempt evidence
currentCheckRollup-->>checkCi: Return current checks and incomplete evidence
PRData->>checkFinalRevision: Re-read final PR revision
checkFinalRevision->>checkConflicts: Evaluate mergeability and base state
checkConflicts-->>PRData: Return final gate result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage remains at 80%, unchanged from the Updated |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 @.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts:
- Around line 357-368: Update checksFromLatestAttempt so it returns null unless
every check maps to a job ID in jobIds; track whether each check is selected and
fail closed when any check is omitted, while preserving the existing null
handling for unmatched URLs and empty selections.
- Around line 317-335: Update the latest-attempt jobs retrieval in the code
using ghJson and latestAttemptJobsByRun so it paginates through all API pages,
aggregates every job, and only then validates the combined result against
total_count. Preserve the existing null fallback for malformed or incomplete
responses, but do not reject valid runs solely because total_count exceeds 100.
🪄 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: 85a08e47-0d65-414d-a09f-aec99150e216
📒 Files selected for processing (3)
.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts.agents/skills/nemoclaw-maintainer-day/scripts/shared.tstest/skills/check-gates-compliance.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
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 @.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts:
- Around line 518-528: Update the candidate selection around
meaningfulExactDiffRun so skipped runs with exactDiff false or null cannot
outrank or hide a validated exact-diff run. When a meaningful exact-diff run
exists, restrict candidates to validated exact-diff runs, or otherwise fail
closed for ambiguous runs, while preserving the existing latestTimestamp and
latestRuns selection for valid candidates.
- Around line 531-534: Update the selected-check logic around
checksFromLatestAttempt so single-check runs are also filtered against the
latest attempt instead of using latest.checks directly. Treat any runId/check
mismatch returned by that filtering as incomplete evidence, preserving the
existing multi-check behavior.
🪄 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: 970b9abe-1df8-4d1f-8338-add9be55c320
📒 Files selected for processing (2)
.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.tstest/skills/check-gates-compliance.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
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 @.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts:
- Around line 397-404: Validate the normalized status and conclusion values
returned by fetchActionRunMetadata() against the supported GitHub Actions enums,
converting unknown values to null before they are used as evidence. Update
isMeaningfulExactDiffRun() and the later exact-diff branch to rely only on
validated values, preserving skipped-run handling so unexpected conclusions
cannot be treated as meaningful.
- Around line 561-573: Update latestAttemptChecks so exact-diff evidence is
required for every Actions check, not only names included in
REQUIRED_CHECK_NAMES. Validate each selected check’s run metadata for exactDiff,
event, and path before allowing it into the result; otherwise mark the evidence
incomplete and preserve the existing fallback behavior so checkCi cannot count
checks from a different PR or base.
🪄 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: 449e40e0-7ea2-48fd-bae1-00086b47cf23
📒 Files selected for processing (2)
.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.tstest/skills/check-gates-compliance.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/skills/check-gates-compliance.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
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 `@test/skills/check-gates-evidence.test.ts`:
- Around line 612-660: Update the older run 199 `matrix-check` fixture in the
test “keeps every duplicate job from the latest workflow run” to have a
successful conclusion, while leaving run 200’s jobs unchanged. Ensure the
expected failure can only result from retaining both jobs from the latest run,
proving the stale run is excluded.
🪄 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: 3a755c03-6773-4a24-91c0-0c26dafa118b
📒 Files selected for processing (4)
.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.tstest/skills/check-gates-compliance.test.tstest/skills/check-gates-evidence.test.tstest/skills/check-gates-test-fixtures.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
…pts' into codex/fix-merge-gate-check-attempts Signed-off-by: Carlos Villela <cvillela@nvidia.com> # Conflicts: # test/skills/check-gates-evidence.test.ts
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
…check-attempts Signed-off-by: Carlos Villela <cvillela@nvidia.com>
…pts' into codex/fix-merge-gate-check-attempts Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts (1)
1077-1085: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOrder cross-run selection by run
createdAt, not job start time. An older queued run can start later and winlatestTimestamp, letting stale evidence override the newer run. UseactionRunMetadata(runId)?.createdAtfor the run-level timestamp and add an overlapping-run regression case; the same fix applies in the laterlatestTimestamppath.🤖 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 @.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts around lines 1077 - 1085, Update the cross-run timestamp in the byRun mapping and the later latestTimestamp selection to use actionRunMetadata(runId)?.createdAt instead of job check start/completion times. Preserve invalid-timestamp handling, and add a regression case with overlapping runs proving the newer run is selected even when the older queued run starts later.
🤖 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 @.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md:
- Line 55: Update the conflicts row in the guidance table to replace the
obsolete “DIRTY” status with the required MERGEABLE status and allowed
current-base merge-state terminology, matching the approval criterion above.
In `@test/skills/check-gates-test-fixtures.ts`:
- Line 478: Update runComparatorGate so the mocked API responses and
request-matching pattern derive the PR number from its prNumber parameter rather
than hard-coding 42. Safely quote prNumber when constructing the generated case
pattern, and ensure the mocked returned PR metadata uses the same value.
- Line 162: Update the startedAt fixture generation in the check-gate test data
so every index produces a valid ISO timestamp, including multi-digit minute
values such as index 5. Use proper minute formatting or date arithmetic while
preserving explicitly provided startedAt values.
---
Outside diff comments:
In @.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts:
- Around line 1077-1085: Update the cross-run timestamp in the byRun mapping and
the later latestTimestamp selection to use actionRunMetadata(runId)?.createdAt
instead of job check start/completion times. Preserve invalid-timestamp
handling, and add a regression case with overlapping runs proving the newer run
is selected even when the older queued run starts later.
🪄 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: 1fefd153-86a9-4bb8-a8fe-ad5b4e688d67
📒 Files selected for processing (12)
.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/collect-gates.sh.github/workflows/dco-check.yaml.github/workflows/installer-hash-check.yaml.github/workflows/pr-e2e-gate.yamlci/source-shape-test-budget.jsontest/pr-e2e-gate-workflow.test.tstest/pr-workflow-contract.test.tstest/skills/check-gates-compliance.test.tstest/skills/check-gates-evidence.test.tstest/skills/check-gates-test-fixtures.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/skills/check-gates-compliance.test.ts
- test/skills/check-gates-evidence.test.ts
|
@jyaunches @ericksoa — independent approval requested on exact head |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Exact-head follow-up at
I authored this follow-up and will not self-approve or merge it. |
|
Exact-head landing evidence for independent review:
The canonical checker from trusted main reports all 53 checks green. Its remaining BLOCKED merge state is review-required, not a conflict. Bootstrap note: this PR adds the immutable run-name used by its proposed stricter checker. GitHub evaluates pull_request_target workflow metadata from the base branch, so this PR cannot self-produce that new title before merge. The exact custom coordination external ID and child artifacts still bind PR, head, base, plan, correlation, and all selected jobs; the new-title behavior is covered by focused tests. Please review this bootstrap boundary explicitly. Root/team authored this prerequisite, so an independent human must approve and merge it. |
|
@jyaunches @ericksoa @apurvvkumaria — expedited v0.0.85 review requested. Exact head/base CI and all four selected E2E jobs are green; full evidence and the pull_request_target bootstrap note are in the comment immediately above. One independent approval is the only remaining gate. |
|
Exact current-base landing evidence for independent review:
Bootstrap boundary remains unchanged: this PR adds stricter immutable @jyaunches @ericksoa @apurvvkumaria @cjagwani — one independent human approval and manual merge are the only remaining gates. The author/contributor will not self-approve or merge. |
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Approved exact head 0c2754d on current base c5b8dba after a workflow/governance security sweep. GitHub reports MERGEABLE; all 53 checks, the exact-diff E2E verdict and coordination, DCO, CodeRabbit, and Advisor are green. Local validation passed 174 focused tests and npm run typecheck:cli. The proposed immutable-title check cannot self-bootstrap because pull_request_target metadata comes from the base workflow; the bound current-base coordination and E2E evidence were verified directly. I contributed commits to this PR; contributor/approver overlap is advisory under the recorded maintainer policy and does not change gate readiness.
|
Post-merge security/correctness finding (reviewed against exact merged head Please require the expected |
wscurran
added
area: ci
4 participants
Summary
Make the maintainer merge gate evaluate only current, complete GitHub Actions evidence for the exact PR diff. Before this change, stale retry jobs and later all-skipped body-edit runs could block an otherwise-green PR; after it, the gate selects the latest complete attempt while failing closed when GitHub evidence is incomplete.
Changes
head_shaas well as PR association metadata so live PR fields cannot make a stale run look current.pr.yamlevidence to its canonical immutable PR/head/base title, requirechecksandchangesfrom the samegate truerun, and fail closed on malformed metadata-onlygate falseshapes.check-hashevidence created at or before the latest paginatedbase_ref_changedevent, preventing a retargeted PR from inheriting a base-trusted result from its prior base.SUCCESS; optional checks may still concludeNEUTRALorSKIPPED.pr.yamlmetadata-edit job shape, preserving earlier substantive matrix evidence while rejecting malformed lookalikes.Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run test/skills/check-gates-compliance.test.ts test/skills/check-gates-evidence.test.ts(133 passed); live checker passes fix(skills): use latest check-run attempts #7024 and rejects feat(openshell): route session reads over authenticated gRPC #6793 only for its stale pre-retargetcheck-hashevidencenpm run check:diffnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Signed-off-by: Charan Jagwani cjagwani@nvidia.com
Summary by CodeRabbit