ci(e2e): unify maintainer approval - #7678
Conversation
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
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 PR consolidates PR E2E authorization into a maintainer-driven ChangesMaintainer E2E approval
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Maintainer
participant PRGateWorkflow
participant E2EController
participant CoordinationCheck
participant E2EJobs
Maintainer->>PRGateWorkflow: Dispatch approve-e2e with PR, SHA, and reason
PRGateWorkflow->>E2EController: Start evaluation with --mode approve-e2e
E2EController->>CoordinationCheck: Validate and update authorization state
E2EController->>E2EJobs: Dispatch selected E2E jobs
E2EJobs->>CoordinationCheck: Publish completion evidence
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
prekshivyas
added
area: ci
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 7604293 in the TypeScript / code-coverage/cliThe overall coverage in commit 7604293 in the Show a code coverage summary of the most impacted files.
Updated |
Security reviewVerdictPASS. The PR narrows credentialed E2E authorization to repository maintainers and administrators and preserves the existing exact-revision, deterministic-plan, trusted-workflow, dispatch-reconciliation, cancellation, and evidence checks. I found no security vulnerability or warning that blocks merge. FindingsNo findings. Detailed analysis
Files reviewed
Reviewed commit: |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported 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: 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/e2e/pr-e2e-gate.mts`:
- Line 3108: Update both workflow URL construction call sites to pass only the
workflow file name, such as pr-e2e-gate.yaml, rather than the full
PR_GATE_WORKFLOW_PATH value that includes .github/workflows/. Preserve the
existing repository and Actions URL structure.
🪄 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: 70b8a824-e851-4d82-9565-90c6fbbc4b05
📒 Files selected for processing (12)
.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-dispatch-recovery.test.tstest/pr-e2e-gate-fork-approval.test.tstest/pr-e2e-gate-internal-approval.test.tstest/pr-e2e-gate-lifecycle.test.tstest/pr-e2e-gate-retry-history.test.tstest/pr-e2e-gate-workflow.test.tstest/pr-e2e-required.test.tstools/e2e/pr-e2e-gate.mts
💤 Files with no reviewable changes (1)
- test/pr-e2e-gate-internal-approval.test.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
…approval Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
…approval Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Maintainer security review: PASS at exact head ff343972b27f9562175d776088c323886ed2579f against current main.
- Secrets and credentials — PASS: untrusted PR code still receives no credential before authorization; evidence validation and exact-plan dispatch remain required.
- Input validation — PASS: PR number, exact head/base SHAs, workflow SHA, first run attempt, maintainer login, bounded reason, pending check identity, and deterministic plan are validated fail-closed.
- Authentication and authorization — PASS: the manual workflow actor must currently have repository
maintainoradminpermission. Oneapprove-e2eoperation now applies the same role check to internal and fork PRs. - Dependencies — PASS: no dependency change.
- Logging and exposure — PASS: coordination guidance publishes only PR/SHA/plan metadata; no secret or credential value is logged.
- Cryptography — PASS: no cryptographic behavior changed; trusted workflow and child-evidence hash bindings remain intact.
- Configuration — PASS: obsolete protected-environment outputs and duplicate modes are removed; the workflow remains main-branch, first-attempt, exact-input gated.
- Tests — PASS: parsing, permissions, fork/internal selection, pending checks, retries, dispatch reconciliation, lifecycle, workflow contract, and maintainer guidance are covered. All required CI, eight CLI shards, E2E support, CodeQL, advisors, sandbox-image tests, and
E2E / PR Gatepassed. - System security — PASS: credentialed execution remains behind trusted-controller authorization; cancellation, live-revision checks, one-time runner-loss retry, evidence download/verification, and terminal check cleanup remain fail-closed.
The final follow-up removes an unused workflow-path constant and clarifies that reviewers must verify the exact head repository. No security findings or waiver remain.
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Maintainer exact-head security review: PASS at dc3f386 against current main.\n\nThis head is a signed mechanical merge of current main; the reviewed control-plane diff is unchanged. Live fork-gate evidence on PRs #7230 and #7463 also confirms the old unprotected environment path failed closed before exposing credentials, which is the operational gap this unified maintainer-dispatch flow removes.\n\n1. Secrets and credentials — PASS: no secret is exposed before exact-revision authorization; child evidence remains required.\n2. Input validation — PASS: PR number, 40-character head/base SHAs, head repository, deterministic plan, workflow commit, coordination identity, and review reason are validated.\n3. Authentication and authorization — PASS: first-attempt actors must have current maintain or admin access.\n4. Dependencies — PASS: none changed by the PR.\n5. Logging and exposure — PASS: guidance contains review metadata only; no credential value is logged.\n6. Cryptography — PASS: existing SHA and evidence integrity checks remain fail-closed.\n7. Configuration — PASS: removes dependence on mutable environment reviewer configuration without weakening repository permission checks.\n8. Tests — PASS: refreshed-head controller tests pass 48/48, maintainer policy tests pass 16/16, and CLI type-check and pre-push checks pass.\n9. System security — PASS: trusted main dispatch, fork repository checkout, live-revision revalidation, cancellation, and verified evidence gates remain intact.\n\nThe exact-head documentation writer receipt passed. No correctness or security findings remain.
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Maintainer exact-head security review: PASS at 3f72fec against current main.
This head adds only the signed merge of the current main changelog commit to the previously reviewed control-plane head; the PR implementation, workflow, guidance, and tests are unchanged.
- Secrets and credentials — PASS: no secret is exposed before exact-revision authorization; verified child evidence remains required.
- Input validation — PASS: exact PR, repository, head/base SHAs, workflow, plan, coordination identity, and review reason remain validated.
- Authentication and authorization — PASS: first-attempt actors still require current maintain or admin access.
- Dependencies — PASS: none changed.
- Logging and exposure — PASS: no credential value is logged.
- Cryptography — PASS: SHA and evidence integrity checks remain fail-closed.
- Configuration — PASS: maintainer dispatch remains independent of mutable environment reviewer configuration.
- Tests — PASS: prior refreshed-head controller tests pass 48/48, maintainer policy tests 16/16, CLI type-check passes, and the intervening docs-only merge passes changelog tests 6/6.
- System security — PASS: trusted main dispatch, live-revision revalidation, cancellation, fork checkout, and verified evidence gates remain intact.
The exact-head documentation writer receipt passed. No correctness or security findings remain.
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Maintainer follow-up on PRA-1 at exact head 3f72fec: BLOCKED pending an explicit architecture/security decision.
The finding is not dismissed by the general contributor/approver-overlap policy alone. Merged PR #6233 makes overlap nonblocking for PR approval and merge readiness, but the credential-release boundary had a separate required-reviewer allowlist. The current internal environment names cv and ericksoa as required reviewers (with prevent-self-review disabled), while the fork environment is misconfigured with no protection and correctly fails closed. This PR replaces those environment allowlists with any account holding current maintain or admin permission.
That broadening is deliberate in the code and fully tested, but the repository evidence does not establish whether the #6233 merge-approval decision is intended to cover credentialed E2E release. Do not approve this PR until a maintainer explicitly chooses one of these contracts: retain maintain/admin authorization as written, reject PR authors/commit authors/co-authors, or preserve an independent reviewer allowlist. No code change is made while that decision is unresolved.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Maintainer exact-head security review: PASS at 7604293.
- Secrets and credentials — PASS. No credential is exposed before independent exact-revision authorization and verified child evidence remains required.
- Input validation — PASS. PR, repository, head/base SHAs, workflow, plan, coordination identity, reason, contributor history, and pagination are validated fail-closed.
- Authentication and authorization — PASS. The actor requires current maintain/admin access and is rejected if they opened the PR or authored/co-authored any current commit. Unassociated commit identities also fail closed before coordination mutation or dispatch.
- Dependencies — PASS. No dependency change.
- Error handling and logging — PASS. Contributor-history errors expose no credentials or author data and occur before credential-bearing dispatch.
- Cryptography — PASS. Existing SHA and evidence-integrity bindings remain unchanged.
- Configuration — PASS. The trusted main workflow and first-attempt restriction remain; the obsolete mutable environment-reviewer dependency stays removed.
- Tests — PASS. The final merged head passes 16 integration files / 280 tests plus CLI typechecking. Tests cover opener, author, co-author, unmapped author, role, exact revision, dispatch reconciliation, and failure recovery.
- System security — PASS. Authorization is now independently separated from every recognized contributor account and fails closed when complete identity proof is unavailable. Live-revision checks, cancellation, one-time retry, and evidence verification remain intact.
The prior advisor blocker is resolved. Because this PR is authored by the current account, this COMMENTED review is not independent merge approval; required CI, automated review, and another eligible reviewer must still evaluate the exact head.
|
Superseded by #7832, which keeps the useful unified |
<!-- markdownlint-disable MD041 --> ## Summary Unify internal and fork pull-request E2E authorization under one maintainer-only `approve-e2e` operation. This removes the protected-environment and approval-history machinery proposed in #7678 while preserving the existing dispatch integrity checks. ## Changes - Require current `maintain` or `admin` repository permission for every `approve-e2e` request. - Use the same authorization and dispatch path for internal and fork pull requests. - Remove the fork protected-environment job, approval-coordinate outputs, GraphQL approval-history parsing, and separate fork command mode. - Preserve exact pull request, head SHA, base SHA, plan, workflow revision, pending-check, and live-state validation. - Update maintainer guidance, E2E operational documentation, and workflow contract tests for the unified flow. - Ratchet the `shell-quote.ts` fan-in budget from 27 to 26 after the source-install path removed by #7632 reduced its consumers. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] 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: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: No user-facing product behavior changed; internal maintainer and E2E operational guidance was updated with the workflow. - [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: Local NemoClaw maintainer security review passed with no findings across all nine review categories. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md` and `test/e2e/README.md` accurately document the unified maintainer-only approval flow; no Fern documentation change is needed. - Agent: Codex Desktop <!-- docs-review-head-sha: 9001231 --> <!-- docs-review-agents-blob-sha: 0082a58 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line 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 — command/result or justification: `npm exec -- vitest run --project integration test/pr-e2e-gate*.test.ts test/pr-e2e-required.test.ts test/maintainer-skills-policy.test.ts` — 16 files and 276 tests passed; focused fork-approval rerun — 19 tests passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: `npm run check` was attempted; the focused architecture test passed, but the full host suite had 275 unrelated environment failures from missing Python `yaml`, an absent official OpenShell Homebrew formula, macOS `/private/tmp` path alias effects, and cascading timeouts. CI provides the authoritative Linux result. - [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: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Consolidated PR E2E maintainer approval into a single `approve-e2e` workflow flow, with updated dispatch behavior. - Added/strengthened pre-approval validation for PR number, expected head/base SHAs, and review reason. - **Documentation** - Refined maintainer approval and fork approval guidance, including updated gate wording and authorization expectations. - **Bug Fixes** - Hardened the gate so approval credentials and sensitive approval outputs aren’t emitted until authorization requirements are satisfied. - **Tests** - Updated E2E gate and lifecycle tests to match the new `approve-e2e` flow and messages. - **Chores** - Minor workflow/tooling budget adjustment. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
5 participants
Summary
Replace the two protected-environment approval paths with one maintainer-only
approve-e2eworkflow operation for internal and fork PRs. The controller binds approval to the exact PR, head SHA, base SHA, deterministic plan, pending coordination check, trusted workflow commit, verified child evidence, and an approver who did not contribute to the PR.Changes
maintainoradminpermissionType of Change
Quality Gates
Documentation Writer Review
docs-updated.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.mdandtest/e2e/README.mdconsistently document independent approval, unresolved contributor-identity failure, exact-revision validation, and pre-dispatch enforcement. Changed test titles followWRITING.md; no blocking documentation finding remains. The final head passes 16 files / 280 tests plus CLI typechecking, Biome, diff check, and normal hooks.DGX Station Hardware Evidence
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 --project integration test/pr-e2e-gate*.test.ts test/pr-e2e-required.test.ts test/maintainer-skills-policy.test.ts(16 files, 280 tests)npm run docsbuilds without warnings (doc changes only)Signed-off-by: Prekshi Vyas prekshiv@nvidia.com