fix(maintainer): verify final E2E coordinator evidence - #8241
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
📝 WalkthroughWalkthroughThe gate evaluator now validates E2E coordinator histories, caches CI evidence, captures status-check rollups, and performs staged consistency checks across PR revisions and final CI observations. Tests add paginated coordinator fixtures and broad acceptance and rejection coverage. ChangesCI and E2E evidence validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (11)
test/skills/check-gates-coordinator-evidence.test.ts (3)
207-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the local
coordinatorJobsto remove the shadow.Line 207 declares a local
const coordinatorJobs. It shadows the module-levelcoordinatorJobshelper from line 31. The two names mean different things: one builds a job list, the other holds a resolved job list. The behavior is correct today because the helper is not called inside this function, but the reuse makes the fixture harder to follow.As per coding guidelines: "Use existing repository vocabulary and one name per concept".♻️ Proposed rename
- const coordinatorJobs = configuredJobs ?? coordinator.jobs ?? defaultCoordinator.jobs; + const resolvedCoordinatorJobs = configuredJobs ?? coordinator.jobs ?? defaultCoordinator.jobs;Update the three references inside
runGate({ ... })(lines 222, 248) toresolvedCoordinatorJobs.🤖 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 207, Rename the local constant `coordinatorJobs` to `resolvedCoordinatorJobs` and update all of its references within the surrounding `runGate({ ... })` setup, while leaving the module-level `coordinatorJobs` helper unchanged.Source: Coding guidelines
1151-1166: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd rejection rows for null run-level
created_atandupdated_at.The table covers run timestamps that are absent (
omitCreatedAt,omitUpdatedAt), malformed, and out of order. It does not cover a run timestamp that is present andnull.This PR widens
ActionRunFixture.createdAtandActionRunFixture.updatedAttostring | nullintest/skills/check-gates-test-fixtures.ts(lines 69-70), andactionRunDatapreserves an explicitnullrather than substituting the default. The job-level equivalents already have rows at lines 1117-1122. The run-level null shape therefore has a supported fixture type and no protecting test. A GitHub workflow-run payload can carry"updated_at": null, so the shape is realistic.Add the two rows, or narrow the two fixture fields back to
stringif the null shape is not required.As per coding guidelines: "Do not add configuration, fallback, migration, compatibility, or extension layers without a current requirement; identify the current consumer and protecting test."♻️ Proposed rows
{ condition: "the coordinator run has no updated_at timestamp", coordinator: { omitUpdatedAt: true }, }, + { + condition: "the coordinator run created_at timestamp is null", + coordinator: { createdAt: null }, + }, + { + condition: "the coordinator run updated_at timestamp is null", + coordinator: { updatedAt: null }, + },🤖 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 1151 - 1166, Extend the rejection-case table in the coordinator evidence tests with cases for run-level created_at and updated_at explicitly set to null, using the existing fixture properties and matching the job-level null cases. Preserve the current absent, malformed, and ordering cases.Source: Coding guidelines
273-298: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winClarify the manual-only fork case in the test title.
check-gates.tsaccepts zero automatic predecessors. Rename the test to state this boundary explicitly.🤖 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 273 - 298, Rename the test describing the repository-owned manual coordinator in runGateWithCoordinator to explicitly state that the authorized fork revision has zero automatic predecessors, while leaving its setup and assertions unchanged.test/skills/check-gates-test-fixtures.ts (3)
352-357: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the default observation timestamp with the default partition window.
Line 356 defaults
observationTimeto"2026-01-01T00:03:00Z". Line 584 independently ends the default coordinator partition at the same instant. The two literals must stay equal, otherwise the injected clock and the discovery window disagree and coordinator tests drift without an obvious cause. Extract one constant.Note on the static analysis hint at line 356:
clockPathderives fromfs.mkdtempSync, so no external input reaches the path. Treat the path-traversal warning as a false positive.♻️ Proposed refactor
+const DEFAULT_OBSERVATION_TIME = "2026-01-01T00:03:00Z"; + function runGate(fixture: ComplianceFixture) {const observationTime = fixture.observationTime ?? fixture.coordinatorRunPartitions?.at(-1)?.createdRange.split("..")[1] ?? - "2026-01-01T00:03:00Z"; + DEFAULT_OBSERVATION_TIME;{ - createdRange: "2026-01-01T00:00:00Z..2026-01-01T00:03:00Z", + createdRange: `2026-01-01T00:00:00Z..${DEFAULT_OBSERVATION_TIME}`, runPages: coordinatorRunPages, },🤖 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 352 - 357, Extract a shared constant for the default observation timestamp and reuse it both in the observationTime fallback near the clock setup and in the default coordinator partition window. Remove the duplicated timestamp literal while preserving the existing fallback behavior; ignore the path-traversal warning for clockPath.Source: Linters/SAST tools
592-604: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard against duplicate (event, createdRange) partitions producing a dead
casearm.The pre-fill loop keys partitions on the
(event, createdRange)pair. The generated bashcaseat line 656 uses one label per partition, built from the same pair. If a fixture declares two partitions with the same event and the samecreatedRange, the second label is unreachable and itsrunPages,finalRunPages, andrunOverridesnever reach the script under test. The test then passes or fails for a reason unrelated to its stated claim.No current fixture hits this. Add a fail-fast check so a future fixture reports the mistake instead of silently losing evidence.
As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."♻️ Proposed guard
const coordinatorWorkflowRunCases = coordinatorRunPartitions .map((partition, partitionIndex) => { const fallbackCreatedAt = partition.fallbackCreatedAt ?? partition.createdRange.split("..")[0]; const event = partition.event ?? "workflow_run"; + const duplicate = coordinatorRunPartitions.findIndex( + (other) => + (other.event ?? "workflow_run") === event && + other.createdRange === partition.createdRange, + ); + if (duplicate !== partitionIndex) { + throw new Error( + `duplicate coordinator partition for event ${event} and range ${partition.createdRange}`, + ); + }Also applies to: 655-656
🤖 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 592 - 604, Add a fail-fast validation for duplicate (event, createdRange) pairs in the fixture partition setup before generating the bash case labels. Update the partition-building logic around coordinatorRunPartitions and the generated case construction so duplicate declarations throw a clear error instead of producing unreachable arms and silently dropping partition data.Source: Path instructions
96-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the untested final partition fields.
No test sets
finalTotalCount,finalPageTotalCounts, orfinalRunOverrides; onlyfinalRunPageshas a test consumer. Remove these fields and their fallback handling, or add tests that require 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 96 - 98, Remove the unused finalTotalCount, finalPageTotalCounts, and finalRunOverrides fields from the fixture type and delete their fallback-handling logic. Preserve finalRunPages and its existing tested behavior; do not add new support unless tests are introduced to exercise these fields.Source: Coding guidelines
.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts (5)
751-762: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the workflow path from
E2E_COORDINATOR_WORKFLOW_PATH.Line 757 hardcodes
pr-e2e-gate.yaml, butparseE2eCoordinatorRunvalidatesrecord.pathagainstE2E_COORDINATOR_WORKFLOW_PATH. If the workflow file is renamed, the two sites drift and the inventory returns runs that the parser then rejects.♻️ Proposed refactor
+const E2E_COORDINATOR_WORKFLOW_FILE = E2E_COORDINATOR_WORKFLOW_PATH.slice( + E2E_COORDINATOR_WORKFLOW_PATH.lastIndexOf("/") + 1, +);"repos/" + repo + - "/actions/workflows/pr-e2e-gate.yaml/runs?event=" + + "/actions/workflows/" + + E2E_COORDINATOR_WORKFLOW_FILE + + "/runs?event=" + event +🤖 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 751 - 762, Update the workflow-runs API path in the pages query to derive the filename from the existing E2E_COORDINATOR_WORKFLOW_PATH constant instead of hardcoding pr-e2e-gate.yaml, keeping it consistent with parseE2eCoordinatorRun validation.
1001-1035: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the external-id and check-name constants.
selectE2eCoordinationCheck(line 1006) ande2eCoordinationHistoryStartedAt(line 1034) build the samenemoclaw-pr-e2e:v2:...string, andfetchE2eCoordinationEvidencerepeatscheckNamesat line 1065. Three copies must change together when the external-id version changes.♻️ Proposed refactor
+const E2E_COORDINATION_CHECK_NAMES = ["E2E / PR Gate", "E2E / PR Gate Coordination"]; + +function e2eCoordinationExternalId(exactDiff: ExactDiffIdentity): string { + return `nemoclaw-pr-e2e:v2:${exactDiff.number}:${exactDiff.headSha}:${exactDiff.baseSha}`; +}🤖 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 1001 - 1035, Extract shared constants or helper symbols for the E2E coordination external ID format and accepted check names, then update selectE2eCoordinationCheck, e2eCoordinationHistoryStartedAt, and fetchE2eCoordinationEvidence to reuse them. Ensure all three paths retain the current values and filtering behavior while eliminating duplicated definitions.
603-635: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the malformed-payload disposition inside the job loop.
Line 604 returns
{ result: null }for a malformed page object, but line 630 returns{ result: false }for a malformed job object. Both cases describe an unusable API payload. The current split makes a malformed job record a hard rejection and a malformed page an indeterminate result. The behavior is fail-closed, so this is a consistency concern only. Pick one disposition for malformed payload shape.🤖 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 603 - 635, Align the malformed-payload handling in the job-page loop by using the same result disposition for invalid page objects and invalid job records. Update the return in the record validation branch alongside the existing page-object check, preserving all validation conditions and fail-closed behavior.
2770-2783: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard
prData.statusCheckRollupwhen buildingcapturedRevision.Line 2756 already treats the field as possibly absent through
statusCheckRollup && statusCheckRollup.length > 0insidecheckCi, and line 2758 guardsprData.fileswith?? []. Line 2782 assigns the value directly into a field declared as non-optionalStatusCheck[]. Ifgh pr viewomits the field, the snapshot holdsundefinedbehind a non-optional type.checkFinalRevisiondoes not read the field today, so no failure occurs now, but a future comparison on it would dereferenceundefined.🛡️ Proposed fix
headRepository, - statusCheckRollup: prData.statusCheckRollup, + statusCheckRollup: prData.statusCheckRollup ?? [], };🤖 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 2770 - 2783, Update the capturedRevision construction to normalize prData.statusCheckRollup to an empty StatusCheck[] when the field is absent, preserving existing values when present. Keep the non-optional statusCheckRollup contract intact and limit the change to the capturedRevision assignment.
741-841: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider the API call volume of the partitioned inventory.
The loop issues one paginated
gh apirequest for each event and partition pair. With the 14-day maximum and 12-hour partitions, that reaches 56 paginated requests before candidate validation adds three more requests per candidate. Each request carries the 120 srun()timeout fromshared.ts. For a maintainer-invoked CLI the latency is acceptable, but consider wideningE2E_COORDINATOR_PARTITION_MSor narrowing the inventory window when the check start is recent.🤖 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 741 - 841, Reduce partitioned inventory API volume for recent checks by widening E2E_COORDINATOR_PARTITION_MS or narrowing the inventory window before e2eCoordinatorRunPartitions creates partitions. Preserve the existing 14-day and 12-hour behavior for older or non-recent checks, and keep candidate validation unchanged.
🤖 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 1973-1974: Update captureRequiredCheckSnapshot to sort
JSON-serialized entries using a deterministic code-unit comparison instead of
localeCompare, returning a total ordering for distinct strings. Keep
checkFinalCi and checkLastCi unchanged so their deep snapshot comparisons no
longer depend on GitHub rollup input order.
- Around line 1533-1534: Update isCurrentE2eSeedRun to require the run’s
coordination-cycle identifier to match the selected coordination check, in
addition to the existing e2eControllerHeadBinding and e2eGateRun conditions. Add
a regression test covering a stale seed run from before a newer coordination
check, ensuring it is rejected even when the head binding is “current.”
---
Nitpick comments:
In @.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts:
- Around line 751-762: Update the workflow-runs API path in the pages query to
derive the filename from the existing E2E_COORDINATOR_WORKFLOW_PATH constant
instead of hardcoding pr-e2e-gate.yaml, keeping it consistent with
parseE2eCoordinatorRun validation.
- Around line 1001-1035: Extract shared constants or helper symbols for the E2E
coordination external ID format and accepted check names, then update
selectE2eCoordinationCheck, e2eCoordinationHistoryStartedAt, and
fetchE2eCoordinationEvidence to reuse them. Ensure all three paths retain the
current values and filtering behavior while eliminating duplicated definitions.
- Around line 603-635: Align the malformed-payload handling in the job-page loop
by using the same result disposition for invalid page objects and invalid job
records. Update the return in the record validation branch alongside the
existing page-object check, preserving all validation conditions and fail-closed
behavior.
- Around line 2770-2783: Update the capturedRevision construction to normalize
prData.statusCheckRollup to an empty StatusCheck[] when the field is absent,
preserving existing values when present. Keep the non-optional statusCheckRollup
contract intact and limit the change to the capturedRevision assignment.
- Around line 741-841: Reduce partitioned inventory API volume for recent checks
by widening E2E_COORDINATOR_PARTITION_MS or narrowing the inventory window
before e2eCoordinatorRunPartitions creates partitions. Preserve the existing
14-day and 12-hour behavior for older or non-recent checks, and keep candidate
validation unchanged.
In `@test/skills/check-gates-coordinator-evidence.test.ts`:
- Line 207: Rename the local constant `coordinatorJobs` to
`resolvedCoordinatorJobs` and update all of its references within the
surrounding `runGate({ ... })` setup, while leaving the module-level
`coordinatorJobs` helper unchanged.
- Around line 1151-1166: Extend the rejection-case table in the coordinator
evidence tests with cases for run-level created_at and updated_at explicitly set
to null, using the existing fixture properties and matching the job-level null
cases. Preserve the current absent, malformed, and ordering cases.
- Around line 273-298: Rename the test describing the repository-owned manual
coordinator in runGateWithCoordinator to explicitly state that the authorized
fork revision has zero automatic predecessors, while leaving its setup and
assertions unchanged.
In `@test/skills/check-gates-test-fixtures.ts`:
- Around line 352-357: Extract a shared constant for the default observation
timestamp and reuse it both in the observationTime fallback near the clock setup
and in the default coordinator partition window. Remove the duplicated timestamp
literal while preserving the existing fallback behavior; ignore the
path-traversal warning for clockPath.
- Around line 592-604: Add a fail-fast validation for duplicate (event,
createdRange) pairs in the fixture partition setup before generating the bash
case labels. Update the partition-building logic around coordinatorRunPartitions
and the generated case construction so duplicate declarations throw a clear
error instead of producing unreachable arms and silently dropping partition
data.
- Around line 96-98: Remove the unused finalTotalCount, finalPageTotalCounts,
and finalRunOverrides fields from the fixture type and delete their
fallback-handling logic. Preserve finalRunPages and its existing tested
behavior; do not add new support unless tests are introduced to exercise these
fields.
🪄 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: 63cfc202-6bcc-4310-a891-bae896e7b3a3
📒 Files selected for processing (3)
.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.tstest/skills/check-gates-coordinator-evidence.test.tstest/skills/check-gates-test-fixtures.ts
| const isCurrentE2eSeedRun = (run: ActionRunMetadata): boolean => | ||
| e2eControllerHeadBinding(run) === "current" && run.e2eGateRun === true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect seed-run recognition, controller head binding, and coordinator enclosure rules.
set -euo pipefail
fd -t f 'check-gates.ts' .agents | while IFS= read -r file; do
echo "== $file"
rg -n -C 6 'isCurrentE2eSeedRun|e2eControllerHeadBinding|coordinateStartedAt <= coordinationCompletedAt' "$file"
done
# Surface tests that assert seed-run timing behavior.
fd -t f -e ts . test | while IFS= read -r file; do
rg -n -C 4 'seed[ -]?run|e2eGateRun|coordination(Started|Completed)At' "$file" || true
doneRepository: NVIDIA/NemoClaw
Length of output: 3510
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts"
echo "== coordinator evidence validation =="
sed -n '650,735p' "$file"
sed -n '1060,1120p' "$file"
echo "== run binding and classification =="
sed -n '1450,1555p' "$file"
sed -n '1650,1745p' "$file"
echo "== test fixtures and timing cases =="
rg -n -C 12 'runGateWithCoordinator|custom check completed|retry check|seedRun|coordinationStartedAt|coordinationCompletedAt|latestAttempt' .agents/skills/nemoclaw-maintainer-day test 2>/dev/null || trueRepository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts"
echo "== rollup selection functions =="
sed -n '1580,1675p' "$file"
sed -n '1740,1905p' "$file"
echo "== all current-run selection references =="
rg -n -C 5 'checksFromLatestAttempt|isMeaningfulExactDiffRun|allActionRunIds|latest.*timestamp|updatedAt|createdAt|detailsUrl|currentCheckRollup' "$file" | sed -n '1,260p'
echo "== focused tests =="
rg -n -C 8 'runGateWithCoordinator|seedRun|e2eGateRun|E2E / PR Gate|coordination' .agents/skills/nemoclaw-maintainer-day --glob '*test*.ts' --glob '*.spec.ts' 2>/dev/null | sed -n '1,300p'Repository: NVIDIA/NemoClaw
Length of output: 20087
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts"
echo "== E2E evidence selection and trusted check identity =="
sed -n '840,920p' "$file"
sed -n '1120,1180p' "$file"
echo "== required check definitions and final E2E handling =="
rg -n -C 8 'REQUIRED_CHECK_NAMES|E2E / PR Gate|trustedCustomCheckId|incompleteAttemptEvidence|allPass|currentCheckRollup' "$file" | sed -n '1,360p'
echo "== test files containing E2E fixtures =="
git ls-files | rg '(^|/)(test|tests|__tests__)/|\\.test\\.|\\.spec\\.' | head -100Repository: NVIDIA/NemoClaw
Length of output: 23396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -l 'runGateWithCoordinator|custom check completed after its seed run|retry check that starts after its authenticated seed run finishes' . | head -20
rg -n -C 18 'custom check completed after its seed run|retry check that starts after its authenticated seed run finishes|seedRun:' . --glob '*.ts' | sed -n '1,420p'Repository: NVIDIA/NemoClaw
Length of output: 11836
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/skills/check-gates-coordinator-evidence.test.ts"
echo "== fixture construction =="
sed -n '1,265p' "$file"
echo "== retry timing fixture =="
sed -n '397,455p' "$file"
echo "== status-check rollup and mocked action runs =="
rg -n -C 10 'statusCheckRollup|check-runs|E2E / PR Gate|detailsUrl|seedJobs|extraRuns|runId' "$file" | sed -n '1,360p'Repository: NVIDIA/NemoClaw
Length of output: 21840
🏁 Script executed:
python3 - <<'PY'
from datetime import datetime, timezone
def ts(value):
return datetime.fromisoformat(value.replace("Z", "+00:00")).timestamp()
# Model the predicates at e2eControllerHeadBinding and isCurrentE2eSeedRun.
seed = {
"event": "pull_request_target",
"path": ".github/workflows/pr-e2e-gate.yaml",
"exactDiff": None,
"e2eGateDiff": True,
"e2eGateRun": True,
"hasPullRequests": False,
"headShaMatches": True,
"headRefNameMatches": True,
"headRepositoryMatches": True,
"createdAt": ts("2026-08-04T14:07:32Z"),
"updatedAt": ts("2026-08-04T14:08:01Z"),
}
coordination = {
"valid": True,
"startedAt": ts("2026-08-04T14:28:06Z"),
"completedAt": ts("2026-08-04T14:38:01Z"),
}
binding = (
coordination["valid"]
and seed["event"] == "pull_request_target"
and seed["path"] == ".github/workflows/pr-e2e-gate.yaml"
and seed["e2eGateDiff"] is True
and seed["hasPullRequests"] is False
and seed["headShaMatches"] is True
and seed["headRefNameMatches"] is True
and seed["headRepositoryMatches"] is True
)
classified_current = binding and seed["e2eGateRun"] is True
print({
"seed_finished_before_coordination_started":
seed["updatedAt"] < coordination["startedAt"],
"controller_binding": "current" if binding else "unknown",
"isCurrentE2eSeedRun": classified_current,
"seed_window_checked": False,
})
assert classified_current is True
assert seed["updatedAt"] < coordination["startedAt"]
PYRepository: NVIDIA/NemoClaw
Length of output: 297
Bind the E2E seed run to the selected coordination cycle. A seed run that finished before the coordination check started still returns current; exact head identity does not distinguish attempts. Add an explicit cycle binding and a regression test for a stale seed with a newer coordination check.
🤖 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
1533 - 1534, Update isCurrentE2eSeedRun to require the run’s coordination-cycle
identifier to match the selected coordination check, in addition to the existing
e2eControllerHeadBinding and e2eGateRun conditions. Add a regression test
covering a stale seed run from before a newer coordination check, ensuring it is
rejected even when the head binding is “current.”
| snapshot.sort((left, right) => JSON.stringify(left).localeCompare(JSON.stringify(right))); | ||
| return snapshot; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Sort the snapshot with a code-unit comparison instead of localeCompare.
captureRequiredCheckSnapshot sorts by JSON.stringify(...).localeCompare(...), and checkFinalCi and checkLastCi then compare snapshots with isDeepStrictEqual. Default collation can report two distinct strings as equal. For such a tie, Array.prototype.sort keeps the input order, and the input order comes from the GitHub rollup, which can differ between the initial, final, and last reads. That produces a false "Required check rollup changed during gate evaluation" rejection. A plain code-unit comparison gives a total order and removes the tie.
🐛 Proposed fix
- snapshot.sort((left, right) => JSON.stringify(left).localeCompare(JSON.stringify(right)));
+ snapshot.sort((left, right) => {
+ const leftKey = JSON.stringify(left);
+ const rightKey = JSON.stringify(right);
+ return leftKey < rightKey ? -1 : leftKey > rightKey ? 1 : 0;
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| snapshot.sort((left, right) => JSON.stringify(left).localeCompare(JSON.stringify(right))); | |
| return snapshot; | |
| snapshot.sort((left, right) => { | |
| const leftKey = JSON.stringify(left); | |
| const rightKey = JSON.stringify(right); | |
| return leftKey < rightKey ? -1 : leftKey > rightKey ? 1 : 0; | |
| }); | |
| return snapshot; |
🤖 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
1973 - 1974, Update captureRequiredCheckSnapshot to sort JSON-serialized entries
using a deterministic code-unit comparison instead of localeCompare, returning a
total ordering for distinct strings. Keep checkFinalCi and checkLastCi unchanged
so their deep snapshot comparisons no longer depend on GitHub rollup input
order.
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. 3 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. |
wscurran
added
area: ci
|
Closed as superseded by #8258. The successor preserves this PR's authored commits and attribution, carries the same effective four-file change plus mechanical merges from main, and avoids force-pushing this published branch. |
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.
Changes
Type of Change
Quality Gates
f3b9686b3.Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
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 unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: GitHub CI pending.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests