diff --git a/.github/workflows/pr-review-advisor.yaml b/.github/workflows/pr-review-advisor.yaml index f3fc66b9ed..fd94afac02 100644 --- a/.github/workflows/pr-review-advisor.yaml +++ b/.github/workflows/pr-review-advisor.yaml @@ -87,21 +87,30 @@ jobs: TYPEBOX_VERSION: "1.1.38" # Workflow-boundary modules parse YAML before the advisor session starts. YAML_VERSION: "2.8.3" + # Embedded Pi SDK sessions use Pi's proxy-aware Undici transport. + UNDICI_VERSION: "8.5.0" # Credential-free inventory discovery executes the trusted Vitest entrypoint. VITEST_VERSION: "4.1.9" FD_FIND_VERSION: "9.0.0-1" RIPGREP_VERSION: "14.1.0-1" + OPENSHELL_GATEWAY_ENDPOINT: http://127.0.0.1:8080 + PI_IMAGE: ghcr.io/nvidia/openshell-community/sandboxes/pi@sha256:00d0c5e9e733f94f6db3eaa2ab70d4fd75bcc4aace6b13a54535cbf2dd20dfcd PR_REVIEW_ADVISOR_TIMEOUT_MS: "900000" PR_REVIEW_ADVISOR_HEARTBEAT_MS: "60000" + PR_REVIEW_ADVISOR_SANDBOX_TIMEOUT_SECONDS: "2100" PR_REVIEW_ADVISOR_MODEL: ${{ matrix.advisor.model }} PR_REVIEW_ADVISOR_ARTIFACT_DIR: ${{ matrix.advisor.artifact_dir }} + PR_REVIEW_ADVISOR_RUN_ANALYSIS: ${{ github.event_name == 'workflow_dispatch' && inputs.run_analysis == false && '0' || '1' }} PR_REVIEW_ADVISOR_COMMENT_MARKER: "" PR_REVIEW_ADVISOR_COMMENT_TITLE: PR Review Advisor PR_REVIEW_ADVISOR_COMMENT_LABEL: PR review advisor PR_REVIEW_ADVISOR_WORKFLOW_NAME: "PR Review / Advisor" PR_REVIEW_ADVISOR_LOAD_PREVIOUS_REVIEW: "false" + SANDBOX_NAME: pr-advisor-${{ github.run_id }}-${{ github.run_attempt }}-${{ matrix.advisor.id }} # Only executable code from this checkout may run in the analysis job. ADVISOR_DIR: ${{ github.workspace }}/advisor + TARGET_REPO: ${{ github.event_name == 'pull_request_target' && github.repository || inputs.target_repo || github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number || inputs.target_pr }} steps: - name: Checkout trusted advisor code (workflow revision) uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -153,13 +162,14 @@ jobs: PR_BASE_SHA: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || '' }} EXPECTED_HEAD_SHA: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.head.sha || '' }} GIT_LFS_SKIP_SMUDGE: "1" + TARGET_DIR: ${{ github.workspace }}/pr-workdir run: | node --experimental-strip-types \ "$ADVISOR_DIR/tools/pr-review-advisor/prepare-target-pr.mts" - # The advisor reads repository files while holding its model key. Remove - # worktree symlinks first so an untrusted link cannot redirect a read to - # runner state. Git still retains the link target in committed objects. + # The advisor reads repository files inside OpenShell. Remove worktree + # symlinks first so an untrusted link cannot redirect a read outside the + # uploaded repository. Git still retains the committed link target. - name: Remove symlinks from analysis workspace shell: bash run: | @@ -209,20 +219,65 @@ jobs: npm ci --ignore-scripts --no-audit --no-fund ) + # Materialize GitHub metadata before sandbox creation. This is the only + # analysis-phase step with a GitHub token, and it has no model credential. + - name: Prepare advisor sandbox inputs + env: + GH_TOKEN: ${{ github.token }} + run: node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" prepare + + - name: Install OpenShell + if: ${{ env.PR_REVIEW_ADVISOR_RUN_ANALYSIS == '1' }} + run: | + env -u GITHUB_TOKEN -u GH_TOKEN -u PR_REVIEW_ADVISOR_API_KEY \ + NEMOCLAW_NON_INTERACTIVE=1 \ + bash "$ADVISOR_DIR/scripts/install-openshell.sh" + + # The upstream model key exists only while the trusted host registers the + # provider. The sandbox uses inference.local and receives an inert key. + - name: Configure OpenShell inference + id: configure-openshell + if: ${{ env.PR_REVIEW_ADVISOR_RUN_ANALYSIS == '1' }} + continue-on-error: true + env: + OPENAI_API_KEY: ${{ secrets.PR_REVIEW_ADVISOR_API_KEY }} + run: node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" configure + + # Preserve a review artifact when the provider credential is unavailable + # or gateway configuration fails. This trusted host fallback has neither + # a GitHub token nor a model credential and never executes PR content. + - name: Write unavailable advisor artifacts + id: unavailable-analysis + if: ${{ always() && steps.configure-openshell.outcome != 'success' }} + env: + BASE_REF: ${{ github.event_name == 'pull_request_target' && 'target/base' || (github.event_name == 'workflow_dispatch' && inputs.target_repo != '' && inputs.target_pr != '' && 'target/base' || inputs.base_ref) }} + HEAD_REF: ${{ github.event_name == 'pull_request_target' && 'HEAD' || (github.event_name == 'workflow_dispatch' && inputs.target_repo != '' && inputs.target_pr != '' && 'HEAD' || inputs.head_ref) }} + PR_REVIEW_ADVISOR_UNAVAILABLE_REASON: ${{ env.PR_REVIEW_ADVISOR_RUN_ANALYSIS == '0' && 'PR_REVIEW_ADVISOR_RUN_ANALYSIS=0' || 'OpenShell inference configuration failed or the advisor credential is unavailable' }} + run: node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" unavailable + + - name: Create credential-free advisor sandbox + if: ${{ steps.configure-openshell.outcome == 'success' }} + run: node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" create + - name: Run PR review advisor id: analysis + if: ${{ steps.configure-openshell.outcome == 'success' }} continue-on-error: true env: BASE_REF: ${{ github.event_name == 'pull_request_target' && 'target/base' || (github.event_name == 'workflow_dispatch' && inputs.target_repo != '' && inputs.target_pr != '' && 'target/base' || inputs.base_ref) }} HEAD_REF: ${{ github.event_name == 'pull_request_target' && 'HEAD' || (github.event_name == 'workflow_dispatch' && inputs.target_repo != '' && inputs.target_pr != '' && 'HEAD' || inputs.head_ref) }} PR_NUMBER: ${{ github.event.pull_request.number || inputs.target_pr }} - PR_REVIEW_ADVISOR_RUN_ANALYSIS: ${{ github.event_name == 'workflow_dispatch' && inputs.run_analysis == false && '0' || '1' }} - GH_TOKEN: ${{ github.token }} - PR_REVIEW_ADVISOR_API_KEY: ${{ secrets.PR_REVIEW_ADVISOR_API_KEY }} - run: | - cd "$ADVISOR_WORKDIR" - node --experimental-strip-types \ - "$ADVISOR_DIR/tools/pr-review-advisor/run-analysis.mts" + run: node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" run + + - id: download-analysis + name: Download advisor artifacts from sandbox + if: ${{ always() && steps.configure-openshell.outcome == 'success' }} + continue-on-error: true + run: node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" download + + - name: Delete advisor sandbox + if: always() + run: node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" delete - name: Publish job summary if: always() @@ -246,11 +301,30 @@ jobs: if: always() env: ANALYSIS_OUTCOME: ${{ steps.analysis.outcome }} + ANALYSIS_REQUESTED: ${{ env.PR_REVIEW_ADVISOR_RUN_ANALYSIS }} + CONFIGURE_OUTCOME: ${{ steps.configure-openshell.outcome }} + DOWNLOAD_OUTCOME: ${{ steps.download-analysis.outcome }} + UNAVAILABLE_OUTCOME: ${{ steps.unavailable-analysis.outcome }} run: | + if [ "$CONFIGURE_OUTCOME" != "success" ]; then + if [ "$UNAVAILABLE_OUTCOME" != "success" ]; then + echo "::error::PR review advisor could not write unavailable artifacts: outcome=$UNAVAILABLE_OUTCOME" + exit 1 + fi + if [ "$ANALYSIS_REQUESTED" = "0" ]; then + exit 0 + fi + echo "::error::PR review advisor inference configuration did not complete: outcome=$CONFIGURE_OUTCOME" + exit 1 + fi if [ "$ANALYSIS_OUTCOME" != "success" ]; then echo "::error::PR review advisor analysis did not complete: outcome=$ANALYSIS_OUTCOME" exit 1 fi + if [ "$DOWNLOAD_OUTCOME" != "success" ]; then + echo "::error::PR review advisor artifacts were not downloaded: outcome=$DOWNLOAD_OUTCOME" + exit 1 + fi publish: name: Publish PR review advisor diff --git a/ci/source-shape-test-budget.json b/ci/source-shape-test-budget.json index 0a91a860de..90fea47def 100644 --- a/ci/source-shape-test-budget.json +++ b/ci/source-shape-test-budget.json @@ -491,6 +491,31 @@ "test": "rejects malformed configured matrix shard selectors", "category": "security" }, + { + "file": "test/pr-review-advisor-openshell-workflow-boundary.test.ts", + "test": "binds host-prepared GitHub context to the selected repository and pull request", + "category": "compatibility" + }, + { + "file": "test/pr-review-advisor-openshell-workflow-boundary.test.ts", + "test": "fails closed around the credential-free OpenShell filesystem and network boundary", + "category": "security" + }, + { + "file": "test/pr-review-advisor-openshell-workflow-boundary.test.ts", + "test": "keeps GitHub and upstream model credentials in separate host-only steps", + "category": "security" + }, + { + "file": "test/pr-review-advisor-openshell-workflow-boundary.test.ts", + "test": "pins trusted helper and bind sources to their checkout directories", + "category": "security" + }, + { + "file": "test/pr-review-advisor-openshell-workflow-boundary.test.ts", + "test": "preserves credential-free unavailable artifacts for manual dry runs", + "category": "compatibility" + }, { "file": "test/pr-review-advisor-workflow-boundary.test.ts", "test": "rejects deleting or weakening analysis-workspace symlink removal", diff --git a/package-lock.json b/package-lock.json index 87c1f26788..3b7137d256 100644 --- a/package-lock.json +++ b/package-lock.json @@ -40,6 +40,7 @@ "tsx": "^4.21.0", "typebox": "1.1.38", "typescript": "6.0.3", + "undici": "8.5.0", "vitest": "^4.1.9" }, "engines": { @@ -6806,6 +6807,16 @@ "node": ">=14.17" } }, + "node_modules/undici": { + "version": "8.5.0", + "resolved": "https://registry.npmjs.org/undici/-/undici-8.5.0.tgz", + "integrity": "sha512-xamtWoB1EshgjpmlXd7GGm2VfdDtw1+rD8uhry8pSNW3If6S8E0m2T2+orSKeZXEn/aPJMviCpDBA65WJt8zhg==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=22.19.0" + } + }, "node_modules/undici-types": { "version": "7.18.2", "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-7.18.2.tgz", diff --git a/package.json b/package.json index 8ce34c9151..18518ac7f7 100644 --- a/package.json +++ b/package.json @@ -135,6 +135,7 @@ "tsx": "^4.21.0", "typebox": "1.1.38", "typescript": "6.0.3", + "undici": "8.5.0", "vitest": "^4.1.9" } } diff --git a/test/advisor-http-dispatcher.test.ts b/test/advisor-http-dispatcher.test.ts new file mode 100644 index 0000000000..40a8ca81a4 --- /dev/null +++ b/test/advisor-http-dispatcher.test.ts @@ -0,0 +1,95 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { type ChildProcess, spawn } from "node:child_process"; +import { once } from "node:events"; +import http from "node:http"; +import type { AddressInfo } from "node:net"; + +import { afterEach, describe, expect, it } from "vitest"; + +const servers: http.Server[] = []; +const children: ChildProcess[] = []; + +afterEach(async () => { + for (const child of children.splice(0)) { + child.kill("SIGKILL"); + } + await Promise.all( + servers.splice(0).map( + (server) => + new Promise((resolve, reject) => { + server.closeAllConnections(); + server.close((error) => (error ? reject(error) : resolve())); + }), + ), + ); +}); + +describe("advisor HTTP dispatcher", () => { + it("routes embedded SDK fetch through the environment proxy", async () => { + const connectTargets: string[] = []; + const proxy = http.createServer(); + proxy.on("connect", (request, socket) => { + connectTargets.push(request.url ?? ""); + socket.end("HTTP/1.1 502 Bad Gateway\r\nConnection: close\r\n\r\n"); + }); + proxy.listen(0, "127.0.0.1"); + await once(proxy, "listening"); + servers.push(proxy); + const address = proxy.address() as AddressInfo; + + const moduleUrl = new URL("../tools/advisors/http-dispatcher.mts", import.meta.url).href; + const script = ` + import { getGlobalDispatcher } from "undici"; + import { configureAdvisorHttpDispatcher } from ${JSON.stringify(moduleUrl)}; + const originalFetch = globalThis.fetch; + configureAdvisorHttpDispatcher(); + if (globalThis.fetch === originalFetch) { + throw new Error("advisor transport did not install npm Undici fetch"); + } + let fetchRejected = false; + try { + await fetch("https://advisor-transport.invalid/v1"); + } catch { + fetchRejected = true; + } + if (!fetchRejected) { + throw new Error("advisor transport swallowed a failed fetch"); + } + getGlobalDispatcher().emit("error", new Error("unpaired-dispatcher-failure")); + `; + const child = spawn( + process.execPath, + ["--experimental-strip-types", "--no-warnings", "--input-type=module", "--eval", script], + { + env: { + ...process.env, + HTTP_PROXY: `http://127.0.0.1:${address.port}`, + HTTPS_PROXY: `http://127.0.0.1:${address.port}`, + ALL_PROXY: "", + NO_PROXY: "", + NODE_USE_ENV_PROXY: "", + all_proxy: "", + http_proxy: `http://127.0.0.1:${address.port}`, + https_proxy: `http://127.0.0.1:${address.port}`, + no_proxy: "", + }, + stdio: ["ignore", "pipe", "pipe"], + }, + ); + children.push(child); + const stderr: Buffer[] = []; + child.stderr?.on("data", (chunk: Buffer) => stderr.push(chunk)); + const timeout = setTimeout(() => child.kill("SIGKILL"), 3_000); + timeout.unref(); + const [exitCode] = (await once(child, "exit")) as [number | null]; + clearTimeout(timeout); + + expect(exitCode, Buffer.concat(stderr).toString("utf8")).toBe(0); + expect(connectTargets).toContain("advisor-transport.invalid:443"); + expect(Buffer.concat(stderr).toString("utf8")).toContain( + "Advisor HTTP dispatcher error: Error: unpaired-dispatcher-failure", + ); + }); +}); diff --git a/test/advisor-session-runner.test.ts b/test/advisor-session-runner.test.ts index ed4562d954..7948802440 100644 --- a/test/advisor-session-runner.test.ts +++ b/test/advisor-session-runner.test.ts @@ -196,12 +196,22 @@ const sdk = vi.hoisted(() => { }; }); +const transport = vi.hoisted(() => ({ + configure: vi.fn(), +})); + vi.mock("@earendil-works/pi-coding-agent", async (importOriginal) => ({ ...(await importOriginal()), createAgentSession: sdk.createAgentSession, })); +vi.mock("../tools/advisors/http-dispatcher.mts", () => ({ + configureAdvisorHttpDispatcher: transport.configure, +})); + import { + ADVISOR_OPENAI_COMPATIBLE_BASE_URL, + ADVISOR_OPENSHELL_INFERENCE_BASE_URL, type AdvisorPromptTurn, advisorRetrySettings, READ_ONLY_TOOLS, @@ -276,6 +286,8 @@ async function run(promptTurns: AdvisorPromptTurn[]) { afterEach(() => { delete process.env.TEST_ADVISOR_KEY; + vi.unstubAllEnvs(); + vi.clearAllMocks(); sdk.reset(); for (const dir of tempDirs.splice(0)) fs.rmSync(dir, { recursive: true, force: true }); }); @@ -294,6 +306,27 @@ describe("advisor session runner", () => { expect(advisorRetrySettings("nvidia/nvidia/nemotron-3-ultra").baseDelayMs).toBe(9_000); }); + it("configures Pi's proxy transport before an OpenShell SDK session", async () => { + vi.stubEnv("PR_REVIEW_ADVISOR_BASE_URL", ADVISOR_OPENSHELL_INFERENCE_BASE_URL); + + const result = await run([analysisTurn("only-analysis")]); + + expect(result.fatalError).toBeUndefined(); + expect(transport.configure).toHaveBeenCalledOnce(); + expect(transport.configure.mock.invocationCallOrder[0]).toBeLessThan( + sdk.createAgentSession.mock.invocationCallOrder[0] as number, + ); + }); + + it("leaves the global transport unchanged for hosted advisor inference", async () => { + vi.stubEnv("PR_REVIEW_ADVISOR_BASE_URL", ADVISOR_OPENAI_COMPATIBLE_BASE_URL); + + const result = await run([analysisTurn("only-analysis")]); + + expect(result.fatalError).toBeUndefined(); + expect(transport.configure).not.toHaveBeenCalled(); + }); + it("clears a transient provider error after the same-session retry succeeds", async () => { sdk.state.retryResponses = ["success"]; const result = await run([analysisTurn("only-analysis")]); diff --git a/test/helpers/vitest-watch-triggers.ts b/test/helpers/vitest-watch-triggers.ts index 906f69c34d..e688029710 100644 --- a/test/helpers/vitest-watch-triggers.ts +++ b/test/helpers/vitest-watch-triggers.ts @@ -102,6 +102,17 @@ export const vitestWatchTriggerPatterns: VitestWatchTriggerPattern[] = [ pattern: /(?:^|\/)\.github\/workflows\/pr-merge-conflict-fixer\.yaml$/, testsToRun: runTests("test/pr-merge-conflict-fixer-workflow-boundary.test.ts"), }, + { + pattern: /(?:^|\/)\.github\/workflows\/pr-review-advisor\.yaml$/, + testsToRun: runTests( + "test/pr-review-advisor-workflow-boundary.test.ts", + "test/pr-review-advisor-openshell-workflow-boundary.test.ts", + ), + }, + { + pattern: /(?:^|\/)tools\/pr-review-advisor\/openshell-policy\.yaml$/, + testsToRun: runTests("test/pr-review-advisor-openshell-workflow-boundary.test.ts"), + }, { pattern: /(?:^|\/)\.github\/workflows\/(?:hosted-runner-recovery|wsl-e2e|macos-e2e|platform-vitest-main)\.yaml$/, diff --git a/test/pr-merge-conflict-fixer.test.ts b/test/pr-merge-conflict-fixer.test.ts index 50896e867d..e3bf8c71a1 100644 --- a/test/pr-merge-conflict-fixer.test.ts +++ b/test/pr-merge-conflict-fixer.test.ts @@ -482,6 +482,7 @@ describe("PR merge conflict fixer", () => { expect(configuration).toContain('bind_address = "127.0.0.1:8080"'); expect(configuration).toContain("allow_unauthenticated_users = true"); expect(configuration).toContain('supervisor_bin = "/trusted/bin/openshell-sandbox"'); + expect(configuration).not.toContain("enable_bind_mounts"); expect(configuration).not.toContain("provider-secret"); expect(fs.statSync(configurationPath).mode & 0o777).toBe(0o600); diff --git a/test/pr-review-advisor-openshell-workflow-boundary.test.ts b/test/pr-review-advisor-openshell-workflow-boundary.test.ts new file mode 100644 index 0000000000..5ab6ef13a3 --- /dev/null +++ b/test/pr-review-advisor-openshell-workflow-boundary.test.ts @@ -0,0 +1,435 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import YAML from "yaml"; +import { validatePrReviewAdvisorWorkflowBoundary } from "../tools/pr-review-advisor/workflow-boundary.mts"; + +const ROOT = path.resolve(import.meta.dirname, ".."); +const WORKFLOW_PATH = path.join(ROOT, ".github/workflows/pr-review-advisor.yaml"); +const OPENSHELL_POLICY_PATH = path.join( + ROOT, + "tools", + "pr-review-advisor", + "openshell-policy.yaml", +); +function workflowSource(): string { + return fs.readFileSync(WORKFLOW_PATH, "utf8"); +} + +function mutateWorkflowSource( + source: string, + mutate: (workflow: Record) => void, +): string { + const workflow = YAML.parse(source) as Record; + mutate(workflow); + return YAML.stringify(workflow); +} + +function validateMutation(mutate: (source: string) => string): string[] { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "pr-review-advisor-boundary-")); + const workflowPath = path.join(tmp, "workflow.yaml"); + fs.writeFileSync(workflowPath, mutate(workflowSource())); + try { + return validatePrReviewAdvisorWorkflowBoundary(workflowPath); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } +} + +function validatePolicyMutation(mutate: (policy: Record) => void): string[] { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "pr-review-advisor-policy-")); + const policyPath = path.join(tmp, "openshell-policy.yaml"); + const policy = YAML.parse(fs.readFileSync(OPENSHELL_POLICY_PATH, "utf8")) as Record; + mutate(policy); + fs.writeFileSync(policyPath, YAML.stringify(policy)); + try { + return validatePrReviewAdvisorWorkflowBoundary( + WORKFLOW_PATH, + path.join(ROOT, "package-lock.json"), + policyPath, + ); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } +} + +describe("PR review advisor OpenShell workflow boundary", () => { + // source-shape-contract: compatibility -- The selected target identity must reach host metadata preparation and the credential-free sandbox + it("binds host-prepared GitHub context to the selected repository and pull request", () => { + const workflow = YAML.parse(workflowSource()) as Record; + expect(workflow.jobs.review.env.TARGET_REPO).toBe( + "${{ github.event_name == 'pull_request_target' && github.repository || inputs.target_repo || github.repository }}", + ); + expect(workflow.jobs.review.env.PR_NUMBER).toBe( + "${{ github.event.pull_request.number || inputs.target_pr }}", + ); + + const missingIdentity = validateMutation((source) => + mutateWorkflowSource(source, (mutated) => { + delete mutated.jobs.review.env.TARGET_REPO; + delete mutated.jobs.review.env.PR_NUMBER; + }), + ); + expect(missingIdentity).toEqual( + expect.arrayContaining([ + "review job env.TARGET_REPO must be ${{ github.event_name == 'pull_request_target' && github.repository || inputs.target_repo || github.repository }}", + "review job env.PR_NUMBER must be ${{ github.event.pull_request.number || inputs.target_pr }}", + ]), + ); + }); + + // source-shape-contract: security -- Executed and mounted trusted code plus the fallback PR worktree must stay on their pinned checkout paths + it("pins trusted helper and bind sources to their checkout directories", () => { + const workflow = YAML.parse(workflowSource()) as Record; + const defaultWorkdir = workflow.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Set default advisor workdir", + ); + expect(workflow.jobs.review.env.ADVISOR_DIR).toBe("${{ github.workspace }}/advisor"); + expect(workflow.jobs.publish.env.ADVISOR_DIR).toBe("${{ github.workspace }}/advisor"); + expect(defaultWorkdir).toMatchObject({ + if: "${{ github.event_name == 'workflow_dispatch' && inputs.target_repo == '' && inputs.target_pr == '' }}", + run: 'echo "ADVISOR_WORKDIR=$GITHUB_WORKSPACE/pr-workdir" >> "$GITHUB_ENV"', + }); + + const detachedSources = validateMutation((source) => + mutateWorkflowSource(source, (mutated) => { + mutated.jobs.review.env.ADVISOR_DIR = "${{ github.workspace }}/pr-workdir/advisor"; + mutated.jobs.publish.env.ADVISOR_DIR = "${{ github.workspace }}/publish-artifacts/advisor"; + const defaultWorkdir = mutated.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Set default advisor workdir", + ); + defaultWorkdir.if = "${{ always() }}"; + defaultWorkdir.run = 'echo "ADVISOR_WORKDIR=$GITHUB_WORKSPACE/advisor" >> "$GITHUB_ENV"'; + }), + ); + expect(detachedSources).toEqual( + expect.arrayContaining([ + "review job env.ADVISOR_DIR must be ${{ github.workspace }}/advisor", + "publish job env.ADVISOR_DIR must be ${{ github.workspace }}/advisor", + "Set default advisor workdir must use the canonical dispatch-only condition", + "Set default advisor workdir must bind ADVISOR_WORKDIR to the fixed pr-workdir checkout", + ]), + ); + }); + + // source-shape-contract: security -- GitHub and model credentials must remain confined to distinct trusted host steps + it("keeps GitHub and upstream model credentials in separate host-only steps", () => { + const workflow = YAML.parse(workflowSource()) as Record; + const steps = workflow.jobs.review.steps as Array>; + const prepareInputs = steps.find((step) => step.name === "Prepare advisor sandbox inputs"); + const configure = steps.find((step) => step.name === "Configure OpenShell inference"); + const install = steps.find((step) => step.name === "Install OpenShell"); + const unavailable = steps.find((step) => step.name === "Write unavailable advisor artifacts"); + expect(prepareInputs?.env).toEqual({ GH_TOKEN: "${{ github.token }}" }); + expect(configure?.id).toBe("configure-openshell"); + expect(configure?.["continue-on-error"]).toBe(true); + expect(install?.if).toBe("${{ env.PR_REVIEW_ADVISOR_RUN_ANALYSIS == '1' }}"); + expect(configure?.if).toBe("${{ env.PR_REVIEW_ADVISOR_RUN_ANALYSIS == '1' }}"); + expect(configure?.env).toEqual({ + OPENAI_API_KEY: "${{ secrets.PR_REVIEW_ADVISOR_API_KEY }}", + }); + expect(unavailable).toMatchObject({ + id: "unavailable-analysis", + if: "${{ always() && steps.configure-openshell.outcome != 'success' }}", + env: { + BASE_REF: expect.any(String), + HEAD_REF: expect.any(String), + PR_REVIEW_ADVISOR_UNAVAILABLE_REASON: + "${{ env.PR_REVIEW_ADVISOR_RUN_ANALYSIS == '0' && 'PR_REVIEW_ADVISOR_RUN_ANALYSIS=0' || 'OpenShell inference configuration failed or the advisor credential is unavailable' }}", + }, + run: 'node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" unavailable', + }); + for (const name of [ + "Write unavailable advisor artifacts", + "Create credential-free advisor sandbox", + "Run PR review advisor", + "Download advisor artifacts from sandbox", + "Delete advisor sandbox", + ]) { + const serialized = JSON.stringify(steps.find((step) => step.name === name)); + expect(serialized, name).not.toContain("${{ github.token }}"); + expect(serialized, name).not.toContain("${{ secrets."); + } + + const inheritedCredentials = validateMutation((source) => + mutateWorkflowSource(source, (mutated) => { + mutated.jobs.review.env.GH_TOKEN = "${{ github.token }}"; + mutated.jobs.review.env.PR_REVIEW_ADVISOR_API_KEY = + "${{ secrets.PR_REVIEW_ADVISOR_API_KEY }}"; + }), + ); + expect(inheritedCredentials).toContain( + "review job-level environment must not expose GitHub or model credentials", + ); + + const duplicateGitHubToken = validateMutation((source) => + mutateWorkflowSource(source, (mutated) => { + const create = mutated.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Create credential-free advisor sandbox", + ); + create.env = { GH_TOKEN: "${{ github.token }}" }; + }), + ); + expect(duplicateGitHubToken).toEqual( + expect.arrayContaining([ + "only advisor sandbox input preparation may receive github.token", + "step 'Create credential-free advisor sandbox' must remain credential-free after OpenShell configuration", + ]), + ); + + const indirectCredential = validateMutation((source) => + mutateWorkflowSource(source, (mutated) => { + const create = mutated.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Create credential-free advisor sandbox", + ); + create.env = { GH_TOKEN: "${{ env.FORWARDED_TOKEN }}" }; + }), + ); + expect(indirectCredential).toContain( + "step 'Create credential-free advisor sandbox' must remain credential-free after OpenShell configuration", + ); + + const bracketTokenExpression = validateMutation((source) => + mutateWorkflowSource(source, (mutated) => { + const create = mutated.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Create credential-free advisor sandbox", + ); + create.env = { FORWARDED_TOKEN: "${{ github['token'] }}" }; + }), + ); + expect(bracketTokenExpression).toEqual( + expect.arrayContaining([ + "only advisor sandbox input preparation may receive github.token", + "step 'Create credential-free advisor sandbox' must remain credential-free after OpenShell configuration", + ]), + ); + + const duplicateModelSecret = validateMutation((source) => + mutateWorkflowSource(source, (mutated) => { + const download = mutated.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Download advisor artifacts from sandbox", + ); + download.env = { + OPENAI_API_KEY: "${{ secrets.PR_REVIEW_ADVISOR_API_KEY }}", + }; + }), + ); + expect(duplicateModelSecret).toEqual( + expect.arrayContaining([ + "only OpenShell provider configuration may receive the advisor model credential", + "step 'Download advisor artifacts from sandbox' must remain credential-free after OpenShell configuration", + ]), + ); + + const combinedSecrets = validateMutation((source) => + mutateWorkflowSource(source, (mutated) => { + const configureStep = mutated.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Configure OpenShell inference", + ); + configureStep.env.GH_TOKEN = "${{ github.token }}"; + const prepareStep = mutated.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Prepare advisor sandbox inputs", + ); + prepareStep.env.OPENAI_API_KEY = "${{ secrets.PR_REVIEW_ADVISOR_API_KEY }}"; + }), + ); + expect(combinedSecrets).toEqual( + expect.arrayContaining([ + "Prepare advisor sandbox inputs must receive only github.token", + "Configure OpenShell inference must receive only secrets.PR_REVIEW_ADVISOR_API_KEY as OPENAI_API_KEY", + "only OpenShell provider configuration may receive the advisor model credential", + "only advisor sandbox input preparation may receive github.token", + ]), + ); + }); + + // source-shape-contract: compatibility -- Manual dry runs must skip inference setup while retaining an auditable unavailable artifact + it("preserves credential-free unavailable artifacts for manual dry runs", () => { + const workflow = YAML.parse(workflowSource()) as Record; + expect(workflow.jobs.review.env.PR_REVIEW_ADVISOR_RUN_ANALYSIS).toBe( + "${{ github.event_name == 'workflow_dispatch' && inputs.run_analysis == false && '0' || '1' }}", + ); + + const verify = workflow.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Verify advisor analysis outcome", + ); + expect(verify.env.ANALYSIS_REQUESTED).toBe("${{ env.PR_REVIEW_ADVISOR_RUN_ANALYSIS }}"); + expect(verify.run).toContain('if [ "$ANALYSIS_REQUESTED" = "0" ]'); + + const weakenedDryRun = validateMutation((source) => + mutateWorkflowSource(source, (mutated) => { + mutated.jobs.review.env.PR_REVIEW_ADVISOR_RUN_ANALYSIS = "1"; + const install = mutated.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Install OpenShell", + ); + const configure = mutated.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Configure OpenShell inference", + ); + const unavailable = mutated.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Write unavailable advisor artifacts", + ); + const outcome = mutated.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Verify advisor analysis outcome", + ); + delete install.if; + delete configure.if; + unavailable.if = "${{ steps.configure-openshell.outcome != 'success' }}"; + unavailable.env.PR_REVIEW_ADVISOR_UNAVAILABLE_REASON = "configuration failed"; + outcome.env.ANALYSIS_REQUESTED = "1"; + outcome.run = outcome.run.replace( + 'if [ "$ANALYSIS_REQUESTED" = "0" ]; then', + 'if [ "$ANALYSIS_REQUESTED" = "1" ]; then', + ); + }), + ); + expect(weakenedDryRun).toEqual( + expect.arrayContaining([ + "review job env.PR_REVIEW_ADVISOR_RUN_ANALYSIS must be ${{ github.event_name == 'workflow_dispatch' && inputs.run_analysis == false && '0' || '1' }}", + "Install OpenShell must run only when advisor analysis is requested", + "Configure OpenShell inference must run only when advisor analysis is requested", + "Write unavailable advisor artifacts must run after skipped or failed configuration", + "Write unavailable advisor artifacts must receive only refs and the canonical unavailable reason", + 'step \'Verify advisor analysis outcome\' run script must include if [ "$ANALYSIS_REQUESTED" = "0" ]', + "Verify advisor analysis outcome must use the trusted analysis request selector", + ]), + ); + }); + + it("pins the OpenShell image, loopback gateway, and per-lane sandbox identity", () => { + const errors = validateMutation((source) => + mutateWorkflowSource(source, (workflow) => { + workflow.jobs.review.env.OPENSHELL_GATEWAY_ENDPOINT = "http://gateway.example:8080"; + workflow.jobs.review.env.PI_IMAGE = + "ghcr.io/nvidia/openshell-community/sandboxes/pi:latest"; + workflow.jobs.review.env.SANDBOX_NAME = "pr-advisor"; + workflow.jobs.review.strategy.matrix.advisor[0].artifact_dir = "../../advisor"; + const prepare = workflow.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Prepare isolated analysis workspace", + ); + prepare.env.TARGET_DIR = "/tmp/pr-workdir"; + }), + ); + + expect(errors).toEqual( + expect.arrayContaining([ + "review job env.OPENSHELL_GATEWAY_ENDPOINT must be http://127.0.0.1:8080", + "review job env.PI_IMAGE must be ghcr.io/nvidia/openshell-community/sandboxes/pi@sha256:00d0c5e9e733f94f6db3eaa2ab70d4fd75bcc4aace6b13a54535cbf2dd20dfcd", + "review job env.SANDBOX_NAME must be pr-advisor-${{ github.run_id }}-${{ github.run_attempt }}-${{ matrix.advisor.id }}", + "advisor matrix entry 1 artifact_dir must be a simple directory name", + "Prepare isolated analysis workspace must use the fixed pr-workdir upload directory", + ]), + ); + }); + + it("downloads sandbox artifacts before upload and always deletes the sandbox", () => { + const weakenedDownload = validateMutation((source) => + mutateWorkflowSource(source, (workflow) => { + const download = workflow.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Download advisor artifacts from sandbox", + ); + download.id = "untrusted-download"; + download.if = "success()"; + download["continue-on-error"] = false; + download.run = + 'node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" run'; + }), + ); + expect(weakenedDownload).toEqual( + expect.arrayContaining([ + "Download advisor artifacts from sandbox id must be download-analysis", + "Download advisor artifacts from sandbox must run after every configured sandbox analysis", + "Download advisor artifacts from sandbox must continue-on-error until artifacts are uploaded", + "step 'Download advisor artifacts from sandbox' must use the canonical trusted OpenShell helper command", + ]), + ); + + const weakenedCleanup = validateMutation((source) => + mutateWorkflowSource(source, (workflow) => { + const cleanup = workflow.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Delete advisor sandbox", + ); + cleanup.if = "success()"; + cleanup.run = + 'node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" create'; + }), + ); + expect(weakenedCleanup).toEqual( + expect.arrayContaining([ + "Delete advisor sandbox must run always", + "step 'Delete advisor sandbox' must use the canonical trusted OpenShell helper command", + ]), + ); + + const detachedOutcome = validateMutation((source) => + mutateWorkflowSource(source, (workflow) => { + const verify = workflow.jobs.review.steps.find( + (step: { name?: string }) => step.name === "Verify advisor analysis outcome", + ); + verify.env.DOWNLOAD_OUTCOME = "${{ steps.analysis.outcome }}"; + verify.env.CONFIGURE_OUTCOME = "${{ steps.analysis.outcome }}"; + verify.env.UNAVAILABLE_OUTCOME = "${{ steps.analysis.outcome }}"; + }), + ); + expect(detachedOutcome).toEqual( + expect.arrayContaining([ + "Verify advisor analysis outcome must use the trusted sandbox download outcome", + "Verify advisor analysis outcome must use the trusted configuration step outcome", + "Verify advisor analysis outcome must use the trusted unavailable step outcome", + ]), + ); + }); + + // source-shape-contract: security -- The no-egress hard-Landlock policy must keep mounted inputs immutable and isolate runtime writes + it("fails closed around the credential-free OpenShell filesystem and network boundary", () => { + const network = validatePolicyMutation((policy) => { + policy.network_policies = { + internet: { endpoints: ["https://example.com"] }, + }; + }); + expect(network).toContain("advisor OpenShell policy must not allow direct network egress"); + + const writableTrustedInputs = validatePolicyMutation((policy) => { + policy.filesystem_policy.read_write = ["/dev", "/advisor", "/pr-workdir"]; + }); + expect(writableTrustedInputs).toEqual( + expect.arrayContaining([ + "advisor OpenShell policy must retain only its writable runtime subtree", + "advisor OpenShell policy must not grant write access to /advisor", + "advisor OpenShell policy must not grant write access to /pr-workdir", + ]), + ); + + const extraWritablePath = validatePolicyMutation((policy) => { + policy.filesystem_policy.read_write.push("/sandbox"); + }); + expect(extraWritablePath).toContain( + "advisor OpenShell policy must not grant write access to /sandbox", + ); + + const broadReadPath = validatePolicyMutation((policy) => { + policy.filesystem_policy.read_only.push("/"); + }); + expect(broadReadPath).toContain("advisor OpenShell policy must not grant read access to /"); + + const missingInputAndRuntimeOrLandlock = validatePolicyMutation((policy) => { + policy.filesystem_policy.read_only = policy.filesystem_policy.read_only.filter( + (entry: string) => entry !== "/advisor", + ); + policy.filesystem_policy.read_write = policy.filesystem_policy.read_write.filter( + (entry: string) => entry !== "/sandbox/pr-review-advisor-runtime", + ); + policy.landlock.compatibility = "best_effort"; + }); + expect(missingInputAndRuntimeOrLandlock).toEqual( + expect.arrayContaining([ + "advisor OpenShell policy must grant read-only access to /advisor", + "advisor OpenShell policy must retain only its writable runtime subtree", + "advisor OpenShell policy must fail closed when Landlock is unavailable", + ]), + ); + }); +}); diff --git a/test/pr-review-advisor-openshell.test.ts b/test/pr-review-advisor-openshell.test.ts new file mode 100644 index 0000000000..158736a225 --- /dev/null +++ b/test/pr-review-advisor-openshell.test.ts @@ -0,0 +1,741 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { execFileSync, spawnSync } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { + ADVISOR_OPENAI_COMPATIBLE_BASE_URL, + ADVISOR_OPENSHELL_INFERENCE_BASE_URL, + advisorInferenceBaseUrl, + openAiAdvisorProviderConfig, +} from "../tools/advisors/session.mts"; +import type { OpenShellTools } from "../tools/openshell-agent/runtime.mts"; +import { + collectGitHubReviewContext, + MAX_PREPARED_GITHUB_CONTEXT_BYTES, + readPreparedGitHubContext, + serializePreparedGitHubContext, +} from "../tools/pr-review-advisor/github-context.mts"; +import { + configureAdvisorOpenShellInference, + createAdvisorSandbox, + deleteAdvisorSandbox, + downloadAdvisorArtifacts, + prepareAdvisorSandboxInputs, + runAdvisorSandbox, + verifyAdvisorGitWorktree, + writeUnavailableAdvisorArtifacts, +} from "../tools/pr-review-advisor/openshell.mts"; +import { runPrReviewAdvisorAnalysis } from "../tools/pr-review-advisor/run-analysis.mts"; + +const temporaryDirectories: string[] = []; +const ROOT = path.resolve(import.meta.dirname, ".."); + +function temporaryDirectory(): string { + const directory = fs.mkdtempSync(path.join(os.tmpdir(), "pr-advisor-openshell-")); + temporaryDirectories.push(directory); + return directory; +} + +function advisorEnvironment(): NodeJS.ProcessEnv { + const root = temporaryDirectory(); + const advisorDirectory = path.join(root, "advisor"); + const workDirectory = path.join(root, "pr-workdir"); + const workspace = path.join(root, "workspace"); + const runnerTemp = path.join(root, "runner-temp"); + for (const directory of [advisorDirectory, workDirectory, workspace, runnerTemp]) { + fs.mkdirSync(directory, { recursive: true }); + } + for (const directory of [advisorDirectory, workDirectory]) { + fs.mkdirSync(path.join(directory, ".git")); + } + for (const name of ["pr-review-advisor-context", "pr-review-advisor-tools"]) { + fs.mkdirSync(path.join(runnerTemp, name)); + } + return { + ADVISOR_DIR: advisorDirectory, + ADVISOR_WORKDIR: workDirectory, + BASE_REF: "target/base", + GH_TOKEN: "github-host-secret", + GITHUB_REPOSITORY: "NVIDIA/NemoClaw", + GITHUB_TOKEN: "github-default-secret", + GITHUB_WORKSPACE: workspace, + HEAD_REF: "HEAD", + HOME: path.join(root, "home"), + OPENAI_API_KEY: "model-host-secret", + OPENSHELL_GATEWAY_ENDPOINT: "http://127.0.0.1:8080", + PATH: "/usr/bin", + PI_IMAGE: "pinned-pi-image", + PR_NUMBER: "7542", + PR_REVIEW_ADVISOR_API_KEY: "advisor-host-secret", + PR_REVIEW_ADVISOR_ARTIFACT_DIR: "pr-review-advisor", + PR_REVIEW_ADVISOR_MODEL: "nvidia/nvidia/nemotron-3-ultra", + PR_REVIEW_ADVISOR_SANDBOX_TIMEOUT_SECONDS: "2100", + RUNNER_TEMP: runnerTemp, + SANDBOX_NAME: "pr-advisor-test", + TARGET_REPO: "NVIDIA/NemoClaw", + }; +} + +function advisorTools(runImplementation?: OpenShellTools["run"]): OpenShellTools { + return { + run: vi.fn( + runImplementation ?? + ((command) => (command === "which" ? "/trusted/bin/openshell-sandbox" : "")), + ), + start: vi.fn(), + wait: vi.fn(async () => undefined), + }; +} + +afterEach(() => { + vi.restoreAllMocks(); + for (const directory of temporaryDirectories.splice(0)) { + fs.rmSync(directory, { recursive: true, force: true }); + } +}); + +describe("PR review advisor OpenShell wrapper", () => { + it("keeps credential-bearing host commands out of the Pi SDK import graph", () => { + for (const relativePath of [ + "tools/pr-review-advisor/openshell.mts", + "tools/pr-review-advisor/github-context.mts", + "tools/advisors/provider-constants.mts", + "tools/advisors/github.mts", + "tools/advisors/json.mts", + "tools/openshell-agent/runtime.mts", + ]) { + const source = fs.readFileSync(path.resolve(import.meta.dirname, "..", relativePath), "utf8"); + expect(source, relativePath).not.toMatch( + /(?:@earendil-works\/pi-coding-agent|\btypebox\b|\/session\.mts|\/analyze\.mts)/u, + ); + } + }); + + it("allows only the hosted service and OpenShell inference gateway", () => { + expect(advisorInferenceBaseUrl({})).toBe(ADVISOR_OPENAI_COMPATIBLE_BASE_URL); + expect( + advisorInferenceBaseUrl({ + PR_REVIEW_ADVISOR_BASE_URL: ADVISOR_OPENSHELL_INFERENCE_BASE_URL, + }), + ).toBe(ADVISOR_OPENSHELL_INFERENCE_BASE_URL); + expect( + ( + openAiAdvisorProviderConfig( + "PR_REVIEW_ADVISOR_API_KEY", + ADVISOR_OPENSHELL_INFERENCE_BASE_URL, + ) as { baseUrl: string } + ).baseUrl, + ).toBe(ADVISOR_OPENSHELL_INFERENCE_BASE_URL); + expect(() => + advisorInferenceBaseUrl({ + PR_REVIEW_ADVISOR_BASE_URL: "https://attacker.example/v1", + }), + ).toThrow("must use an approved advisor inference endpoint"); + }); + + it("recognizes advisor models declared in the extracted provider constants", () => { + const runNode = vi.fn( + (_script: string, _args: string[], _env: NodeJS.ProcessEnv, _cwd: string) => 0, + ); + const appendEnv = vi.fn(); + + runPrReviewAdvisorAnalysis( + { + advisorDir: ROOT, + advisorWorkdir: ROOT, + outDir: path.join(temporaryDirectory(), "artifacts"), + baseRef: "origin/main", + headRef: "HEAD", + model: "nvidia/nvidia/nemotron-3-ultra", + title: "PR Review Advisor", + runAnalysis: "0", + }, + { appendEnv, runNode }, + ); + + expect(appendEnv).toHaveBeenCalledWith("PR_REVIEW_ADVISOR_SUPPORTED", "1"); + expect(runNode).toHaveBeenCalledTimes(1); + expect(runNode.mock.calls[0]?.[2].PR_REVIEW_ADVISOR_UNAVAILABLE_REASON).toBeUndefined(); + }); + + it("loads host-prepared GitHub context without a GitHub token", async () => { + const directory = temporaryDirectory(); + const contextPath = path.join(directory, "github-context.json"); + const context = { + repo: "NVIDIA/NemoClaw", + prNumber: 7542, + pullRequest: { title: "Wrap the advisor" }, + }; + fs.writeFileSync(contextPath, JSON.stringify(context), { mode: 0o600 }); + const fetchMock = vi.spyOn(globalThis, "fetch"); + + await expect( + collectGitHubReviewContext({ + GITHUB_REPOSITORY: "NVIDIA/workflow-repository", + PR_NUMBER: String(context.prNumber), + PR_REVIEW_ADVISOR_GITHUB_CONTEXT_PATH: contextPath, + TARGET_REPO: context.repo, + }), + ).resolves.toEqual(context); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("preserves GitHub field names and marks bounded context explicitly", async () => { + const longBody = `${"head ".repeat(10_000)}binding decision at the tail`; + vi.spyOn(globalThis, "fetch").mockImplementation(async (input) => { + const url = String(input); + const payload = url.endsWith("/pulls/7542") + ? { + number: 7542, + title: "Wrap the advisor", + body: longBody, + author_association: "MEMBER", + created_at: "2026-07-26T00:00:00Z", + head: { ref: "feature", sha: "b".repeat(40), repo: { full_name: "NVIDIA/NemoClaw" } }, + base: { ref: "main", sha: "a".repeat(40), repo: { full_name: "NVIDIA/NemoClaw" } }, + } + : []; + return { + ok: true, + json: async () => payload, + } as Response; + }); + + const context = await collectGitHubReviewContext({ + GH_TOKEN: "host-token", + GITHUB_REPOSITORY: "NVIDIA/NemoClaw", + PR_NUMBER: "7542", + PR_REVIEW_ADVISOR_LOAD_PREVIOUS_REVIEW: "false", + }); + const pullRequest = context?.pullRequest as Record; + expect(pullRequest.author_association).toBe("MEMBER"); + expect(pullRequest).not.toHaveProperty("authorAssociation"); + expect((pullRequest.head as { repo: Record }).repo.full_name).toBe( + "NVIDIA/NemoClaw", + ); + expect(String(pullRequest.body)).toContain("PR Review Advisor truncated content"); + expect(String(pullRequest.body)).toContain("binding decision at the tail"); + expect(Buffer.byteLength(serializePreparedGitHubContext(context), "utf8")).toBeLessThanOrEqual( + MAX_PREPARED_GITHUB_CONTEXT_BYTES, + ); + }); + + it("bounds large overlap path sets before serializing sandbox context", async () => { + const longFiles = Array.from({ length: 300 }, (_, index) => ({ + filename: `deep/${String(index).padStart(3, "0")}/${"segment/".repeat(480)}file.ts`, + })); + const openPulls = Array.from({ length: 5 }, (_, index) => ({ + number: 8_000 + index, + title: `Overlapping PR ${index}`, + body: "", + labels: [], + })); + expect( + Buffer.byteLength(JSON.stringify(openPulls.map(() => longFiles)), "utf8"), + ).toBeGreaterThan(MAX_PREPARED_GITHUB_CONTEXT_BYTES); + + vi.spyOn(globalThis, "fetch").mockImplementation(async (input) => { + const url = String(input); + const routes: Array<{ matches: (requestUrl: string) => boolean; payload: unknown }> = [ + { + matches: (requestUrl) => requestUrl.endsWith("/pulls/7542"), + payload: { + number: 7542, + title: "Current PR", + body: "", + head: { ref: "feature", sha: "b".repeat(40) }, + base: { ref: "main", sha: "a".repeat(40) }, + }, + }, + { + matches: (requestUrl) => requestUrl.includes("/pulls?state=open"), + payload: openPulls, + }, + { + matches: (requestUrl) => requestUrl.includes("/files?"), + payload: longFiles, + }, + ]; + const payload = routes.find(({ matches }) => matches(url))?.payload ?? []; + return { + ok: true, + json: async () => payload, + } as Response; + }); + + const context = await collectGitHubReviewContext({ + GH_TOKEN: "host-token", + GITHUB_REPOSITORY: "NVIDIA/NemoClaw", + PR_NUMBER: "7542", + PR_REVIEW_ADVISOR_LOAD_PREVIOUS_REVIEW: "false", + }); + expect(context?.openPrOverlaps).toHaveLength(5); + for (const overlap of context?.openPrOverlaps ?? []) { + expect(overlap.sameFileCount).toBe(300); + expect(overlap.sameFiles).toHaveLength(20); + expect(overlap.sameFiles.every((file) => file.length <= 300)).toBe(true); + } + expect(() => serializePreparedGitHubContext(context)).not.toThrow(); + }); + + it("rejects substituted or non-regular prepared GitHub context", () => { + const directory = temporaryDirectory(); + const contextPath = path.join(directory, "github-context.json"); + const symlinkPath = path.join(directory, "github-context-link.json"); + fs.writeFileSync(contextPath, JSON.stringify({ repo: "NVIDIA/NemoClaw", prNumber: 7542 }), { + mode: 0o600, + }); + fs.symlinkSync(contextPath, symlinkPath); + + expect(() => + readPreparedGitHubContext(contextPath, { + repo: "NVIDIA/NemoClaw", + prNumber: 9999, + }), + ).toThrow("pull request does not match"); + expect(() => + readPreparedGitHubContext(contextPath, { + repo: "attacker/NemoClaw", + prNumber: 7542, + }), + ).toThrow("repository does not match"); + expect(() => readPreparedGitHubContext(symlinkPath)).toThrow("must be a regular file"); + }); + + it("bounds prepared GitHub context before parsing", () => { + const contextPath = path.join(temporaryDirectory(), "github-context.json"); + fs.writeFileSync(contextPath, Buffer.alloc(5 * 1024 * 1024 + 1, 0x20)); + + expect(() => readPreparedGitHubContext(contextPath)).toThrow("exceeds the 5 MiB limit"); + }); + + it.skipIf( + process.platform === "win32" || + typeof fs.constants.O_NONBLOCK !== "number" || + typeof fs.constants.O_NOFOLLOW !== "number", + )("rejects a prepared-context FIFO without blocking", () => { + const fifoPath = path.join(temporaryDirectory(), "github-context.json"); + const created = spawnSync("mkfifo", [fifoPath], { encoding: "utf8", timeout: 5_000 }); + expect(created.status, created.stderr).toBe(0); + + const moduleUrl = new URL("../tools/pr-review-advisor/github-context.mts", import.meta.url) + .href; + const read = spawnSync( + process.execPath, + [ + "--experimental-strip-types", + "--no-warnings", + "--input-type=module", + "--eval", + `import { readPreparedGitHubContext } from ${JSON.stringify(moduleUrl)}; readPreparedGitHubContext(${JSON.stringify(fifoPath)});`, + ], + { encoding: "utf8", timeout: 2_000 }, + ); + + expect(read.error).toBeUndefined(); + expect(read.status).not.toBe(0); + expect(read.stderr).toContain("Prepared GitHub context must be a regular file"); + }); + + it("bounds a prepared context that grows after descriptor validation", () => { + const contextPath = path.join(temporaryDirectory(), "github-context.json"); + fs.writeFileSync(contextPath, Buffer.alloc(MAX_PREPARED_GITHUB_CONTEXT_BYTES, 0x20)); + const originalFstatSync = fs.fstatSync; + vi.spyOn(fs, "fstatSync").mockImplementation((descriptor) => { + const stat = originalFstatSync(descriptor); + fs.appendFileSync(contextPath, "x"); + return stat; + }); + + expect(() => readPreparedGitHubContext(contextPath)).toThrow("exceeds the 5 MiB limit"); + }); + + it("materializes bounded host context and pinned read tools for read-only mounts", async () => { + const env = advisorEnvironment(); + env.PR_REVIEW_ADVISOR_GITHUB_CONTEXT_PATH = "/untrusted/recursive-context.json"; + const binaries = path.join(temporaryDirectory(), "binaries"); + fs.mkdirSync(binaries); + for (const name of ["rg", "fdfind"]) { + const executable = path.join(binaries, name); + fs.writeFileSync(executable, `${name}\n`, { mode: 0o755 }); + } + const collectContext = vi.fn(async (contextEnv: NodeJS.ProcessEnv) => { + expect(contextEnv.GH_TOKEN).toBe("github-host-secret"); + expect(contextEnv.PR_REVIEW_ADVISOR_GITHUB_CONTEXT_PATH).toBeUndefined(); + return { + repo: "NVIDIA/NemoClaw", + prNumber: 7542, + pullRequest: { title: "Wrap the advisor" }, + }; + }); + + await prepareAdvisorSandboxInputs(env, { + collectContext, + resolveExecutable: (name) => path.join(binaries, name), + }); + + const runnerTemp = env.RUNNER_TEMP as string; + const contextPath = path.join(runnerTemp, "pr-review-advisor-context", "github-context.json"); + const contextContent = fs.readFileSync(contextPath, "utf8"); + expect(JSON.parse(contextContent)).toMatchObject({ + repo: "NVIDIA/NemoClaw", + prNumber: 7542, + }); + expect(fs.statSync(contextPath).mode & 0o777).toBe(0o444); + expect(contextContent).not.toContain("github-host-secret"); + expect(fs.existsSync(path.join(runnerTemp, "pr-review-advisor-runtime"))).toBe(false); + for (const name of ["rg", "fdfind", "fd"]) { + const executable = path.join(runnerTemp, "pr-review-advisor-tools", name); + expect(fs.statSync(executable).mode & 0o777).toBe(0o555); + } + for (const [directory, relativeProofDirectory] of [ + [env.ADVISOR_DIR as string, ".git/.pr-review-advisor-boundary-proof"], + [env.ADVISOR_WORKDIR as string, ".git/.pr-review-advisor-boundary-proof"], + [path.join(runnerTemp, "pr-review-advisor-context"), ".pr-review-advisor-boundary-proof"], + [path.join(runnerTemp, "pr-review-advisor-tools"), ".pr-review-advisor-boundary-proof"], + ]) { + const proofDirectory = path.join(directory, relativeProofDirectory); + expect(fs.statSync(proofDirectory).isDirectory()).toBe(true); + expect(fs.statSync(proofDirectory).mode & 0o777).toBe(0o777); + for (const name of ["source", "target"]) { + expect(fs.statSync(path.join(proofDirectory, name)).mode & 0o777).toBe(0o666); + } + } + }); + + it("requires repository metadata before placing immutable-boundary proof files", async () => { + const env = advisorEnvironment(); + fs.rmSync(path.join(env.ADVISOR_WORKDIR as string, ".git"), { + recursive: true, + force: true, + }); + + await expect(prepareAdvisorSandboxInputs(env)).rejects.toThrow( + "ADVISOR_WORKDIR must contain a .git directory", + ); + }); + + it("pins the readable Git worktree explicitly across the sandbox ownership boundary", () => { + const workdir = path.join(temporaryDirectory(), "pr-workdir"); + fs.mkdirSync(workdir); + execFileSync("git", ["init", "--quiet"], { cwd: workdir }); + fs.writeFileSync(path.join(workdir, "tracked.txt"), "tracked\n"); + execFileSync("git", ["add", "tracked.txt"], { cwd: workdir }); + execFileSync( + "git", + [ + "-c", + "user.name=PR Review Advisor", + "-c", + "user.email=advisor@example.invalid", + "commit", + "--quiet", + "-m", + "test: initialize advisor worktree", + ], + { cwd: workdir }, + ); + + const emptyGitConfig = path.join(workdir, "empty-gitconfig"); + fs.writeFileSync(emptyGitConfig, ""); + const differentOwnerEnv: NodeJS.ProcessEnv = { + ...process.env, + GIT_CONFIG_GLOBAL: emptyGitConfig, + GIT_CONFIG_NOSYSTEM: "1", + GIT_TEST_ASSUME_DIFFERENT_OWNER: "1", + }; + delete differentOwnerEnv.GIT_DIR; + delete differentOwnerEnv.GIT_WORK_TREE; + expect(() => + execFileSync("git", ["rev-parse", "--is-inside-work-tree"], { + cwd: workdir, + env: differentOwnerEnv, + stdio: "pipe", + }), + ).toThrow(); + + vi.stubEnv("GIT_CONFIG_GLOBAL", emptyGitConfig); + vi.stubEnv("GIT_CONFIG_NOSYSTEM", "1"); + vi.stubEnv("GIT_TEST_ASSUME_DIFFERENT_OWNER", "1"); + try { + expect(() => verifyAdvisorGitWorktree(workdir)).not.toThrow(); + } finally { + vi.unstubAllEnvs(); + } + fs.rmSync(path.join(workdir, ".git", "HEAD")); + expect(() => verifyAdvisorGitWorktree(workdir)).toThrow( + "Advisor sandbox Git checkout is unreadable or invalid", + ); + }); + + it("rejects oversized prepared context before writing a sandbox input", async () => { + const env = advisorEnvironment(); + + await expect( + prepareAdvisorSandboxInputs(env, { + collectContext: async () => ({ + repo: "NVIDIA/NemoClaw", + prNumber: 7542, + pullRequest: { body: "x".repeat(MAX_PREPARED_GITHUB_CONTEXT_BYTES) }, + }), + }), + ).rejects.toThrow("Prepared GitHub context exceeds the 5 MiB limit"); + expect( + fs.existsSync( + path.join(env.RUNNER_TEMP as string, "pr-review-advisor-context", "github-context.json"), + ), + ).toBe(false); + }); + + it("registers the selected model while confining the upstream key to provider creation", async () => { + const env = advisorEnvironment(); + const tools = advisorTools(); + + await configureAdvisorOpenShellInference(env, tools); + + const calls = vi.mocked(tools.run).mock.calls; + expect(calls).toContainEqual([ + "openshell", + [ + "inference", + "set", + "--provider", + "advisor", + "--model", + "nvidia/nvidia/nemotron-3-ultra", + "--timeout", + "900", + ], + expect.anything(), + ]); + const providerCalls = calls.filter( + ([command, args]) => + command === "openshell" && args.slice(0, 2).join(" ") === "provider create", + ); + expect(providerCalls).toHaveLength(1); + expect(providerCalls[0]?.[2].env.OPENAI_API_KEY).toBe("model-host-secret"); + for (const [command, args, options] of calls) { + expect(options.env.GH_TOKEN, `${command} ${args.join(" ")}`).toBeUndefined(); + expect(options.env.GITHUB_TOKEN, `${command} ${args.join(" ")}`).toBeUndefined(); + expect(options.env.PR_REVIEW_ADVISOR_API_KEY, `${command} ${args.join(" ")}`).toBeUndefined(); + } + expect(calls.filter(([, , options]) => options.env.OPENAI_API_KEY)).toHaveLength(1); + expect(vi.mocked(tools.start).mock.calls[0]?.[2].env.OPENAI_API_KEY).toBeUndefined(); + const gatewayConfig = fs.readFileSync( + path.join(env.RUNNER_TEMP as string, "openshell-gateway", "gateway.toml"), + "utf8", + ); + expect(gatewayConfig).not.toContain("model-host-secret"); + expect(gatewayConfig).toContain("enable_bind_mounts = true"); + }); + + it("writes unavailable artifacts through a credential-free trusted host fallback", () => { + const env = advisorEnvironment(); + env.PR_REVIEW_ADVISOR_UNAVAILABLE_REASON = "provider configuration failed"; + const tools = advisorTools(); + + writeUnavailableAdvisorArtifacts(env, tools); + + expect(tools.run).toHaveBeenCalledTimes(1); + const [command, args, options] = vi.mocked(tools.run).mock.calls[0]!; + expect(command).toBe(process.execPath); + expect(args).toEqual([ + "--experimental-strip-types", + "--no-warnings", + path.join(env.ADVISOR_DIR as string, "tools", "pr-review-advisor", "run-analysis.mts"), + ]); + expect(options.env.PR_REVIEW_ADVISOR_RUN_ANALYSIS).toBe("0"); + expect(options.env.PR_REVIEW_ADVISOR_UNAVAILABLE_REASON).toBe("provider configuration failed"); + expect(options.env.PR_REVIEW_ADVISOR_GITHUB_CONTEXT_PATH).toBe( + path.join(env.RUNNER_TEMP as string, "pr-review-advisor-context", "github-context.json"), + ); + for (const name of [ + "GH_TOKEN", + "GITHUB_TOKEN", + "OPENAI_API_KEY", + "PR_REVIEW_ADVISOR_API_KEY", + ]) { + expect(options.env[name]).toBeUndefined(); + } + }); + + it("creates, runs, downloads, and deletes the sandbox without host credentials", () => { + const env = advisorEnvironment(); + env.GIT_DIR = "/untrusted/ambient-git-dir"; + env.GIT_WORK_TREE = "/untrusted/ambient-worktree"; + const commandResponses = new Map([["openshell sandbox list --names", "pr-advisor-test\n"]]); + const tools = advisorTools( + (command, args) => commandResponses.get(`${command} ${args.slice(0, 3).join(" ")}`) ?? "", + ); + + createAdvisorSandbox(env, tools); + runAdvisorSandbox(env, tools); + downloadAdvisorArtifacts(env, tools); + deleteAdvisorSandbox(env, tools); + + const calls = vi.mocked(tools.run).mock.calls; + const createArgs = + calls.find( + ([command, args]) => + command === "openshell" && args.slice(0, 2).join(" ") === "sandbox create", + )?.[1] ?? []; + expect(createArgs).toEqual( + expect.arrayContaining([ + "sandbox", + "create", + "--name", + "pr-advisor-test", + "--from", + "pinned-pi-image", + "--driver-config-json", + "--policy", + path.join( + fs.realpathSync(env.ADVISOR_DIR as string), + "tools", + "pr-review-advisor", + "openshell-policy.yaml", + ), + "/advisor/tools/pr-review-advisor/openshell.mts", + "initialize", + ]), + ); + const driverConfigIndex = createArgs.indexOf("--driver-config-json"); + expect(JSON.parse(createArgs[driverConfigIndex + 1] as string)).toEqual({ + docker: { + mounts: [ + { + type: "bind", + source: fs.realpathSync(env.ADVISOR_DIR as string), + target: "/advisor", + read_only: true, + }, + { + type: "bind", + source: fs.realpathSync(env.ADVISOR_WORKDIR as string), + target: "/pr-workdir", + read_only: true, + }, + { + type: "bind", + source: fs.realpathSync( + path.join(env.RUNNER_TEMP as string, "pr-review-advisor-context"), + ), + target: "/pr-review-advisor-context", + read_only: true, + }, + { + type: "bind", + source: fs.realpathSync( + path.join(env.RUNNER_TEMP as string, "pr-review-advisor-tools"), + ), + target: "/pr-review-advisor-tools", + read_only: true, + }, + { + type: "tmpfs", + target: "/sandbox/pr-review-advisor-runtime", + size_bytes: 512 * 1024 * 1024, + mode: 0o1777, + }, + ], + }, + }); + expect(createArgs).not.toContain("--upload"); + expect(createArgs).not.toContain("--no-git-ignore"); + expect(createArgs.slice(-6)).toEqual([ + "--", + "/usr/bin/node", + "--experimental-strip-types", + "--no-warnings", + "/advisor/tools/pr-review-advisor/openshell.mts", + "initialize", + ]); + expect(calls.some(([, args]) => args.slice(0, 2).join(" ") === "policy set")).toBe(false); + + const sandboxExecCalls = calls.filter( + ([command, args]) => command === "openshell" && args.slice(0, 2).join(" ") === "sandbox exec", + ); + const runArgs = + sandboxExecCalls.find(([, args]) => + args.includes("/advisor/tools/pr-review-advisor/run-analysis.mts"), + )?.[1] ?? []; + expect(runArgs).toEqual( + expect.arrayContaining([ + "sandbox", + "exec", + "--name", + "pr-advisor-test", + "--timeout", + "2100", + "--workdir", + "/pr-workdir", + "PR_REVIEW_ADVISOR_API_KEY=unused", + "PR_REVIEW_ADVISOR_BASE_URL=https://inference.local/v1", + "PR_REVIEW_ADVISOR_GITHUB_CONTEXT_PATH=/pr-review-advisor-context/github-context.json", + "GIT_DIR=/pr-workdir/.git", + "GIT_WORK_TREE=/pr-workdir", + "TARGET_REPO=NVIDIA/NemoClaw", + "/advisor/tools/pr-review-advisor/run-analysis.mts", + ]), + ); + expect(runArgs.join("\n")).not.toContain("github-host-secret"); + expect(runArgs.join("\n")).not.toContain("model-host-secret"); + expect(runArgs.join("\n")).not.toContain("advisor-host-secret"); + expect(runArgs.join("\n")).not.toContain("/untrusted/ambient"); + + expect( + calls.find( + ([command, args]) => + command === "openshell" && args.slice(0, 2).join(" ") === "sandbox download", + )?.[1], + ).toEqual([ + "sandbox", + "download", + "pr-advisor-test", + "/sandbox/pr-review-advisor-runtime/artifacts/pr-review-advisor", + path.join(env.GITHUB_WORKSPACE as string, "artifacts", "pr-review-advisor"), + ]); + expect( + fs + .statSync(path.join(env.GITHUB_WORKSPACE as string, "artifacts", "pr-review-advisor")) + .isDirectory(), + ).toBe(true); + expect( + calls.find( + ([command, args]) => + command === "openshell" && args.slice(0, 3).join(" ") === "sandbox list --names", + )?.[1], + ).toEqual(["sandbox", "list", "--names"]); + expect( + calls.find( + ([command, args]) => + command === "openshell" && args.slice(0, 2).join(" ") === "sandbox delete", + )?.[1], + ).toEqual(["sandbox", "delete", "pr-advisor-test"]); + for (const [command, args, options] of calls) { + expect(options.env.GH_TOKEN, `${command} ${args.join(" ")}`).toBeUndefined(); + expect(options.env.GITHUB_TOKEN, `${command} ${args.join(" ")}`).toBeUndefined(); + expect(options.env.OPENAI_API_KEY, `${command} ${args.join(" ")}`).toBeUndefined(); + expect(options.env.PR_REVIEW_ADVISOR_API_KEY, `${command} ${args.join(" ")}`).toBeUndefined(); + } + }); + + it("rejects artifact paths that could escape the sandbox runtime directory", () => { + const env = advisorEnvironment(); + env.PR_REVIEW_ADVISOR_ARTIFACT_DIR = "../../advisor"; + const tools = advisorTools(); + + expect(() => runAdvisorSandbox(env, tools)).toThrow( + "PR_REVIEW_ADVISOR_ARTIFACT_DIR must be a simple directory name", + ); + expect(() => downloadAdvisorArtifacts(env, tools)).toThrow( + "PR_REVIEW_ADVISOR_ARTIFACT_DIR must be a simple directory name", + ); + expect(tools.run).not.toHaveBeenCalled(); + }); +}); diff --git a/test/pr-review-advisor-prepare-target-pr.test.ts b/test/pr-review-advisor-prepare-target-pr.test.ts index 17939db437..6519e50182 100644 --- a/test/pr-review-advisor-prepare-target-pr.test.ts +++ b/test/pr-review-advisor-prepare-target-pr.test.ts @@ -10,6 +10,7 @@ import { afterEach, describe, expect, it } from "vitest"; import { PrepareTargetPrError, prepareTargetPr, + validatePrepareTargetDirectory, validatePrepareTargetPrInput, } from "../tools/pr-review-advisor/prepare-target-pr.mts"; @@ -44,7 +45,11 @@ function harness(shas: { base?: string; head?: string } = {}) { const appendEnv = (key: string, value: string): void => { env.push([key, value]); }; - return { gitCalls, env, options: { targetDir: tempDir(), runGit, appendEnv } }; + return { + gitCalls, + env, + options: { targetDir: path.join(tempDir(), "pr-workdir"), runGit, appendEnv }, + }; } describe("validatePrepareTargetPrInput", () => { @@ -72,6 +77,22 @@ describe("validatePrepareTargetPrInput", () => { }); describe("prepareTargetPr", () => { + it("accepts only a dedicated pr-workdir cleanup target", () => { + const parent = tempDir(); + const dedicated = path.join(parent, "pr-workdir"); + + expect(validatePrepareTargetDirectory(dedicated)).toBe(path.resolve(dedicated)); + for (const unsafe of ["", path.parse(dedicated).root, parent, path.join(parent, "repo")]) { + expect(() => validatePrepareTargetDirectory(unsafe)).toThrow(PrepareTargetPrError); + expect(() => validatePrepareTargetDirectory(unsafe)).toThrow(/target directory|pr-workdir/u); + } + for (const currentDirectory of [dedicated, path.join(dedicated, "checkout")]) { + expect(() => validatePrepareTargetDirectory(dedicated, currentDirectory)).toThrow( + /pr-workdir/u, + ); + } + }); + it("fetches, verifies SHAs, and exports env on the pull_request_target path", () => { const { gitCalls, env, options } = harness({ base: BASE_SHA, head: HEAD_SHA }); diff --git a/test/pr-review-advisor-workflow-boundary.test.ts b/test/pr-review-advisor-workflow-boundary.test.ts index bff2219017..57dbc0ffa4 100644 --- a/test/pr-review-advisor-workflow-boundary.test.ts +++ b/test/pr-review-advisor-workflow-boundary.test.ts @@ -334,7 +334,8 @@ function readTextBySuffix( function supportedAdvisorReadText(input: ReturnType) { return readTextBySuffix([ - ["session.mts", input.model], + ["session.mts", "provider constants import"], + ["provider-constants.mts", input.model], ["analyze.mts", "PR_REVIEW_ADVISOR_MODEL"], ["comment.mts", "PR_REVIEW_ADVISOR_COMMENT_MARKER"], ]); @@ -343,6 +344,7 @@ function supportedAdvisorReadText(input: ReturnType function missingSessionReadText() { return readTextBySuffix([ ["session.mts", new Error("missing session.mts")], + ["provider-constants.mts", new Error("missing provider-constants.mts")], ["analyze.mts", "PR_REVIEW_ADVISOR_MODEL"], ["comment.mts", "PR_REVIEW_ADVISOR_COMMENT_MARKER"], ]); @@ -495,20 +497,26 @@ describe("PR review advisor workflow boundary", () => { it("rejects executing advisor helpers from the untrusted analysis worktree", () => { const errors = validateMutation((source) => - source.replace( - '"$ADVISOR_DIR/tools/pr-review-advisor/run-analysis.mts"', - '"$ADVISOR_DIR/tools/pr-review-advisor/run-analysis.mts"\n node --experimental-strip-types "$ADVISOR_WORKDIR/tools/pr-review-advisor/run-analysis.mts"', - ), + mutateWorkflowSource(source, (workflow) => { + const step = workflow.jobs.review.steps.find( + (candidate: { name?: string }) => candidate.name === "Run PR review advisor", + ); + step.run += + '\nnode --experimental-strip-types --no-warnings "$ADVISOR_WORKDIR/tools/pr-review-advisor/openshell.mts" run'; + }), ); expect(errors).toContain( "review step 'Run PR review advisor' must not execute pr-review-advisor helpers from ADVISOR_WORKDIR", ); const bracedWorkdir = validateMutation((source) => - source.replace( - '"$ADVISOR_DIR/tools/pr-review-advisor/run-analysis.mts"', - '"${ADVISOR_WORKDIR}/tools/pr-review-advisor/run-analysis.mts"', - ), + mutateWorkflowSource(source, (workflow) => { + const step = workflow.jobs.review.steps.find( + (candidate: { name?: string }) => candidate.name === "Run PR review advisor", + ); + step.run = + 'node --experimental-strip-types --no-warnings "${ADVISOR_WORKDIR}/tools/pr-review-advisor/openshell.mts" run'; + }), ); expect(bracedWorkdir).toEqual( expect.arrayContaining([ @@ -518,10 +526,13 @@ describe("PR review advisor workflow boundary", () => { ); const relativeAfterCd = validateMutation((source) => - source.replace( - '"$ADVISOR_DIR/tools/pr-review-advisor/run-analysis.mts"', - '"tools/pr-review-advisor/run-analysis.mts"', - ), + mutateWorkflowSource(source, (workflow) => { + const step = workflow.jobs.review.steps.find( + (candidate: { name?: string }) => candidate.name === "Run PR review advisor", + ); + step.run = + "node --experimental-strip-types --no-warnings tools/pr-review-advisor/openshell.mts run"; + }), ); expect(relativeAfterCd).toEqual( expect.arrayContaining([ @@ -628,8 +639,10 @@ describe("PR review advisor workflow boundary", () => { (step) => step.name === "Remove symlinks from analysis workspace", ); const cleanup = steps.splice(cleanupIndex, 1)[0]!; - const analysisIndex = steps.findIndex((step) => step.name === "Run PR review advisor"); - steps.splice(analysisIndex + 1, 0, cleanup); + const configureIndex = steps.findIndex( + (step) => step.name === "Configure OpenShell inference", + ); + steps.splice(configureIndex + 1, 0, cleanup); }, }, { @@ -1292,6 +1305,7 @@ process.exitCode = valid ? 0 : 1;`, const errors = validateMutation((source) => source .replace(' FD_FIND_VERSION: "9.0.0-1"', ' FD_FIND_VERSION: "latest"') + .replace(' UNDICI_VERSION: "8.5.0"', ' UNDICI_VERSION: "latest"') .replace(' VITEST_VERSION: "4.1.9"', ' VITEST_VERSION: "latest"') .replace(' YAML_VERSION: "2.8.3"', ' YAML_VERSION: "latest"') .replace( @@ -1303,6 +1317,7 @@ process.exitCode = valid ? 0 : 1;`, expect(errors).toEqual( expect.arrayContaining([ "review job env.FD_FIND_VERSION must be 9.0.0-1", + "review job env.UNDICI_VERSION must be 8.5.0", "review job env.VITEST_VERSION must be 4.1.9", "review job env.YAML_VERSION must be 2.8.3", "review job env.PR_REVIEW_ADVISOR_LOAD_PREVIOUS_REVIEW must be false", @@ -1360,6 +1375,7 @@ process.exitCode = valid ? 0 : 1;`, expect.arrayContaining([ "advisor package lock must pin @earendil-works/pi-coding-agent@0.80.6", "advisor package lock must pin typebox@1.1.38", + "advisor package lock must pin undici@8.5.0", "advisor package lock must pin yaml@2.8.3", "advisor package lock must pin vitest@4.1.9", ]), diff --git a/test/vitest-watch-triggers.test.ts b/test/vitest-watch-triggers.test.ts index 8ce6f3b349..8517b474ca 100644 --- a/test/vitest-watch-triggers.test.ts +++ b/test/vitest-watch-triggers.test.ts @@ -60,6 +60,8 @@ const OPAQUE_INPUTS = [ ".github/workflows/e2e.yaml", ".github/workflows/code-scanning.yaml", ".github/workflows/pr-e2e-gate.yaml", + ".github/workflows/pr-review-advisor.yaml", + "tools/pr-review-advisor/openshell-policy.yaml", ".github/workflows/hosted-runner-recovery.yaml", ".github/workflows/wsl-e2e.yaml", ".github/workflows/macos-e2e.yaml", @@ -116,6 +118,13 @@ describe("Vitest opaque-input watch triggers", () => { "test/pr-e2e-gate-workflow.test.ts", "test/pr-e2e-required.test.ts", ]); + expect(triggeredBy(".github/workflows/pr-review-advisor.yaml")).toEqual([ + "test/pr-review-advisor-workflow-boundary.test.ts", + "test/pr-review-advisor-openshell-workflow-boundary.test.ts", + ]); + expect(triggeredBy("tools/pr-review-advisor/openshell-policy.yaml")).toEqual([ + "test/pr-review-advisor-openshell-workflow-boundary.test.ts", + ]); expect(triggeredBy(".github/workflows/hosted-runner-recovery.yaml")).toEqual([ "test/hosted-runner-recovery-workflow.test.ts", ]); diff --git a/tools/advisors/http-dispatcher.mts b/tools/advisors/http-dispatcher.mts new file mode 100644 index 0000000000..9e6b966234 --- /dev/null +++ b/tools/advisors/http-dispatcher.mts @@ -0,0 +1,68 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { EventEmitter } from "node:events"; + +import * as undici from "undici"; + +const ADVISOR_HTTP_IDLE_TIMEOUT_MS = 300_000; +const originalGlobalFetch = globalThis.fetch; +let installedGlobalFetch: typeof globalThis.fetch | undefined; + +// Undici can emit a dispatcher error while the corresponding fetch rejects normally. +// Keep that EventEmitter path from terminating the process, but report every event so +// an independent transport failure remains observable. Remove this listener when +// Undici no longer emits an uncaught dispatcher error for a rejected proxy request. +const reportUndiciDispatcherError = (error: unknown): void => { + console.error("Advisor HTTP dispatcher error:", error); +}; + +function withUndiciErrorListener(dispatcher: T): T { + if (dispatcher instanceof EventEmitter) { + EventEmitter.prototype.on.call(dispatcher, "error", reportUndiciDispatcherError); + } + return dispatcher; +} + +function createUndiciClient(origin: string | URL, options: object): undici.Dispatcher { + return withUndiciErrorListener(new undici.Client(origin, options as undici.Client.Options)); +} + +function createUndiciOriginDispatcher(origin: string | URL, options: object): undici.Dispatcher { + const dispatcherOptions = options as undici.Pool.Options; + if (dispatcherOptions.connections === 1) { + return createUndiciClient(origin, dispatcherOptions); + } + return withUndiciErrorListener( + new undici.Pool(origin, { + ...dispatcherOptions, + factory: createUndiciClient, + }), + ); +} + +/** + * Give the embedded Pi SDK the same proxy-aware fetch transport as Pi's CLI. + * OpenShell's inference.local route is reachable only through the sandbox proxy. + */ +export function configureAdvisorHttpDispatcher(): void { + const dispatcher = withUndiciErrorListener( + new undici.EnvHttpProxyAgent({ + allowH2: false, + bodyTimeout: ADVISOR_HTTP_IDLE_TIMEOUT_MS, + headersTimeout: ADVISOR_HTTP_IDLE_TIMEOUT_MS, + clientFactory: createUndiciClient, + factory: createUndiciOriginDispatcher, + }), + ); + undici.setGlobalDispatcher(dispatcher); + + const shouldInstallGlobals = + installedGlobalFetch === undefined + ? globalThis.fetch === originalGlobalFetch + : globalThis.fetch === installedGlobalFetch; + if (shouldInstallGlobals) { + undici.install?.(); + installedGlobalFetch = globalThis.fetch; + } +} diff --git a/tools/advisors/provider-constants.mts b/tools/advisors/provider-constants.mts new file mode 100644 index 0000000000..11a961401c --- /dev/null +++ b/tools/advisors/provider-constants.mts @@ -0,0 +1,8 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +export const DEFAULT_ADVISOR_PROVIDER = "openai"; +export const DEFAULT_ADVISOR_MODEL = "azure/openai/gpt-5.6-terra"; +export const NEMOTRON_ULTRA_ADVISOR_MODEL = "nvidia/nvidia/nemotron-3-ultra"; +export const ADVISOR_OPENAI_COMPATIBLE_BASE_URL = "https://inference-api.nvidia.com/v1"; +export const ADVISOR_OPENSHELL_INFERENCE_BASE_URL = "https://inference.local/v1"; diff --git a/tools/advisors/session.mts b/tools/advisors/session.mts index b870066d18..0c18384c84 100644 --- a/tools/advisors/session.mts +++ b/tools/advisors/session.mts @@ -14,6 +14,14 @@ import { SettingsManager, } from "@earendil-works/pi-coding-agent"; +import { configureAdvisorHttpDispatcher } from "./http-dispatcher.mts"; +import { + ADVISOR_OPENAI_COMPATIBLE_BASE_URL, + ADVISOR_OPENSHELL_INFERENCE_BASE_URL, + DEFAULT_ADVISOR_MODEL, + DEFAULT_ADVISOR_PROVIDER, + NEMOTRON_ULTRA_ADVISOR_MODEL, +} from "./provider-constants.mts"; import { createRepoConfinedReadOnlyTools } from "./repo-read-only-tools.mts"; import { type AdvisorContextToolResult, @@ -31,6 +39,13 @@ import { sanitizeToolName, } from "./turn-protocol.mts"; +export { + ADVISOR_OPENAI_COMPATIBLE_BASE_URL, + ADVISOR_OPENSHELL_INFERENCE_BASE_URL, + DEFAULT_ADVISOR_MODEL, + DEFAULT_ADVISOR_PROVIDER, + NEMOTRON_ULTRA_ADVISOR_MODEL, +} from "./provider-constants.mts"; export { type AdvisorContextToolContentType, type AdvisorContextToolResult, @@ -46,10 +61,7 @@ export { resolveAdvisorTurnTools, } from "./turn-protocol.mts"; -export const DEFAULT_ADVISOR_PROVIDER = "openai"; -export const DEFAULT_ADVISOR_MODEL = "azure/openai/gpt-5.6-terra"; -export const NEMOTRON_ULTRA_ADVISOR_MODEL = "nvidia/nvidia/nemotron-3-ultra"; -export const ADVISOR_OPENAI_COMPATIBLE_BASE_URL = "https://inference-api.nvidia.com/v1"; +const ADVISOR_BASE_URL_ENV = "PR_REVIEW_ADVISOR_BASE_URL"; export function advisorRetrySettings(modelId: string) { return { @@ -170,10 +182,21 @@ export async function settleAdvisorTurn(options: { return { turn, didThrow, thrown, callbackError }; } -export function openAiAdvisorProviderConfig(credentialEnv: string): AdvisorProviderConfig { +export function advisorInferenceBaseUrl(env: NodeJS.ProcessEnv = process.env): string { + const value = env[ADVISOR_BASE_URL_ENV] || ADVISOR_OPENAI_COMPATIBLE_BASE_URL; + if (![ADVISOR_OPENAI_COMPATIBLE_BASE_URL, ADVISOR_OPENSHELL_INFERENCE_BASE_URL].includes(value)) { + throw new Error(`${ADVISOR_BASE_URL_ENV} must use an approved advisor inference endpoint`); + } + return value; +} + +export function openAiAdvisorProviderConfig( + credentialEnv: string, + baseUrl = ADVISOR_OPENAI_COMPATIBLE_BASE_URL, +): AdvisorProviderConfig { return { api: "openai-completions", - baseUrl: ADVISOR_OPENAI_COMPATIBLE_BASE_URL, + baseUrl, models: [ advisorModel( DEFAULT_ADVISOR_MODEL, @@ -315,13 +338,21 @@ export async function runReadOnlyAdvisor( fs.mkdirSync(options.configDir, { recursive: true }); const provider = options.provider || DEFAULT_ADVISOR_PROVIDER; const modelId = options.modelId || DEFAULT_ADVISOR_MODEL; - const { authStorage, modelRegistry } = prepareAdvisorConfig(provider, options.credentialEnv); + const baseUrl = advisorInferenceBaseUrl(); + const { authStorage, modelRegistry } = prepareAdvisorConfig( + provider, + options.credentialEnv, + baseUrl, + ); const model = modelRegistry.find(provider, modelId); if (!model || !modelRegistry.hasConfiguredAuth(model)) { throw new Error( `Could not configure advisor model ${provider}/${modelId}; set ${options.credentialEnv}`, ); } + if (baseUrl === ADVISOR_OPENSHELL_INFERENCE_BASE_URL) { + configureAdvisorHttpDispatcher(); + } const promptTurns = normalizePromptTurns(options.promptTurns); const contextTools = createAdvisorContextToolRuntime(promptTurns); @@ -754,6 +785,7 @@ export class CappedBuffer { function prepareAdvisorConfig( provider: string, credentialEnv: string, + baseUrl: string, ): { authStorage: AuthStorage; modelRegistry: ModelRegistry } { const authStorage = AuthStorage.inMemory(); const modelRegistry = ModelRegistry.inMemory(authStorage); @@ -761,7 +793,7 @@ function prepareAdvisorConfig( if (credential) { try { authStorage.setRuntimeApiKey(provider, credential); - modelRegistry.registerProvider(provider, openAiAdvisorProviderConfig(credentialEnv)); + modelRegistry.registerProvider(provider, openAiAdvisorProviderConfig(credentialEnv, baseUrl)); } finally { delete process.env[credentialEnv]; } diff --git a/tools/openshell-agent/runtime.mts b/tools/openshell-agent/runtime.mts new file mode 100644 index 0000000000..211deac129 --- /dev/null +++ b/tools/openshell-agent/runtime.mts @@ -0,0 +1,353 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { execFileSync, spawn } from "node:child_process"; +import { closeSync, mkdirSync, openSync, writeFileSync } from "node:fs"; +import path from "node:path"; + +const HOST_CREDENTIALS = [ + "GH_TOKEN", + "GITHUB_TOKEN", + "NVIDIA_API_KEY", + "OPENAI_API_KEY", + "PR_REVIEW_ADVISOR_API_KEY", +] as const; + +export interface OpenShellCommandOptions { + capture?: boolean; + env: NodeJS.ProcessEnv; + timeout?: number; +} + +export interface OpenShellStartOptions { + env: NodeJS.ProcessEnv; + logPath: string; +} + +export interface OpenShellTools { + run: (command: string, args: readonly string[], options: OpenShellCommandOptions) => string; + start: (command: string, args: readonly string[], options: OpenShellStartOptions) => void; + wait: (milliseconds: number) => Promise; +} + +export type OpenShellInferenceOptions = { + enableBindMounts?: boolean; + gatewayId: string; + modelId: string; + providerName: string; +}; + +export type OpenShellUpload = { + source: string; + destination: string; +}; + +export type CreateOpenShellSandboxOptions = { + command: readonly string[]; + driverConfig?: Readonly>; + image: string; + name: string; + policyPath: string; + uploads: readonly OpenShellUpload[]; +}; + +export type ExecOpenShellSandboxOptions = { + command: readonly string[]; + environment?: Readonly>; + name: string; + timeoutSeconds?: number; + workdir?: string; +}; + +export class OpenShellAgentError extends Error { + constructor(message: string) { + super(message); + this.name = "OpenShellAgentError"; + } +} + +export function required(value: string | undefined, name: string): string { + if (!value) throw new OpenShellAgentError(`${name} is required`); + return value; +} + +function tomlString(value: string): string { + return JSON.stringify(value); +} + +function gatewayConfiguration(input: { + bindAddress: string; + directory: string; + enableBindMounts: boolean; + gatewayId: string; + supervisor: string; +}): string { + const bindMountConfiguration = input.enableBindMounts ? "\nenable_bind_mounts = true" : ""; + return `[openshell] +version = 1 + +[openshell.gateway] +bind_address = ${tomlString(input.bindAddress)} +compute_drivers = ["docker"] +disable_tls = true + +[openshell.gateway.auth] +allow_unauthenticated_users = true + +[openshell.gateway.gateway_jwt] +signing_key_path = ${tomlString(path.join(input.directory, "jwt", "signing.pem"))} +public_key_path = ${tomlString(path.join(input.directory, "jwt", "public.pem"))} +kid_path = ${tomlString(path.join(input.directory, "jwt", "kid"))} +gateway_id = ${tomlString(input.gatewayId)} +ttl_secs = 3600 + +[openshell.drivers.docker] +grpc_endpoint = "http://host.openshell.internal:8080" +supervisor_bin = ${tomlString(input.supervisor)}${bindMountConfiguration} +`; +} + +function loopbackBindAddress(endpoint: URL): string { + if (!["127.0.0.1", "[::1]"].includes(endpoint.hostname)) { + throw new OpenShellAgentError("OPENSHELL_GATEWAY_ENDPOINT must use a loopback address"); + } + return endpoint.host; +} + +function validateIdentifier(value: string, name: string): void { + if (!/^[A-Za-z0-9][A-Za-z0-9._/-]*$/u.test(value)) { + throw new OpenShellAgentError(`${name} contains unsupported characters`); + } +} + +export function openshellEnvironment(env: NodeJS.ProcessEnv): NodeJS.ProcessEnv { + const home = required(env.HOME, "HOME"); + const binaryDirectory = env.XDG_BIN_HOME ?? path.join(home, ".local", "bin"); + return { + ...env, + PATH: [binaryDirectory, env.PATH ?? ""].filter(Boolean).join(path.delimiter), + }; +} + +export function credentialFreeEnvironment(env: NodeJS.ProcessEnv): NodeJS.ProcessEnv { + const result = openshellEnvironment(env); + for (const name of HOST_CREDENTIALS) delete result[name]; + return result; +} + +export const defaultOpenShellTools: OpenShellTools = { + run(command, args, options): string { + const output = execFileSync(command, [...args], { + encoding: "utf8", + env: options.env, + stdio: options.capture ? ["ignore", "pipe", "inherit"] : "inherit", + timeout: options.timeout, + }); + return String(output ?? "").trim(); + }, + start(command, args, options): void { + const log = openSync(options.logPath, "w", 0o600); + try { + const child = spawn(command, [...args], { + detached: true, + env: options.env, + stdio: ["ignore", log, log], + }); + child.on("error", () => undefined); + child.unref(); + } finally { + closeSync(log); + } + }, + wait(milliseconds): Promise { + return new Promise((resolve) => setTimeout(resolve, milliseconds)); + }, +}; + +export async function configureOpenShellInference( + env: NodeJS.ProcessEnv, + input: OpenShellInferenceOptions, + tools: OpenShellTools = defaultOpenShellTools, +): Promise { + validateIdentifier(input.gatewayId, "gatewayId"); + validateIdentifier(input.modelId, "modelId"); + validateIdentifier(input.providerName, "providerName"); + + const providerApiKey = required(env.OPENAI_API_KEY, "OPENAI_API_KEY"); + const commandEnv = credentialFreeEnvironment(env); + const providerEnv = { ...commandEnv, OPENAI_API_KEY: providerApiKey }; + const gatewayDirectory = path.join(required(env.RUNNER_TEMP, "RUNNER_TEMP"), "openshell-gateway"); + const gatewayEndpoint = new URL( + required(env.OPENSHELL_GATEWAY_ENDPOINT, "OPENSHELL_GATEWAY_ENDPOINT"), + ); + const bindAddress = loopbackBindAddress(gatewayEndpoint); + const supervisor = required( + tools.run("which", ["openshell-sandbox"], { capture: true, env: commandEnv }), + "openshell-sandbox", + ); + + mkdirSync(gatewayDirectory, { recursive: true }); + tools.run("openshell-gateway", ["generate-certs", "--output-dir", gatewayDirectory], { + env: commandEnv, + }); + const configurationPath = path.join(gatewayDirectory, "gateway.toml"); + writeFileSync( + configurationPath, + gatewayConfiguration({ + bindAddress, + directory: gatewayDirectory, + enableBindMounts: input.enableBindMounts === true, + gatewayId: input.gatewayId, + supervisor, + }), + { mode: 0o600 }, + ); + tools.start("openshell-gateway", ["--config", configurationPath], { + env: commandEnv, + logPath: path.join(gatewayDirectory, "gateway.log"), + }); + + for (let attempt = 0; attempt < 30; attempt += 1) { + try { + tools.run("openshell", ["gateway", "info"], { env: commandEnv, timeout: 10_000 }); + break; + } catch { + await tools.wait(1000); + } + } + tools.run("openshell", ["gateway", "info"], { env: commandEnv, timeout: 10_000 }); + tools.run( + "openshell", + [ + "provider", + "create", + "--name", + input.providerName, + "--type", + "openai", + "--credential", + "OPENAI_API_KEY", + "--config", + "OPENAI_BASE_URL=https://inference-api.nvidia.com/v1", + ], + { env: providerEnv }, + ); + tools.run( + "openshell", + [ + "inference", + "set", + "--provider", + input.providerName, + "--model", + input.modelId, + "--timeout", + "900", + ], + { env: commandEnv }, + ); +} + +export function createOpenShellSandbox( + env: NodeJS.ProcessEnv, + input: CreateOpenShellSandboxOptions, + tools: OpenShellTools = defaultOpenShellTools, +): void { + const uploadArgs = input.uploads.flatMap(({ source, destination }) => [ + "--upload", + `${source}:${destination}`, + ]); + const uploadOptions = input.uploads.length > 0 ? [...uploadArgs, "--no-git-ignore"] : []; + const driverConfigArgs = input.driverConfig + ? ["--driver-config-json", JSON.stringify(input.driverConfig)] + : []; + tools.run( + "openshell", + [ + "sandbox", + "create", + "--name", + input.name, + "--from", + input.image, + ...driverConfigArgs, + "--policy", + input.policyPath, + ...uploadOptions, + "--no-tty", + "--", + ...input.command, + ], + { env: credentialFreeEnvironment(env) }, + ); +} + +export function execOpenShellSandbox( + env: NodeJS.ProcessEnv, + input: ExecOpenShellSandboxOptions, + tools: OpenShellTools = defaultOpenShellTools, +): void { + const workdirArgs = input.workdir ? ["--workdir", input.workdir] : []; + const timeoutArgs = input.timeoutSeconds ? ["--timeout", String(input.timeoutSeconds)] : []; + const environmentArgs = Object.entries(input.environment ?? {}).flatMap(([name, value]) => { + if (!/^[A-Z_][A-Z0-9_]*$/u.test(name) || /[\0\r\n]/u.test(value)) { + throw new OpenShellAgentError(`Unsafe sandbox environment entry: ${name}`); + } + return ["--env", `${name}=${value}`]; + }); + tools.run( + "openshell", + [ + "sandbox", + "exec", + "--name", + input.name, + ...timeoutArgs, + ...workdirArgs, + ...environmentArgs, + "--", + ...input.command, + ], + { env: credentialFreeEnvironment(env) }, + ); +} + +export function downloadOpenShellPath( + env: NodeJS.ProcessEnv, + input: { destination: string; name: string; source: string }, + tools: OpenShellTools = defaultOpenShellTools, +): void { + tools.run("openshell", ["sandbox", "download", input.name, input.source, input.destination], { + env: credentialFreeEnvironment(env), + }); +} + +export function setOpenShellSandboxPolicy( + env: NodeJS.ProcessEnv, + input: { name: string; policyPath: string }, + tools: OpenShellTools = defaultOpenShellTools, +): void { + tools.run("openshell", ["policy", "set", "--policy", input.policyPath, "--wait", input.name], { + env: credentialFreeEnvironment(env), + }); +} + +export function deleteOpenShellSandbox( + env: NodeJS.ProcessEnv, + name: string, + tools: OpenShellTools = defaultOpenShellTools, +): void { + const commandEnv = credentialFreeEnvironment(env); + let names: string; + try { + names = tools.run("openshell", ["sandbox", "list", "--names"], { + capture: true, + env: commandEnv, + }); + } catch { + return; + } + if (names.split(/\r?\n/u).includes(name)) { + tools.run("openshell", ["sandbox", "delete", name], { env: commandEnv }); + } +} diff --git a/tools/pr-merge-conflict-fixer/resolve.mts b/tools/pr-merge-conflict-fixer/resolve.mts index 7bbabe4b39..5896019f6c 100755 --- a/tools/pr-merge-conflict-fixer/resolve.mts +++ b/tools/pr-merge-conflict-fixer/resolve.mts @@ -2,22 +2,27 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { execFileSync, spawn } from "node:child_process"; -import { appendFileSync, closeSync, mkdirSync, openSync, writeFileSync } from "node:fs"; +import { appendFileSync, mkdirSync, writeFileSync } from "node:fs"; import path from "node:path"; import { pathToFileURL } from "node:url"; +import { + configureOpenShellInference as configureSharedOpenShellInference, + createOpenShellSandbox, + defaultOpenShellTools, + deleteOpenShellSandbox, + downloadOpenShellPath, + execOpenShellSandbox, + type OpenShellCommandOptions, + type OpenShellStartOptions, + type OpenShellTools, + required, +} from "../openshell-agent/runtime.mts"; import { type ConflictMatrixEntry, parseConflictMatrixEntry } from "./discover.mts"; import { ConflictFixerError, prepareMerge, samePaths } from "./merge.mts"; export const RESOLVER_MODEL_ID = "azure/openai/gpt-5.6-terra"; -const HOST_CREDENTIALS = [ - "GH_TOKEN", - "GITHUB_TOKEN", - "OPENAI_API_KEY", - "PR_REVIEW_ADVISOR_API_KEY", -] as const; const PI_COMMAND = [ "/usr/bin/node", "/usr/lib/node_modules/@earendil-works/pi-coding-agent/dist/cli.js", @@ -49,27 +54,9 @@ final_tree="$(git write-tree)" git diff --binary "$CONFLICT_TREE" "$final_tree" > /sandbox/resolution.patch `.trim(); -export interface ResolverCommandOptions { - capture?: boolean; - env: NodeJS.ProcessEnv; - timeout?: number; -} - -export interface ResolverStartOptions { - env: NodeJS.ProcessEnv; - logPath: string; -} - -export interface ResolverTools { - run: (command: string, args: readonly string[], options: ResolverCommandOptions) => string; - start: (command: string, args: readonly string[], options: ResolverStartOptions) => void; - wait: (milliseconds: number) => Promise; -} - -function required(value: string | undefined, name: string): string { - if (!value) throw new ConflictFixerError(`${name} is required`); - return value; -} +export type ResolverCommandOptions = OpenShellCommandOptions; +export type ResolverStartOptions = OpenShellStartOptions; +export type ResolverTools = OpenShellTools; export function resolverModelConfiguration(): string { return `${JSON.stringify( @@ -106,35 +93,6 @@ export function resolverModelConfiguration(): string { )}\n`; } -function gatewayConfiguration(input: { - bindAddress: string; - directory: string; - supervisor: string; -}): string { - return `[openshell] -version = 1 - -[openshell.gateway] -bind_address = "${input.bindAddress}" -compute_drivers = ["docker"] -disable_tls = true - -[openshell.gateway.auth] -allow_unauthenticated_users = true - -[openshell.gateway.gateway_jwt] -signing_key_path = "${path.join(input.directory, "jwt", "signing.pem")}" -public_key_path = "${path.join(input.directory, "jwt", "public.pem")}" -kid_path = "${path.join(input.directory, "jwt", "kid")}" -gateway_id = "pr-conflict-fixer" -ttl_secs = 3600 - -[openshell.drivers.docker] -grpc_endpoint = "http://host.openshell.internal:8080" -supervisor_bin = "${input.supervisor}" -`; -} - export function resolverPrompt(): string { return [ "Resolve the Git merge conflicts in this repository.", @@ -174,241 +132,109 @@ export function prepareResolutionWorkspace(input: { return merge.conflictTree; } -function openshellEnvironment(env: NodeJS.ProcessEnv): NodeJS.ProcessEnv { - const home = required(env.HOME, "HOME"); - const binaryDirectory = env.XDG_BIN_HOME ?? path.join(home, ".local", "bin"); - return { - ...env, - PATH: [binaryDirectory, env.PATH ?? ""].filter(Boolean).join(path.delimiter), - }; -} - -function credentialFreeEnvironment(env: NodeJS.ProcessEnv): NodeJS.ProcessEnv { - const result = openshellEnvironment(env); - for (const name of HOST_CREDENTIALS) delete result[name]; - return result; -} - -function loopbackBindAddress(endpoint: URL): string { - if (!["127.0.0.1", "[::1]"].includes(endpoint.hostname)) { - throw new ConflictFixerError("OPENSHELL_GATEWAY_ENDPOINT must use a loopback address"); - } - return endpoint.host; -} - -const defaultTools: ResolverTools = { - run(command, args, options): string { - const output = execFileSync(command, [...args], { - encoding: "utf8", - env: options.env, - stdio: options.capture ? ["ignore", "pipe", "inherit"] : "inherit", - timeout: options.timeout, - }); - return String(output ?? "").trim(); - }, - start(command, args, options): void { - const log = openSync(options.logPath, "w", 0o600); - try { - const child = spawn(command, [...args], { - detached: true, - env: options.env, - stdio: ["ignore", log, log], - }); - child.on("error", () => undefined); - child.unref(); - } finally { - closeSync(log); - } - }, - wait(milliseconds): Promise { - return new Promise((resolve) => setTimeout(resolve, milliseconds)); - }, -}; - export async function configureOpenShellInference( env: NodeJS.ProcessEnv, - tools: ResolverTools = defaultTools, + tools: ResolverTools = defaultOpenShellTools, ): Promise { - const providerApiKey = required(env.OPENAI_API_KEY, "OPENAI_API_KEY"); - const commandEnv = credentialFreeEnvironment(env); - const providerEnv = { ...commandEnv, OPENAI_API_KEY: providerApiKey }; - const gatewayDirectory = path.join(required(env.RUNNER_TEMP, "RUNNER_TEMP"), "openshell-gateway"); - const gatewayEndpoint = new URL( - required(env.OPENSHELL_GATEWAY_ENDPOINT, "OPENSHELL_GATEWAY_ENDPOINT"), - ); - const bindAddress = loopbackBindAddress(gatewayEndpoint); - const supervisor = required( - tools.run("which", ["openshell-sandbox"], { capture: true, env: commandEnv }), - "openshell-sandbox", - ); - mkdirSync(gatewayDirectory, { recursive: true }); - tools.run("openshell-gateway", ["generate-certs", "--output-dir", gatewayDirectory], { - env: commandEnv, - }); - const configurationPath = path.join(gatewayDirectory, "gateway.toml"); - writeFileSync( - configurationPath, - gatewayConfiguration({ - bindAddress, - directory: gatewayDirectory, - supervisor, - }), - { mode: 0o600 }, - ); - tools.start("openshell-gateway", ["--config", configurationPath], { - env: commandEnv, - logPath: path.join(gatewayDirectory, "gateway.log"), - }); - for (let attempt = 0; attempt < 30; attempt += 1) { - try { - tools.run("openshell", ["gateway", "info"], { env: commandEnv, timeout: 10_000 }); - break; - } catch { - await tools.wait(1000); - } - } - tools.run("openshell", ["gateway", "info"], { env: commandEnv, timeout: 10_000 }); - tools.run( - "openshell", - [ - "provider", - "create", - "--name", - "terra", - "--type", - "openai", - "--credential", - "OPENAI_API_KEY", - "--config", - "OPENAI_BASE_URL=https://inference-api.nvidia.com/v1", - ], - { env: providerEnv }, - ); - tools.run( - "openshell", - ["inference", "set", "--provider", "terra", "--model", RESOLVER_MODEL_ID, "--timeout", "900"], - { env: commandEnv }, + await configureSharedOpenShellInference( + env, + { + gatewayId: "pr-conflict-fixer", + modelId: RESOLVER_MODEL_ID, + providerName: "terra", + }, + tools, ); } export function createResolutionSandbox( env: NodeJS.ProcessEnv, - tools: ResolverTools = defaultTools, + tools: ResolverTools = defaultOpenShellTools, ): void { - tools.run( - "openshell", - [ - "sandbox", - "create", - "--name", - required(env.SANDBOX_NAME, "SANDBOX_NAME"), - "--from", - required(env.PI_IMAGE, "PI_IMAGE"), - "--policy", - path.join( + createOpenShellSandbox( + env, + { + name: required(env.SANDBOX_NAME, "SANDBOX_NAME"), + image: required(env.PI_IMAGE, "PI_IMAGE"), + policyPath: path.join( required(env.TRUSTED_CHECKOUT, "TRUSTED_CHECKOUT"), "tools", "pr-merge-conflict-fixer", "policy.yaml", ), - "--upload", - `${required(env.RESOLUTION_WORKDIR, "RESOLUTION_WORKDIR")}:/sandbox`, - "--upload", - `${required(env.RESOLVER_CONFIG_DIR, "RESOLVER_CONFIG_DIR")}:/sandbox`, - "--no-git-ignore", - "--no-tty", - "--", - "/usr/bin/git", - "-C", - "/sandbox/repo", - "status", - "--short", - ], - { env: credentialFreeEnvironment(env) }, + uploads: [ + { + source: required(env.RESOLUTION_WORKDIR, "RESOLUTION_WORKDIR"), + destination: "/sandbox", + }, + { + source: required(env.RESOLVER_CONFIG_DIR, "RESOLVER_CONFIG_DIR"), + destination: "/sandbox", + }, + ], + command: ["/usr/bin/git", "-C", "/sandbox/repo", "status", "--short"], + }, + tools, ); } export function runResolutionTask( env: NodeJS.ProcessEnv, - tools: ResolverTools = defaultTools, + tools: ResolverTools = defaultOpenShellTools, ): void { - tools.run( - "openshell", - [ - "sandbox", - "exec", - "--name", - required(env.SANDBOX_NAME, "SANDBOX_NAME"), - "--timeout", - "1200", - "--workdir", - "/sandbox/repo", - "--env", - "HOME=/sandbox", - "--env", - "PI_CODING_AGENT_DIR=/sandbox/pi-config", - "--env", - "PI_OFFLINE=1", - "--env", - "TMPDIR=/sandbox", - "--", - ...PI_COMMAND, - ], - { env: credentialFreeEnvironment(env) }, + execOpenShellSandbox( + env, + { + name: required(env.SANDBOX_NAME, "SANDBOX_NAME"), + timeoutSeconds: 1200, + workdir: "/sandbox/repo", + environment: { + HOME: "/sandbox", + PI_CODING_AGENT_DIR: "/sandbox/pi-config", + PI_OFFLINE: "1", + TMPDIR: "/sandbox", + }, + command: PI_COMMAND, + }, + tools, ); } export function exportResolutionPatch( env: NodeJS.ProcessEnv, - tools: ResolverTools = defaultTools, + tools: ResolverTools = defaultOpenShellTools, ): void { - const commandEnv = credentialFreeEnvironment(env); const sandboxName = required(env.SANDBOX_NAME, "SANDBOX_NAME"); - tools.run( - "openshell", - [ - "sandbox", - "exec", - "--name", - sandboxName, - "--workdir", - "/sandbox/repo", - "--env", - `CONFLICT_TREE=${required(env.CONFLICT_TREE, "CONFLICT_TREE")}`, - "--", - "/usr/bin/bash", - "-c", - EXPORT_PATCH_COMMAND, - ], - { env: commandEnv }, + execOpenShellSandbox( + env, + { + name: sandboxName, + workdir: "/sandbox/repo", + environment: { + CONFLICT_TREE: required(env.CONFLICT_TREE, "CONFLICT_TREE"), + }, + command: ["/usr/bin/bash", "-c", EXPORT_PATCH_COMMAND], + }, + tools, ); const artifactDirectory = required(env.ARTIFACT_DIR, "ARTIFACT_DIR"); mkdirSync(artifactDirectory, { recursive: true }); - tools.run( - "openshell", - ["sandbox", "download", sandboxName, "/sandbox/resolution.patch", `${artifactDirectory}/`], - { env: commandEnv }, + downloadOpenShellPath( + env, + { + name: sandboxName, + source: "/sandbox/resolution.patch", + destination: `${artifactDirectory}/`, + }, + tools, ); } export function deleteResolutionSandbox( env: NodeJS.ProcessEnv, - tools: ResolverTools = defaultTools, + tools: ResolverTools = defaultOpenShellTools, ): void { - const commandEnv = credentialFreeEnvironment(env); - const sandboxName = required(env.SANDBOX_NAME, "SANDBOX_NAME"); - let names: string; - try { - names = tools.run("openshell", ["sandbox", "list", "--names"], { - capture: true, - env: commandEnv, - }); - } catch { - return; - } - if (names.split(/\r?\n/u).includes(sandboxName)) { - tools.run("openshell", ["sandbox", "delete", sandboxName], { env: commandEnv }); - } + deleteOpenShellSandbox(env, required(env.SANDBOX_NAME, "SANDBOX_NAME"), tools); } function prepare(env: NodeJS.ProcessEnv): void { diff --git a/tools/pr-review-advisor/README.md b/tools/pr-review-advisor/README.md index 6ccf4ac4c2..eff195bd1e 100644 --- a/tools/pr-review-advisor/README.md +++ b/tools/pr-review-advisor/README.md @@ -3,10 +3,10 @@ # PR Review Advisor -The PR Review Advisor is an SDK-powered, NemoClaw-specific pull request reviewer. It runs as a -trusted GitHub Actions job, inspects PRs as read-only data, and posts a sticky comment with blockers, -warnings, and suggestions. Artifacts retain -acceptance coverage, security notes, and other review context. +The PR Review Advisor is an SDK-powered, NemoClaw-specific pull request reviewer. It runs its +model-backed analysis in an OpenShell sandbox from a trusted GitHub Actions job, inspects PRs as +read-only data, and posts a sticky comment with blockers, warnings, and suggestions. Artifacts +retain acceptance coverage, security notes, and other review context. It complements the existing PR surfaces by keeping a NemoClaw maintainer code-review lens focused on the patch itself and by including E2E coverage and target guidance in the same model session: @@ -33,22 +33,28 @@ It intentionally does not report GitHub mergeability, branch protection, CI stat 2. Checks out advisor implementation code at the immutable trusted `github.workflow_sha` into `advisor/`. 3. Fetches the event's PR base and head SHAs into an isolated analysis workspace without running PR-controlled actions, hooks, submodules, LFS filters, package setup, scripts, or tests. 4. Installs and verifies pinned `ripgrep` and `fd-find` packages on a pinned Ubuntu runner, then installs a pinned Pi SDK package with lifecycle scripts disabled. -5. Builds the deterministic regression risk plan and E2E inventory in trusted code and injects them into the review contexts. -6. Runs `tools/pr-review-advisor/analyze.mts` from the trusted checkout. -7. Runs the same advisor conversation in parallel for the primary GPT-5.6 Terra lane and an artifact-only Nemotron Ultra evaluation lane. -8. Opens one Pi session per model variant and reviews the PR in 14 bounded turns: six small analysis/commit pairs for scope/risk, correctness/state, security/trust, tests/regressions, CI/operations, and reconciliation, followed by draft and validation JSON synthesis turns in that same session. The tests/regressions turn analyzes E2E coverage and new-test gaps, while the CI/operations turn selects supported E2E jobs, targets, or fan-out. Only trusted identifiers from these receipts reach normalized E2E output; free-form model E2E prose is discarded. No second advisor session is opened, including for synthesis repair. -9. Gives each commit turn one job: apply one successful atomic ledger commit for the preceding analysis. The model-facing commit is one flat object with homogeneous additions, updates, resolutions, and supersessions arrays plus a no-change reason; legacy nested operation unions and stringified arrays are rejected. Additions require a structured observed-versus-expected basis, a file and line, and eligibility for the active stage. Positives, advisor/provider state, prior-review process state, open-PR overlap, merge coordination, and live CI/E2E status stay in prose receipts rather than becoming findings. The ledger mutation tool is the turn's only active tool, and the runner rejects prose, other tool calls, or activity after the successful commit. Rejected attempts do not mutate the ledger and may be corrected before one success. If a commit turn ends with no successful call and every attempt settled without mutating state, the runner permits one tool-only retry and then fails closed. Ledger findings receive stable `F-...` IDs, and conclusion changes require a reason plus new evidence; final synthesis can only read the ledger. -10. Treats open ledger records as the canonical finding set. Final synthesis cannot silently add, drop, merge, reword, or reclassify those findings. Unresolved source-of-truth review entries must reference their covering open ledger ID structurally rather than relying on prose matching. -11. Logs each turn start and settled status and writes the assistant response immediately, preserving partial failed/timed-out turn evidence and the raw transcript. If a later stage fails, already-committed canonical findings remain in the low-confidence incomplete result instead of being replaced by a generic unavailable finding. -12. Retries transient provider failures such as HTTP 429 within the same session using one bounded exponential-backoff layer. GPT waits 6s, 12s, 24s, and 48s; Nemotron waits 9s, 18s, 36s, and 72s so parallel lanes do not retry in lockstep. The workflow still publishes the primary comment and lane artifacts after an incomplete analysis. An incomplete primary review fails its outcome step; the artifact-only evaluation lane does not affect the workflow result. -13. Validates and repairs the draft synthesis in the final turn of the same session. If that turn fails or emits malformed output, the runner preserves a schema-valid canonical draft with a limitation; a post-validation ledger mismatch still fails closed. -14. Writes artifacts under the model-specific artifact directory, for example `artifacts/pr-review-advisor/` and `artifacts/pr-review-advisor-nemotron-ultra/`. -15. Uploads each lane's artifacts from the read-only analysis job. A separate publisher job receives no model credential or untrusted worktree, validates the primary artifact and live PR head/base, then posts or updates one combined sticky PR comment marked by ``. The evaluation lane does not publish another review. Previous sticky-comment ingestion is disabled for both lanes. +5. Materializes deterministic GitHub context on the trusted host before sandbox creation. This is the only analysis-phase step that receives `github.token`, and it receives no model credential. +6. Installs OpenShell, starts a loopback-only gateway, and registers the selected model provider in a dedicated trusted-host step. Only that provider-configuration step receives the upstream model credential. +7. Creates a sandbox from a digest-pinned Pi image under a no-egress, hard-Landlock policy. The trusted advisor checkout, PR workspace, prepared GitHub context, and verified search binaries enter through advisor-only read-only Docker bind mounts before the first sandbox process starts. A capped tmpfs is the only writable application-data subtree. Before model code runs, a trusted probe reads every input canary, resolves the mounted checkout and `HEAD` through an explicit `GIT_DIR` and `GIT_WORK_TREE`, verifies that chmod, overwrite, replacement, and creation fail in every input, and exercises the complete runtime write lifecycle. +8. Runs the trusted `tools/pr-review-advisor/run-analysis.mts` entrypoint inside the sandbox. The unchanged multi-turn Pi SDK session reaches the host-configured model only through `https://inference.local/v1`; the sandbox receives an inert SDK key and neither the upstream model credential nor a GitHub token. +9. Runs the same advisor conversation in parallel for the primary GPT-5.6 Terra lane and an artifact-only Nemotron Ultra evaluation lane. +10. Opens one Pi session per model variant and reviews the PR in 14 bounded turns: six small analysis/commit pairs for scope/risk, correctness/state, security/trust, tests/regressions, CI/operations, and reconciliation, followed by draft and validation JSON synthesis turns in that same session. The tests/regressions turn analyzes E2E coverage and new-test gaps, while the CI/operations turn selects supported E2E jobs, targets, or fan-out. Only trusted identifiers from these receipts reach normalized E2E output; free-form model E2E prose is discarded. No second advisor session is opened, including for synthesis repair. +11. Gives each commit turn one job: apply one successful atomic ledger commit for the preceding analysis. The model-facing commit is one flat object with homogeneous additions, updates, resolutions, and supersessions arrays plus a no-change reason; legacy nested operation unions and stringified arrays are rejected. Additions require a structured observed-versus-expected basis, a file and line, and eligibility for the active stage. Positives, advisor/provider state, prior-review process state, open-PR overlap, merge coordination, and live CI/E2E status stay in prose receipts rather than becoming findings. The ledger mutation tool is the turn's only active tool, and the runner rejects prose, other tool calls, or activity after the successful commit. Rejected attempts do not mutate the ledger and may be corrected before one success. If a commit turn ends with no successful call and every attempt settled without mutating state, the runner permits one tool-only retry and then fails closed. Ledger findings receive stable `F-...` IDs, and conclusion changes require a reason plus new evidence; final synthesis can only read the ledger. +12. Treats open ledger records as the canonical finding set. Final synthesis cannot silently add, drop, merge, reword, or reclassify those findings. Unresolved source-of-truth review entries must reference their covering open ledger ID structurally rather than relying on prose matching. +13. Logs each turn start and settled status and writes the assistant response immediately, preserving partial failed/timed-out turn evidence and the raw transcript. If a later stage fails, already-committed canonical findings remain in the low-confidence incomplete result instead of being replaced by a generic unavailable finding. +14. Retries transient provider failures such as HTTP 429 within the same session using one bounded exponential-backoff layer. GPT waits 6s, 12s, 24s, and 48s; Nemotron waits 9s, 18s, 36s, and 72s so parallel lanes do not retry in lockstep. The workflow still publishes the primary comment and lane artifacts after an incomplete analysis. An incomplete primary review fails its outcome step; the artifact-only evaluation lane does not affect the workflow result. +15. Validates and repairs the draft synthesis in the final turn of the same session. If that turn fails or emits malformed output, the runner preserves a schema-valid canonical draft with a limitation; a post-validation ledger mismatch still fails closed. +16. Writes artifacts under the model-specific artifact directory in the writable runtime subtree, downloads them to the trusted host, and uploads them from the read-only analysis job. Example directories are `artifacts/pr-review-advisor/` and `artifacts/pr-review-advisor-nemotron-ultra/`. +17. Uses a separate publisher job with no model credential or untrusted worktree. It validates the primary artifact and live PR head/base, then posts or updates one combined sticky PR comment marked by ``. The evaluation lane does not publish another review. Previous sticky-comment ingestion is disabled for both lanes. The ordered stage array in `buildPromptTurns` is the source of truth for stage order, evidence, and prompt text. Runtime numbering and prompt artifact names derive from that array, so adding or reordering a stage does not require parallel orchestration changes. +`tools/pr-review-advisor/openshell.mts` owns the advisor-specific prepare, create, run, download, and +cleanup sequence. It uses the shared lifecycle and credential-boundary helpers in +`tools/openshell-agent/runtime.mts`, which are also used by the merge-conflict fixer. + Provider failures and timeouts settle the active turn before the analysis fails, so its status and partial response remain available beside the raw transcript. Turn-artifact persistence failures are also fatal. A finding mismatch after same-session synthesis validation is fatal as well. Fatal runs remain @@ -75,13 +81,14 @@ Authors and coding agents should follow the shared [PR CI and Review Follow-Up]( - Static analysis only. - PR-provided scripts, tests, package lifecycle hooks, and build tools are never executed. +- The model session runs in a digest-pinned OpenShell sandbox under a hard-required Landlock policy with no direct network policy and no ambient workdir. Four canonical host inputs are mounted read-only through the advisor's ephemeral Docker gateway outside `/sandbox`, so OpenShell v0.0.85 applies the final immutable boundary before the first process starts. Landlock independently grants those inputs read-only access. It grants application-data writes only to a bounded runtime tmpfs; required device access remains writable under `/dev`. The sandbox pins Git to `/pr-workdir/.git` and `/pr-workdir` instead of relying on cross-UID repository discovery. A startup proof must read every input canary, resolve the checkout and `HEAD`, fail chmod, overwrite, replacement, and creation in each input, and complete runtime writes. The model-facing Advisor tools remain repository-confined and read-only; generated configuration and artifacts use the dedicated runtime subtree. - The advisor receives repo-confined read-only repository tools plus deterministic context tools. Repository paths must remain inside the checked-out analysis workspace after lexical and symlink resolution. Its only mutation tool updates the in-memory finding ledger; it cannot change repository or GitHub state. - PR bodies, comments, titles, branch names, and diffs are treated as untrusted evidence, never as instructions. - Manual target analysis validates the repository token, decimal PR number, and base-ref token before running any `git` command. -- Generated advisor credential config is written under `/tmp`, not uploaded artifacts. -- The analysis job is limited to `NVIDIA/NemoClaw`, has read-only GitHub permissions, and is the only job that receives the model secret. -- The analyzer collects deterministic GitHub context before model work, then removes GitHub tokens from the process environment. - After registering the model credential in the in-memory SDK auth store, it also removes that credential from the process environment before model turns begin. +- Generated Pi configuration is written under the sandbox's runtime-only configuration directory, not uploaded artifacts. +- The review job is limited to `NVIDIA/NemoClaw` and has read-only GitHub permissions. Within it, only the trusted host provider-configuration step receives the upstream model secret. +- A separate trusted host step collects deterministic GitHub context with `github.token` and writes a bounded, identity-checked context file before model work. The sandbox receives that file, not the token. +- The OpenShell gateway binds only to loopback and holds the upstream provider credential. The sandbox uses `https://inference.local/v1` with an inert SDK key, and receives neither the provider credential nor a GitHub token. - The separate publisher has pull-request write permission, but receives neither the model secret nor the untrusted PR worktree. It accepts only the bounded primary artifact from the same workflow run and rechecks the live PR head and base before commenting. Before rendering E2E guidance, it independently allowlists coverage IDs and selector tuples and ignores artifact-authored E2E prose. A newly added credential-free test can extend the job allowlist only through trusted-normalizer evidence bound to the same head SHA, changed-file path, and basename-derived selector ID. - Sticky publication updates only a marker-bearing comment owned by `github-actions[bot]`; a user-authored marker cannot claim the update target. The rendered comment preserves its hidden identity metadata while enforcing a 60 KiB UTF-8 limit, and publication errors remain visible in the publisher logs. @@ -99,7 +106,9 @@ Configure this repository secret for review analysis: - `PR_REVIEW_ADVISOR_API_KEY` -The analyzer uses the OpenAI-compatible `https://inference-api.nvidia.com/v1` service. +The trusted host uses this secret only to register the OpenAI-compatible +`https://inference-api.nvidia.com/v1` service with OpenShell. The sandboxed analyzer reaches that +provider through `https://inference.local/v1` and does not receive the secret. The primary lane uses `azure/openai/gpt-5.6-terra`; the parallel Nemotron lane sets `PR_REVIEW_ADVISOR_MODEL=nvidia/nvidia/nemotron-3-ultra` and reuses the same analyzer, prompts, schema, safety boundary, and credential secret. @@ -142,10 +151,10 @@ node --experimental-strip-types tools/pr-review-advisor/analyze.mts \ --out-dir artifacts/pr-review-advisor ``` -Set `PR_REVIEW_ADVISOR_API_KEY` locally, or configure the repository -`PR_REVIEW_ADVISOR_API_KEY` secret. Add `PR_REVIEW_ADVISOR_MODEL=nvidia/nvidia/nemotron-3-ultra` -to exercise the Nemotron Ultra lane locally. Run `npm install` first so the Pi SDK dependency is -available. +For this direct local invocation outside the workflow's OpenShell wrapper, set +`PR_REVIEW_ADVISOR_API_KEY` locally. Add +`PR_REVIEW_ADVISOR_MODEL=nvidia/nvidia/nemotron-3-ultra` to exercise the Nemotron Ultra lane +locally. Run `npm install` first so the Pi SDK dependency is available. ## Output contract diff --git a/tools/pr-review-advisor/analyze.mts b/tools/pr-review-advisor/analyze.mts index 026eeb2085..2e31916b11 100755 --- a/tools/pr-review-advisor/analyze.mts +++ b/tools/pr-review-advisor/analyze.mts @@ -22,7 +22,7 @@ import { getHeadSha, gitOutput, } from "../advisors/git.mts"; -import { githubRest, githubRestPaginated } from "../advisors/github.mts"; +import { githubRest } from "../advisors/github.mts"; import { parseArgs, parsePositiveInt, readJson, writeJson } from "../advisors/io.mts"; import { enumValue, @@ -47,6 +47,13 @@ import { runReadOnlyAdvisor, } from "../advisors/session.mts"; import { focusedE2eJobsForChangedFiles } from "../e2e/workflow-boundary.mts"; +import { + collectGitHubReviewContext, + extractIssueRefs, + type GitHubReviewContext, + type PreviousAdvisorReview, + readPreparedGitHubContext, +} from "./github-context.mts"; import { createReviewFindingLedger, createReviewLedgerToolController, @@ -56,6 +63,9 @@ import { reviewLedgerStageCommitGuidance, } from "./review-ledger.mts"; +export type { GitHubReviewContext, PreviousAdvisorReview }; +export { extractIssueRefs, readPreparedGitHubContext }; + const root = process.cwd(); export const DEFAULT_ADVISOR_COMMENT_MARKER = ""; export const DEFAULT_ADVISOR_WORKFLOW_NAME = "PR Review / Advisor"; @@ -69,8 +79,6 @@ const ADVISOR_WORKFLOW_NAME = const ADVISOR_WORKFLOW_PATH = process.env.PR_REVIEW_ADVISOR_WORKFLOW_PATH || DEFAULT_ADVISOR_WORKFLOW_PATH; const ADVISOR_CREDENTIAL_ENV = ["PR", "REVIEW", "ADVISOR", "API", "KEY"].join("_"); -const OPEN_PR_OVERLAP_LIMIT = 80; -const OPEN_PR_OVERLAP_CONCURRENCY = 6; const RISK_CONTEXT_PATH_SAMPLE_LIMIT = 20; const RISK_CONTEXT_PATH_CHARACTER_LIMIT = 240; const METADATA_CHANGED_FILE_LIMIT = 20; @@ -298,38 +306,6 @@ type DriftEvidence = { renameHints: string[]; }; -type OpenPrOverlap = { - number: number; - title: string; - labels: string[]; - linkedIssues: number[]; - sameFiles: string[]; - duplicateLinkedIssues: number[]; -}; - -type GitHubReviewContext = { - repo: string; - prNumber: number; - fetchError?: string; - pullRequest?: unknown; - issueReferenceLines?: string[]; - linkedIssues?: LinkedIssue[]; - openPrOverlaps?: OpenPrOverlap[]; - previousAdvisorReview?: PreviousAdvisorReview | null; -}; - -export type PreviousAdvisorReview = { - headSha?: string; - body: string; -}; - -type LinkedIssue = { - number: number; - issue?: unknown; - comments?: unknown[]; - fetchError?: string; -}; - if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { main().catch((error: unknown) => { console.error(error instanceof Error ? error.message : String(error)); @@ -1281,188 +1257,19 @@ function collectDriftEvidence(baseRef: string, changedFiles: string[]): DriftEvi }); } -async function collectGitHubContext(): Promise { - const repo = process.env.GITHUB_REPOSITORY; - const prNumber = Number.parseInt( - process.env.PR_NUMBER || process.env.GITHUB_REF_NAME?.match(/^(\d+)\//)?.[1] || "", - 10, - ); - const token = process.env.GH_TOKEN || process.env.GITHUB_TOKEN; - if (!repo || !Number.isFinite(prNumber) || prNumber <= 0 || !token) return null; - - const context: GitHubReviewContext = { repo, prNumber }; - try { - const loadPreviousReview = process.env.PR_REVIEW_ADVISOR_LOAD_PREVIOUS_REVIEW === "true"; - const [pullRequest, issueComments, openPulls] = await Promise.all([ - githubRest(`repos/${repo}/pulls/${prNumber}`, token), - loadPreviousReview - ? githubRestPaginated(`repos/${repo}/issues/${prNumber}/comments`, token, 100) - : Promise.resolve([]), - githubRestPaginated( - `repos/${repo}/pulls?state=open&sort=updated&direction=desc`, - token, - 100, - ), - ]); - context.pullRequest = pullRequest; - context.previousAdvisorReview = loadPreviousReview - ? await collectTrustedPreviousAdvisorReview(repo, token, issueComments, { - marker: ADVISOR_COMMENT_MARKER, - workflowName: ADVISOR_WORKFLOW_NAME, - workflowPath: ADVISOR_WORKFLOW_PATH, - prNumber, - currentBaseSha: stringOrUndefined(getPath(pullRequest, ["base", "sha"])), - }) - : null; - const prTitle = stringOrUndefined(getPath(pullRequest, ["title"])) || ""; - const prBody = stringOrUndefined(getPath(pullRequest, ["body"])) || ""; - const prText = [ - prTitle, - prBody, - stringOrUndefined(getPath(pullRequest, ["head", "ref"])), - ] - .filter(Boolean) - .join("\n"); - const issueNumbers = extractIssueRefs(prText, prNumber).slice(0, 5); - context.issueReferenceLines = [prTitle, ...prBody.split("\n")] - .map((line) => line.trim()) - .filter((line) => line && extractIssueRefs(line, prNumber).length > 0) - .slice(0, 20); - context.linkedIssues = await Promise.all( - issueNumbers.map((issue) => collectLinkedIssue(repo, issue, token)), - ); - context.openPrOverlaps = await collectOpenPrOverlaps( - repo, - prNumber, - token, - openPulls, - issueNumbers, - ); - } catch (error: unknown) { - context.fetchError = error instanceof Error ? error.message : String(error); - } - return context; -} - -async function collectLinkedIssue( - repo: string, - number: number, - token: string, -): Promise { - try { - const [issue, comments] = await Promise.all([ - githubRest(`repos/${repo}/issues/${number}`, token), - githubRestPaginated(`repos/${repo}/issues/${number}/comments`, token, 50), - ]); - return { number, issue, comments }; - } catch (error: unknown) { - return { number, fetchError: error instanceof Error ? error.message : String(error) }; - } -} - -async function mapWithConcurrency( - items: readonly T[], - concurrency: number, - mapper: (item: T, index: number) => Promise, -): Promise { - const results = new Array(items.length); - let nextIndex = 0; - const workerCount = Math.min(Math.max(1, concurrency), items.length); - const workers = Array.from({ length: workerCount }, async () => { - while (nextIndex < items.length) { - const index = nextIndex; - nextIndex += 1; - results[index] = await mapper(items[index] as T, index); - } +export async function collectGitHubContext( + env: NodeJS.ProcessEnv = process.env, +): Promise { + return collectGitHubReviewContext(env, { + collectPreviousReview: ({ currentBaseSha, issueComments, prNumber, repo, token }) => + collectTrustedPreviousAdvisorReview(repo, token, issueComments, { + marker: ADVISOR_COMMENT_MARKER, + workflowName: ADVISOR_WORKFLOW_NAME, + workflowPath: ADVISOR_WORKFLOW_PATH, + prNumber, + currentBaseSha, + }), }); - await Promise.all(workers); - return results; -} - -async function collectOpenPrOverlaps( - repo: string, - currentPrNumber: number, - token: string, - openPulls: unknown[], - currentLinkedIssues: number[], -): Promise { - const currentFiles = new Set( - ( - await githubRestPaginated<{ filename?: string }>( - `repos/${repo}/pulls/${currentPrNumber}/files`, - token, - 300, - ) - ) - .map((file) => file.filename) - .filter((file): file is string => typeof file === "string"), - ); - const candidatePulls = openPulls - .filter((pull) => getPath(pull, ["number"]) !== currentPrNumber) - .slice(0, OPEN_PR_OVERLAP_LIMIT); - const overlaps = await mapWithConcurrency( - candidatePulls, - OPEN_PR_OVERLAP_CONCURRENCY, - async (pull): Promise => { - const number = getPath(pull, ["number"]); - if (!number) return null; - const title = stringOrDefault(getPath(pull, ["title"]), `PR #${number}`); - const body = stringOrDefault(getPath(pull, ["body"]), ""); - const labels = recordItems(getPath(pull, ["labels"])) - .map((label) => stringOrUndefined(label.name)) - .filter((label): label is string => Boolean(label)); - const linkedIssues = extractIssueRefs(`${title}\n${body}`, number); - const duplicateLinkedIssues = linkedIssues.filter((issue) => - currentLinkedIssues.includes(issue), - ); - let sameFiles: string[] = []; - if (currentFiles.size > 0) { - try { - sameFiles = ( - await githubRestPaginated<{ filename?: string }>( - `repos/${repo}/pulls/${number}/files`, - token, - 300, - ) - ) - .map((file) => file.filename) - .filter((file): file is string => typeof file === "string" && currentFiles.has(file)); - } catch { - sameFiles = []; - } - } - if (sameFiles.length === 0 && duplicateLinkedIssues.length === 0) return null; - return { number, title, labels, linkedIssues, sameFiles, duplicateLinkedIssues }; - }, - ); - return overlaps - .filter((overlap): overlap is OpenPrOverlap => overlap !== null) - .sort( - (a, b) => - b.sameFiles.length - a.sameFiles.length || - b.duplicateLinkedIssues.length - a.duplicateLinkedIssues.length || - a.number - b.number, - ) - .slice(0, 25); -} - -export function extractIssueRefs(text: string, prNumber: number): number[] { - const numbers = new Set(); - const relationPattern = - /\b(?:fixes|closes|resolves|refs?|references?|related(?:\s+issue)?|linked(?:\s+issue)?|follow[- ]?up(?:\s+to)?)\s+(#\d+(?:\s*(?:,\s*(?:and\s+)?|and\s+|&\s*)#\d+)*)/giu; - for (const relation of text.matchAll(relationPattern)) { - for (const match of (relation[1] ?? "").matchAll(/#(\d+)/gu)) { - const number = Number.parseInt(match[1] || "", 10); - if (Number.isFinite(number) && number > 0 && number !== prNumber) numbers.add(number); - } - } - for (const pattern of [/\(#(\d+)\)/gu, /issue[-_/](\d+)/giu]) { - for (const match of text.matchAll(pattern)) { - const number = Number.parseInt(match[1] || "", 10); - if (Number.isFinite(number) && number > 0 && number !== prNumber) numbers.add(number); - } - } - return [...numbers].sort((a, b) => a - b); } export function extractPreviousAdvisorReview( diff --git a/tools/pr-review-advisor/github-context.mts b/tools/pr-review-advisor/github-context.mts new file mode 100644 index 0000000000..bee5d1667e --- /dev/null +++ b/tools/pr-review-advisor/github-context.mts @@ -0,0 +1,443 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import fs from "node:fs"; + +import { githubRest, githubRestPaginated } from "../advisors/github.mts"; +import { + getPath, + isObjectRecord, + recordItems, + stringOrDefault, + stringOrUndefined, +} from "../advisors/json.mts"; + +export const MAX_PREPARED_GITHUB_CONTEXT_BYTES = 5 * 1024 * 1024; +const OPEN_PR_OVERLAP_LIMIT = 80; +const OPEN_PR_OVERLAP_CONCURRENCY = 6; +const OVERLAP_LINKED_ISSUE_LIMIT = 50; +const OVERLAP_SAME_FILE_SAMPLE_LIMIT = 20; +const OVERLAP_PATH_CHARACTER_LIMIT = 300; +const BODY_CHARACTER_LIMIT = 20_000; +const COMMENT_BODY_CHARACTER_LIMIT = 4_000; + +export type PreviousAdvisorReview = { + headSha?: string; + body: string; +}; + +type OpenPrOverlap = { + number: number; + title: string; + labels: string[]; + linkedIssues: number[]; + linkedIssueCount: number; + sameFiles: string[]; + sameFileCount: number; + duplicateLinkedIssues: number[]; +}; + +type LinkedIssue = { + number: number; + issue?: unknown; + comments?: unknown[]; + fetchError?: string; +}; + +export type GitHubReviewContext = { + repo: string; + prNumber: number; + fetchError?: string; + pullRequest?: unknown; + issueReferenceLines?: string[]; + linkedIssues?: LinkedIssue[]; + openPrOverlaps?: OpenPrOverlap[]; + previousAdvisorReview?: PreviousAdvisorReview | null; +}; + +type PreviousReviewCollectorInput = { + currentBaseSha?: string; + issueComments: unknown[]; + prNumber: number; + repo: string; + token: string; +}; + +type CollectGitHubReviewContextOptions = { + collectPreviousReview?: ( + input: PreviousReviewCollectorInput, + ) => Promise; +}; + +export function serializePreparedGitHubContext(context: GitHubReviewContext | null): string { + const serialized = `${JSON.stringify(context, null, 2)}\n`; + if (Buffer.byteLength(serialized, "utf8") > MAX_PREPARED_GITHUB_CONTEXT_BYTES) { + throw new Error("Prepared GitHub context exceeds the 5 MiB limit"); + } + return serialized; +} + +export function readPreparedGitHubContext( + filePath: string, + expected: { prNumber?: number; repo?: string } = {}, +): GitHubReviewContext | null { + const noFollow = fs.constants.O_NOFOLLOW; + if (typeof noFollow !== "number") { + throw new Error("Prepared GitHub context requires secure no-follow file access"); + } + + let descriptor: number; + try { + descriptor = fs.openSync( + filePath, + fs.constants.O_RDONLY | noFollow | (fs.constants.O_NONBLOCK ?? 0), + ); + } catch (error) { + throw new Error("Prepared GitHub context must be a regular file", { cause: error }); + } + + const content = Buffer.allocUnsafe(MAX_PREPARED_GITHUB_CONTEXT_BYTES + 1); + let bytesRead = 0; + try { + const stat = fs.fstatSync(descriptor); + if (!stat.isFile()) { + throw new Error("Prepared GitHub context must be a regular file"); + } + if (stat.size > MAX_PREPARED_GITHUB_CONTEXT_BYTES) { + throw new Error("Prepared GitHub context exceeds the 5 MiB limit"); + } + while (bytesRead < content.length) { + const count = fs.readSync(descriptor, content, bytesRead, content.length - bytesRead, null); + if (count === 0) break; + bytesRead += count; + } + } finally { + fs.closeSync(descriptor); + } + if (bytesRead > MAX_PREPARED_GITHUB_CONTEXT_BYTES) { + throw new Error("Prepared GitHub context exceeds the 5 MiB limit"); + } + + const parsed = JSON.parse(content.toString("utf8", 0, bytesRead)) as unknown; + if (parsed === null) return null; + if (!isObjectRecord(parsed)) { + throw new Error("Prepared GitHub context must be a JSON object or null"); + } + const repo = stringOrUndefined(parsed.repo); + const prNumber = parsed.prNumber; + if (!repo || typeof prNumber !== "number" || !Number.isSafeInteger(prNumber) || prNumber <= 0) { + throw new Error("Prepared GitHub context is missing its repository or pull request identity"); + } + if (expected.repo && repo !== expected.repo) { + throw new Error("Prepared GitHub context repository does not match the workflow target"); + } + if (expected.prNumber && prNumber !== expected.prNumber) { + throw new Error("Prepared GitHub context pull request does not match the workflow target"); + } + return parsed as GitHubReviewContext; +} + +export async function collectGitHubReviewContext( + env: NodeJS.ProcessEnv, + options: CollectGitHubReviewContextOptions = {}, +): Promise { + const repo = env.TARGET_REPO || env.GITHUB_REPOSITORY; + const prNumber = Number.parseInt( + env.PR_NUMBER || env.GITHUB_REF_NAME?.match(/^(\d+)\//u)?.[1] || "", + 10, + ); + const preparedPath = env.PR_REVIEW_ADVISOR_GITHUB_CONTEXT_PATH; + if (preparedPath) { + return readPreparedGitHubContext(preparedPath, { + repo, + prNumber: Number.isFinite(prNumber) && prNumber > 0 ? prNumber : undefined, + }); + } + + const token = env.GH_TOKEN || env.GITHUB_TOKEN; + if (!repo || !Number.isFinite(prNumber) || prNumber <= 0 || !token) return null; + + const loadPreviousReview = env.PR_REVIEW_ADVISOR_LOAD_PREVIOUS_REVIEW === "true"; + if (loadPreviousReview && !options.collectPreviousReview) { + throw new Error("Prepared GitHub context cannot load a previous review without provenance"); + } + + const context: GitHubReviewContext = { repo, prNumber }; + try { + const [rawPullRequest, issueComments, openPulls] = await Promise.all([ + githubRest(`repos/${repo}/pulls/${prNumber}`, token), + loadPreviousReview + ? githubRestPaginated(`repos/${repo}/issues/${prNumber}/comments`, token, 100) + : Promise.resolve([]), + githubRestPaginated( + `repos/${repo}/pulls?state=open&sort=updated&direction=desc`, + token, + 100, + ), + ]); + context.pullRequest = summarizePullRequest(rawPullRequest); + context.previousAdvisorReview = loadPreviousReview + ? await options.collectPreviousReview?.({ + repo, + token, + issueComments, + prNumber, + currentBaseSha: stringOrUndefined(getPath(rawPullRequest, ["base", "sha"])), + }) + : null; + const prTitle = stringOrUndefined(getPath(rawPullRequest, ["title"])) || ""; + const prBody = stringOrUndefined(getPath(rawPullRequest, ["body"])) || ""; + const prText = [ + prTitle, + prBody, + stringOrUndefined(getPath(rawPullRequest, ["head", "ref"])), + ] + .filter(Boolean) + .join("\n"); + const issueNumbers = extractIssueRefs(prText, prNumber).slice(0, 5); + context.issueReferenceLines = [prTitle, ...prBody.split("\n")] + .map((line) => line.trim()) + .filter((line) => line && extractIssueRefs(line, prNumber).length > 0) + .map((line) => boundedText(line, 2_000, "issue-reference line") as string) + .slice(0, 20); + context.linkedIssues = await Promise.all( + issueNumbers.map((issue) => collectLinkedIssue(repo, issue, token)), + ); + context.openPrOverlaps = await collectOpenPrOverlaps( + repo, + prNumber, + token, + openPulls, + issueNumbers, + ); + } catch (error: unknown) { + context.fetchError = error instanceof Error ? error.message : String(error); + } + return context; +} + +function summarizePullRequest(value: unknown): unknown { + if (!isObjectRecord(value)) return value; + return { + number: getPath(value, ["number"]), + title: stringOrUndefined(getPath(value, ["title"])), + body: boundedText(getPath(value, ["body"]), BODY_CHARACTER_LIMIT, "pull-request body"), + state: stringOrUndefined(getPath(value, ["state"])), + draft: getPath(value, ["draft"]), + author_association: stringOrUndefined(getPath(value, ["author_association"])), + user: summarizeUser(getPath(value, ["user"])), + labels: summarizeLabels(getPath(value, ["labels"])), + head: summarizeGitRef(getPath(value, ["head"])), + base: summarizeGitRef(getPath(value, ["base"])), + created_at: stringOrUndefined(getPath(value, ["created_at"])), + updated_at: stringOrUndefined(getPath(value, ["updated_at"])), + }; +} + +function summarizeIssue(value: unknown): unknown { + if (!isObjectRecord(value)) return value; + return { + number: getPath(value, ["number"]), + title: stringOrUndefined(getPath(value, ["title"])), + body: boundedText(getPath(value, ["body"]), BODY_CHARACTER_LIMIT, "issue body"), + state: stringOrUndefined(getPath(value, ["state"])), + state_reason: stringOrUndefined(getPath(value, ["state_reason"])), + author_association: stringOrUndefined(getPath(value, ["author_association"])), + user: summarizeUser(getPath(value, ["user"])), + labels: summarizeLabels(getPath(value, ["labels"])), + created_at: stringOrUndefined(getPath(value, ["created_at"])), + updated_at: stringOrUndefined(getPath(value, ["updated_at"])), + }; +} + +function summarizeComment(value: unknown): unknown { + if (!isObjectRecord(value)) return value; + return { + id: getPath(value, ["id"]), + body: boundedText( + getPath(value, ["body"]), + COMMENT_BODY_CHARACTER_LIMIT, + "issue comment", + ), + author_association: stringOrUndefined(getPath(value, ["author_association"])), + user: summarizeUser(getPath(value, ["user"])), + created_at: stringOrUndefined(getPath(value, ["created_at"])), + updated_at: stringOrUndefined(getPath(value, ["updated_at"])), + }; +} + +function summarizeUser(value: unknown): unknown { + const login = stringOrUndefined(getPath(value, ["login"])); + return login ? { login } : undefined; +} + +function summarizeLabels(value: unknown): Array> { + return recordItems(value) + .map((label) => ({ + name: stringOrUndefined(label.name), + color: stringOrUndefined(label.color), + description: boundedText(label.description, 1_000, "label description"), + })) + .slice(0, 100); +} + +function summarizeGitRef(value: unknown): unknown { + if (!isObjectRecord(value)) return value; + return { + ref: stringOrUndefined(value.ref), + sha: stringOrUndefined(value.sha), + repo: { + full_name: stringOrUndefined(getPath(value, ["repo", "full_name"])), + }, + }; +} + +function boundedText(value: unknown, limit: number, label: string): string | undefined { + const text = stringOrUndefined(value); + if (!text || text.length <= limit) return text; + const marker = `\n\n[PR Review Advisor truncated content from the middle of this ${label}.]\n\n`; + const retained = Math.max(0, limit - marker.length); + const headLength = Math.ceil(retained / 2); + return `${text.slice(0, headLength)}${marker}${text.slice(text.length - (retained - headLength))}`; +} + +async function collectLinkedIssue( + repo: string, + number: number, + token: string, +): Promise { + try { + const [issue, comments] = await Promise.all([ + githubRest(`repos/${repo}/issues/${number}`, token), + githubRestPaginated(`repos/${repo}/issues/${number}/comments`, token, 50), + ]); + return { + number, + issue: summarizeIssue(issue), + comments: comments.map(summarizeComment), + }; + } catch (error: unknown) { + return { number, fetchError: error instanceof Error ? error.message : String(error) }; + } +} + +async function mapWithConcurrency( + items: readonly T[], + concurrency: number, + mapper: (item: T, index: number) => Promise, +): Promise { + const results = new Array(items.length); + let nextIndex = 0; + const workerCount = Math.min(Math.max(1, concurrency), items.length); + const workers = Array.from({ length: workerCount }, async () => { + while (nextIndex < items.length) { + const index = nextIndex; + nextIndex += 1; + results[index] = await mapper(items[index] as T, index); + } + }); + await Promise.all(workers); + return results; +} + +async function collectOpenPrOverlaps( + repo: string, + currentPrNumber: number, + token: string, + openPulls: unknown[], + currentLinkedIssues: number[], +): Promise { + const currentFiles = new Set( + ( + await githubRestPaginated<{ filename?: string }>( + `repos/${repo}/pulls/${currentPrNumber}/files`, + token, + 300, + ) + ) + .map((file) => file.filename) + .filter((file): file is string => typeof file === "string"), + ); + const candidatePulls = openPulls + .filter((pull) => getPath(pull, ["number"]) !== currentPrNumber) + .slice(0, OPEN_PR_OVERLAP_LIMIT); + const overlaps = await mapWithConcurrency( + candidatePulls, + OPEN_PR_OVERLAP_CONCURRENCY, + async (pull): Promise => { + const number = getPath(pull, ["number"]); + if (!number) return null; + const title = stringOrDefault(getPath(pull, ["title"]), `PR #${number}`); + const body = stringOrDefault(getPath(pull, ["body"]), ""); + const labels = recordItems(getPath(pull, ["labels"])) + .map((label) => stringOrUndefined(label.name)) + .filter((label): label is string => Boolean(label)) + .slice(0, 100) + .map((label) => boundedText(label, 200, "pull-request label") as string); + const allLinkedIssues = extractIssueRefs(`${title}\n${body}`, number); + const duplicateLinkedIssues = allLinkedIssues.filter((issue) => + currentLinkedIssues.includes(issue), + ); + let allSameFiles: string[] = []; + if (currentFiles.size > 0) { + try { + allSameFiles = ( + await githubRestPaginated<{ filename?: string }>( + `repos/${repo}/pulls/${number}/files`, + token, + 300, + ) + ) + .map((file) => file.filename) + .filter((file): file is string => typeof file === "string" && currentFiles.has(file)); + } catch { + allSameFiles = []; + } + } + const uniqueSameFiles = [...new Set(allSameFiles)]; + if (uniqueSameFiles.length === 0 && duplicateLinkedIssues.length === 0) return null; + return { + number, + title: boundedText(title, 1_000, "pull-request title") as string, + labels, + linkedIssues: allLinkedIssues.slice(0, OVERLAP_LINKED_ISSUE_LIMIT), + linkedIssueCount: allLinkedIssues.length, + sameFiles: uniqueSameFiles + .slice(0, OVERLAP_SAME_FILE_SAMPLE_LIMIT) + .map( + (file) => boundedText(file, OVERLAP_PATH_CHARACTER_LIMIT, "overlapping path") as string, + ), + sameFileCount: uniqueSameFiles.length, + duplicateLinkedIssues, + }; + }, + ); + return overlaps + .filter((overlap): overlap is OpenPrOverlap => overlap !== null) + .sort( + (a, b) => + b.sameFileCount - a.sameFileCount || + b.duplicateLinkedIssues.length - a.duplicateLinkedIssues.length || + a.number - b.number, + ) + .slice(0, 25); +} + +export function extractIssueRefs(text: string, prNumber: number): number[] { + const numbers = new Set(); + const relationPattern = + /\b(?:fixes|closes|resolves|refs?|references?|related(?:\s+issue)?|linked(?:\s+issue)?|follow[- ]?up(?:\s+to)?)\s+(#\d+(?:\s*(?:,\s*(?:and\s+)?|and\s+|&\s*)#\d+)*)/giu; + for (const relation of text.matchAll(relationPattern)) { + for (const match of (relation[1] ?? "").matchAll(/#(\d+)/gu)) { + const number = Number.parseInt(match[1] || "", 10); + if (Number.isFinite(number) && number > 0 && number !== prNumber) numbers.add(number); + } + } + for (const pattern of [/\(#(\d+)\)/gu, /issue[-_/](\d+)/giu]) { + for (const match of text.matchAll(pattern)) { + const number = Number.parseInt(match[1] || "", 10); + if (Number.isFinite(number) && number > 0 && number !== prNumber) numbers.add(number); + } + } + return [...numbers].sort((a, b) => a - b); +} diff --git a/tools/pr-review-advisor/openshell-policy.yaml b/tools/pr-review-advisor/openshell-policy.yaml new file mode 100644 index 0000000000..0b08bd28f9 --- /dev/null +++ b/tools/pr-review-advisor/openshell-policy.yaml @@ -0,0 +1,28 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +version: 1 + +filesystem_policy: + include_workdir: false + read_only: + - /usr/bin + - /usr/lib + - /usr/share/git-core + - /etc + - /advisor + - /pr-workdir + - /pr-review-advisor-context + - /pr-review-advisor-tools + read_write: + - /dev + - /sandbox/pr-review-advisor-runtime + +landlock: + compatibility: hard_requirement + +process: + run_as_user: sandbox + run_as_group: sandbox + +network_policies: {} diff --git a/tools/pr-review-advisor/openshell.mts b/tools/pr-review-advisor/openshell.mts new file mode 100755 index 0000000000..72def4f96f --- /dev/null +++ b/tools/pr-review-advisor/openshell.mts @@ -0,0 +1,613 @@ +#!/usr/bin/env node +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { execFileSync } from "node:child_process"; +import { randomUUID } from "node:crypto"; +import fs from "node:fs"; +import path from "node:path"; +import { pathToFileURL } from "node:url"; + +import { ADVISOR_OPENSHELL_INFERENCE_BASE_URL } from "../advisors/provider-constants.mts"; +import { + configureOpenShellInference, + createOpenShellSandbox, + credentialFreeEnvironment, + defaultOpenShellTools, + deleteOpenShellSandbox, + downloadOpenShellPath, + execOpenShellSandbox, + type OpenShellTools, + required, +} from "../openshell-agent/runtime.mts"; +import { + collectGitHubReviewContext, + type GitHubReviewContext, + serializePreparedGitHubContext, +} from "./github-context.mts"; + +const ADVISOR_CONTEXT_DIRECTORY_NAME = "pr-review-advisor-context"; +const ADVISOR_RUNTIME_DIRECTORY_NAME = "pr-review-advisor-runtime"; +const ADVISOR_TOOLS_DIRECTORY_NAME = "pr-review-advisor-tools"; +const ADVISOR_BOUNDARY_PROOF_DIRECTORY_NAME = ".pr-review-advisor-boundary-proof"; +const REPOSITORY_BOUNDARY_PROOF_DIRECTORY = `.git/${ADVISOR_BOUNDARY_PROOF_DIRECTORY_NAME}`; +const ADVISOR_BOUNDARY_PROOF_SOURCE_NAME = "source"; +const ADVISOR_BOUNDARY_PROOF_TARGET_NAME = "target"; +const ADVISOR_CONTEXT_FILE_NAME = "github-context.json"; +const SANDBOX_ADVISOR_DIR = "/advisor"; +const SANDBOX_WORKDIR = "/pr-workdir"; +const SANDBOX_GIT_DIR = `${SANDBOX_WORKDIR}/.git`; +const SANDBOX_CONTEXT_DIR = `/${ADVISOR_CONTEXT_DIRECTORY_NAME}`; +const SANDBOX_RUNTIME_DIR = `/sandbox/${ADVISOR_RUNTIME_DIRECTORY_NAME}`; +const SANDBOX_TOOLS_DIR = `/${ADVISOR_TOOLS_DIRECTORY_NAME}`; +const SANDBOX_CONTEXT_PATH = `${SANDBOX_CONTEXT_DIR}/${ADVISOR_CONTEXT_FILE_NAME}`; +const ADVISOR_RUNTIME_TMPFS_BYTES = 512 * 1024 * 1024; +const SANDBOX_API_KEY = "unused"; +const DEFAULT_SANDBOX_TIMEOUT_SECONDS = 2100; +const DEFAULT_UNAVAILABLE_REASON = + "OpenShell inference configuration failed or the advisor credential is unavailable"; +const EXPECTED_WRITE_DENIAL_CODES = new Set(["EACCES", "EPERM", "EROFS"]); + +type PrepareAdvisorSandboxOptions = { + collectContext?: (env: NodeJS.ProcessEnv) => Promise; + resolveExecutable?: (name: string, env: NodeJS.ProcessEnv) => string; +}; + +function runnerDirectory(env: NodeJS.ProcessEnv, name: string): string { + return path.join(required(env.RUNNER_TEMP, "RUNNER_TEMP"), name); +} + +function resetDirectory(directory: string): void { + fs.rmSync(directory, { recursive: true, force: true }); + fs.mkdirSync(directory, { recursive: true, mode: 0o700 }); +} + +function createBoundaryProof(directory: string, relativeProofDirectory: string): void { + const proofDirectory = path.join(directory, relativeProofDirectory); + resetDirectory(proofDirectory); + // These canaries are intentionally writable by an unrelated UID. That + // prevents ordinary host ownership from making the sandbox proof pass when + // neither the read-only mount nor Landlock actually blocks mutation. + for (const [name, content] of [ + [ADVISOR_BOUNDARY_PROOF_SOURCE_NAME, "source\n"], + [ADVISOR_BOUNDARY_PROOF_TARGET_NAME, "target\n"], + ] as const) { + const proofFile = path.join(proofDirectory, name); + fs.writeFileSync(proofFile, content, { flag: "wx", mode: 0o666 }); + fs.chmodSync(proofFile, 0o666); + } + fs.chmodSync(proofDirectory, 0o777); +} + +function writeExclusive(file: string, content: string): void { + const fd = fs.openSync( + file, + fs.constants.O_CREAT | fs.constants.O_EXCL | fs.constants.O_WRONLY, + 0o600, + ); + try { + // lgtm[js/network-data-to-file] The prepared GitHub context is bounded, + // serialized JSON written to a fixed runner-owned path through an exclusive + // 0600 descriptor. The sandbox mounts it read-only and never executes it. + // lgtm[js/http-to-file-access] + fs.writeFileSync(fd, content); + } finally { + fs.closeSync(fd); + } +} + +function resolveExecutable(name: string, env: NodeJS.ProcessEnv): string { + return execFileSync("which", [name], { + encoding: "utf8", + env, + stdio: ["ignore", "pipe", "inherit"], + }).trim(); +} + +function copyExecutable(source: string, destination: string): void { + const resolvedSource = fs.realpathSync(source); + if (!fs.statSync(resolvedSource).isFile()) { + throw new Error(`Advisor runtime executable is not a regular file: ${source}`); + } + fs.copyFileSync(resolvedSource, destination); + fs.chmodSync(destination, 0o755); +} + +function requireDirectoryBasename(directory: string, expected: string, name: string): void { + if (path.basename(path.resolve(directory)) !== expected) { + throw new Error(`${name} must end in ${expected}`); + } +} + +function canonicalDirectory(directory: string, expected: string, name: string): string { + const canonical = fs.realpathSync(directory); + requireDirectoryBasename(canonical, expected, name); + if (!fs.statSync(canonical).isDirectory()) { + throw new Error(`${name} must be a directory`); + } + return canonical; +} + +function requireGitMetadataDirectory(directory: string, name: string): void { + const gitDirectory = path.join(directory, ".git"); + if (!fs.existsSync(gitDirectory) || !fs.lstatSync(gitDirectory).isDirectory()) { + throw new Error(`${name} must contain a .git directory`); + } +} + +function advisorSandboxDriverConfig(input: { + advisorDirectory: string; + contextDirectory: string; + toolsDirectory: string; + workdir: string; +}): Readonly> { + return { + docker: { + mounts: [ + { + type: "bind", + source: input.advisorDirectory, + target: SANDBOX_ADVISOR_DIR, + read_only: true, + }, + { + type: "bind", + source: input.workdir, + target: SANDBOX_WORKDIR, + read_only: true, + }, + { + type: "bind", + source: input.contextDirectory, + target: SANDBOX_CONTEXT_DIR, + read_only: true, + }, + { + type: "bind", + source: input.toolsDirectory, + target: SANDBOX_TOOLS_DIR, + read_only: true, + }, + { + type: "tmpfs", + target: SANDBOX_RUNTIME_DIR, + size_bytes: ADVISOR_RUNTIME_TMPFS_BYTES, + // Docker creates tmpfs mounts as root. The sandbox user needs the + // mount root only to create private 0700 application directories. + mode: 0o1777, + }, + ], + }, + }; +} + +export async function prepareAdvisorSandboxInputs( + env: NodeJS.ProcessEnv, + options: PrepareAdvisorSandboxOptions = {}, +): Promise { + const advisorDirectory = canonicalDirectory( + required(env.ADVISOR_DIR, "ADVISOR_DIR"), + "advisor", + "ADVISOR_DIR", + ); + const advisorWorkdir = canonicalDirectory( + required(env.ADVISOR_WORKDIR, "ADVISOR_WORKDIR"), + "pr-workdir", + "ADVISOR_WORKDIR", + ); + const contextDirectory = runnerDirectory(env, ADVISOR_CONTEXT_DIRECTORY_NAME); + const toolsDirectory = runnerDirectory(env, ADVISOR_TOOLS_DIRECTORY_NAME); + requireGitMetadataDirectory(advisorDirectory, "ADVISOR_DIR"); + requireGitMetadataDirectory(advisorWorkdir, "ADVISOR_WORKDIR"); + resetDirectory(contextDirectory); + resetDirectory(toolsDirectory); + + const contextEnv = { ...env }; + delete contextEnv.PR_REVIEW_ADVISOR_GITHUB_CONTEXT_PATH; + const context = await (options.collectContext ?? collectGitHubReviewContext)(contextEnv); + writeExclusive( + path.join(contextDirectory, ADVISOR_CONTEXT_FILE_NAME), + serializePreparedGitHubContext(context), + ); + fs.chmodSync(path.join(contextDirectory, ADVISOR_CONTEXT_FILE_NAME), 0o444); + + const findExecutable = options.resolveExecutable ?? resolveExecutable; + const rg = findExecutable("rg", env); + const fdfind = findExecutable("fdfind", env); + copyExecutable(rg, path.join(toolsDirectory, "rg")); + copyExecutable(fdfind, path.join(toolsDirectory, "fdfind")); + copyExecutable(fdfind, path.join(toolsDirectory, "fd")); + for (const executable of ["rg", "fdfind", "fd"]) { + fs.chmodSync(path.join(toolsDirectory, executable), 0o555); + } + + createBoundaryProof(advisorDirectory, REPOSITORY_BOUNDARY_PROOF_DIRECTORY); + createBoundaryProof(advisorWorkdir, REPOSITORY_BOUNDARY_PROOF_DIRECTORY); + createBoundaryProof(contextDirectory, ADVISOR_BOUNDARY_PROOF_DIRECTORY_NAME); + createBoundaryProof(toolsDirectory, ADVISOR_BOUNDARY_PROOF_DIRECTORY_NAME); + fs.chmodSync(contextDirectory, 0o755); + fs.chmodSync(toolsDirectory, 0o755); +} + +export async function configureAdvisorOpenShellInference( + env: NodeJS.ProcessEnv, + tools: OpenShellTools = defaultOpenShellTools, +): Promise { + await configureOpenShellInference( + env, + { + enableBindMounts: true, + gatewayId: "pr-review-advisor", + modelId: required(env.PR_REVIEW_ADVISOR_MODEL, "PR_REVIEW_ADVISOR_MODEL"), + providerName: "advisor", + }, + tools, + ); +} + +export function writeUnavailableAdvisorArtifacts( + env: NodeJS.ProcessEnv, + tools: OpenShellTools = defaultOpenShellTools, +): void { + const advisorDirectory = required(env.ADVISOR_DIR, "ADVISOR_DIR"); + const commandEnv = credentialFreeEnvironment({ + ...env, + PR_REVIEW_ADVISOR_GITHUB_CONTEXT_PATH: path.join( + runnerDirectory(env, ADVISOR_CONTEXT_DIRECTORY_NAME), + ADVISOR_CONTEXT_FILE_NAME, + ), + PR_REVIEW_ADVISOR_RUN_ANALYSIS: "0", + PR_REVIEW_ADVISOR_UNAVAILABLE_REASON: + env.PR_REVIEW_ADVISOR_UNAVAILABLE_REASON || DEFAULT_UNAVAILABLE_REASON, + }); + tools.run( + process.execPath, + [ + "--experimental-strip-types", + "--no-warnings", + path.join(advisorDirectory, "tools", "pr-review-advisor", "run-analysis.mts"), + ], + { env: commandEnv }, + ); +} + +export function createAdvisorSandbox( + env: NodeJS.ProcessEnv, + tools: OpenShellTools = defaultOpenShellTools, +): void { + const advisorDirectory = canonicalDirectory( + required(env.ADVISOR_DIR, "ADVISOR_DIR"), + "advisor", + "ADVISOR_DIR", + ); + const advisorWorkdir = canonicalDirectory( + required(env.ADVISOR_WORKDIR, "ADVISOR_WORKDIR"), + "pr-workdir", + "ADVISOR_WORKDIR", + ); + const contextDirectory = canonicalDirectory( + runnerDirectory(env, ADVISOR_CONTEXT_DIRECTORY_NAME), + ADVISOR_CONTEXT_DIRECTORY_NAME, + "advisor context directory", + ); + const toolsDirectory = canonicalDirectory( + runnerDirectory(env, ADVISOR_TOOLS_DIRECTORY_NAME), + ADVISOR_TOOLS_DIRECTORY_NAME, + "advisor tools directory", + ); + const sandboxName = required(env.SANDBOX_NAME, "SANDBOX_NAME"); + + createOpenShellSandbox( + env, + { + name: sandboxName, + image: required(env.PI_IMAGE, "PI_IMAGE"), + policyPath: path.join( + advisorDirectory, + "tools", + "pr-review-advisor", + "openshell-policy.yaml", + ), + driverConfig: advisorSandboxDriverConfig({ + advisorDirectory, + contextDirectory, + toolsDirectory, + workdir: advisorWorkdir, + }), + uploads: [], + command: [ + "/usr/bin/node", + "--experimental-strip-types", + "--no-warnings", + `${SANDBOX_ADVISOR_DIR}/tools/pr-review-advisor/openshell.mts`, + "initialize", + ], + }, + tools, + ); +} + +function passthroughEnvironment(env: NodeJS.ProcessEnv): Record { + const result: Record = {}; + for (const name of [ + "BASE_REF", + "GITHUB_REPOSITORY", + "HEAD_REF", + "PR_NUMBER", + "PR_REVIEW_ADVISOR_ARTIFACT_DIR", + "PR_REVIEW_ADVISOR_COMMENT_LABEL", + "PR_REVIEW_ADVISOR_COMMENT_MARKER", + "PR_REVIEW_ADVISOR_COMMENT_TITLE", + "PR_REVIEW_ADVISOR_HEARTBEAT_MS", + "PR_REVIEW_ADVISOR_LOAD_PREVIOUS_REVIEW", + "PR_REVIEW_ADVISOR_MAX_CAPTURE_BYTES", + "PR_REVIEW_ADVISOR_MODEL", + "PR_REVIEW_ADVISOR_RUN_ANALYSIS", + "PR_REVIEW_ADVISOR_TIMEOUT_MS", + "PR_REVIEW_ADVISOR_UNAVAILABLE_REASON", + "PR_REVIEW_ADVISOR_WORKFLOW_NAME", + "PR_REVIEW_ADVISOR_WORKFLOW_PATH", + "TARGET_REPO", + ] as const) { + if (env[name]) result[name] = env[name] as string; + } + return result; +} + +function sandboxTimeoutSeconds(env: NodeJS.ProcessEnv): number { + const value = Number.parseInt(env.PR_REVIEW_ADVISOR_SANDBOX_TIMEOUT_SECONDS ?? "", 10); + return Number.isSafeInteger(value) && value > 0 ? value : DEFAULT_SANDBOX_TIMEOUT_SECONDS; +} + +function advisorArtifactDirectory(env: NodeJS.ProcessEnv): string { + const value = required(env.PR_REVIEW_ADVISOR_ARTIFACT_DIR, "PR_REVIEW_ADVISOR_ARTIFACT_DIR"); + if (!/^[a-z0-9][a-z0-9-]*$/u.test(value)) { + throw new Error("PR_REVIEW_ADVISOR_ARTIFACT_DIR must be a simple directory name"); + } + return value; +} + +export function runAdvisorSandbox( + env: NodeJS.ProcessEnv, + tools: OpenShellTools = defaultOpenShellTools, +): void { + advisorArtifactDirectory(env); + execOpenShellSandbox( + env, + { + name: required(env.SANDBOX_NAME, "SANDBOX_NAME"), + timeoutSeconds: sandboxTimeoutSeconds(env), + workdir: SANDBOX_WORKDIR, + environment: { + ...passthroughEnvironment(env), + ADVISOR_DIR: SANDBOX_ADVISOR_DIR, + ADVISOR_WORKDIR: SANDBOX_WORKDIR, + GIT_DIR: SANDBOX_GIT_DIR, + GIT_WORK_TREE: SANDBOX_WORKDIR, + GITHUB_WORKSPACE: SANDBOX_RUNTIME_DIR, + HOME: SANDBOX_RUNTIME_DIR, + PATH: `${SANDBOX_TOOLS_DIR}:/usr/bin`, + PI_OFFLINE: "1", + PR_REVIEW_ADVISOR_API_KEY: SANDBOX_API_KEY, + PR_REVIEW_ADVISOR_BASE_URL: ADVISOR_OPENSHELL_INFERENCE_BASE_URL, + PR_REVIEW_ADVISOR_CONFIG_DIR: `${SANDBOX_RUNTIME_DIR}/config`, + PR_REVIEW_ADVISOR_GITHUB_CONTEXT_PATH: SANDBOX_CONTEXT_PATH, + TMPDIR: `${SANDBOX_RUNTIME_DIR}/tmp`, + }, + command: [ + "/usr/bin/node", + "--experimental-strip-types", + "--no-warnings", + `${SANDBOX_ADVISOR_DIR}/tools/pr-review-advisor/run-analysis.mts`, + ], + }, + tools, + ); +} + +export function downloadAdvisorArtifacts( + env: NodeJS.ProcessEnv, + tools: OpenShellTools = defaultOpenShellTools, +): void { + const artifactDirectory = advisorArtifactDirectory(env); + const destination = path.join( + required(env.GITHUB_WORKSPACE, "GITHUB_WORKSPACE"), + "artifacts", + artifactDirectory, + ); + fs.mkdirSync(destination, { recursive: true }); + downloadOpenShellPath( + env, + { + name: required(env.SANDBOX_NAME, "SANDBOX_NAME"), + source: `${SANDBOX_RUNTIME_DIR}/artifacts/${artifactDirectory}`, + destination, + }, + tools, + ); +} + +export function deleteAdvisorSandbox( + env: NodeJS.ProcessEnv, + tools: OpenShellTools = defaultOpenShellTools, +): void { + deleteOpenShellSandbox(env, required(env.SANDBOX_NAME, "SANDBOX_NAME"), tools); +} + +export function checkAdvisorSandboxRuntime(): void { + for (const [command, expectedPrefix] of [ + ["git", "git version "], + ["rg", "ripgrep "], + ["fdfind", "fdfind "], + ] as const) { + const output = execFileSync(command, ["--version"], { + encoding: "utf8", + env: { ...process.env, PATH: `${SANDBOX_TOOLS_DIR}:/usr/bin` }, + stdio: ["ignore", "pipe", "inherit"], + }); + if (!output.startsWith(expectedPrefix)) { + throw new Error(`Unexpected ${command} identity in advisor sandbox`); + } + } + for (const directory of [ + SANDBOX_ADVISOR_DIR, + SANDBOX_WORKDIR, + SANDBOX_CONTEXT_DIR, + SANDBOX_RUNTIME_DIR, + SANDBOX_TOOLS_DIR, + ]) { + if (!fs.statSync(directory).isDirectory()) { + throw new Error(`Advisor sandbox input is not a directory: ${directory}`); + } + } + verifyAdvisorGitWorktree(); + + const probeName = `.pr-review-advisor-write-check-${randomUUID()}`; + for (const directory of [ + SANDBOX_RUNTIME_DIR, + `${SANDBOX_RUNTIME_DIR}/artifacts`, + `${SANDBOX_RUNTIME_DIR}/config`, + `${SANDBOX_RUNTIME_DIR}/tmp`, + ]) { + const runtimeProbe = path.join(directory, probeName); + const runtimeTarget = `${runtimeProbe}-target`; + fs.writeFileSync(runtimeProbe, "runtime create check\n", { + flag: "wx", + mode: 0o600, + }); + fs.writeFileSync(runtimeTarget, "runtime target check\n", { + flag: "wx", + mode: 0o600, + }); + fs.writeFileSync(runtimeProbe, "runtime overwrite check\n", { flag: "w" }); + fs.chmodSync(runtimeProbe, 0o640); + fs.renameSync(runtimeProbe, runtimeTarget); + if (fs.readFileSync(runtimeTarget, "utf8") !== "runtime overwrite check\n") { + throw new Error(`Advisor sandbox runtime replacement failed: ${directory}`); + } + fs.rmSync(runtimeTarget); + } + for (const [directory, relativeProofDirectory] of [ + [SANDBOX_ADVISOR_DIR, REPOSITORY_BOUNDARY_PROOF_DIRECTORY], + [SANDBOX_WORKDIR, REPOSITORY_BOUNDARY_PROOF_DIRECTORY], + [SANDBOX_CONTEXT_DIR, ADVISOR_BOUNDARY_PROOF_DIRECTORY_NAME], + [SANDBOX_TOOLS_DIR, ADVISOR_BOUNDARY_PROOF_DIRECTORY_NAME], + ] as const) { + const proofDirectory = path.join(directory, relativeProofDirectory); + const source = path.join(proofDirectory, ADVISOR_BOUNDARY_PROOF_SOURCE_NAME); + const target = path.join(proofDirectory, ADVISOR_BOUNDARY_PROOF_TARGET_NAME); + if ( + fs.readFileSync(source, "utf8") !== "source\n" || + fs.readFileSync(target, "utf8") !== "target\n" + ) { + throw new Error(`Advisor sandbox input canary is unreadable or invalid: ${directory}`); + } + expectWriteDenied(`${directory} chmod`, () => fs.chmodSync(source, 0o600)); + expectWriteDenied(`${directory} overwrite`, () => + fs.writeFileSync(target, "unexpected replacement\n", { flag: "w" }), + ); + expectWriteDenied(`${directory} replacement`, () => fs.renameSync(source, target)); + expectWriteDenied(`${directory} create`, () => + fs.writeFileSync(path.join(proofDirectory, probeName), "unexpected create\n", { + flag: "wx", + mode: 0o600, + }), + ); + } + console.log( + "Advisor sandbox filesystem proof passed: four immutable inputs and one writable runtime", + ); +} + +export function verifyAdvisorGitWorktree( + workdir = SANDBOX_WORKDIR, + gitDirectory = path.join(workdir, ".git"), +): void { + const gitEnvironment = { + ...process.env, + GIT_DIR: gitDirectory, + GIT_WORK_TREE: workdir, + PATH: `${SANDBOX_TOOLS_DIR}:/usr/bin`, + }; + let topLevel: string; + let head: string; + try { + topLevel = execFileSync("git", ["rev-parse", "--show-toplevel"], { + encoding: "utf8", + env: gitEnvironment, + stdio: ["ignore", "pipe", "pipe"], + }).trim(); + head = execFileSync("git", ["rev-parse", "--verify", "HEAD^{commit}"], { + encoding: "utf8", + env: gitEnvironment, + stdio: ["ignore", "pipe", "pipe"], + }).trim(); + } catch { + throw new Error(`Advisor sandbox Git checkout is unreadable or invalid: ${workdir}`); + } + if (fs.realpathSync(topLevel) !== fs.realpathSync(workdir)) { + throw new Error(`Advisor sandbox Git worktree resolved outside ${workdir}: ${topLevel}`); + } + if (!/^[0-9a-f]{40}$/u.test(head)) { + throw new Error(`Advisor sandbox Git HEAD is invalid: ${head}`); + } +} + +function expectWriteDenied(label: string, operation: () => void): void { + try { + operation(); + } catch (error: unknown) { + if (EXPECTED_WRITE_DENIAL_CODES.has((error as NodeJS.ErrnoException).code ?? "")) return; + throw error; + } + throw new Error(`Advisor sandbox input mutation unexpectedly succeeded: ${label}`); +} + +export function initializeAdvisorSandboxRuntime(): void { + for (const name of ["artifacts", "config", "tmp"]) { + fs.mkdirSync(path.join(SANDBOX_RUNTIME_DIR, name), { mode: 0o700 }); + } + checkAdvisorSandboxRuntime(); +} + +async function main(): Promise { + const command = required(process.argv[2], "openshell command"); + switch (command) { + case "prepare": + await prepareAdvisorSandboxInputs(process.env); + return; + case "configure": + await configureAdvisorOpenShellInference(process.env); + return; + case "unavailable": + writeUnavailableAdvisorArtifacts(process.env); + return; + case "create": + createAdvisorSandbox(process.env); + return; + case "run": + runAdvisorSandbox(process.env); + return; + case "download": + downloadAdvisorArtifacts(process.env); + return; + case "delete": + deleteAdvisorSandbox(process.env); + return; + case "initialize": + initializeAdvisorSandboxRuntime(); + return; + case "check": + checkAdvisorSandboxRuntime(); + return; + default: + throw new Error(`Unsupported OpenShell advisor command: ${command}`); + } +} + +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + main().catch((error: unknown) => { + console.error(error instanceof Error ? error.message : String(error)); + process.exit(1); + }); +} diff --git a/tools/pr-review-advisor/prepare-target-pr.mts b/tools/pr-review-advisor/prepare-target-pr.mts index 71755dcdec..b3b0f3937d 100755 --- a/tools/pr-review-advisor/prepare-target-pr.mts +++ b/tools/pr-review-advisor/prepare-target-pr.mts @@ -4,6 +4,7 @@ import { execFileSync } from "node:child_process"; import fs from "node:fs"; +import path from "node:path"; import { pathToFileURL } from "node:url"; // pull_request_target content is fetched manually so no PR-controlled action, @@ -16,7 +17,7 @@ const TARGET_REPO_PATTERN = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/u; const TARGET_PR_PATTERN = /^[0-9]+$/u; const TARGET_BASE_PATTERN = /^[A-Za-z0-9._/-]+$/u; const SHA_PATTERN = /^[0-9a-f]{40}$/u; -const DEFAULT_TARGET_DIR = "/tmp/pr-review-advisor-target"; +const DEFAULT_TARGET_DIR = "/tmp/pr-review-advisor-target/pr-workdir"; export class PrepareTargetPrError extends Error { constructor(message: string) { @@ -72,6 +73,28 @@ export function validatePrepareTargetPrInput(input: PrepareTargetPrInput): void } } +export function validatePrepareTargetDirectory( + value: string, + currentDirectory = process.cwd(), +): string { + if (!value || value.includes("\0")) { + fail("target directory must be a non-empty filesystem path"); + } + const resolved = path.resolve(value); + const relativeCurrentDirectory = path.relative(resolved, path.resolve(currentDirectory)); + const containsCurrentDirectory = + relativeCurrentDirectory === "" || + (!relativeCurrentDirectory.startsWith("..") && !path.isAbsolute(relativeCurrentDirectory)); + if ( + resolved === path.parse(resolved).root || + path.basename(resolved) !== "pr-workdir" || + containsCurrentDirectory + ) { + fail("target directory must resolve to a dedicated pr-workdir directory"); + } + return resolved; +} + /** * Fetch and check out a target PR's head into an isolated, hardened workspace, * verifying the fetched base/head against the immutable SHAs from the @@ -85,7 +108,7 @@ export function prepareTargetPr( ): { workdir: string; prNumber: string } { validatePrepareTargetPrInput(input); - const targetDir = options.targetDir ?? DEFAULT_TARGET_DIR; + const targetDir = validatePrepareTargetDirectory(options.targetDir ?? DEFAULT_TARGET_DIR); const runGit = options.runGit ?? ((args: string[]): string => @@ -149,13 +172,16 @@ export function prepareTargetPr( function main(): void { try { - prepareTargetPr({ - targetRepo: process.env.TARGET_REPO ?? "", - targetPr: process.env.TARGET_PR ?? "", - targetBase: process.env.TARGET_BASE ?? "", - prBaseSha: process.env.PR_BASE_SHA || undefined, - expectedHeadSha: process.env.EXPECTED_HEAD_SHA || undefined, - }); + prepareTargetPr( + { + targetRepo: process.env.TARGET_REPO ?? "", + targetPr: process.env.TARGET_PR ?? "", + targetBase: process.env.TARGET_BASE ?? "", + prBaseSha: process.env.PR_BASE_SHA || undefined, + expectedHeadSha: process.env.EXPECTED_HEAD_SHA || undefined, + }, + { targetDir: process.env.TARGET_DIR || undefined }, + ); } catch (error) { const message = error instanceof Error ? error.message : String(error); console.error(`::error::${message}`); diff --git a/tools/pr-review-advisor/run-analysis.mts b/tools/pr-review-advisor/run-analysis.mts index a07bd5f5c9..9c19b4eceb 100755 --- a/tools/pr-review-advisor/run-analysis.mts +++ b/tools/pr-review-advisor/run-analysis.mts @@ -138,6 +138,12 @@ export function runPrReviewAdvisorAnalysis( const analyzePath = path.join(input.advisorDir, "tools", "pr-review-advisor", "analyze.mts"); const schemaPath = path.join(input.advisorDir, "tools", "pr-review-advisor", "schema.json"); const sessionPath = path.join(input.advisorDir, "tools", "advisors", "session.mts"); + const providerConstantsPath = path.join( + input.advisorDir, + "tools", + "advisors", + "provider-constants.mts", + ); const commentPath = path.join(input.advisorDir, "tools", "pr-review-advisor", "comment.mts"); const fileExists = options.fileExists ?? fs.existsSync; const readText = options.readText ?? ((file: string): string => fs.readFileSync(file, "utf8")); @@ -228,7 +234,10 @@ export function runPrReviewAdvisorAnalysis( if ( input.model !== LEGACY_PRIMARY_MODEL && - (!trustedTextIncludes(sessionPath, input.model) || + (!( + trustedTextIncludes(sessionPath, input.model) || + trustedTextIncludes(providerConstantsPath, input.model) + ) || !trustedTextIncludes(analyzePath, "PR_REVIEW_ADVISOR_MODEL") || !trustedTextIncludes(commentPath, "PR_REVIEW_ADVISOR_COMMENT_MARKER")) ) { diff --git a/tools/pr-review-advisor/workflow-boundary.mts b/tools/pr-review-advisor/workflow-boundary.mts index 3a8d7851f8..50a9e75a94 100644 --- a/tools/pr-review-advisor/workflow-boundary.mts +++ b/tools/pr-review-advisor/workflow-boundary.mts @@ -9,13 +9,40 @@ import YAML from "yaml"; const REPO_ROOT = join(dirname(fileURLToPath(import.meta.url)), "..", ".."); const DEFAULT_WORKFLOW_PATH = join(REPO_ROOT, ".github", "workflows", "pr-review-advisor.yaml"); const DEFAULT_PACKAGE_LOCK_PATH = join(REPO_ROOT, "package-lock.json"); +const DEFAULT_OPENSHELL_POLICY_PATH = join( + REPO_ROOT, + "tools", + "pr-review-advisor", + "openshell-policy.yaml", +); +const CANONICAL_ADVISOR_DIR = "${{ github.workspace }}/advisor"; +const CANONICAL_DEFAULT_WORKDIR_IF = + "${{ github.event_name == 'workflow_dispatch' && inputs.target_repo == '' && inputs.target_pr == '' }}"; +const CANONICAL_DEFAULT_WORKDIR = + 'echo "ADVISOR_WORKDIR=$GITHUB_WORKSPACE/pr-workdir" >> "$GITHUB_ENV"'; const TRUSTED_WORKFLOW_REF = "${{ github.workflow_sha }}"; const CANONICAL_ADVISOR_NPM_CI = "npm ci --ignore-scripts --no-audit --no-fund"; const PINNED_SETUP_NODE_ACTION = "actions/setup-node@820762786026740c76f36085b0efc47a31fe5020"; const CANONICAL_PREPARE_TARGET_PR = `node --experimental-strip-types "$ADVISOR_DIR/tools/pr-review-advisor/prepare-target-pr.mts"`; -const CANONICAL_RUN_ANALYSIS = `cd "$ADVISOR_WORKDIR" -node --experimental-strip-types "$ADVISOR_DIR/tools/pr-review-advisor/run-analysis.mts"`; +const CANONICAL_PREPARE_SANDBOX = `node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" prepare`; +const CANONICAL_INSTALL_OPENSHELL = + 'env -u GITHUB_TOKEN -u GH_TOKEN -u PR_REVIEW_ADVISOR_API_KEY NEMOCLAW_NON_INTERACTIVE=1 bash "$ADVISOR_DIR/scripts/install-openshell.sh"'; +const CANONICAL_RUN_ANALYSIS_SELECTOR = + "${{ github.event_name == 'workflow_dispatch' && inputs.run_analysis == false && '0' || '1' }}"; +const CANONICAL_ANALYSIS_ENABLED_IF = "${{ env.PR_REVIEW_ADVISOR_RUN_ANALYSIS == '1' }}"; +const CANONICAL_UNAVAILABLE_IF = + "${{ always() && steps.configure-openshell.outcome != 'success' }}"; +const CANONICAL_UNAVAILABLE_REASON = + "${{ env.PR_REVIEW_ADVISOR_RUN_ANALYSIS == '0' && 'PR_REVIEW_ADVISOR_RUN_ANALYSIS=0' || 'OpenShell inference configuration failed or the advisor credential is unavailable' }}"; +const CANONICAL_CONFIGURE_OPENSHELL = `node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" configure`; +const CANONICAL_UNAVAILABLE_ANALYSIS = `node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" unavailable`; +const CANONICAL_CREATE_SANDBOX = `node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" create`; +const CANONICAL_RUN_ANALYSIS = `node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" run`; +const CANONICAL_DOWNLOAD_ARTIFACTS = `node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" download`; +const CANONICAL_DELETE_SANDBOX = `node --experimental-strip-types --no-warnings "$ADVISOR_DIR/tools/pr-review-advisor/openshell.mts" delete`; const CANONICAL_VALIDATE_ARTIFACTS = `node --experimental-strip-types "$ADVISOR_DIR/tools/pr-review-advisor/validate-artifacts.mts"`; +const PINNED_PI_IMAGE = + "ghcr.io/nvidia/openshell-community/sandboxes/pi@sha256:00d0c5e9e733f94f6db3eaa2ab70d4fd75bcc4aace6b13a54535cbf2dd20dfcd"; const FORBIDDEN_ARTIFACT_DOWNLOAD_WITH_KEYS = [ "run-id", "github-token", @@ -26,6 +53,7 @@ const FORBIDDEN_ARTIFACT_DOWNLOAD_WITH_KEYS = [ const ADVISOR_RUNTIME_PACKAGE_PINS = [ { packageName: "@earendil-works/pi-coding-agent", envName: "PI_SDK_VERSION", version: "0.80.6" }, { packageName: "typebox", envName: "TYPEBOX_VERSION", version: "1.1.38" }, + { packageName: "undici", envName: "UNDICI_VERSION", version: "8.5.0" }, { packageName: "yaml", envName: "YAML_VERSION", version: "2.8.3" }, { packageName: "vitest", envName: "VITEST_VERSION", version: "4.1.9" }, ] as const; @@ -58,6 +86,31 @@ function booleanValue(value: unknown): boolean | undefined { return typeof value === "boolean" ? value : undefined; } +function stringArray(value: unknown): string[] { + return Array.isArray(value) + ? value.filter((entry): entry is string => typeof entry === "string") + : []; +} + +function containsStringMatching(value: unknown, pattern: RegExp): boolean { + if (typeof value === "string") return pattern.test(value); + if (Array.isArray(value)) { + return value.some((entry) => containsStringMatching(entry, pattern)); + } + if (value && typeof value === "object") { + return Object.values(value).some((entry) => containsStringMatching(entry, pattern)); + } + return false; +} + +function containsSecretExpression(value: unknown): boolean { + return containsStringMatching(value, /\$\{\{\s*secrets(?:\.|\s*\[)/u); +} + +function containsGitHubTokenExpression(value: unknown): boolean { + return containsStringMatching(value, /\$\{\{\s*github(?:\.token|\s*\[\s*["']token["']\s*\])/u); +} + function namedStep(steps: readonly WorkflowStep[], name: string): WorkflowStep | undefined { return steps.find((step) => step.name === name); } @@ -211,6 +264,69 @@ function checkAdvisorRuntimePackageLock(errors: string[], packageLockPath: strin } } +function checkOpenShellPolicy(errors: string[], policyPath: string): void { + let policy: WorkflowRecord; + try { + policy = asRecord(YAML.parse(readFileSync(policyPath, "utf8"))); + } catch { + errors.push(`failed to read or parse advisor OpenShell policy: ${policyPath}`); + return; + } + if (policy.version !== 1) { + errors.push("advisor OpenShell policy version must remain 1"); + } + + const filesystem = asRecord(policy.filesystem_policy); + if (booleanValue(filesystem.include_workdir) !== false) { + errors.push("advisor OpenShell policy must not include the default workdir"); + } + const readOnly = stringArray(filesystem.read_only); + const allowedReadOnlyPaths = [ + "/usr/bin", + "/usr/lib", + "/usr/share/git-core", + "/etc", + "/advisor", + "/pr-workdir", + "/pr-review-advisor-context", + "/pr-review-advisor-tools", + ]; + for (const requiredPath of allowedReadOnlyPaths) { + if (!readOnly.includes(requiredPath)) { + errors.push(`advisor OpenShell policy must grant read-only access to ${requiredPath}`); + } + } + for (const readOnlyPath of readOnly) { + if (!allowedReadOnlyPaths.includes(readOnlyPath)) { + errors.push(`advisor OpenShell policy must not grant read access to ${readOnlyPath}`); + } + } + const readWrite = stringArray(filesystem.read_write); + if (!readWrite.includes("/dev")) { + errors.push("advisor OpenShell policy must retain writable device access"); + } + if (!readWrite.includes("/sandbox/pr-review-advisor-runtime")) { + errors.push("advisor OpenShell policy must retain only its writable runtime subtree"); + } + for (const writablePath of readWrite) { + if (!["/dev", "/sandbox/pr-review-advisor-runtime"].includes(writablePath)) { + errors.push(`advisor OpenShell policy must not grant write access to ${writablePath}`); + } + } + + if (asRecord(policy.landlock).compatibility !== "hard_requirement") { + errors.push("advisor OpenShell policy must fail closed when Landlock is unavailable"); + } + const processPolicy = asRecord(policy.process); + if (processPolicy.run_as_user !== "sandbox" || processPolicy.run_as_group !== "sandbox") { + errors.push("advisor OpenShell policy must run as the sandbox user and group"); + } + const networkPolicies = asRecord(policy.network_policies); + if (networkPolicies !== policy.network_policies || Object.keys(networkPolicies).length !== 0) { + errors.push("advisor OpenShell policy must not allow direct network egress"); + } +} + function requireActionPins( errors: string[], jobName: string, @@ -311,6 +427,20 @@ function checkAnalysisJob(errors: string[], reviewJob: WorkflowRecord): void { if (stringValue(reviewJob["continue-on-error"]) !== "${{ !matrix.advisor.publish_comment }}") { errors.push("review job failures must be non-blocking only for non-publishing advisor lanes"); } + const reviewEnv = asRecord(reviewJob.env); + const forbiddenJobCredentials = [ + "GH_TOKEN", + "GITHUB_TOKEN", + "OPENAI_API_KEY", + "PR_REVIEW_ADVISOR_API_KEY", + ]; + if ( + forbiddenJobCredentials.some((name) => Object.hasOwn(reviewEnv, name)) || + containsSecretExpression(reviewEnv) || + containsGitHubTokenExpression(reviewEnv) + ) { + errors.push("review job-level environment must not expose GitHub or model credentials"); + } const entries = advisorMatrixEntries(errors, reviewJob); for (const [index, entry] of entries.entries()) { @@ -324,6 +454,11 @@ function checkAnalysisJob(errors: string[], reviewJob: WorkflowRecord): void { for (const field of ["model", "artifact_dir", "artifact_name"]) { requireUniqueMatrixField(errors, entries, field); } + for (const [index, entry] of entries.entries()) { + if (!/^[a-z0-9][a-z0-9-]*$/u.test(stringValue(entry.artifact_dir))) { + errors.push(`advisor matrix entry ${index + 1} artifact_dir must be a simple directory name`); + } + } requireEnv( errors, @@ -332,6 +467,7 @@ function checkAnalysisJob(errors: string[], reviewJob: WorkflowRecord): void { "PR_REVIEW_ADVISOR_MODEL", "${{ matrix.advisor.model }}", ); + requireEnv(errors, "review job", reviewJob, "ADVISOR_DIR", CANONICAL_ADVISOR_DIR); requireEnv(errors, "review job", reviewJob, "FD_FIND_VERSION", "9.0.0-1"); requireEnv(errors, "review job", reviewJob, "RIPGREP_VERSION", "14.1.0-1"); for (const { envName, version } of ADVISOR_RUNTIME_PACKAGE_PINS) { @@ -344,6 +480,27 @@ function checkAnalysisJob(errors: string[], reviewJob: WorkflowRecord): void { "PR_REVIEW_ADVISOR_ARTIFACT_DIR", "${{ matrix.advisor.artifact_dir }}", ); + requireEnv( + errors, + "review job", + reviewJob, + "PR_REVIEW_ADVISOR_RUN_ANALYSIS", + CANONICAL_RUN_ANALYSIS_SELECTOR, + ); + requireEnv( + errors, + "review job", + reviewJob, + "TARGET_REPO", + "${{ github.event_name == 'pull_request_target' && github.repository || inputs.target_repo || github.repository }}", + ); + requireEnv( + errors, + "review job", + reviewJob, + "PR_NUMBER", + "${{ github.event.pull_request.number || inputs.target_pr }}", + ); requireEnv( errors, "review job", @@ -352,6 +509,22 @@ function checkAnalysisJob(errors: string[], reviewJob: WorkflowRecord): void { "PR Review / Advisor", ); requireEnv(errors, "review job", reviewJob, "PR_REVIEW_ADVISOR_LOAD_PREVIOUS_REVIEW", "false"); + requireEnv( + errors, + "review job", + reviewJob, + "OPENSHELL_GATEWAY_ENDPOINT", + "http://127.0.0.1:8080", + ); + requireEnv(errors, "review job", reviewJob, "PI_IMAGE", PINNED_PI_IMAGE); + requireEnv(errors, "review job", reviewJob, "PR_REVIEW_ADVISOR_SANDBOX_TIMEOUT_SECONDS", "2100"); + requireEnv( + errors, + "review job", + reviewJob, + "SANDBOX_NAME", + "pr-advisor-${{ github.run_id }}-${{ github.run_attempt }}-${{ matrix.advisor.id }}", + ); const steps = asSteps(reviewJob.steps); if (steps.length === 0) errors.push("review job must declare steps"); @@ -384,11 +557,27 @@ function checkAnalysisJob(errors: string[], reviewJob: WorkflowRecord): void { requireWith(errors, dispatchCheckout, "lfs", false); requireWith(errors, dispatchCheckout, "submodules", false); + const defaultWorkdir = requireStep(errors, steps, "Set default advisor workdir"); + if (defaultWorkdir && stringValue(defaultWorkdir.if) !== CANONICAL_DEFAULT_WORKDIR_IF) { + errors.push("Set default advisor workdir must use the canonical dispatch-only condition"); + } + requireCanonicalRun( + errors, + defaultWorkdir, + CANONICAL_DEFAULT_WORKDIR, + "Set default advisor workdir must bind ADVISOR_WORKDIR to the fixed pr-workdir checkout", + ); + const prepare = requireStep(errors, steps, "Prepare isolated analysis workspace"); const prepareEnv = asRecord(prepare?.env); if (prepareEnv.GIT_LFS_SKIP_SMUDGE !== "1") { errors.push("Prepare isolated analysis workspace must disable LFS smudging"); } + if (prepareEnv.TARGET_DIR !== "${{ github.workspace }}/pr-workdir") { + errors.push( + "Prepare isolated analysis workspace must use the fixed pr-workdir upload directory", + ); + } // The fetch/validate/checkout logic lives in the trusted, unit-tested helper // (prepare-target-pr.mts); the workflow must invoke it from the pinned advisor // checkout ($ADVISOR_DIR), never from PR-controlled content. @@ -467,6 +656,98 @@ done < <(find "$ADVISOR_WORKDIR" -type l -print0)`; ); requireRunOrder(errors, install, 'cd "$ADVISOR_DIR"', CANONICAL_ADVISOR_NPM_CI); + const prepareSandbox = requireStep(errors, steps, "Prepare advisor sandbox inputs"); + requireCanonicalRun( + errors, + prepareSandbox, + CANONICAL_PREPARE_SANDBOX, + "step 'Prepare advisor sandbox inputs' must use the canonical trusted OpenShell helper command", + ); + const prepareSandboxEnv = asRecord(prepareSandbox?.env); + if ( + prepareSandboxEnv.GH_TOKEN !== "${{ github.token }}" || + Object.keys(prepareSandboxEnv).length !== 1 + ) { + errors.push("Prepare advisor sandbox inputs must receive only github.token"); + } + + const installOpenShell = requireStep(errors, steps, "Install OpenShell"); + requireCanonicalRun( + errors, + installOpenShell, + CANONICAL_INSTALL_OPENSHELL, + "step 'Install OpenShell' must use the canonical credential-free trusted installer command", + ); + if (stringValue(installOpenShell?.if) !== CANONICAL_ANALYSIS_ENABLED_IF) { + errors.push("Install OpenShell must run only when advisor analysis is requested"); + } + + const configureOpenShell = requireStep(errors, steps, "Configure OpenShell inference"); + requireCanonicalRun( + errors, + configureOpenShell, + CANONICAL_CONFIGURE_OPENSHELL, + "step 'Configure OpenShell inference' must use the canonical trusted OpenShell helper command", + ); + const configureEnv = asRecord(configureOpenShell?.env); + if ( + configureEnv.OPENAI_API_KEY !== "${{ secrets.PR_REVIEW_ADVISOR_API_KEY }}" || + Object.keys(configureEnv).length !== 1 + ) { + errors.push( + "Configure OpenShell inference must receive only secrets.PR_REVIEW_ADVISOR_API_KEY as OPENAI_API_KEY", + ); + } + if (stringValue(configureOpenShell?.id) !== "configure-openshell") { + errors.push("Configure OpenShell inference id must be configure-openshell"); + } + if (stringValue(configureOpenShell?.if) !== CANONICAL_ANALYSIS_ENABLED_IF) { + errors.push("Configure OpenShell inference must run only when advisor analysis is requested"); + } + if (configureOpenShell && booleanValue(configureOpenShell["continue-on-error"]) !== true) { + errors.push( + "Configure OpenShell inference must continue-on-error until unavailable artifacts are written", + ); + } + + const unavailable = requireStep(errors, steps, "Write unavailable advisor artifacts"); + if (stringValue(unavailable?.id) !== "unavailable-analysis") { + errors.push("Write unavailable advisor artifacts id must be unavailable-analysis"); + } + if (stringValue(unavailable?.if) !== CANONICAL_UNAVAILABLE_IF) { + errors.push( + "Write unavailable advisor artifacts must run after skipped or failed configuration", + ); + } + requireCanonicalRun( + errors, + unavailable, + CANONICAL_UNAVAILABLE_ANALYSIS, + "step 'Write unavailable advisor artifacts' must use the canonical trusted fallback command", + ); + const unavailableEnv = asRecord(unavailable?.env); + if ( + unavailableEnv.PR_REVIEW_ADVISOR_UNAVAILABLE_REASON !== CANONICAL_UNAVAILABLE_REASON || + !Object.hasOwn(unavailableEnv, "BASE_REF") || + !Object.hasOwn(unavailableEnv, "HEAD_REF") || + Object.keys(unavailableEnv).length !== 3 + ) { + errors.push( + "Write unavailable advisor artifacts must receive only refs and the canonical unavailable reason", + ); + } + + const createSandbox = requireStep(errors, steps, "Create credential-free advisor sandbox"); + requireCanonicalRun( + errors, + createSandbox, + CANONICAL_CREATE_SANDBOX, + "step 'Create credential-free advisor sandbox' must use the canonical trusted OpenShell helper command", + ); + if (stringValue(createSandbox?.if) !== "${{ steps.configure-openshell.outcome == 'success' }}") { + errors.push("Create credential-free advisor sandbox must require successful configuration"); + } + const analyze = requireStep(errors, steps, "Run PR review advisor"); requireCanonicalRun( errors, @@ -474,21 +755,83 @@ done < <(find "$ADVISOR_WORKDIR" -type l -print0)`; CANONICAL_RUN_ANALYSIS, "step 'Run PR review advisor' must use the canonical trusted analysis command", ); + if (stringValue(analyze?.if) !== "${{ steps.configure-openshell.outcome == 'success' }}") { + errors.push("Run PR review advisor must require successful configuration"); + } if (analyze && booleanValue(analyze["continue-on-error"]) !== true) { errors.push("Run PR review advisor must continue-on-error until artifacts are uploaded"); } + if ( + containsSecretExpression(analyze) || + Object.hasOwn(asRecord(analyze?.env), "OPENAI_API_KEY") + ) { + errors.push("Run PR review advisor must not receive the upstream model credential"); + } const analyzeEnv = asRecord(analyze?.env); - if (analyzeEnv.PR_REVIEW_ADVISOR_API_KEY !== "${{ secrets.PR_REVIEW_ADVISOR_API_KEY }}") { - errors.push("Run PR review advisor must receive only secrets.PR_REVIEW_ADVISOR_API_KEY"); + if ( + unavailableEnv.BASE_REF !== analyzeEnv.BASE_REF || + unavailableEnv.HEAD_REF !== analyzeEnv.HEAD_REF + ) { + errors.push("Write unavailable advisor artifacts must use the same refs as sandbox analysis"); + } + + const download = requireStep(errors, steps, "Download advisor artifacts from sandbox"); + if (stringValue(download?.id) !== "download-analysis") { + errors.push("Download advisor artifacts from sandbox id must be download-analysis"); + } + if ( + stringValue(download?.if) !== + "${{ always() && steps.configure-openshell.outcome == 'success' }}" + ) { + errors.push( + "Download advisor artifacts from sandbox must run after every configured sandbox analysis", + ); } - if (Object.hasOwn(analyzeEnv, "OPENAI_API_KEY")) { - errors.push("Run PR review advisor must not receive OPENAI_API_KEY"); + if (download && booleanValue(download["continue-on-error"]) !== true) { + errors.push( + "Download advisor artifacts from sandbox must continue-on-error until artifacts are uploaded", + ); } - const modelSecretSteps = steps.filter((step) => - JSON.stringify(step).includes("PR_REVIEW_ADVISOR_API_KEY"), + requireCanonicalRun( + errors, + download, + CANONICAL_DOWNLOAD_ARTIFACTS, + "step 'Download advisor artifacts from sandbox' must use the canonical trusted OpenShell helper command", ); - if (modelSecretSteps.length !== 1 || modelSecretSteps[0] !== analyze) { - errors.push("only the analysis step may receive the advisor model credential"); + + const deleteSandbox = requireStep(errors, steps, "Delete advisor sandbox"); + if (stringValue(deleteSandbox?.if) !== "always()") { + errors.push("Delete advisor sandbox must run always"); + } + requireCanonicalRun( + errors, + deleteSandbox, + CANONICAL_DELETE_SANDBOX, + "step 'Delete advisor sandbox' must use the canonical trusted OpenShell helper command", + ); + + const modelSecretSteps = steps.filter((step) => containsSecretExpression(step)); + if (modelSecretSteps.length !== 1 || modelSecretSteps[0] !== configureOpenShell) { + errors.push("only OpenShell provider configuration may receive the advisor model credential"); + } + const githubTokenSteps = steps.filter((step) => containsGitHubTokenExpression(step)); + if (githubTokenSteps.length !== 1 || githubTokenSteps[0] !== prepareSandbox) { + errors.push("only advisor sandbox input preparation may receive github.token"); + } + for (const step of [unavailable, createSandbox, analyze, download, deleteSandbox]) { + const stepEnv = asRecord(step?.env); + if ( + step && + (["GH_TOKEN", "GITHUB_TOKEN", "OPENAI_API_KEY", "PR_REVIEW_ADVISOR_API_KEY"].some((name) => + Object.hasOwn(stepEnv, name), + ) || + containsGitHubTokenExpression(step) || + containsSecretExpression(step)) + ) { + errors.push( + `step '${step.name ?? ""}' must remain credential-free after OpenShell configuration`, + ); + } } const symlinkIndex = steps.findIndex( @@ -508,16 +851,52 @@ done < <(find "$ADVISOR_WORKDIR" -type l -print0)`; } } } + const prepareSandboxIndex = steps.findIndex( + (step) => step.name === "Prepare advisor sandbox inputs", + ); + const installOpenShellIndex = steps.findIndex((step) => step.name === "Install OpenShell"); + const configureIndex = steps.findIndex((step) => step.name === "Configure OpenShell inference"); + const unavailableIndex = steps.findIndex( + (step) => step.name === "Write unavailable advisor artifacts", + ); + const createIndex = steps.findIndex( + (step) => step.name === "Create credential-free advisor sandbox", + ); const analyzeIndex = steps.findIndex((step) => step.name === "Run PR review advisor"); + const downloadIndex = steps.findIndex( + (step) => step.name === "Download advisor artifacts from sandbox", + ); + const deleteIndex = steps.findIndex((step) => step.name === "Delete advisor sandbox"); const installIndex = steps.findIndex((step) => step.name === "Install Pi SDK"); - if (installIndex < 0 || analyzeIndex < 0 || installIndex > analyzeIndex) { + if (installIndex < 0 || configureIndex < 0 || installIndex > configureIndex) { errors.push("pinned advisor tools must be installed before the model credential is exposed"); } - if (symlinkIndex < 0 || analyzeIndex < 0 || symlinkIndex > analyzeIndex) { + if (symlinkIndex < 0 || configureIndex < 0 || symlinkIndex > configureIndex) { errors.push( "analysis workspace symlinks must be removed before the model credential is exposed", ); } + if ( + prepareSandboxIndex < 0 || + installOpenShellIndex < 0 || + configureIndex < 0 || + unavailableIndex < 0 || + createIndex < 0 || + analyzeIndex < 0 || + downloadIndex < 0 || + deleteIndex < 0 || + prepareSandboxIndex > configureIndex || + installOpenShellIndex > configureIndex || + configureIndex > unavailableIndex || + unavailableIndex > createIndex || + createIndex > analyzeIndex || + analyzeIndex > downloadIndex || + downloadIndex > deleteIndex + ) { + errors.push( + "advisor inputs, OpenShell, sandbox execution, artifact download, and cleanup must run in the canonical order", + ); + } if (steps.some((step) => step.name === "Post PR review advisor comment")) { errors.push("analysis job must not publish PR comments"); } @@ -529,7 +908,26 @@ done < <(find "$ADVISOR_WORKDIR" -type l -print0)`; if (outcome && booleanValue(outcome["continue-on-error"]) === true) { errors.push("Verify advisor analysis outcome must not continue on error"); } + requireRunContains(errors, outcome, 'if [ "$CONFIGURE_OUTCOME" != "success" ]'); + requireRunContains(errors, outcome, 'if [ "$UNAVAILABLE_OUTCOME" != "success" ]'); + requireRunContains(errors, outcome, 'if [ "$ANALYSIS_REQUESTED" = "0" ]'); requireRunContains(errors, outcome, 'if [ "$ANALYSIS_OUTCOME" != "success" ]'); + requireRunContains(errors, outcome, 'if [ "$DOWNLOAD_OUTCOME" != "success" ]'); + if (asRecord(outcome?.env).ANALYSIS_OUTCOME !== "${{ steps.analysis.outcome }}") { + errors.push("Verify advisor analysis outcome must use the trusted analysis step outcome"); + } + if (asRecord(outcome?.env).ANALYSIS_REQUESTED !== "${{ env.PR_REVIEW_ADVISOR_RUN_ANALYSIS }}") { + errors.push("Verify advisor analysis outcome must use the trusted analysis request selector"); + } + if (asRecord(outcome?.env).DOWNLOAD_OUTCOME !== "${{ steps.download-analysis.outcome }}") { + errors.push("Verify advisor analysis outcome must use the trusted sandbox download outcome"); + } + if (asRecord(outcome?.env).CONFIGURE_OUTCOME !== "${{ steps.configure-openshell.outcome }}") { + errors.push("Verify advisor analysis outcome must use the trusted configuration step outcome"); + } + if (asRecord(outcome?.env).UNAVAILABLE_OUTCOME !== "${{ steps.unavailable-analysis.outcome }}") { + errors.push("Verify advisor analysis outcome must use the trusted unavailable step outcome"); + } const uploadIndex = steps.findIndex((step) => step.name === "Upload advisor artifacts"); const outcomeIndex = steps.findIndex((step) => step.name === "Verify advisor analysis outcome"); if (uploadIndex >= 0 && outcomeIndex >= 0 && outcomeIndex < uploadIndex) { @@ -547,6 +945,7 @@ function checkPublishJob(errors: string[], publishJob: WorkflowRecord): void { errors.push("publish job must run best-effort only for pull_request_target events"); } for (const [key, expected] of Object.entries({ + ADVISOR_DIR: CANONICAL_ADVISOR_DIR, PR_REVIEW_ADVISOR_WORKFLOW_NAME: "PR Review / Advisor", PR_REVIEW_ADVISOR_WORKFLOW_PATH: ".github/workflows/pr-review-advisor.yaml", PR_REVIEW_ADVISOR_EVENT_NAME: "${{ github.event_name }}", @@ -730,6 +1129,7 @@ function checkPublishJob(errors: string[], publishJob: WorkflowRecord): void { export function validatePrReviewAdvisorWorkflowBoundary( workflowPath = DEFAULT_WORKFLOW_PATH, packageLockPath = DEFAULT_PACKAGE_LOCK_PATH, + openshellPolicyPath = DEFAULT_OPENSHELL_POLICY_PATH, ): string[] { const errors: string[] = []; let workflow: WorkflowRecord; @@ -743,6 +1143,7 @@ export function validatePrReviewAdvisorWorkflowBoundary( errors.push("workflow name must remain PR Review / Advisor"); } checkAdvisorRuntimePackageLock(errors, packageLockPath); + checkOpenShellPolicy(errors, openshellPolicyPath); checkTargetTriggers(errors, workflow); const concurrencyGroup = stringValue(asRecord(workflow.concurrency).group); if (!concurrencyGroup.includes("github.event_name")) {