Skip to content

fix(ci): run approved E2E for fork PRs#7505

Merged
prekshivyas merged 11 commits into
mainfrom
codex/authorize-fork-e2e-run
Jul 25, 2026
Merged

fix(ci): run approved E2E for fork PRs#7505
prekshivyas merged 11 commits into
mainfrom
codex/authorize-fork-e2e-run

Conversation

@prekshivyas

@prekshivyas prekshivyas commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fork PRs previously became mergeable after an approved credentialed-E2E skip, so their selected E2E work never ran. This change makes protected approval authorize the exact fork repository and SHA to run through the trusted E2E workflow, and only verified passing evidence can make the required gate green. The strict merge checker also authenticates fork runs when GitHub omits their PR association, without weakening the exact-diff or E2E coordination requirements.

Changes

  • Keep selected fork E2E coordination in progress until a required reviewer approves approve-credentialed-e2e-for-fork-pr.
  • Dispatch the exact live fork head repository, PR SHA, base SHA, trusted workflow SHA, and deterministic plan after approval.
  • Bind each PR E2E child to the controller-owned coordination check. Before PR checkout, require the GitHub Actions actor, first run attempt, exact check app, PR/head/base identity, plan summary, and current child-run URL; direct manual dispatches fail closed.
  • Add the checkout_repository dispatch input because a workflow dispatched in the base repository cannot resolve a fork-only SHA from checkout_sha alone. Every PR-code checkout and runner-loss retry consumes this value, and e2e-operations-workflow-boundary.test.ts enforces the repository/SHA pairing.
  • Remove the protected and manual fork-skip paths. Approval starts E2E; verified passing evidence remains the only success path.
  • Authenticate association-less fork runs only when GitHub Actions metadata matches the exact fork repository, branch, and PR SHA. Keep immutable PR/base titles for PR CI and installer checks, and retain trusted coordination for E2E.
  • Update fork-approval, workflow-boundary, retry, required-gate, and maintainer-guidance coverage.

Type of Change

  • 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

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification:
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Maintainer-requested follow-up to the fork E2E skip defect fixed in fix(ci): prevent skipped E2E gate from satisfying merge #7501; the exact-revision credential boundary, evidence-only success path, and fail-closed association-less run authentication are covered by workflow, controller, and merge-checker tests. Security review passed after binding child execution to the exact controller-owned check; direct manual fork dispatch, check replay, mismatched app/identity/plan/run, and rerun attempts fail closed before PR checkout.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: docs-updated
  • Evidence: .agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md, test/e2e/README.md, and test/e2e/docs/README.md; exact-head review confirmed controller-check authentication is documented before PR checkout and direct manual-dispatch rejection is accurate.
  • Agent: Codex Desktop

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit:
  • Station profile/scenario:
  • Result:
  • Supporting evidence:

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run check:diff passed when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: 10 current PR-gate integration files passed 182 tests; two current E2E-support workflow-boundary files passed 46 tests; the parallel test:changed timeout passed alone with 31 tests; the maintainer policy contract passed 15 tests; 3 strict-checker files passed 147 tests; live fix(cli): exit shields commands cleanly on deferred shields failure #7395 validation reports allPass: true with all 43 checks green.
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Additional verification: npm run typecheck:cli, npm run check:diff, and npm run docs passed.


Signed-off-by: Prekshi Vyas prekshiv@nvidia.com

Summary by CodeRabbit

  • New Features
    • Added protected, exact-revision fork approval to authorize credentialed E2E execution.
    • Added checkout_repository to let controller-driven E2E runs check out the intended PR head repository.
  • Bug Fixes
    • PR E2E gate completion is now evidence-based, blocking pass when evidence is missing, mismatched, skipped, or pending.
    • Tightened fork/internal authorization and superseded-check cancellation behavior.
  • Documentation
    • Updated PR E2E gate and risky fork coordination guidance for the new protected approval flow.
  • Tests
    • Expanded and updated gate lifecycle, fork-approval, and boundary validation coverage.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
@prekshivyas prekshivyas self-assigned this Jul 24, 2026
@prekshivyas prekshivyas added the area: security Security controls, permissions, secrets, or hardening label Jul 24, 2026
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR replaces fork credentialed-E2E skip recording with protected exact-revision authorization, binds dispatch and checkout to the PR head repository, unifies approval workflow wiring, tightens evidence identity validation, and updates documentation and tests.

Changes

PR E2E authorization and repository binding

Layer / File(s) Summary
Controller contracts and repository-bound dispatch
tools/e2e/pr-e2e-gate.mts, test/pr-e2e-gate*.test.ts
Adds start-approved-fork, persists checkoutRepository in version 4 state, and passes the selected repository through dispatch and retry paths.
Protected authorization lifecycle
tools/e2e/pr-e2e-gate.mts, tools/e2e/pr-e2e-required.mts, test/pr-e2e-gate-fork-approval.test.ts
Replaces fork skip recording with protected fork authorization, shared internal/fork authorization handling, exact identity checks, and terminal authorization-failure classification.
Repository-aware E2E checkout validation
.github/workflows/e2e.yaml, tools/e2e/operations-workflow-boundary.mts, test/e2e/*
Adds checkout_repository, validates it against the PR head repository, authenticates controller dispatches, and applies it across E2E checkout steps.
Approval workflow orchestration
.github/workflows/pr-e2e-gate.yaml, test/pr-e2e-gate-workflow.test.ts
Replaces separate approval wiring with approve-e2e, unified approval_* outputs, dynamic protected environments, and mode-specific controller invocation.
Merge-gate evidence identity
.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts, test/skills/check-gates-*
Canonicalizes fork head repositories and verifies association-less evidence against exact head metadata, rejecting contradictory or incomplete identities.
Operational documentation and boundary coverage
.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md, test/e2e/README.md, test/e2e/docs/README.md, test/e2e/support/*
Documents exact-revision protected authorization and updates workflow-boundary assertions for repository selection, controller authentication, and evidence validation.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related issues

Possibly related PRs

Suggested labels: area: ci, area: e2e, bug-fix

Suggested reviewers: cv

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: approved E2E runs for fork PRs.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/authorize-fork-e2e-run

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit cb752a8 in the codex/authorize-fork... branch remains at 96%, unchanged from commit ee7a7eb in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit cb752a8 in the codex/authorize-fork... branch remains at 80%, unchanged from commit ee7a7eb in the main branch.

Show a code coverage summary of the most impacted files.
File main ee7a7eb codex/authorize-fork... cb752a8 +/-
src/lib/adapter.../auth-config.ts 96% 96% 0%
src/lib/inferen...lama-version.ts 100% 100% 0%
src/lib/sandbox...rce-identity.ts 87% 87% 0%
src/lib/state/m...-acquisition.ts 84% 84% 0%
src/lib/state/g...way-registry.ts 94% 95% +1%
src/lib/inferen...del-registry.ts 93% 98% +5%
src/lib/inferen...time-context.ts 87% 93% +6%

Updated July 25, 2026 11:01 UTC

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
test/pr-e2e-gate-fork-approval.test.ts (1)

646-654: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Restore the locally created fetch spy in the finally blocks.

Both new tests create vi.spyOn(globalThis, "fetch") but the finally blocks only remove the temp work dir, so a failure path leaves the global fetch replaced and relies solely on the global restoreMocks behavior. Add vi.restoreAllMocks() alongside the fs.rmSync cleanup.

Based on learnings: "tests must still explicitly restore any locally created spies/mocks—especially in try/finally blocks so they're restored on failure paths."

♻️ Proposed cleanup
     } finally {
+      vi.restoreAllMocks();
       fs.rmSync(workDir, { recursive: true, force: true });
     }

Also applies to: 693-698

🤖 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-fork-approval.test.ts` around lines 646 - 654, Update the
finally blocks in both affected tests to call vi.restoreAllMocks() alongside the
existing fs.rmSync cleanup, ensuring the globalThis.fetch spy created by
vi.spyOn is restored even when the test fails.

Source: Learnings

tools/e2e/pr-e2e-gate.mts (1)

4137-4187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider collapsing the two approved-start entrypoints into one shared helper.

startApprovedControlPlanePrGate and startApprovedForkPrGate are now line-for-line identical apart from the environment constant and the authorizationKind. A single private startApprovedPrGate(command, environment, authorizationKind) with two thin exported wrappers keeps the public surface while ensuring future hardening (e.g. an extra approval-run assertion) can't be applied to only one path.

♻️ Sketch
+async function startApprovedPrGate(
+  command: ApprovedControlPlaneDispatchCommand | ApprovedForkE2EDispatchCommand,
+  environment: typeof INTERNAL_E2E_APPROVAL_ENVIRONMENT | typeof FORK_E2E_APPROVAL_ENVIRONMENT,
+  authorizationKind: "internal-control-plane" | "fork",
+): Promise<void> {
+  // ...existing shared validation, then:
+  await startAuthorizedPrGate(
+    { ...command, maintainer: review.reviewer, reason: approvedE2EReason(review.comment) },
+    authorizationKind,
+  );
+}
+
+export async function startApprovedForkPrGate(
+  command: ApprovedForkE2EDispatchCommand,
+): Promise<void> {
+  await startApprovedPrGate(command, FORK_E2E_APPROVAL_ENVIRONMENT, "fork");
+}
🤖 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 4137 - 4187, Collapse the duplicated
validation and approval flow in startApprovedControlPlanePrGate and
startApprovedForkPrGate into one private startApprovedPrGate helper accepting
the command, environment, and authorizationKind. Keep both exported entrypoints
as thin wrappers that pass their respective constants, while preserving all
existing validation and the final startAuthorizedPrGate behavior.
tools/e2e/operations-workflow-boundary.mts (1)

138-276: 📐 Maintainability & Code Quality | 🔵 Trivial

Consider extracting the checkout-trust loop to curb growing complexity.

The additions are correct and consistent with e2e.yaml, but validatePrGateDispatch keeps accumulating unrelated concerns (input shape, env bindings, script fragments, and now a second per-step checkout/repository check). Extracting the trustedCheckout loop (Lines 235-275) into its own helper would keep this hotspot from growing further with each new controller field.

As per coding guidelines, "Keep function complexity low; tracked complexity hotspots should not be expanded unnecessarily."

🤖 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/operations-workflow-boundary.mts` around lines 138 - 276, Extract
the per-job checkout validation loop from validatePrGateDispatch into a
dedicated helper, preserving the existing trusted checkout exceptions and
checkout ref/repository checks unchanged. Invoke the helper from
validatePrGateDispatch so input, environment, script, and checkout validation
remain behaviorally identical while reducing complexity in the main function.

Source: Coding guidelines

🤖 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 @.github/workflows/pr-e2e-gate.yaml:
- Around line 400-417: Ensure the GitHub repository has both environments
referenced by approve-e2e’s
environment.name—approve-credentialed-e2e-for-internal-pr and
approve-credentialed-e2e-for-fork-pr—pre-created with required-reviewer
protection rules, so the credentialed E2E job cannot run without approval.

In `@test/e2e/README.md`:
- Around line 550-553: Update the coordination summary description in the E2E
README to list only fields actually published by the fork authorization summary:
head SHA, base SHA, selection summary, and plan hash. Remove claims that it
includes the PR number, head repository, or explicit job/target lists unless
those fields are added to the summary built by the relevant
authorization-summary code.

---

Nitpick comments:
In `@test/pr-e2e-gate-fork-approval.test.ts`:
- Around line 646-654: Update the finally blocks in both affected tests to call
vi.restoreAllMocks() alongside the existing fs.rmSync cleanup, ensuring the
globalThis.fetch spy created by vi.spyOn is restored even when the test fails.

In `@tools/e2e/operations-workflow-boundary.mts`:
- Around line 138-276: Extract the per-job checkout validation loop from
validatePrGateDispatch into a dedicated helper, preserving the existing trusted
checkout exceptions and checkout ref/repository checks unchanged. Invoke the
helper from validatePrGateDispatch so input, environment, script, and checkout
validation remain behaviorally identical while reducing complexity in the main
function.

In `@tools/e2e/pr-e2e-gate.mts`:
- Around line 4137-4187: Collapse the duplicated validation and approval flow in
startApprovedControlPlanePrGate and startApprovedForkPrGate into one private
startApprovedPrGate helper accepting the command, environment, and
authorizationKind. Keep both exported entrypoints as thin wrappers that pass
their respective constants, while preserving all existing validation and the
final startAuthorizedPrGate 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: 6bb3f3cd-174d-4e6d-b837-4dc6393933a6

📥 Commits

Reviewing files that changed from the base of the PR and between 3a94d96 and b494c80.

📒 Files selected for processing (18)
  • .agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md
  • .github/workflows/e2e.yaml
  • .github/workflows/pr-e2e-gate.yaml
  • test/e2e/README.md
  • test/e2e/docs/README.md
  • test/e2e/support/e2e-operations-workflow-boundary.test.ts
  • test/pr-e2e-gate-command.test.ts
  • test/pr-e2e-gate-fork-approval.test.ts
  • test/pr-e2e-gate-lifecycle.test.ts
  • test/pr-e2e-gate-retry-history.test.ts
  • test/pr-e2e-gate-runner-loss-retry.test.ts
  • test/pr-e2e-gate-typed-target.test.ts
  • test/pr-e2e-gate-workflow.test.ts
  • test/pr-e2e-gate.test.ts
  • test/pr-e2e-required.test.ts
  • tools/e2e/operations-workflow-boundary.mts
  • tools/e2e/pr-e2e-gate.mts
  • tools/e2e/pr-e2e-required.mts

Comment thread .github/workflows/pr-e2e-gate.yaml
Comment thread test/e2e/README.md
@prekshivyas
prekshivyas enabled auto-merge (squash) July 24, 2026 21:29
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Informational

Advisor assessment: Informational / medium confidence
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions
Status: No actionable findings remain in the canonical review ledger.

Model lanes

  • GPT-5.6 Terra (primary): Completed · medium confidence · 0 blockers · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · high confidence · 7 blockers · 8 warnings · 0 suggestions
  • Model comparison: normalized findings differ; normalized E2E selections differ; Nemotron reported 7 more blockers, 8 more warnings, the same number of suggestions.

Nemotron output stays in workflow artifacts and does not change the assessment above.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: cloud-onboard, credential-sanitization, security-posture

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
tools/e2e/pr-e2e-gate.mts (2)

3943-3948: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Do not treat a missing head repository as a fork.

pull.head.repo?.full_name !== repository evaluates to true when GitHub returns no head repository, so fork authorization can pass its shape check and only fail later when dispatch requires checkoutRepository. Reject the missing identity before deriving isFork.

🤖 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 3943 - 3948, Update the authorization
validation around isFork so a missing pull.head.repo or its full_name is
rejected before deriving fork status. Validate the head repository identity
first, then compute isFork only from a present full_name, preserving the
existing internal-control-plane and fork authorization checks.

3993-3997: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Bind approval validation to checkoutRepository, not only the generic title.

The pending state records the fork repository in its summary, but resumption checks only status, conclusion, and pendingTitle. Validate the recorded repository against the live PR before dispatch so approval cannot be reused after the PR’s head repository changes.

🤖 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 3993 - 3997, Update the pending
authorization validation around matchingChecks[0] to also verify that the
repository recorded in check.output matches the live PR’s checkoutRepository
before dispatch. Preserve the existing status, conclusion, and pendingTitle
checks, and reject the approval when the repository differs.
🤖 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.

Outside diff comments:
In `@tools/e2e/pr-e2e-gate.mts`:
- Around line 3943-3948: Update the authorization validation around isFork so a
missing pull.head.repo or its full_name is rejected before deriving fork status.
Validate the head repository identity first, then compute isFork only from a
present full_name, preserving the existing internal-control-plane and fork
authorization checks.
- Around line 3993-3997: Update the pending authorization validation around
matchingChecks[0] to also verify that the repository recorded in check.output
matches the live PR’s checkoutRepository before dispatch. Preserve the existing
status, conclusion, and pendingTitle checks, and reject the approval when the
repository differs.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 4e421e62-7393-4db0-8e66-4bbbfe31dbc7

📥 Commits

Reviewing files that changed from the base of the PR and between 4ae794c and f8d59ca.

📒 Files selected for processing (2)
  • test/pr-e2e-gate-fork-approval.test.ts
  • tools/e2e/pr-e2e-gate.mts

@prekshivyas prekshivyas added v0.0.95 v0.0.96 Release target and removed v0.0.95 labels Jul 25, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
.github/workflows/e2e.yaml (1)

5974-5974: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Keep needs out of the generated JavaScript source.

Line 5974 interpolates the complete needs object into an actions/github-script program. If an upstream output is attacker-controlled, this becomes a code-injection boundary in a step with GitHub API access and access to previously downloaded artifacts. Pass the serialized value through env and parse it at runtime instead.

Suggested fix
         env:
           EXPLICIT_ONLY_JOBS: ${{ needs.generate-matrix.outputs.explicit_only_jobs }}
           JOBS: ${{ inputs.jobs }}
           RUNTIME_ARTIFACTS: ${{ runner.temp }}/e2e-runtime-audit
           RUNTIME_SUMMARY_FILE: ${{ runner.temp }}/e2e-runtime-summary.json
           TARGETS: ${{ inputs.targets }}
+          NEEDS_JSON: ${{ toJSON(needs) }}
...
-            const needs = ${{ toJSON(needs) }};
+            const needs = JSON.parse(process.env.NEEDS_JSON || '{}');

As per path instructions, untrusted values must be passed as data rather than interpolated into automation code.

🤖 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 @.github/workflows/e2e.yaml at line 5974, Remove direct needs interpolation
from the generated JavaScript in the actions/github-script step. Pass the
serialized needs value through the step’s env configuration, then parse that
environment variable at runtime inside the script while preserving the existing
needs data behavior.

Sources: Path instructions, Linters/SAST tools

🤖 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.

Outside diff comments:
In @.github/workflows/e2e.yaml:
- Line 5974: Remove direct needs interpolation from the generated JavaScript in
the actions/github-script step. Pass the serialized needs value through the
step’s env configuration, then parse that environment variable at runtime inside
the script while preserving the existing needs data behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e0a9ead6-5b3d-4a7b-9c96-4aea9cccec87

📥 Commits

Reviewing files that changed from the base of the PR and between f8d59ca and 5fda2f5.

📒 Files selected for processing (4)
  • .github/workflows/e2e.yaml
  • test/e2e/README.md
  • test/e2e/support/e2e-operations-workflow-boundary.test.ts
  • tools/e2e/operations-workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (3)
  • tools/e2e/operations-workflow-boundary.mts
  • test/e2e/support/e2e-operations-workflow-boundary.test.ts
  • test/e2e/README.md

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 @.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts:
- Around line 972-1017: Refactor runIdentityEvidence to consolidate the
identical classification logic currently repeated for installer-hash-check.yaml
and pr.yaml. After validating the pull_request event and allowing only those two
workflow paths, apply the shared immutablePrDiff, exactDiff, headShaMatches, and
headBinding checks once, preserving the existing current, other, and unknown
results.
🪄 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: 0537d833-6144-42bb-8487-4f6e6cb6f624

📥 Commits

Reviewing files that changed from the base of the PR and between 5fda2f5 and 70dbdac.

📒 Files selected for processing (4)
  • .agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md
  • .agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts
  • test/skills/check-gates-fork-evidence.test.ts
  • test/skills/check-gates-test-fixtures.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • .agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md

Comment thread .agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts Outdated
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

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/e2e/support/e2e-operations-workflow-boundary.test.ts`:
- Around line 187-210: Make the negative-path subprocess in the
authentication-ordering test hermetic by stubbing curl in the spawned shell
environment, matching the existing success-path test behavior. Ensure any
attempted curl invocation fails immediately, preventing real controller API
requests while preserving the current test assertions.
🪄 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: 29c4f23c-948f-4feb-a920-f392d64cbfc5

📥 Commits

Reviewing files that changed from the base of the PR and between 5fda2f5 and dad7515.

📒 Files selected for processing (15)
  • .agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md
  • .agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts
  • .github/workflows/e2e.yaml
  • test/e2e/README.md
  • test/e2e/docs/README.md
  • test/e2e/support/e2e-operations-workflow-boundary.test.ts
  • test/e2e/support/sandbox-operations-workflow-boundary.test.ts
  • test/pr-e2e-gate-fork-approval.test.ts
  • test/pr-e2e-gate-internal-approval.test.ts
  • test/pr-e2e-gate-typed-target.test.ts
  • test/pr-e2e-gate.test.ts
  • test/skills/check-gates-fork-evidence.test.ts
  • test/skills/check-gates-test-fixtures.ts
  • tools/e2e/operations-workflow-boundary.mts
  • tools/e2e/pr-e2e-gate.mts
🚧 Files skipped from review as they are similar to previous changes (12)
  • test/e2e/docs/README.md
  • test/pr-e2e-gate-typed-target.test.ts
  • test/skills/check-gates-fork-evidence.test.ts
  • test/skills/check-gates-test-fixtures.ts
  • tools/e2e/operations-workflow-boundary.mts
  • test/pr-e2e-gate.test.ts
  • .github/workflows/e2e.yaml
  • .agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts
  • test/e2e/README.md
  • .agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md
  • test/pr-e2e-gate-fork-approval.test.ts
  • tools/e2e/pr-e2e-gate.mts

Comment on lines +187 to +210
const result = spawnSync(
"bash",
["--noprofile", "--norc", "-e", "-o", "pipefail", "-c", authentication.run!],
{
encoding: "utf8",
env: {
...process.env,
ACTOR: "maintainer",
BASE_SHA: "b".repeat(40),
CHECKOUT_REPOSITORY: "contributor/NemoClaw",
CHECKOUT_SHA: "a".repeat(40),
CONTROLLER_CHECK_ID: "17",
CORRELATION_ID: "123e4567-e89b-42d3-a456-426614174000",
GITHUB_REPOSITORY: "NVIDIA/NemoClaw",
GITHUB_TOKEN: "unused",
JOBS: "credential-sanitization",
PLAN_HASH: "c".repeat(64),
PR_NUMBER: "42",
RUN_ATTEMPT: "1",
RUN_ID: "23",
TARGETS: "",
},
},
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Make the negative-path subprocess hermetic.

This invocation does not stub curl; an authentication-ordering regression could turn the test into a real controller API request. Override curl to fail if reached, as the success-path test already does.

Proposed fix
       [
         "--noprofile",
         "--norc",
         "-e",
         "-o",
         "pipefail",
         "-c",
-        authentication.run!,
+        `curl() { printf '%s\n' "unexpected curl" >&2; return 99; }\n${authentication.run!}`,
       ],

As per coding guidelines, “Unit tests must mock external dependencies and must not call real NVIDIA APIs.”

📝 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.

Suggested change
const result = spawnSync(
"bash",
["--noprofile", "--norc", "-e", "-o", "pipefail", "-c", authentication.run!],
{
encoding: "utf8",
env: {
...process.env,
ACTOR: "maintainer",
BASE_SHA: "b".repeat(40),
CHECKOUT_REPOSITORY: "contributor/NemoClaw",
CHECKOUT_SHA: "a".repeat(40),
CONTROLLER_CHECK_ID: "17",
CORRELATION_ID: "123e4567-e89b-42d3-a456-426614174000",
GITHUB_REPOSITORY: "NVIDIA/NemoClaw",
GITHUB_TOKEN: "unused",
JOBS: "credential-sanitization",
PLAN_HASH: "c".repeat(64),
PR_NUMBER: "42",
RUN_ATTEMPT: "1",
RUN_ID: "23",
TARGETS: "",
},
},
);
const result = spawnSync(
"bash",
["--noprofile", "--norc", "-e", "-o", "pipefail", "-c", `curl() { printf '%s\n' "unexpected curl" >&2; return 99; }\n${authentication.run!}`],
{
encoding: "utf8",
env: {
...process.env,
ACTOR: "maintainer",
BASE_SHA: "b".repeat(40),
CHECKOUT_REPOSITORY: "contributor/NemoClaw",
CHECKOUT_SHA: "a".repeat(40),
CONTROLLER_CHECK_ID: "17",
CORRELATION_ID: "123e4567-e89b-42d3-a456-426614174000",
GITHUB_REPOSITORY: "NVIDIA/NemoClaw",
GITHUB_TOKEN: "unused",
JOBS: "credential-sanitization",
PLAN_HASH: "c".repeat(64),
PR_NUMBER: "42",
RUN_ATTEMPT: "1",
RUN_ID: "23",
TARGETS: "",
},
},
);
🤖 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/e2e/support/e2e-operations-workflow-boundary.test.ts` around lines 187 -
210, Make the negative-path subprocess in the authentication-ordering test
hermetic by stubbing curl in the spawned shell environment, matching the
existing success-path test behavior. Ensure any attempted curl invocation fails
immediately, preventing real controller API requests while preserving the
current test assertions.

Source: Coding guidelines

prekshivyas and others added 3 commits July 25, 2026 02:02
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
@prekshivyas
prekshivyas merged commit 5afc5bc into main Jul 25, 2026
57 of 58 checks passed
@prekshivyas
prekshivyas deleted the codex/authorize-fork-e2e-run branch July 25, 2026 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: security Security controls, permissions, secrets, or hardening v0.0.96 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants