diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 36d30b0982d..8009d65f210 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -40,6 +40,7 @@ # ── Tests ── /test/ @NVIDIA/nemoclaw-engineer +/test/e2e/mock-parity.json @NVIDIA/nemoclaw-maintainer @NVIDIA/nemoclaw-engineer # ── CI / GitHub config ── /.github/ @NVIDIA/nemoclaw-maintainer diff --git a/.github/workflows/e2e-advisor.yaml b/.github/workflows/e2e-advisor.yaml index 87d98316996..9bd6c2f8c94 100644 --- a/.github/workflows/e2e-advisor.yaml +++ b/.github/workflows/e2e-advisor.yaml @@ -6,6 +6,11 @@ name: E2E / Advisor on: pull_request: types: [opened, synchronize, reopened, ready_for_review] + # Fork PRs cannot access advisor secrets in pull_request context. This + # parallel path executes the workflow from trusted main and treats the PR + # pull ref only as inert analysis data. + pull_request_target: + types: [opened, synchronize, reopened, ready_for_review] workflow_dispatch: inputs: base_ref: @@ -47,16 +52,15 @@ permissions: # by integration" despite `issues: write`. See the comment step below and # https://github.com/orgs/community/discussions/56632. pull-requests: write - issues: write concurrency: - group: e2e-advisor-${{ github.event.pull_request.number || github.ref }} + group: e2e-advisor-${{ github.event_name }}-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true jobs: advise: name: E2E recommendation - if: ${{ github.repository == 'NVIDIA/NemoClaw' && (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == 'NVIDIA/NemoClaw') }} + if: ${{ github.repository == 'NVIDIA/NemoClaw' && (github.event_name == 'workflow_dispatch' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == 'NVIDIA/NemoClaw') || (github.event_name == 'pull_request_target' && github.event.pull_request.head.repo.full_name != 'NVIDIA/NemoClaw')) }} runs-on: ubuntu-latest timeout-minutes: 20 env: @@ -121,11 +125,12 @@ jobs: run: echo "ADVISOR_WORKDIR=$GITHUB_WORKSPACE/pr-workdir" >> "$GITHUB_ENV" - name: Prepare target PR checkout - if: ${{ github.event_name == 'workflow_dispatch' && inputs.target_repo != '' && inputs.target_pr != '' }} + if: ${{ github.event_name == 'pull_request_target' || (github.event_name == 'workflow_dispatch' && inputs.target_repo != '' && inputs.target_pr != '') }} env: - TARGET_REPO: ${{ inputs.target_repo }} - TARGET_PR: ${{ inputs.target_pr }} - TARGET_BASE: ${{ inputs.target_base }} + TARGET_REPO: ${{ github.event_name == 'pull_request_target' && github.repository || inputs.target_repo }} + TARGET_PR: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.number || inputs.target_pr }} + TARGET_BASE: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.ref || inputs.target_base }} + EXPECTED_HEAD_SHA: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.head.sha || '' }} run: | if [[ ! "$TARGET_REPO" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]]; then echo "::error::target_repo must match owner/repo with GitHub-safe characters" @@ -144,10 +149,26 @@ jobs: mkdir -p "$TARGET_DIR" git -C "$TARGET_DIR" init git -C "$TARGET_DIR" remote add target "https://github.com/${TARGET_REPO}.git" - git -C "$TARGET_DIR" fetch --no-tags target "$TARGET_BASE" + git -C "$TARGET_DIR" fetch --no-tags target "refs/heads/${TARGET_BASE}:refs/remotes/target/${TARGET_BASE}" git -C "$TARGET_DIR" fetch --no-tags target "pull/${TARGET_PR}/head:refs/remotes/target/pr-${TARGET_PR}" git -C "$TARGET_DIR" checkout --detach "refs/remotes/target/pr-${TARGET_PR}" + if [ -n "$EXPECTED_HEAD_SHA" ] && [ "$(git -C "$TARGET_DIR" rev-parse HEAD)" != "$EXPECTED_HEAD_SHA" ]; then + echo "::error::Fetched pull ref does not match the triggering PR head SHA" + exit 1 + fi echo "ADVISOR_WORKDIR=$TARGET_DIR" >> "$GITHUB_ENV" + echo "PR_NUMBER=$TARGET_PR" >> "$GITHUB_ENV" + + # The advisor reads repository files while holding its model API key. + # Remove worktree symlinks so untrusted PR data cannot redirect a read + # to runner files such as /proc/self/environ. Git diff still reads the + # committed objects, so symlink additions and changes remain visible. + - name: Remove symlinks from analysis workspace + shell: bash + run: | + while IFS= read -r -d '' link; do + rm -- "$link" + done < <(find "$ADVISOR_WORKDIR" -type l -print0) # Pinned SDK install. The version is held in PI_SDK_VERSION above so # the pin is reviewed as a code change, not silently inherited from @@ -164,8 +185,8 @@ jobs: id: analysis continue-on-error: true env: - BASE_REF: ${{ github.event_name == 'pull_request' && format('origin/{0}', github.base_ref) || (github.event_name == 'workflow_dispatch' && inputs.target_repo != '' && inputs.target_pr != '' && format('target/{0}', inputs.target_base) || inputs.base_ref) }} - HEAD_REF: ${{ github.event_name == 'pull_request' && 'HEAD' || (github.event_name == 'workflow_dispatch' && inputs.target_repo != '' && inputs.target_pr != '' && 'HEAD' || inputs.head_ref) }} + BASE_REF: ${{ github.event_name == 'pull_request' && format('origin/{0}', github.base_ref) || (github.event_name == 'pull_request_target' && format('target/{0}', github.event.pull_request.base.ref) || (github.event_name == 'workflow_dispatch' && inputs.target_repo != '' && inputs.target_pr != '' && format('target/{0}', inputs.target_base) || inputs.base_ref)) }} + HEAD_REF: ${{ (github.event_name == 'pull_request' || github.event_name == 'pull_request_target') && 'HEAD' || (github.event_name == 'workflow_dispatch' && inputs.target_repo != '' && inputs.target_pr != '' && 'HEAD' || inputs.head_ref) }} E2E_ADVISOR_RUN_ANALYSIS: ${{ github.event_name == 'workflow_dispatch' && inputs.run_analysis == false && '0' || '1' }} # Preferred E2E advisor secret. E2E_ADVISOR_API_KEY: ${{ secrets.PI_E2E_ADVISOR_API_KEY }} @@ -181,8 +202,8 @@ jobs: id: target-analysis continue-on-error: true env: - BASE_REF: ${{ github.event_name == 'pull_request' && format('origin/{0}', github.base_ref) || (github.event_name == 'workflow_dispatch' && inputs.target_repo != '' && inputs.target_pr != '' && format('target/{0}', inputs.target_base) || inputs.base_ref) }} - HEAD_REF: ${{ github.event_name == 'pull_request' && 'HEAD' || (github.event_name == 'workflow_dispatch' && inputs.target_repo != '' && inputs.target_pr != '' && 'HEAD' || inputs.head_ref) }} + BASE_REF: ${{ github.event_name == 'pull_request' && format('origin/{0}', github.base_ref) || (github.event_name == 'pull_request_target' && format('target/{0}', github.event.pull_request.base.ref) || (github.event_name == 'workflow_dispatch' && inputs.target_repo != '' && inputs.target_pr != '' && format('target/{0}', inputs.target_base) || inputs.base_ref)) }} + HEAD_REF: ${{ (github.event_name == 'pull_request' || github.event_name == 'pull_request_target') && 'HEAD' || (github.event_name == 'workflow_dispatch' && inputs.target_repo != '' && inputs.target_pr != '' && 'HEAD' || inputs.head_ref) }} E2E_TARGET_ADVISOR_RUN_ANALYSIS: ${{ github.event_name == 'workflow_dispatch' && inputs.run_analysis == false && '0' || '1' }} # Reuse the shared E2E advisor secret. The target advisor is a # separate prompt/agent but uses the same model and credential. @@ -208,7 +229,7 @@ jobs: fi - name: Post E2E advisor PR comment - if: ${{ always() && github.event_name == 'pull_request' }} + if: ${{ always() && (github.event_name == 'pull_request' || github.event_name == 'pull_request_target') }} continue-on-error: true env: GH_TOKEN: ${{ secrets.E2E_ADVISOR_GITHUB_TOKEN || github.token }} @@ -229,7 +250,7 @@ jobs: fi - name: Post E2E target advisor PR comment - if: ${{ always() && github.event_name == 'pull_request' }} + if: ${{ always() && (github.event_name == 'pull_request' || github.event_name == 'pull_request_target') }} continue-on-error: true env: GH_TOKEN: ${{ secrets.E2E_ADVISOR_GITHUB_TOKEN || github.token }} diff --git a/.github/workflows/main.yaml b/.github/workflows/main.yaml index d2b65b414fb..4e1de9d347f 100644 --- a/.github/workflows/main.yaml +++ b/.github/workflows/main.yaml @@ -158,6 +158,7 @@ jobs: - name: Checkout uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 with: + fetch-depth: 0 persist-credentials: false - name: Setup Node.js @@ -169,6 +170,16 @@ jobs: - name: Install E2E support dependencies run: npm ci --ignore-scripts + - name: Validate changed live E2E mock parity + env: + BASE_SHA: ${{ github.event.before }} + run: | + if [ "$BASE_SHA" = "0000000000000000000000000000000000000000" ]; then + echo "Skipping changed live E2E parity: main has no prior commit." + exit 0 + fi + npx tsx scripts/checks/e2e-mock-parity.ts --base "$BASE_SHA" --head HEAD + - name: Build CLI artifacts for E2E support run: npm run build:cli diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index 9e7c9599bc9..19432af7882 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -317,6 +317,7 @@ jobs: - name: Checkout uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 with: + fetch-depth: 0 persist-credentials: false - name: Setup Node.js @@ -328,6 +329,12 @@ jobs: - name: Install E2E support dependencies run: npm ci --ignore-scripts + - name: Validate changed live E2E mock parity + # The pull_request payload can retain an older base SHA while GitHub's + # checked-out merge ref already targets newer main. Diff the merge + # parents so base-only changes are never attributed to the PR. + run: npx tsx scripts/checks/e2e-mock-parity.ts --base HEAD^1 --head HEAD^2 + - name: Build CLI artifacts for E2E support run: npm run build:cli diff --git a/scripts/checks/e2e-mock-parity.ts b/scripts/checks/e2e-mock-parity.ts new file mode 100644 index 00000000000..febaa1d1a51 --- /dev/null +++ b/scripts/checks/e2e-mock-parity.ts @@ -0,0 +1,146 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { execFileSync } from "node:child_process"; +import fs from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "../.."); +export const DEFAULT_PARITY_MANIFEST = "test/e2e/mock-parity.json"; + +export type MockParityEntry = { + live: string; + fast?: string[]; + liveOnlyReason?: string; +}; + +export type MockParityManifest = { + version: 1; + entries: MockParityEntry[]; +}; + +const LIVE_TEST = /^test\/e2e\/live\/.+\.test\.ts$/u; +const FAST_TESTS = [ + /^src\/.+\.test\.ts$/u, + /^nemoclaw\/src\/.+\.test\.ts$/u, + /^test\/e2e\/support\/.+\.test\.ts$/u, + /^test\/(?!e2e\/|package-contract\/).+\.test\.(?:js|ts)$/u, +] as const; + +function isSafeRepoPath(file: string): boolean { + return ( + file.length > 0 && + !path.posix.isAbsolute(file) && + !file.includes("\\") && + !file.split("/").includes("..") + ); +} + +function isFastPrTest(file: string): boolean { + return isSafeRepoPath(file) && FAST_TESTS.some((pattern) => pattern.test(file)); +} + +export function validateMockParity(options: { + manifest: MockParityManifest; + changedFiles: readonly string[]; + fileExists?: (file: string) => boolean; +}): string[] { + const { + manifest, + changedFiles, + fileExists = (file) => fs.existsSync(path.join(REPO_ROOT, file)), + } = options; + const errors: string[] = []; + + if (manifest.version !== 1 || !Array.isArray(manifest.entries)) { + return ["mock parity manifest must have version 1 and an entries array"]; + } + + const entries = new Map(); + for (const entry of manifest.entries) { + if (!entry || typeof entry !== "object" || typeof entry.live !== "string") { + errors.push("mock parity entries must be objects with a live path"); + continue; + } + if (!isSafeRepoPath(entry.live) || !LIVE_TEST.test(entry.live)) { + errors.push(`${entry.live}: live path must be a test/e2e/live/**/*.test.ts file`); + continue; + } + if (entries.has(entry.live)) { + errors.push(`${entry.live}: duplicate mock parity entry`); + continue; + } + entries.set(entry.live, entry); + + if ( + entry.fast !== undefined && + (!Array.isArray(entry.fast) || entry.fast.some((file) => typeof file !== "string")) + ) { + errors.push(`${entry.live}: fast must be an array of test paths`); + continue; + } + if (entry.liveOnlyReason !== undefined && typeof entry.liveOnlyReason !== "string") { + errors.push(`${entry.live}: liveOnlyReason must be a string`); + continue; + } + const fast = entry.fast ?? []; + const liveOnlyReason = entry.liveOnlyReason?.trim() ?? ""; + if (fast.length > 0 && liveOnlyReason) { + errors.push(`${entry.live}: choose fast tests or a live-only reason, not both`); + } else if (fast.length === 0 && !liveOnlyReason) { + errors.push(`${entry.live}: map at least one fast test or provide a live-only reason`); + } + + if (!fileExists(entry.live)) errors.push(`${entry.live}: live test does not exist`); + for (const fastFile of new Set(fast)) { + if (!isFastPrTest(fastFile)) { + errors.push(`${entry.live}: ${fastFile} is not collected by a fast PR test project`); + } else if (!fileExists(fastFile)) { + errors.push(`${entry.live}: mapped fast test does not exist: ${fastFile}`); + } + } + } + + for (const liveFile of [...new Set(changedFiles)].filter((file) => LIVE_TEST.test(file))) { + if (!entries.has(liveFile)) { + errors.push(`${liveFile}: changed live E2E needs an entry in ${DEFAULT_PARITY_MANIFEST}`); + } + } + + return errors.sort(); +} + +function argument(name: string): string | undefined { + const index = process.argv.indexOf(name); + return index >= 0 ? process.argv[index + 1] : undefined; +} + +function changedFiles(base: string, head: string): string[] { + return execFileSync("git", ["diff", "--name-only", "--diff-filter=ACMR", `${base}...${head}`], { + cwd: REPO_ROOT, + encoding: "utf8", + }) + .split(/\r?\n/u) + .filter(Boolean); +} + +function main(): void { + const base = argument("--base"); + const head = argument("--head") ?? "HEAD"; + if (!base) throw new Error("usage: e2e-mock-parity.ts --base [--head ]"); + + const manifestPath = path.join(REPO_ROOT, DEFAULT_PARITY_MANIFEST); + const manifest = JSON.parse(fs.readFileSync(manifestPath, "utf8")) as MockParityManifest; + const errors = validateMockParity({ manifest, changedFiles: changedFiles(base, head) }); + if (errors.length > 0) { + console.error( + ["E2E mock/live parity check failed:", ...errors.map((error) => `- ${error}`)].join("\n"), + ); + process.exitCode = 1; + return; + } + console.log("E2E mock/live parity check passed."); +} + +if (process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url)) main(); diff --git a/test/e2e-advisor.test.ts b/test/e2e-advisor.test.ts index 6eb4b356cea..6bde4be53e5 100644 --- a/test/e2e-advisor.test.ts +++ b/test/e2e-advisor.test.ts @@ -15,10 +15,12 @@ import { buildSystemPrompt, requiresCloudOnboardE2e, } from "../tools/e2e-advisor/analyze.mts"; +import { validateE2eAdvisorEventBoundary } from "../tools/e2e-advisor/workflow-boundary.mts"; const REPO_ROOT = path.resolve(import.meta.dirname, ".."); interface WorkflowStep { + env?: Record; name?: string; run?: string; uses?: string; @@ -29,6 +31,7 @@ interface WorkflowJob { } interface Workflow { + permissions?: Record; jobs?: Record; } @@ -55,6 +58,8 @@ function prepareTargetCheckoutScript(): string { } function runPrepareTargetCheckout(env: { + EXPECTED_HEAD_SHA?: string; + FAKE_HEAD_SHA?: string; TARGET_REPO: string; TARGET_PR: string; TARGET_BASE: string; @@ -66,7 +71,7 @@ function runPrepareTargetCheckout(env: { fs.mkdirSync(binDir); fs.writeFileSync( path.join(binDir, "git"), - '#!/usr/bin/env bash\nprintf \'%s\\n\' "$*" >> "$FAKE_GIT_LOG"\n', + '#!/usr/bin/env bash\nprintf \'%s\\n\' "$*" >> "$FAKE_GIT_LOG"\nif [[ "$*" == *"rev-parse HEAD" ]]; then\n printf \'%s\\n\' "$FAKE_HEAD_SHA"\nfi\n', { mode: 0o755 }, ); const result = spawnSync("bash", ["-c", prepareTargetCheckoutScript()], { @@ -89,6 +94,17 @@ function runPrepareTargetCheckout(env: { } describe("E2E recommendation advisor prompt", () => { + it("limits the trusted advisor token to PR-comment writes", () => { + expect(readAdvisorWorkflow().permissions).toEqual({ + contents: "read", + "pull-requests": "write", + }); + }); + + it("gates privileged fork events and isolates their concurrency", () => { + expect(validateE2eAdvisorEventBoundary()).toEqual([]); + }); + it("requires cloud-onboard for timing-sensitive infrastructure changes", () => { for (const file of [ "src/lib/onboard/command.ts", @@ -250,13 +266,52 @@ describe("E2E recommendation advisor prompt", () => { expect(valid.gitCalls).toEqual([ "-C /tmp/e2e-advisor-target init", "-C /tmp/e2e-advisor-target remote add target https://github.com/NVIDIA/NemoClaw.git", - "-C /tmp/e2e-advisor-target fetch --no-tags target main", + "-C /tmp/e2e-advisor-target fetch --no-tags target refs/heads/main:refs/remotes/target/main", "-C /tmp/e2e-advisor-target fetch --no-tags target pull/5756/head:refs/remotes/target/pr-5756", "-C /tmp/e2e-advisor-target checkout --detach refs/remotes/target/pr-5756", ]); - expect(valid.githubEnv).toBe("ADVISOR_WORKDIR=/tmp/e2e-advisor-target\n"); + expect(valid.githubEnv).toBe("ADVISOR_WORKDIR=/tmp/e2e-advisor-target\nPR_NUMBER=5756\n"); } finally { valid.cleanup(); } + + const mismatchedHead = runPrepareTargetCheckout({ + EXPECTED_HEAD_SHA: "a".repeat(40), + FAKE_HEAD_SHA: "b".repeat(40), + TARGET_REPO: "NVIDIA/NemoClaw", + TARGET_PR: "5756", + TARGET_BASE: "main", + }); + try { + expect(mismatchedHead.status).toBe(1); + expect(mismatchedHead.stdout).toContain( + "Fetched pull ref does not match the triggering PR head SHA", + ); + expect(mismatchedHead.gitCalls).toContain("-C /tmp/e2e-advisor-target rev-parse HEAD"); + expect(mismatchedHead.githubEnv).toBe(""); + } finally { + mismatchedHead.cleanup(); + } + }); + + it("strips untrusted symlinks before secret-bearing advisor steps", () => { + const steps = readAdvisorWorkflow().jobs?.advise?.steps ?? []; + const removeSymlinksIndex = steps.findIndex( + (step) => step.name === "Remove symlinks from analysis workspace", + ); + expect(removeSymlinksIndex).toBeGreaterThanOrEqual(0); + + const removeSymlinks = steps[removeSymlinksIndex]; + expect(removeSymlinks?.run).toContain('find "$ADVISOR_WORKDIR" -type l -print0'); + expect(removeSymlinks?.run).toContain('rm -- "$link"'); + + const secretConsumingSteps = steps + .map((step, index) => ({ index, step })) + .filter(({ step }) => JSON.stringify(step).includes("secrets.")); + expect(secretConsumingSteps.length).toBeGreaterThan(0); + + for (const { index, step } of secretConsumingSteps) { + expect(index, step.name ?? `workflow step ${index}`).toBeGreaterThan(removeSymlinksIndex); + } }); }); diff --git a/test/e2e-mock-parity.test.ts b/test/e2e-mock-parity.test.ts new file mode 100644 index 00000000000..d02ca15f993 --- /dev/null +++ b/test/e2e-mock-parity.test.ts @@ -0,0 +1,64 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it } from "vitest"; +import { type MockParityManifest, validateMockParity } from "../scripts/checks/e2e-mock-parity"; + +const live = "test/e2e/live/example.test.ts"; +const fast = "test/e2e/support/example.test.ts"; +const exists = (file: string) => file === live || file === fast; + +function manifest(entries: MockParityManifest["entries"]): MockParityManifest { + return { version: 1, entries }; +} + +describe("changed live E2E mock parity", () => { + it("accepts a changed live E2E mapped to a fast PR test", () => { + expect( + validateMockParity({ + manifest: manifest([{ live, fast: [fast] }]), + changedFiles: [live], + fileExists: exists, + }), + ).toEqual([]); + }); + + it("rejects a changed live E2E without a parity decision", () => { + expect( + validateMockParity({ manifest: manifest([]), changedFiles: [live], fileExists: exists }), + ).toEqual([`${live}: changed live E2E needs an entry in test/e2e/mock-parity.json`]); + }); + + it("rejects mappings to missing or non-PR tests", () => { + expect( + validateMockParity({ + manifest: manifest([{ live, fast: ["test/e2e/live/not-fast.test.ts", fast] }]), + changedFiles: [live], + fileExists: (file) => file === live, + }), + ).toEqual([ + `${live}: mapped fast test does not exist: ${fast}`, + `${live}: test/e2e/live/not-fast.test.ts is not collected by a fast PR test project`, + ]); + }); + + it("accepts an explicit decision for behavior that cannot be mocked", () => { + expect( + validateMockParity({ + manifest: manifest([{ live, liveOnlyReason: "Requires public TLS and provider auth" }]), + changedFiles: [live], + fileExists: exists, + }), + ).toEqual([]); + }); + + it("reports a non-string live-only reason as a validation error", () => { + expect( + validateMockParity({ + manifest: manifest([{ live, liveOnlyReason: 42 as unknown as string }]), + changedFiles: [live], + fileExists: exists, + }), + ).toEqual([`${live}: liveOnlyReason must be a string`]); + }); +}); diff --git a/test/e2e/docs/README.md b/test/e2e/docs/README.md index 8e2893e8728..27eaceda958 100644 --- a/test/e2e/docs/README.md +++ b/test/e2e/docs/README.md @@ -84,6 +84,12 @@ boundary. Retry a full target by starting a fresh workflow run and runner. The retired `--emit-matrix` and `--plan-only` paths must not be reintroduced. +When adding or changing a live test, update `test/e2e/mock-parity.json` with +the fast PR-collected test that covers its mockable contract. If the behavior +cannot be reproduced without real infrastructure, record a concise +`liveOnlyReason` instead. The PR and `main` `e2e-support` lanes enforce this +changed-file policy without requiring an immediate backfill of untouched tests. + ## Repository Layout ```text @@ -92,6 +98,7 @@ test/e2e/ fixtures/ # Vitest fixtures, clients, redaction, artifacts, cleanup live/ # Opt-in live E2E target tests manifests/ # Product-facing NemoClawInstance desired state + mock-parity.json # Changed live-test to fast-test parity decisions registry/ # Typed registry, matrix helpers, expected states support/ # Fast fixture/support and metadata tests ``` diff --git a/test/e2e/mock-parity.json b/test/e2e/mock-parity.json new file mode 100644 index 00000000000..cb4307863ef --- /dev/null +++ b/test/e2e/mock-parity.json @@ -0,0 +1,5 @@ +{ + "$comment": "SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.\nSPDX-License-Identifier: Apache-2.0", + "version": 1, + "entries": [] +} diff --git a/test/pr-workflow-contract.test.ts b/test/pr-workflow-contract.test.ts index c4e52e803f2..04f7b474fc6 100644 --- a/test/pr-workflow-contract.test.ts +++ b/test/pr-workflow-contract.test.ts @@ -1009,11 +1009,18 @@ describe("pull request and main workflow contracts", () => { expect(vitestConfig).toContain('name: "e2e-support"'); expect(stepRuns(prWorkflow.jobs["e2e-support"])).toEqual([ "npm ci --ignore-scripts", + "npx tsx scripts/checks/e2e-mock-parity.ts --base HEAD^1 --head HEAD^2", "npm run build:cli", "npx vitest run --project e2e-support", ]); expect(stepRuns(mainWorkflow.jobs["e2e-support"])).toEqual([ "npm ci --ignore-scripts", + `if [ "$BASE_SHA" = "0000000000000000000000000000000000000000" ]; then + echo "Skipping changed live E2E parity: main has no prior commit." + exit 0 +fi +npx tsx scripts/checks/e2e-mock-parity.ts --base "$BASE_SHA" --head HEAD +`, "npm run build:cli", "npx vitest run --project e2e-support", ]); diff --git a/tools/e2e-advisor/README.md b/tools/e2e-advisor/README.md index fba49452dc3..11b6991f91f 100644 --- a/tools/e2e-advisor/README.md +++ b/tools/e2e-advisor/README.md @@ -3,9 +3,7 @@ # E2E Advisor -The E2E Advisor is an SDK-powered PR reviewer for NemoClaw E2E coverage. It runs on internal -`NVIDIA/NemoClaw` pull requests, asks the advisor model to inspect the PR diff and repository, and posts a sticky -PR comment with required/optional E2E recommendations. +The E2E Advisor is an SDK-powered PR reviewer for NemoClaw E2E coverage. It analyzes same-repository and fork pull requests, asks the advisor model to inspect the PR diff and repository, and posts a sticky PR comment with required/optional E2E recommendations. The advisor combines a small checked-in regression risk plan with model review of the PR diff and repository context. The deterministic plan establishes the minimum required jobs for known high-risk lifecycle, upgrade, agent, inference, messaging, platform, credential, and security surfaces. The model may add adjacent coverage but cannot remove that floor. The target advisor also emits canonical `gh workflow run e2e.yaml` commands that use the workflow's `targets` or `jobs` inputs. @@ -19,20 +17,23 @@ the trusted timing signal. `.github/workflows/e2e-advisor.yaml`: -1. Runs on `pull_request` and `workflow_dispatch`. -2. Skips user-fork PRs; it only analyzes PRs whose head repo is `NVIDIA/NemoClaw`. -3. Installs the pinned Pi SDK package. -4. Runs `tools/e2e-advisor/analyze.mts` and `tools/e2e-advisor/targets.mts`. -5. Writes `risk-plan.json` and advisor artifacts under `artifacts/e2e-advisor/`. -6. Posts or updates sticky PR comments marked by `` and ``. +1. Runs same-repository PRs on `pull_request`, fork PRs on `pull_request_target`, and maintainer-requested analysis on `workflow_dispatch`. +2. Checks out executable advisor code from trusted `NVIDIA/NemoClaw` `main` and treats the PR checkout as inert analysis data. +3. For `pull_request_target`, fetches the PR head into an isolated worktree and verifies it matches the triggering head SHA before exporting the analysis path. +4. Removes symlinks from the analysis worktree before any secret-bearing advisor step. The event name is part of the concurrency key so the skipped `pull_request` run cannot cancel the fork's useful `pull_request_target` run. +5. Installs the pinned Pi SDK package. +6. Runs `tools/e2e-advisor/analyze.mts` and `tools/e2e-advisor/targets.mts`. +7. Writes `risk-plan.json` and advisor artifacts under `artifacts/e2e-advisor/`. +8. Posts or updates sticky PR comments marked by `` and ``. ## Safety model - Static analysis only. - The advisor receives repo-confined `read`, `grep`, `find`, and `ls` tools plus deterministic, turn-scoped read-only context tools for metadata, changed files, risk plans, diffs, and response schemas. -- The workflow does not execute PR-provided scripts, tests, or package-manager lifecycle hooks. +- The workflow executes advisor implementation only from trusted `main`; it does not execute PR-provided scripts, tests, or package-manager lifecycle hooks. +- Fork PRs use `pull_request_target` only when the head repository differs from `NVIDIA/NemoClaw`. The triggering head SHA is bound to the fetched pull ref before analysis, and symlinks are removed from the inert worktree before the model credential is exposed. +- `pull_request` and `pull_request_target` use separate concurrency groups so parallel trigger paths cannot cancel one another. - Generated advisor credential config is written under `/tmp`, not under uploaded artifacts. -- The job is gated to internal upstream PRs only. - Target recommendations include canonical `gh workflow run` commands for `.github/workflows/e2e.yaml`, but the advisor job does not trigger those commands automatically. diff --git a/tools/e2e-advisor/workflow-boundary.mts b/tools/e2e-advisor/workflow-boundary.mts new file mode 100644 index 00000000000..2474fd102c4 --- /dev/null +++ b/tools/e2e-advisor/workflow-boundary.mts @@ -0,0 +1,58 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import fs from "node:fs"; +import path from "node:path"; + +import YAML from "yaml"; + +interface AdvisorWorkflow { + concurrency?: { group?: unknown }; + jobs?: { advise?: { if?: unknown } }; + on?: Record; +} + +const WORKFLOW_PATH = path.resolve(import.meta.dirname, "../../.github/workflows/e2e-advisor.yaml"); + +function readAdvisorWorkflow(): AdvisorWorkflow { + return YAML.parse(fs.readFileSync(WORKFLOW_PATH, "utf8")) as AdvisorWorkflow; +} + +/** + * Validates the event split that keeps fork analysis on trusted workflow code. + */ +export function validateE2eAdvisorEventBoundary( + workflow: AdvisorWorkflow = readAdvisorWorkflow(), +): string[] { + const errors: string[] = []; + const triggers = workflow.on ?? {}; + if (!Object.hasOwn(triggers, "pull_request")) { + errors.push("E2E advisor must retain the pull_request trigger for first-party PRs"); + } + if (!Object.hasOwn(triggers, "pull_request_target")) { + errors.push("E2E advisor must retain the pull_request_target trigger for fork PRs"); + } + + const condition = workflow.jobs?.advise?.if; + if (typeof condition !== "string") { + errors.push("E2E advisor job must define an event trust-boundary condition"); + } else { + for (const requiredFragment of [ + "github.repository == 'NVIDIA/NemoClaw'", + "github.event_name == 'pull_request'", + "github.event.pull_request.head.repo.full_name == 'NVIDIA/NemoClaw'", + "github.event_name == 'pull_request_target'", + "github.event.pull_request.head.repo.full_name != 'NVIDIA/NemoClaw'", + ]) { + if (!condition.includes(requiredFragment)) { + errors.push(`E2E advisor job condition is missing: ${requiredFragment}`); + } + } + } + + const concurrencyGroup = workflow.concurrency?.group; + if (typeof concurrencyGroup !== "string" || !concurrencyGroup.includes("github.event_name")) { + errors.push("E2E advisor concurrency must distinguish pull_request from pull_request_target"); + } + return errors; +}