fix(ci): clarify E2E gate outcomes - #6821
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
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)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR updates the E2E gate controller’s workflow identity, command contracts, evidence handling, exact-diff lifecycle, CI reporting, exception resolution, documentation, and test coverage. ChangesPR E2E Gate control plane
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Sequence Diagram(s)sequenceDiagram
participant ControllerWorkflow
participant PrGateController
participant E2EWorkflow
participant GitHubCheck
ControllerWorkflow->>PrGateController: pass command and evidence outcome
PrGateController->>E2EWorkflow: inspect workflow status and jobs
E2EWorkflow-->>PrGateController: return completion and evidence results
PrGateController->>GitHubCheck: finalize verdict, conclusion, and output
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 in the Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron is a non-blocking second opinion. Its prose, findings, and E2E guidance do not change the primary assessment above and remain in workflow artifacts only. E2E guidanceAdvisory only: coverage and selector recommendations are non-authoritative. E2E / PR Gate independently computes and dispatches trusted jobs without consuming this output. Recommended coverage:
This is an automated, non-authoritative review. Findings are inputs to maintainer adjudication. Warnings and optional suggestions do not require a response or follow-up. A human maintainer makes the final merge decision. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tools/e2e/pr-e2e-gate.mts (1)
1774-1786: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded workflow-name string duplicated instead of reusing
WORKFLOW_NAME.Both the fork-exception and control-plane-exception summaries build
gateRunLinkfrom the literal`[E2E / PR Gate Controller run ${command.gateRunId}](${gateRunUrl})`, duplicating theWORKFLOW_NAMEconstant (line 34, also used at line 2138) as free text in two places. IfWORKFLOW_NAMEis ever renamed, these two summaries will silently go stale.♻️ Proposed fix: reuse WORKFLOW_NAME
- const gateRunLink = `[E2E / PR Gate Controller run ${command.gateRunId}](${gateRunUrl})`; + const gateRunLink = `[${WORKFLOW_NAME} run ${command.gateRunId}](${gateRunUrl})`;Apply the same change at the second occurrence (control-plane exception summary).
Also applies to: 1799-1813
🤖 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/e2e/pr-e2e-gate.mts` around lines 1774 - 1786, Update both fork-exception and control-plane-exception summary constructions to derive the gateRunLink label from the existing WORKFLOW_NAME constant instead of hardcoding “E2E / PR Gate Controller”. Preserve the current run ID and URL formatting while reusing WORKFLOW_NAME in both occurrences.test/pr-e2e-gate-command.test.ts (1)
44-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider splitting this into multiple focused tests.
One
it(...)asserts 8+ independent command-parsing scenarios (start/cancel/seed/finish/resolve-fork/resolve-control-plane/abandon plus two distinct error paths) sharing a single workDir. This works, but a failure anywhere in the middle produces one generic failure that doesn't pinpoint which scenario broke, and the title ("parses one lifecycle command set...") doesn't reflect the negative-path assertions (safe-integer overflow, insecure directory permissions).Splitting into per-scenario
itblocks (viabeforeEach/afterEachfor the shared workDir) would improve failure isolation and readability at low cost.🤖 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/pr-e2e-gate-command.test.ts` around lines 44 - 172, Split the single test in the “PR E2E controller commands” suite into focused it blocks for each command-parsing scenario and each validation/error path, using beforeEach/afterEach or equivalent lifecycle setup for the shared temporary workDir. Give each test a title matching its scenario, while preserving all existing assertions and cleanup behavior, including safe-integer overflow and insecure-directory validation.
🤖 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 `@test/pr-e2e-gate-command.test.ts`:
- Around line 44-172: Split the single test in the “PR E2E controller commands”
suite into focused it blocks for each command-parsing scenario and each
validation/error path, using beforeEach/afterEach or equivalent lifecycle setup
for the shared temporary workDir. Give each test a title matching its scenario,
while preserving all existing assertions and cleanup behavior, including
safe-integer overflow and insecure-directory validation.
In `@tools/e2e/pr-e2e-gate.mts`:
- Around line 1774-1786: Update both fork-exception and control-plane-exception
summary constructions to derive the gateRunLink label from the existing
WORKFLOW_NAME constant instead of hardcoding “E2E / PR Gate Controller”.
Preserve the current run ID and URL formatting while reusing WORKFLOW_NAME in
both occurrences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 69aa3846-6f12-45d6-bc62-91b83a2c2a98
📒 Files selected for processing (9)
.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md.github/workflows/pr-e2e-gate.yamltest/e2e/README.mdtest/pr-e2e-gate-command.test.tstest/pr-e2e-gate-exceptions.test.tstest/pr-e2e-gate-lifecycle.test.tstest/pr-e2e-gate-workflow.test.tstest/pr-e2e-gate.test.tstools/e2e/pr-e2e-gate.mts
## Summary Controller-only internal PRs now automatically dispatch the selected exact-SHA E2E jobs instead of requiring a no-run exception. Internal changes that alter PR-controlled E2E execution or evidence code require an explicit maintainer/admin `run-control-plane` authorization, then use the normal dispatch, wait, evidence, and finish path so only verified results can clear `E2E / PR Gate`. Fork handling remains a no-secret exception. ## Related Issue Part of #6145 ## Changes - Treat only `.github/workflows/pr-e2e-gate.yaml` and `tools/e2e/pr-e2e-gate.mts` as controller-only control-plane changes eligible for automatic internal dispatch. - Replace the internal `resolve-control-plane` no-run waiver with exact-head/base `run-control-plane` authorization that requires `maintain` or `admin` and dispatches the selected jobs. - Serialize automatic and manual coordination on the exact head SHA, preserve a retryable failure after authorized-start errors, and keep success exclusive to verified evidence. - Expand command, workflow, risk-plan, lifecycle, authorization, retry, origin, and #6821 regression coverage; update contributor and maintainer E2E guidance. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Maintainer-directed scope; an independent implementation audit found no success bypass, identified two lifecycle races, and both were fixed and retested before submission. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run test/pr-risk-plan.test.ts test/pr-e2e-gate-command.test.ts test/pr-e2e-gate-workflow.test.ts test/pr-e2e-gate-exceptions.test.ts test/pr-e2e-gate.test.ts` (154 passed) - [ ] Applicable broad gate passed — local `npm test` did not complete: after eight minutes it remained on one idle integration worker with an orphaned persistent-log `tail -F`; the process was terminated and GitHub CI is required for the broad verdict. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added maintainer-authorized, exact-revision execution for control-plane E2E checks. * Added clearer fork exception resolution with validated evidence links and protected approvals. * Improved risk-based handling of E2E changes and credentialed test authorization. * **Bug Fixes** * Prevented unauthorized or outdated control-plane and fork workflows from dispatching E2E jobs. * Improved cancellation and supersession handling when pull requests change. * **Documentation** * Updated E2E gate guidance, authorization requirements, exception workflows, and manual recovery steps. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Carlos Villela <cvillela@nvidia.com>
wscurran
added
area: ci
2 participants
Summary
Separate trusted controller health from the exact-diff merge verdict so handled PR CI/E2E failures and superseded or closed revisions no longer masquerade as controller failures. Add self-explanatory diagnostics and exception links while preserving red controller status for real controller, correlation, and evidence-download faults. This follows the actual-run review of #6807 and #6810.
Changes
E2E / PR Gate Controllerwhile preserving the required custom check nameE2E / PR Gate, and document which outcome controls merge authority.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project integration test/pr-e2e-gate-command.test.ts test/pr-e2e-gate.test.ts test/pr-e2e-gate-lifecycle.test.ts test/pr-e2e-gate-exceptions.test.ts test/pr-e2e-gate-workflow.test.ts(5 files, 107 tests passed)GIT_CONFIG_GLOBAL=/dev/null SSH_AUTH_SOCK=/tmp/ssh-qmH9wX3sr5/agent.4170011 npm test(1,502 files and 17,062 tests passed; 3 files and 40 tests skipped)npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Improvements
Documentation
Tests