ci(advisor): run PR Review Advisor in OpenShell#7600
Conversation
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
added
the
area: security
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces direct advisor execution with a staged OpenShell sandbox workflow, adds prepared GitHub context and credential separation, centralizes runtime and transport helpers, introduces restrictive policy validation, and expands workflow, integration, security, and boundary tests. ChangesOpenShell advisor integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant AdvisorWrapper
participant OpenShellGateway
participant AdvisorSandbox
participant ArtifactStore
GitHubActions->>AdvisorWrapper: prepare GitHub context and sandbox inputs
AdvisorWrapper->>OpenShellGateway: configure provider and inference model
GitHubActions->>AdvisorWrapper: create and run sandbox analysis
AdvisorWrapper->>AdvisorSandbox: execute advisor analysis
AdvisorSandbox-->>AdvisorWrapper: produce review artifacts
AdvisorWrapper->>ArtifactStore: download artifacts
GitHubActions->>AdvisorWrapper: delete sandbox and verify outcomes
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 82e56aa in the TypeScript / code-coverage/cliThe overall coverage in commit 82e56aa in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-7600.docs.buildwithfern.com/nemoclaw |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| 0o600, | ||
| ); | ||
| try { | ||
| fs.writeFileSync(fd, content); |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tools/pr-review-advisor/prepare-target-pr.mts (1)
76-96: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider requiring an absolute path.
A relative
TARGET_DIRsuch aspr-workdirresolves under the current working directory, passes every guard (basename matches, cwd isn't contained), and is thenrmSync(..., { recursive: true, force: true })'d — deleting a directory inside the trusted checkout. Rejecting non-absolute input closes that path.🛡️ Proposed guard
if (!value || value.includes("\0")) { fail("target directory must be a non-empty filesystem path"); } + if (!path.isAbsolute(value)) { + fail("target directory must be an absolute filesystem path"); + } const resolved = path.resolve(value);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/pr-review-advisor/prepare-target-pr.mts` around lines 76 - 96, Update validatePrepareTargetDirectory to reject non-absolute value inputs before path resolution, while preserving the existing non-empty, NUL-byte, root, basename, and current-directory containment checks for accepted paths.tools/openshell-agent/runtime.mts (2)
276-304: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a default
timeoutSecondsfor sandbox exec.
execOpenShellSandboxomits--timeoutentirely whentimeoutSecondsis unset (e.g. the patch-export path), so a wedged sandbox command blocks until the job-level timeout. A conservative default bounds the failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/openshell-agent/runtime.mts` around lines 276 - 304, Update execOpenShellSandbox to apply a conservative default timeout when input.timeoutSeconds is unset, while preserving explicitly provided timeout values. Ensure timeoutArgs always includes the resulting --timeout argument before invoking tools.run.
251-254: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnvalidated
:in upload paths silently mis-splits the mapping.
--upload ${source}:${destination}is ambiguous if either path contains a colon. Callers pass env-derived directories (RESOLUTION_WORKDIR,RESOLVER_CONFIG_DIR), so a guard here keeps the failure explicit rather than uploading to a wrong destination.🛡️ Proposed guard
const uploadArgs = input.uploads.flatMap(({ source, destination }) => [ "--upload", - `${source}:${destination}`, + ((): string => { + if (source.includes(":") || destination.includes(":")) { + throw new OpenShellAgentError("Sandbox upload paths must not contain ':'"); + } + return `${source}:${destination}`; + })(), ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/openshell-agent/runtime.mts` around lines 251 - 254, Validate each source and destination path in the uploadArgs construction before forming the mapping, rejecting any path containing “:” with a clear error. Keep the existing --upload argument format for valid paths and ensure env-derived directories such as RESOLUTION_WORKDIR and RESOLVER_CONFIG_DIR fail explicitly rather than being mis-split.tools/pr-review-advisor/github-context.mts (1)
102-107: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winIdentity binding is skipped when the expected repo/PR are absent.
readPreparedGitHubContextonly comparesrepo/prNumberwhen the caller supplies them, andcollectGitHubReviewContextpassesundefinedwheneverTARGET_REPO/GITHUB_REPOSITORYorPR_NUMBERare unset. Since this is the boundary that binds sandbox-supplied context to the workflow target, consider requiring both whenpreparedPathis set.🛡️ Proposed tightening
if (preparedPath) { + if (!repo || !Number.isFinite(prNumber) || prNumber <= 0) { + throw new Error("Prepared GitHub context requires a known repository and pull request"); + } return readPreparedGitHubContext(preparedPath, { repo, - prNumber: Number.isFinite(prNumber) && prNumber > 0 ? prNumber : undefined, + prNumber, }); }Also applies to: 120-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/pr-review-advisor/github-context.mts` around lines 102 - 107, Update readPreparedGitHubContext and its collectGitHubReviewContext call path so that when preparedPath is set, both the expected repository and pull request number are required and validated; do not skip identity checks merely because TARGET_REPO/GITHUB_REPOSITORY or PR_NUMBER are unset. Preserve the existing mismatch errors and normal behavior when no prepared context is used.tools/pr-review-advisor/workflow-boundary.mts (1)
443-447: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the artifact-directory pattern with the runtime validator.
^[a-z0-9][a-z0-9-]*$is now defined here and independently intools/pr-review-advisor/openshell.mts(Line 248). If one side loosens, the other silently stops matching and the boundary claim no longer reflects runtime behavior. Export the pattern from one module and import it here.As per path instructions, "Derive inventories and limits from a canonical source where possible; flag duplicated lists that can silently drift."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/pr-review-advisor/workflow-boundary.mts` around lines 443 - 447, Centralize the artifact-directory validation pattern currently duplicated in the workflow-boundary loop and the runtime validator in openshell.mts. Export the canonical pattern from openshell.mts, import and reuse it in the entries validation, and preserve the existing validation behavior.Source: Path instructions
test/pr-review-advisor-openshell-workflow-boundary.test.ts (1)
353-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPolicy invariants without negative coverage.
version,filesystem_policy.include_workdir, andprocess.run_as_user/run_as_groupare enforced incheckOpenShellPolicybut never exercised by a mutation here, so a regression that drops those branches would go unnoticed.validatePolicyMutationmakes each addition a two-line case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/pr-review-advisor-openshell-workflow-boundary.test.ts` around lines 353 - 397, Extend the test in “fails closed around the credential-free OpenShell filesystem and network boundary” with mutation cases for version, filesystem_policy.include_workdir, and process.run_as_user/run_as_group. Use validatePolicyMutation for each mutation and assert the corresponding checkOpenShellPolicy invariant message, ensuring regressions in each enforced branch are covered.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/advisor-http-dispatcher.test.ts`:
- Around line 80-84: Guard child.stderr before registering the "data" listener
in the child-process test setup, using optional chaining or an explicit null
check so the code handles the declared Readable | null type while preserving
stderr collection when available.
---
Nitpick comments:
In `@test/pr-review-advisor-openshell-workflow-boundary.test.ts`:
- Around line 353-397: Extend the test in “fails closed around the
credential-free OpenShell filesystem and network boundary” with mutation cases
for version, filesystem_policy.include_workdir, and
process.run_as_user/run_as_group. Use validatePolicyMutation for each mutation
and assert the corresponding checkOpenShellPolicy invariant message, ensuring
regressions in each enforced branch are covered.
In `@tools/openshell-agent/runtime.mts`:
- Around line 276-304: Update execOpenShellSandbox to apply a conservative
default timeout when input.timeoutSeconds is unset, while preserving explicitly
provided timeout values. Ensure timeoutArgs always includes the resulting
--timeout argument before invoking tools.run.
- Around line 251-254: Validate each source and destination path in the
uploadArgs construction before forming the mapping, rejecting any path
containing “:” with a clear error. Keep the existing --upload argument format
for valid paths and ensure env-derived directories such as RESOLUTION_WORKDIR
and RESOLVER_CONFIG_DIR fail explicitly rather than being mis-split.
In `@tools/pr-review-advisor/github-context.mts`:
- Around line 102-107: Update readPreparedGitHubContext and its
collectGitHubReviewContext call path so that when preparedPath is set, both the
expected repository and pull request number are required and validated; do not
skip identity checks merely because TARGET_REPO/GITHUB_REPOSITORY or PR_NUMBER
are unset. Preserve the existing mismatch errors and normal behavior when no
prepared context is used.
In `@tools/pr-review-advisor/prepare-target-pr.mts`:
- Around line 76-96: Update validatePrepareTargetDirectory to reject
non-absolute value inputs before path resolution, while preserving the existing
non-empty, NUL-byte, root, basename, and current-directory containment checks
for accepted paths.
In `@tools/pr-review-advisor/workflow-boundary.mts`:
- Around line 443-447: Centralize the artifact-directory validation pattern
currently duplicated in the workflow-boundary loop and the runtime validator in
openshell.mts. Export the canonical pattern from openshell.mts, import and reuse
it in the entries validation, and preserve the existing validation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a5ec61e6-d1db-4cae-a1c2-bacff6b56c88
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
.github/workflows/pr-review-advisor.yamlci/source-shape-test-budget.jsonpackage.jsontest/advisor-http-dispatcher.test.tstest/advisor-session-runner.test.tstest/helpers/vitest-watch-triggers.tstest/pr-review-advisor-openshell-workflow-boundary.test.tstest/pr-review-advisor-openshell.test.tstest/pr-review-advisor-prepare-target-pr.test.tstest/pr-review-advisor-workflow-boundary.test.tstest/vitest-watch-triggers.test.tstools/advisors/http-dispatcher.mtstools/advisors/provider-constants.mtstools/advisors/session.mtstools/openshell-agent/runtime.mtstools/pr-merge-conflict-fixer/resolve.mtstools/pr-review-advisor/README.mdtools/pr-review-advisor/analyze.mtstools/pr-review-advisor/github-context.mtstools/pr-review-advisor/openshell-policy.yamltools/pr-review-advisor/openshell.mtstools/pr-review-advisor/prepare-target-pr.mtstools/pr-review-advisor/run-analysis.mtstools/pr-review-advisor/workflow-boundary.mts
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/pr-review-advisor-openshell.test.ts`:
- Around line 311-316: Update the skipIf platform guard for the “rejects a
prepared-context FIFO without blocking” test to also require
fs.constants.O_NOFOLLOW, while preserving the existing win32 and O_NONBLOCK
checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8d448c74-f301-428d-a3dd-29ed8ca59907
📒 Files selected for processing (5)
test/advisor-http-dispatcher.test.tstest/pr-review-advisor-openshell.test.tstools/advisors/http-dispatcher.mtstools/pr-review-advisor/github-context.mtstools/pr-review-advisor/openshell.mts
🚧 Files skipped from review as they are similar to previous changes (4)
- tools/advisors/http-dispatcher.mts
- test/advisor-http-dispatcher.test.ts
- tools/pr-review-advisor/github-context.mts
- tools/pr-review-advisor/openshell.mts
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/pr-review-advisor/workflow-boundary.mts`:
- Around line 1130-1146: Extract the duplicated OS baseline paths into a shared
constant near the two policy checks, then spread that constant into the readOnly
arrays passed to both checkOpenShellPolicy calls. Keep each policy’s additional
paths unchanged, including the runtime-only sandbox paths and the upload-policy
readWrite paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 282adb05-b09f-4a82-8403-b84b316c5642
📒 Files selected for processing (11)
test/advisor-http-dispatcher.test.tstest/helpers/vitest-watch-triggers.tstest/pr-review-advisor-openshell-workflow-boundary.test.tstest/pr-review-advisor-openshell.test.tstest/vitest-watch-triggers.test.tstools/openshell-agent/runtime.mtstools/pr-review-advisor/README.mdtools/pr-review-advisor/openshell-policy.yamltools/pr-review-advisor/openshell-upload-policy.yamltools/pr-review-advisor/openshell.mtstools/pr-review-advisor/workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (7)
- test/helpers/vitest-watch-triggers.ts
- tools/pr-review-advisor/openshell-policy.yaml
- test/advisor-http-dispatcher.test.ts
- tools/openshell-agent/runtime.mts
- tools/pr-review-advisor/README.md
- tools/pr-review-advisor/openshell.mts
- test/pr-review-advisor-openshell.test.ts
Mount trusted Advisor inputs read-only before the first sandbox process. Confine application writes to a capped tmpfs and prove the boundary with cross-UID canaries. Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Pin the sandbox to its immutable Git directory and worktree so Git does not reject the runner-owned bind mount across the sandbox UID boundary. Require readable boundary canaries and a canonical commit-form HEAD before analysis starts. Cover the exact different-owner failure and ambient Git environment overrides. Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Ignore runner-level Git safety configuration while exercising the different-owner control so hosted Git installations reproduce the intended rejection consistently. Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
3 participants
Summary
The PR Review Advisor currently runs directly on the Actions host with the model credential in its process. This change reuses the conflict fixer's OpenShell lifecycle so both advisor lanes run through
inference.local.The Advisor opts into four read-only Docker bind mounts for its trusted code, PR data, prepared GitHub context, and verified search tools. Those mounts and the final hard-Landlock policy exist before the first sandbox process starts. Application writes are confined to a capped runtime tmpfs.
Changes
tools/openshell-agent/runtime.mtsfor the conflict fixer and PR Review Advisor. The shared helper now supports opt-in bind mounts and driver configuration while preserving the conflict fixer's upload behavior./sandbox; a 512 MiB tmpfs is the only application-data write subtree.HEAD, rejects chmod, overwrite, rename, and creation in every input, and exercises the complete create/overwrite/chmod/rename/delete lifecycle in the runtime.GIT_DIR=/pr-workdir/.gitandGIT_WORK_TREE=/pr-workdir. This narrowly trusts the immutable metadata created by the trusted prepare helper without using a broadsafe.directory=*exception.Regression Fix
0700application directories created at startup.GIT_DIRandGIT_WORK_TREEvalues bypass only discovery for the already-immutable checkout. The startup proof now fails closed unless that checkout andHEADare readable.Type of Change
Quality Gates
8fa580d2b; the test-only runner-config isolation was revalidated at82e56aad1. The final local regression passed 19 files / 263 tests, including the exact different-owner Git path.Documentation Writer Review
docs-updatedtools/pr-review-advisor/README.mdaccurately documents four pre-start read-only mounts, one final hard-Landlock policy, the bounded runtime tmpfs, the mutation proof, and credential boundaries. No Fern documentation, changelog, or skill update is needed because this is internal review automation.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailable82e56aad1, the isolated different-owner Advisor test passed 18/18. CLI type-check, Biome, source-shape, test-size, normal pre-commit, commit-msg, and pre-push checks passed. The pinned Pi image reproduced implicit Git rejection and passed the explicit worktree proof as its realsandboxuser.npm run docsbuilds without warnings (doc changes only)GitHub Actions E2E
792f1cfab. Artifact schema, SHA binding, cleanup, and outcome checks passed.ac4154daf. Exact analyzer/target SHA binding, artifact schema parity, credential scans, and sandbox cleanup passed.8f9c52f79. Exact analyzer/target SHA binding, artifact schema parity, credential scans, inference routing, and sandbox cleanup passed.5327f5ff6: both lanes proved four immutable inputs and one writable runtime, then failed before turn 1 because Git rejected implicit discovery of the runner-owned worktree. Both sandboxes were deleted and no credentials leaked.8fa580d2b: both lanes passed the strengthened immutable-input/Git proof, completed 14/14 turns, downloaded valid artifacts, deleted their sandboxes, and returnedmerge_as_iswith high confidence and zero findings.82e56aad1: both lanes passed the strengthened immutable-input/Git proof, completed 14/14 turns, downloaded valid artifacts, and deleted their sandboxes. Terra returnedmerge_as_iswith high confidence and zero findings. Nemotron completed with one non-actionable documentation suggestion whose requested sandbox lifecycle, credential separation, no-egress/Landlock policy, runtime tmpfs, and unavailable-artifact fallback are already explicit intools/pr-review-advisor/README.md.Signed-off-by: Charan Jagwani cjagwani@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation