perf(test): reduce sandbox lifecycle subprocess isolation - #6280
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
📝 WalkthroughWalkthroughThis PR expands and tightens sandbox CLI test coverage across connect, gateway state, status, registry recovery, dispatch, and process recovery flows. It also updates shared test harnesses and removes some older assertions. ChangesSandbox test coverage expansion
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
E2E Target RecommendationRequired E2E targets: None Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/lib/actions/sandbox/connect-flow.test.ts (1)
420-459: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor redundancy: two assertions confirm the same exit.
Line 432 already asserts
connectSandboxrejects with"process.exit(1)"; line 458'sexpect(exitSpy).toHaveBeenCalledWith(1)is redundant given the mockedprocess.exitthrow already encodes the exit code. Not incorrect, just belt-and-suspenders.🤖 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 `@src/lib/actions/sandbox/connect-flow.test.ts` around lines 420 - 459, This test is asserting the same exit behavior twice in connectSandbox; keep the rejection expectation that captures the mocked process.exit throw and remove the extra exitSpy assertion in the connect-flow test. Use the existing harness.connectSandbox, exitSpy, and process.exit mock setup to leave only one assertion covering the failure path.src/lib/actions/sandbox/skill-install.test.ts (1)
196-213: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest logic matches guard-then-capture ordering contract.
Assertions correctly verify
ensureLiveSandboxOrExitshort-circuits beforecaptureSandboxSshConfig/uploadDirectory, consistent withinstallSandboxSkill's ordering inskill-install.ts.One minor note: the
console.logspy at line 199 is not restored (nomockRestore()/afterEach). If the suite doesn't have globalrestoreMocks: truein its vitest config, this could leak into subsequent tests in the same file.🤖 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 `@src/lib/actions/sandbox/skill-install.test.ts` around lines 196 - 213, The test in skill-install.test.ts leaves a console.log spy active without cleanup, which can leak into later tests. Update the "stops skill installation at the shared gateway liveness guard" test to restore the console.log spy after the assertion, or move the cleanup into a shared afterEach if that pattern is used in this suite. Use the existing vi.spyOn(console, "log") setup in the test as the anchor for the fix.test/process-recovery-primitives.test.ts (1)
292-292: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest title missing required issue-ref suffix.
New test title
"honors the sandbox-exec timeout without falling back to SSH"doesn't include a local issue ref in a final(#1234)suffix.As per coding guidelines,
test/**/*.test.{js,ts}: "use behavior-oriented titles with local issue refs in a final(#1234)suffix."🤖 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/process-recovery-primitives.test.ts` at line 292, Update the test case title in the affected test block to follow the required behavior-oriented format and append a local issue reference suffix like "(`#1234`)". Make the change on the it() call for the sandbox-exec timeout test in process-recovery-primitives.test.ts, keeping the existing behavior text but adding the final issue-ref suffix to the title.Source: Coding guidelines
test/cli-oclif-compatibility.test.ts (1)
384-758: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew test titles are missing the required local issue-ref suffix.
None of the new tests here (e.g. "recovers a requested sandbox, rereads the registry, and dispatches connect", "corrects a single sandbox-like global status argument without a CLI subprocess", etc.) include a trailing
(#1234)-style issue reference.As per coding guidelines,
test/**/*.test.{js,ts}: "Root-level integration tests undertest/should import source code, use ESM imports, and use behavior-oriented titles with local issue refs in a final(#1234)suffix."🤖 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/cli-oclif-compatibility.test.ts` around lines 384 - 758, The new integration test titles in this spec are missing the required local issue-ref suffix. Update each newly added `it(...)` title in the test file to end with a `(`#1234`)`-style reference, keeping the existing behavior-oriented wording intact; use the test names around `withDirectSandboxRecoveryDispatch`, `withDirectStatusDispatch`, and the direct `dispatchCli` cases as the places to fix.Source: Coding guidelines
test/cli/status-routing.test.ts (1)
12-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew test titles are missing the required local issue-ref suffix.
As with the other
test/files in this cohort, none of the three new tests include a trailing(#1234)-style issue reference.As per coding guidelines,
test/**/*.test.{js,ts}: "use behavior-oriented titles with local issue refs in a final(#1234)suffix."🤖 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/cli/status-routing.test.ts` around lines 12 - 37, Update the three new test case titles in status-routing.test.ts to include the required local issue reference suffix, matching the pattern used by other test files. Keep the behavior-oriented wording intact in each it(...) block, and append a final “(`#1234`)”-style issue ref to each title in the test definitions for status --help, sandbox-first status rejects unexpected positional arguments through command-id dispatch, and never emits an unsafe sandbox token in a copy-paste status command.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/actions/sandbox/connect-flow.test.ts`:
- Around line 420-459: This test is asserting the same exit behavior twice in
connectSandbox; keep the rejection expectation that captures the mocked
process.exit throw and remove the extra exitSpy assertion in the connect-flow
test. Use the existing harness.connectSandbox, exitSpy, and process.exit mock
setup to leave only one assertion covering the failure path.
In `@src/lib/actions/sandbox/skill-install.test.ts`:
- Around line 196-213: The test in skill-install.test.ts leaves a console.log
spy active without cleanup, which can leak into later tests. Update the "stops
skill installation at the shared gateway liveness guard" test to restore the
console.log spy after the assertion, or move the cleanup into a shared afterEach
if that pattern is used in this suite. Use the existing vi.spyOn(console, "log")
setup in the test as the anchor for the fix.
In `@test/cli-oclif-compatibility.test.ts`:
- Around line 384-758: The new integration test titles in this spec are missing
the required local issue-ref suffix. Update each newly added `it(...)` title in
the test file to end with a `(`#1234`)`-style reference, keeping the existing
behavior-oriented wording intact; use the test names around
`withDirectSandboxRecoveryDispatch`, `withDirectStatusDispatch`, and the direct
`dispatchCli` cases as the places to fix.
In `@test/cli/status-routing.test.ts`:
- Around line 12-37: Update the three new test case titles in
status-routing.test.ts to include the required local issue reference suffix,
matching the pattern used by other test files. Keep the behavior-oriented
wording intact in each it(...) block, and append a final “(`#1234`)”-style issue
ref to each title in the test definitions for status --help, sandbox-first
status rejects unexpected positional arguments through command-id dispatch, and
never emits an unsafe sandbox token in a copy-paste status command.
In `@test/process-recovery-primitives.test.ts`:
- Line 292: Update the test case title in the affected test block to follow the
required behavior-oriented format and append a local issue reference suffix like
"(`#1234`)". Make the change on the it() call for the sandbox-exec timeout test in
process-recovery-primitives.test.ts, keeping the existing behavior text but
adding the final issue-ref suffix to the title.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 09947a1e-d376-46b0-8924-6562012a2918
📒 Files selected for processing (20)
src/commands/sandbox/oclif-command-adapters.test.tssrc/lib/actions/sandbox/connect-flow.test.tssrc/lib/actions/sandbox/connect-route-repair.test.tssrc/lib/actions/sandbox/gateway-state-drift.test.tssrc/lib/actions/sandbox/gateway-state-hints.test.tssrc/lib/actions/sandbox/skill-install.test.tssrc/lib/actions/sandbox/status-flow.test.tssrc/lib/cli/argv-normalizer.test.tssrc/lib/gateway-runtime-action.test.tssrc/lib/registry-recovery-action.test.tstest/cli-oclif-compatibility.test.tstest/cli/connect-recovery.test.tstest/cli/status-gateway-lifecycle.test.tstest/cli/status-routing.test.tstest/gateway-state-reconcile-2276.test.tstest/process-recovery-managed-controller.test.tstest/process-recovery-primitives.test.tstest/process-recovery.test.tstest/sandbox-connect-inference/route-swap-repair.test.tstest/support/connect-flow-test-harness.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/cli/connect-recovery.test.ts (1)
367-460: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLarge hand-written onboard-session JSON duplicates schema knowledge inline.
This test constructs the full
onboard-session.jsonpayload by hand (all step statuses, metadata, etc.) rather than via a shared helper (the file already haswriteSandboxRegistryfromtest/cli/helpers.tsfor the analogous registry case). If the onboard-session schema changes elsewhere, this inline literal can silently drift and either break or mask the intended "recover from last session" behavior.Consider extracting a
writeOnboardSession(home, overrides)helper alongsidewriteSandboxRegistryfor reuse and to keep the schema definition in one place.🤖 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/cli/connect-recovery.test.ts` around lines 367 - 460, The `connect recovers a named sandbox from the last onboard session when the registry is empty` test is duplicating the full onboard-session schema inline, which can drift from the real shape. Add a shared `writeOnboardSession(home, overrides)` helper next to `writeSandboxRegistry` in the CLI test helpers, and use it here to generate `onboard-session.json` from one central schema definition. Keep the test-specific values (`sandboxName`, `provider`, `metadata`, step statuses, etc.) as overrides so the recovery scenario stays explicit without hardcoding the whole payload.
🤖 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 `@src/lib/registry-recovery-seeded-paths.test.ts`:
- Around line 56-63: The mock for validateName in
registry-recovery-seeded-paths.test.ts is duplicating production validation
logic instead of exercising the real implementation, which can let tests drift
from src/lib/runner.js. Update the vi.mock("./runner.js") setup to import the
actual validateName via vi.importActual (or otherwise delegate to the real
function) and only wrap it for the test’s needs, so the assertions about skipped
names stay aligned with the production validator.
In `@test/support/status-flow-test-harness.ts`:
- Line 60: The status-flow test harness is collapsing an explicit null value for
processRunning into false, so null-specific behavior cannot be exercised. Update
the option resolution in status-flow-test-harness around the processRunning
handling so it preserves an explicitly passed null instead of defaulting it
away, while still falling back to false only when the option is undefined. Check
the processRunning field usage in the harness setup and any related helper logic
so tests can pass through boolean | null unchanged.
---
Nitpick comments:
In `@test/cli/connect-recovery.test.ts`:
- Around line 367-460: The `connect recovers a named sandbox from the last
onboard session when the registry is empty` test is duplicating the full
onboard-session schema inline, which can drift from the real shape. Add a shared
`writeOnboardSession(home, overrides)` helper next to `writeSandboxRegistry` in
the CLI test helpers, and use it here to generate `onboard-session.json` from
one central schema definition. Keep the test-specific values (`sandboxName`,
`provider`, `metadata`, step statuses, etc.) as overrides so the recovery
scenario stays explicit without hardcoding the whole payload.
🪄 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: 5bf8de47-df16-458c-9598-789287dfeb5e
📒 Files selected for processing (9)
src/lib/actions/sandbox/connect-flow.test.tssrc/lib/actions/sandbox/connect-route-lifecycle.test.tssrc/lib/actions/sandbox/gateway-state-hints.test.tssrc/lib/actions/sandbox/status-flow.test.tssrc/lib/registry-recovery-action.test.tssrc/lib/registry-recovery-seeded-paths.test.tstest/cli-oclif-compatibility.test.tstest/cli/connect-recovery.test.tstest/support/status-flow-test-harness.ts
💤 Files with no reviewable changes (2)
- src/lib/actions/sandbox/connect-flow.test.ts
- src/lib/registry-recovery-action.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/actions/sandbox/gateway-state-hints.test.ts
- src/lib/actions/sandbox/status-flow.test.ts
- test/cli-oclif-compatibility.test.ts
| lookup?: SandboxGatewayState; | ||
| lookupState?: "present" | "missing"; | ||
| preflight?: SandboxStatusPreflightResult; | ||
| processRunning?: boolean | null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
?? collapses null into false, making the processRunning: null option unusable.
options.processRunning is typed boolean | null, but options.processRunning ?? false treats null the same as undefined, so a caller passing processRunning: null will always get false resolved instead of null. Any future test intending to exercise null-specific gateway-running behavior via this harness silently can't.
🐛 Proposed fix
- vi.spyOn(processRecovery, "isSandboxGatewayRunningForStatus").mockResolvedValue(
- options.processRunning ?? false,
- );
+ vi.spyOn(processRecovery, "isSandboxGatewayRunningForStatus").mockResolvedValue(
+ options.processRunning === undefined ? false : options.processRunning,
+ );Also applies to: 168-170
🤖 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/support/status-flow-test-harness.ts` at line 60, The status-flow test
harness is collapsing an explicit null value for processRunning into false, so
null-specific behavior cannot be exercised. Update the option resolution in
status-flow-test-harness around the processRunning handling so it preserves an
explicitly passed null instead of defaulting it away, while still falling back
to false only when the option is undefined. Check the processRunning field usage
in the harness setup and any related helper logic so tests can pass through
boolean | null unchanged.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Final automated-review disposition for Addressed in this PR:
No further code change is warranted for the remaining Nemotron notes:
Final evidence: 217/217 shuffled changed-file tests, all normal hooks, 43 CI checks passed, 3 expected skips, and no failed/pending checks. CodeRabbit reports no actionable comments; GPT-5.5 recommends |
<!-- markdownlint-disable MD041 --> ## Summary Reduce unnecessary process isolation across sandbox status, gateway reconciliation, connect recovery, and inference route repair tests while retaining representative CLI, timeout, listener, security, and cross-command contracts. The five affected process suites now launch 20 first-level processes instead of 101, with unchanged runtime behavior and no production-code changes. ## Related Issue Part of NVIDIA#6245 ## Changes - Move status routing and gateway lifecycle branches into direct public-dispatch, status-flow, and gateway-state tests while retaining CLI help/parser, unsafe-token, hanging-pipe timeout, and healthy-ordering contracts. - Move route-repair branches into the direct connect harness while retaining route-swap plumbing, local Ollama proxy/secret isolation, and WSL fallback contracts. - Move gateway reconciliation scenarios 1–12 into direct gateway lifecycle, status rendering, and skill liveness seams while retaining the real `connect` → `rebuild` scenario 14 contract. - Move connect recovery branches into argv, Oclif adapter, registry recovery, and process-recovery seams while retaining successful and failed privileged-Docker recovery plus real session-backed registry recovery contracts. - Reset registry-recovery dependency mocks deterministically and cover requested-sandbox recovery through public dispatch under shuffled test order. - Extract focused status, connect-route, and seeded-registry test surfaces so existing large test files do not grow past the advisor policy. - Reduce outer CLI loaders from 83 to 14 (−83.1%) and explicit test-owned first-level processes from 101 to 20 (−80.2%). ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: all 23 changed paths are tests or test support; the final docs-writer review found no runtime command, flag, configuration, API, policy, or workflow change. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: independent local review covered assertion migration, retained process/security boundaries, mock and module-cache isolation, environment restoration, shuffled-order determinism, and the CI/advisor follow-up; all findings were fixed and re-reviewed with no remaining findings. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — 217/217 changed-file tests passed under shuffled order (seed 6245) in 39.76s; CLI typecheck, Biome, project-overlap, source-shape, test-size, conditional-growth, and monolith-growth checks passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — `npm test` completed in 15m25.22s with 13,079 passing and 36 skipped tests. Seven mode assertions inherited the host's `077` umask and passed 68/68 under `022`; one E2E source-hygiene failure is byte-identical on `origin/main`. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) ## Benchmark - Same five process suites on `origin/main`: 76 tests in 57.10s. - Final branch: 13 retained process tests in 20.19s, 64.6% faster; migrated decision coverage remains in direct tests. - Outer CLI loaders: 83 → 14; explicit first-level processes including listeners: 101 → 20. - Final-head coverage artifacts measured the five migrated files at 32.42s versus 107.73s on `main` (−69.9%); aggregate blob execution fell 7.4% and summed shard job time fell 6.2%, while critical shard wall time was 8m39s versus 8m13s on `main`. - `gateway-state-drift` now executes its tests in 26ms, but its warmed source graph still costs 10.13s during collection and remains a shard-3 hotspot for the next batch. - The single full clean-build `npm test` run was 15m25.22s on Node 25.9.0 versus the issue's 14m19.65s reference, so this PR does not claim a repo-wide local wall-time win yet. The remaining untouched corpus still dominates and needs additional batches. --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added and strengthened CLI and sandbox connect/recovery contract coverage, including safer failure behavior, deterministic guidance, and stricter validation of unsafe/privileged flows. * Expanded status and lifecycle classification scenarios (including inference and gateway-state edge cases) and tightened assertions around when sandbox teardown does or does not occur. * Introduced reusable status/connect-flow test harnesses and improved mocking/teardown isolation; added seeded registry recovery tests and additional timeout/probe-only orchestration checks. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Summary
Reduce unnecessary process isolation across sandbox status, gateway reconciliation, connect recovery, and inference route repair tests while retaining representative CLI, timeout, listener, security, and cross-command contracts. The five affected process suites now launch 20 first-level processes instead of 101, with unchanged runtime behavior and no production-code changes.
Related Issue
Part of #6245
Changes
connect→rebuildscenario 14 contract.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes —npm testcompleted in 15m25.22s with 13,079 passing and 36 skipped tests. Seven mode assertions inherited the host's077umask and passed 68/68 under022; one E2E source-hygiene failure is byte-identical onorigin/main.npm run docsbuilds without warnings (doc changes only)Benchmark
origin/main: 76 tests in 57.10s.main(−69.9%); aggregate blob execution fell 7.4% and summed shard job time fell 6.2%, while critical shard wall time was 8m39s versus 8m13s onmain.gateway-state-driftnow executes its tests in 26ms, but its warmed source graph still costs 10.13s during collection and remains a shard-3 hotspot for the next batch.npm testrun was 15m25.22s on Node 25.9.0 versus the issue's 14m19.65s reference, so this PR does not claim a repo-wide local wall-time win yet. The remaining untouched corpus still dominates and needs additional batches.Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit