fix(maintainer): verify final E2E coordinator evidence - #8258
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>
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: Carlos Villela <cvillela@nvidia.com>
cv
added
area: ci
|
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:
📝 WalkthroughWalkthroughThe gate checker validates paginated coordinator workflow evidence, E2E lineage, job timing, cached CI evidence, required-check snapshots, and PR revision stability through staged evaluation. Fixtures and tests cover valid, invalid, stale, incomplete, and changing evidence. ChangesE2E merge-gate validation
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit a07ee89 in the TypeScript / code-coverage/cliThe overall coverage in commit a07ee89 in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
7 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None 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.
🧹 Nitpick comments (8)
test/skills/check-gates-test-fixtures.ts (4)
196-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse one seed-job helper across the suites.
initialE2eSeedJobs()duplicatesseedJobs()intest/skills/check-gates-coordinator-evidence.test.ts(ids 471/472/473, identical timestamps). Two copies can drift, and the run407fixture at Lines 517-523 depends on those ids matching. ExportinitialE2eSeedJobsand use it in the coordinator suite instead of the local copy. The guideline also asks for one name per concept.🤖 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 `@test/skills/check-gates-test-fixtures.ts` around lines 196 - 219, Export initialE2eSeedJobs from check-gates-test-fixtures.ts and replace the duplicate seedJobs helper in check-gates-coordinator-evidence.test.ts with this shared helper. Preserve the existing job IDs and timestamps so the run 407 fixture continues matching, and use only the initialE2eSeedJobs name for this concept.Source: Coding guidelines
376-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate
createdRangein one place. Both sites splitcreatedRangeon".."and use the result with no validation. A range without..returnsundefinedfrom the split, and each site then degrades silently instead of failing: the clock becomesDate.now = () => NaN, and an unrelated run loses itscreated_atfield. Add one helper that parses a range into a validated start and end, and throw on a malformed value.
test/skills/check-gates-test-fixtures.ts#L376-L381: derive the observation time from the validated range end, and assert the parsed epoch is finite before writing the clock module. Select the latest range end rather than.at(-1)of the array, because the coordinator suite appendsworkflow_dispatchpartitions last.test/skills/check-gates-test-fixtures.ts#L639-L642: derivefallbackCreatedAtfrom the validated range start, so a synthetic unrelated run always carries acreated_atvalue.🤖 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 `@test/skills/check-gates-test-fixtures.ts` around lines 376 - 381, In test/skills/check-gates-test-fixtures.ts lines 376-381 and 639-642, add one shared helper to parse createdRange into validated start and end values, throwing for malformed ranges without "..". Update the observation-time setup at lines 376-381 to select the latest range end by timestamp rather than using coordinatorRunPartitions.at(-1), validate that Date.parse produces a finite epoch before writing clock.mjs, and update fallbackCreatedAt at lines 639-642 to use the validated range start.
530-539: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the shared default timestamps.
The values
"2026-01-01T00:01:00Z"and"2026-01-01T00:03:00Z"now appear as literals inactionRunData, the job serializer,initialE2eSeedJobs,initialE2eSeedCheck, the run407entry, and the default observation time. The gate script compares run, job, and observation times against each other, so these literals form one coupled timeline. Extract constants such asDEFAULT_STARTED_ATandDEFAULT_COMPLETED_ATso a timeline change stays in one place.Also applies to: 567-580
🤖 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 `@test/skills/check-gates-test-fixtures.ts` around lines 530 - 539, Extract shared constants for the default timeline timestamps, such as DEFAULT_STARTED_AT and DEFAULT_COMPLETED_AT, and replace the repeated literals throughout actionRunData, the job serializer, initialE2eSeedJobs, initialE2eSeedCheck, the run 407 entry, and the default observation time. Ensure all run, job, and observation defaults reference these constants so timeline changes remain centralized.
53-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove untested final-response knobs.
finalTotalCount,finalPageTotalCounts, andfinalRunOverrideshave no test consumers. Remove them and their serializer branches, or add tests that use them.🤖 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 `@test/skills/check-gates-test-fixtures.ts` around lines 53 - 56, Remove the unused final-response knobs finalTotalCount, finalPageTotalCounts, and finalRunOverrides from the fixture configuration and delete their corresponding serializer branches. Update related types or references as needed, without adding new behavior or tests.Source: Coding guidelines
test/skills/check-gates-coordinator-evidence.test.ts (2)
296-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the local
coordinatorJobsbinding.Line 296 declares
const coordinatorJobsinsiderunGateWithCoordinator. That binding shadows the module-levelcoordinatorJobshelper declared at Line 44 for the whole function body. The destructured parameter at Line 264 is already renamed toconfiguredJobsto avoid this collision, so the clash is known.Nothing breaks today, because
runGateWithCoordinatornever calls the helper. If a later edit callscoordinatorJobs(...)anywhere in this function, the call resolves to theconstin its temporal dead zone and throwsReferenceErrorinstead of reaching the helper. Rename the local binding.♻️ Proposed rename
- const coordinatorJobs = configuredJobs ?? coordinator.jobs ?? defaultCoordinator.jobs; + const resolvedCoordinatorJobs = configuredJobs ?? coordinator.jobs ?? defaultCoordinator.jobs;Update the three references at Lines 311, 337, and any other use inside this function:
- jobs: coordinatorJobs, + jobs: resolvedCoordinatorJobs,🤖 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 `@test/skills/check-gates-coordinator-evidence.test.ts` at line 296, Rename the local const binding `coordinatorJobs` inside `runGateWithCoordinator` to a distinct name, and update all references to that local value within the function, including the uses around lines 311 and 337. Preserve the module-level `coordinatorJobs` helper name unchanged.
1291-1310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd table cases for the coordinator head-branch and head-sha rules.
The table perturbs one field per entry from a passing baseline, which proves each production rule is necessary. Three rules in
parseE2eCoordinatorRunhave no entry:
record.head_branch !== "main"(check-gates.ts Line 512).- The 40-hex
head_shaformat test (check-gates.ts Line 513).event === "workflow_run" && headSha !== exactDiff.baseSha(check-gates.ts Line 514).The third rule binds an automatic coordinator to the pull request's base commit, so it carries the authorization weight for the same-repository path. No entry sets
headShaon the coordinator. The fork fixture does setheadSha: "d".repeat(40)at Line 137, but only on the manual dispatch coordinator, where the production code skips this binding. The rule is therefore unexercised, and a later refactor can remove it without failing any test.♻️ Suggested table entries
{ condition: "the coordinator uses another attempt", coordinator: { attempt: 2 }, }, + { + condition: "the automatic coordinator does not run on the base commit", + coordinator: { headSha: "c".repeat(40) }, + }, + { + condition: "the coordinator head sha is malformed", + coordinator: { headSha: "not-a-sha" }, + }, + { + condition: "the coordinator runs on another branch", + coordinator: { headBranch: "release" }, + },The third entry needs
headBranchonActionRunFixtureif the fixture does not already model it.🤖 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 `@test/skills/check-gates-coordinator-evidence.test.ts` around lines 1291 - 1310, Extend the coordinator evidence table in the check-gates coordinator tests with cases that independently violate headBranch, the 40-character hexadecimal headSha format, and the workflow_run headSha mismatch against exactDiff.baseSha. Add headBranch to ActionRunFixture if needed, and ensure the headSha binding case uses an automatic workflow_run coordinator with an otherwise passing baseline so each parseE2eCoordinatorRun rule is exercised.Source: Path instructions
.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts (2)
2965-2967: 📐 Maintainability & Code Quality | 🔵 TrivialConsider logging progress for the second coordinator inventory pass.
mainnow callsfetchE2eCoordinationEvidencetwice. Each call walks two events across up toE2E_COORDINATOR_MAX_PARTITIONS(28) partitions, and each partition issues at least one paginatedgh apiinvocation. Each candidate then costs three more reads. For a 14-day history window the gate can issue well over one hundred separateghprocesses, and each one carries the 120 s timeout fromrunin.agents/skills/nemoclaw-maintainer-day/scripts/shared.ts.The checker prints nothing until it finishes, so a maintainer cannot distinguish a slow inventory from a hang. Write a short progress line to stderr before each inventory pass.
🤖 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 2965 - 2967, Update the two call sites that invoke fetchE2eCoordinationEvidence, including the conditional call around finalE2eEvidence, to write a concise progress message to stderr immediately before each inventory pass. Keep the existing evidence evaluation and return behavior unchanged.
1014-1060: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated claimed-check derivation into one helper.
Three functions now build the same values independently: the
checkNameslist, theexternalIdtemplate, and theclaimedChecks→currentNameChecks→exactCheckssequence.selectE2eCoordinationCheck(Lines 1014-1041),selectedE2eLineageTiming(Lines 1048-1060), ande2eCoordinationHistoryStartedAt(Lines 1086-1092) each repeat part of it. The name-preference rule is duplicated verbatim in two of them. A change to the check-name list or the external-id format must be applied in three places.Extract one helper that returns
{ claimedChecks, exactChecks }and call it from all three functions.♻️ Suggested shape
const E2E_COORDINATION_CHECK_NAMES = ["E2E / PR Gate", "E2E / PR Gate Coordination"] as const; function e2eCoordinationExternalId(exactDiff: ExactDiffIdentity): string { return `nemoclaw-pr-e2e:v2:${exactDiff.number}:${exactDiff.headSha}:${exactDiff.baseSha}`; } function claimedE2eCoordinationChecks( snapshot: E2eCoordinationCheckSnapshot, exactDiff: ExactDiffIdentity, ): { claimedChecks: Array<Record<string, unknown>>; exactChecks: Array<Record<string, unknown>> } { const externalId = e2eCoordinationExternalId(exactDiff); const claimedChecks = snapshot.checkRuns.filter((check) => check.external_id === externalId); const currentNameChecks = claimedChecks.filter((check) => check.name === "E2E / PR Gate"); const exactChecks = currentNameChecks.length > 0 ? currentNameChecks : claimedChecks.filter((check) => check.name === "E2E / PR Gate Coordination"); return { claimedChecks, exactChecks }; }🤖 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 1014 - 1060, Extract the duplicated E2E check derivation into shared symbols near selectE2eCoordinationCheck: centralize the check-name list, external-ID construction, and claimedChecks-to-exactChecks name-preference logic in one helper returning both arrays. Update selectE2eCoordinationCheck, selectedE2eLineageTiming, and e2eCoordinationHistoryStartedAt to reuse that helper while preserving existing validation and selection 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.
Nitpick comments:
In @.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts:
- Around line 2965-2967: Update the two call sites that invoke
fetchE2eCoordinationEvidence, including the conditional call around
finalE2eEvidence, to write a concise progress message to stderr immediately
before each inventory pass. Keep the existing evidence evaluation and return
behavior unchanged.
- Around line 1014-1060: Extract the duplicated E2E check derivation into shared
symbols near selectE2eCoordinationCheck: centralize the check-name list,
external-ID construction, and claimedChecks-to-exactChecks name-preference logic
in one helper returning both arrays. Update selectE2eCoordinationCheck,
selectedE2eLineageTiming, and e2eCoordinationHistoryStartedAt to reuse that
helper while preserving existing validation and selection behavior.
In `@test/skills/check-gates-coordinator-evidence.test.ts`:
- Line 296: Rename the local const binding `coordinatorJobs` inside
`runGateWithCoordinator` to a distinct name, and update all references to that
local value within the function, including the uses around lines 311 and 337.
Preserve the module-level `coordinatorJobs` helper name unchanged.
- Around line 1291-1310: Extend the coordinator evidence table in the
check-gates coordinator tests with cases that independently violate headBranch,
the 40-character hexadecimal headSha format, and the workflow_run headSha
mismatch against exactDiff.baseSha. Add headBranch to ActionRunFixture if
needed, and ensure the headSha binding case uses an automatic workflow_run
coordinator with an otherwise passing baseline so each parseE2eCoordinatorRun
rule is exercised.
In `@test/skills/check-gates-test-fixtures.ts`:
- Around line 196-219: Export initialE2eSeedJobs from
check-gates-test-fixtures.ts and replace the duplicate seedJobs helper in
check-gates-coordinator-evidence.test.ts with this shared helper. Preserve the
existing job IDs and timestamps so the run 407 fixture continues matching, and
use only the initialE2eSeedJobs name for this concept.
- Around line 376-381: In test/skills/check-gates-test-fixtures.ts lines 376-381
and 639-642, add one shared helper to parse createdRange into validated start
and end values, throwing for malformed ranges without "..". Update the
observation-time setup at lines 376-381 to select the latest range end by
timestamp rather than using coordinatorRunPartitions.at(-1), validate that
Date.parse produces a finite epoch before writing clock.mjs, and update
fallbackCreatedAt at lines 639-642 to use the validated range start.
- Around line 530-539: Extract shared constants for the default timeline
timestamps, such as DEFAULT_STARTED_AT and DEFAULT_COMPLETED_AT, and replace the
repeated literals throughout actionRunData, the job serializer,
initialE2eSeedJobs, initialE2eSeedCheck, the run 407 entry, and the default
observation time. Ensure all run, job, and observation defaults reference these
constants so timeline changes remain centralized.
- Around line 53-56: Remove the unused final-response knobs finalTotalCount,
finalPageTotalCounts, and finalRunOverrides from the fixture configuration and
delete their corresponding serializer branches. Update related types or
references as needed, without adding new behavior or tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 635ffe0a-71cc-4b06-a446-d23f28db6916
📒 Files selected for processing (4)
.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.tstest/skills/check-gates-coordinator-evidence.test.tstest/skills/check-gates-evidence.test.tstest/skills/check-gates-test-fixtures.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>
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-retry-history.test.ts`:
- Around line 25-46: Update the checkRunsWithTiming fixture generation to use
Date arithmetic that evenly divides a fixed valid time window across every
record, ensuring started_at always precedes completed_at and timestamps remain
valid for any history size. Preserve current first/last record semantics and add
a regression case covering more than 31 check runs.
🪄 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: CHILL
Plan: Enterprise
Run ID: 420aa637-172a-4a4e-a5bc-0f52ce90100f
📒 Files selected for processing (5)
.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.tstest/skills/check-gates-coordinator-evidence.test.tstest/skills/check-gates-evidence.test.tstest/skills/check-gates-fork-evidence.test.tstest/skills/check-gates-retry-history.test.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: Carlos Villela <cvillela@nvidia.com>
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: Carlos Villela <cvillela@nvidia.com> (cherry picked from commit 5c3e773e5f6e5b51bb0474f09d6107a08005666b)
Signed-off-by: Carlos Villela <cvillela@nvidia.com> (cherry picked from commit 3f6a4b67be3bc9d083626dcb989a431ed36ee9cb)
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
2 participants
Summary
Make the maintainer merge check verify the complete E2E coordinator history and the final pull request state. This prevents a passing result when coordinator evidence is ambiguous or another repository check changes before the merge decision.
This PR supersedes #8241. It preserves #8241's authored commits and attribution. It starts from that PR's effective change and adds focused gate and fixture corrections required by GitHub CI. The new branch avoids a force-push to #8241's published branch. #8241 was closed only after this successor was confirmed open.
Changes
mainworkflow SHA and the exact pull request head, base, repository, and immutable title.Type of Change
Quality Gates
a07ee89de438a555685bf83bbffde578b64cd2e2against5fe23192a6fe378537fc2730c081730bc3ff72b9: PASS with no findings. The stable effective patch ID is unchanged from prior reviewed heada388b32739237226cac49d866c1e1caa5378542d, and all eight effective file blobs are identical. The review covers trusted workflow provenance; exact pull request, head, base, repository, and title binding; fork authorization lineage; complete bounded pagination; duplicate and timestamp rejection; retry history; required-check and E2E snapshot revalidation; and the terminal atomic pull request and base read. It adds no secret, credential, dependency, cryptography, permission, or user-facing surface. All nine security categories pass.Documentation Writer Review
no-docs-neededa07ee89de438a555685bf83bbffde578b64cd2e2against base5fe23192a6fe378537fc2730c081730bc3ff72b9confirmed that the change affects only the internal maintainer merge checker, its risky-file classification, and regression fixtures. It changes no supported product or user-facing documentation surface. The reviewer inspected all changed diagnostics, comments, fixture labels, test titles, and manually authored pull request text against the repository writing and documentation rules. All eight effective pull request file blobs are byte-identical to prior reviewed heada388b32739237226cac49d866c1e1caa5378542d, and both base revisions contain identical blobs for those paths.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablea388b32739237226cac49d866c1e1caa5378542dare byte-identical to the focused-test revisionffe14d072c33383c6dcf97c6145eacc1af9d32db, where the complete merge-gate suite passed 247/247 tests andnpm run checks:repository,npm run build:cli, andnpm run typecheck:clipassed. Interveningmainchanges do not alter those paths, and exact-headgit diff --checkpassed.a07ee89de438a555685bf83bbffde578b64cd2e2is running against main after test(cli): isolate Shields and tunnel tests under coverage #8293 landed the shared Shields and tunnel test isolation repairs. No non-success check is accepted or waived.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests