test(sandbox): retire local fork-storm duplicate by cv · Pull Request #5900 · NVIDIA/NemoClaw

cv · 2026-06-27T07:26:23Z

Summary

Retire the host-local fork-storm test now that the isolated live Vitest scenario owns the exact 5,000-process acceptance check. This prevents the ordinary CLI suite from creating hundreds of sleeper processes that compete with parallel Vitest workers.

Related Issue

Refs #2173 and follow-up coverage from #5865.

Changes

Delete the redundant fork-storm test and its private process-count/probe helpers from test/sandbox-rlimit-hooks.test.ts.
Keep deterministic local coverage for hook installation and limit enforcement; keep the destructive fork-denial assertion in sandbox-rlimits-connect.

Type of Change

Code change (feature, bug fix, or refactor)
Code change with doc updates
Doc only (prose changes, no code sample modifications)
Doc only (includes code sample changes)

Quality Gates

Tests added or updated for changed behavior
Existing tests cover changed behavior — justification: the live sandbox-rlimits-connect scenario performs the exact 5,000-process fork-denial acceptance check, while four deterministic local tests retain hook and limit coverage.
Tests not applicable — justification:
Docs updated for user-facing behavior changes
Docs not applicable — justification: test ownership changes only; sandbox behavior is unchanged.
Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: verified that the isolated live scenario retains the exact security acceptance assertion and the local suite retains deterministic enforcement checks.
Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
Targeted tests pass for changed behavior
Full npm test passes (broad runtime changes only)
Quality Gates section completed with required justifications or waivers
No secrets, API keys, or credentials committed
npm run docs builds without warnings (doc changes only)
Doc pages follow the style guide (doc changes only)
New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

Tests
- Updated the sandbox rlimit test suite by removing an obsolete fork-storm scenario and related helper checks.
- Kept the remaining replay coverage for rlimit hook behavior intact.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>

coderabbitai · 2026-06-27T07:26:38Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 594eef05-6433-424c-ac83-d729e4814c1f

📥 Commits

Reviewing files that changed from the base of the PR and between 2330e39 and 866cff3.

📒 Files selected for processing (1)

test/sandbox-rlimit-hooks.test.ts

💤 Files with no reviewable changes (1)

test/sandbox-rlimit-hooks.test.ts

📝 Walkthrough

Walkthrough

The sandbox rlimit hook test file removes fork-storm helper code and the bounded fork-storm denial test. The remaining replay tests for the sandbox rlimit hook suite stay in place.

Changes

Sandbox rlimit hook test cleanup

Layer / File(s)	Summary
Remove fork-storm test coverage `test/sandbox-rlimit-hooks.test.ts`	The test file drops the fork-storm-specific import, safety constants, process-count helper, and `forkStormIt(...)` test wiring from the sandbox rlimit hook suite.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Possibly related PRs

NVIDIA/NemoClaw#5682: Touches the same sandbox rlimit fork-storm test area, but adds the fork-storm denial check that this PR removes.

Suggested labels

area: sandbox

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: removing a redundant local fork-storm test from the sandbox test suite.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch codex/retire-local-fork-storm

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

github-code-quality · 2026-06-27T07:27:00Z

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the codex/retire-local-f... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.

File	`main`	`codex/retire-local-f...` 866cff3	+/-
`nemoclaw/src/se...cret-scanner.ts`	—	100%	—
`nemoclaw/src/commands/slash.ts`	—	100%	—
`nemoclaw/src/li...bprocess-env.ts`	—	100%	—
`nemoclaw/src/bl...eprint/state.ts`	—	98%	—
`nemoclaw/src/onboard/config.ts`	—	98%	—
`nemoclaw/src/bl...int/snapshot.ts`	—	97%	—
`nemoclaw/src/bl...print/runner.ts`	—	95%	—
`nemoclaw/src/co...ration-state.ts`	—	94%	—
`nemoclaw/src/bl...ate-networks.ts`	—	94%	—
`nemoclaw/src/index.ts`	—	94%	—

TypeScript / code-coverage/cli

The overall coverage in the codex/retire-local-f... branch is 47%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.

File	`main`	`codex/retire-local-f...` 866cff3	+/-
`src/lib/state/o...oard-session.ts`	—	91%	—
`src/lib/actions...dbox/rebuild.ts`	—	73%	—
`src/lib/sandbox/config.ts`	—	72%	—
`src/lib/onboard/preflight.ts`	—	62%	—
`src/lib/shields/index.ts`	—	62%	—
`src/lib/actions...licy-channel.ts`	—	60%	—
`src/lib/state/sandbox.ts`	—	56%	—
`src/lib/policy/index.ts`	—	48%	—
`src/lib/onboard...er-gpu-patch.ts`	—	47%	—
`src/lib/onboard.ts`	—	18%	—

_{Updated June 27, 2026 07:30 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.}

github-actions · 2026-06-27T07:27:10Z

E2E Advisor Recommendation

Required E2E: None
Optional E2E: None

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

None. No existing E2E is required because this is a tests-only change and cannot affect runtime/user flows directly. Reviewers should still consider whether removing the fork-storm coverage is acceptable for the sandbox rlimit security boundary.

Optional E2E

None.

New E2E recommendations

sandbox resource-limit security boundary (medium): The diff removes the bounded fork-storm regression check from test coverage. If that behavior remains security-critical, consider adding or restoring coverage that validates connect-shell rlimit enforcement against process exhaustion in an isolated E2E/smoke environment.
- Suggested test: sandbox-rlimit-fork-storm-e2e

github-actions · 2026-06-27T07:27:11Z

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: None
Optional Vitest E2E scenarios: None

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

None. Only a non-scenario Vitest test outside test/e2e-scenario/ changed; this does not affect the Vitest E2E scenario registry, workflow, live support, fixtures, or scenario behavior.

Optional Vitest E2E scenarios

None.

Relevant changed files

None.

github-actions · 2026-06-27T07:28:37Z

PR Review Advisor — No blocking findings

Merge posture: No blocking advisor findings
Primary next action: Add or justify PRA-T1 and any related test follow-ups.
Open items: 0 required · 0 warnings · 0 suggestions · 1 test follow-up
Since last review: 0 prior items resolved · 0 still apply · 0 new items found

Action checklist

PRA-T1 Add or justify test follow-up: Acceptance clause

Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

PRA-T1 Acceptance clause — Refs [All Platform][Security] Sandbox does not enforce nproc / nofile rlimits — ulimit -u = unlimited, ulimit -n = 1048576 inside sandbox shell #2173 and follow-up coverage from fix(sandbox): keep rlimit shell hooks quiet #5865. — add test evidence or identify existing coverage. The deterministic validation context did not include linked issue bodies or comments, so literal issue clauses could not be verified. Nearby code evidence does show `test/e2e-scenario/live/sandbox-rlimits-connect.test.ts` records `issue: 2173` and preserves the live rlimit-connect acceptance path.

Workflow run details

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.

github-actions · 2026-06-27T07:29:08Z

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Fork storm unit test removed; e2e replacement runs only on explicit opt-in.
Open items: 0 required · 1 warning · 2 suggestions · 2 test follow-ups
Since last review: 0 prior items resolved · 0 still apply · 3 new items found

Action checklist

PRA-1 Resolve or justify: Fork storm unit test removed; e2e replacement runs only on explicit opt-in in test/sandbox-rlimit-hooks.test.ts:1
PRA-T1 Add or justify test follow-up: Remaining unit tests cover static rlimit mechanics but not dynamic fork denial under load
PRA-T2 Add or justify test follow-up: Acceptance clause
PRA-2 In-scope improvement: Dead code correctly removed: FORK_STORM constants and currentUserProcessCount helper in test/sandbox-rlimit-hooks.test.ts:1
PRA-3 In-scope improvement: Remaining unit tests cover static rlimit mechanics but not dynamic fork denial under load in test/sandbox-rlimit-hooks.test.ts:292

Findings index

ID	Severity	Category	Location	Required action
`PRA-1`	Resolve/justify	security	`test/sandbox-rlimit-hooks.test.ts:1`	Either: (a) add sandbox-rlimits-connect to a scheduled/nightly workflow (e.g., nightly-e2e.yaml), or (b) retain a lightweight unit test in sandbox-rlimit-hooks.test.ts that validates nproc enforcement by spawning children until EAGAIN, without Docker/connect overhead. Option (b) is lower cost and restores push-frequency coverage.
`PRA-2`	Improvement	security	`test/sandbox-rlimit-hooks.test.ts:1`	No action needed. Dead code correctly removed.
`PRA-3`	Improvement	tests	`test/sandbox-rlimit-hooks.test.ts:292`	Add a focused unit test in sandbox-rlimit-hooks.test.ts that sources the rlimit shim (rlimitShim helper) and spawns child processes in a loop until EAGAIN/Resource temporarily unavailable, asserting denial occurs at or below the configured nproc limit. This restores push-frequency coverage for the specific threat model without Docker overhead.

Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 2 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

`PRA-1` Resolve/justify — Fork storm unit test removed; e2e replacement runs only on explicit opt-in

Location: test/sandbox-rlimit-hooks.test.ts:1
Category: security
Problem: The deleted unit test (forkStormIt) exercised nproc rlimit fork-bomb protection on every push to main via platform-vitest-main.yaml (macOS skipped, WSL Ubuntu ran it). Its replacement e2e test (sandbox-rlimits-connect) requires NEMOCLAW_E2E_CONNECT_RLIMITS=1 and NEMOCLAW_RUN_E2E_SCENARIOS=1 — only triggered via workflow_dispatch or manual selection in e2e-vitest-scenarios.yaml. Not included in nightly-e2e.yaml, regression-e2e.yaml, or pr.yaml. This reduces regression detection frequency for a primary sandbox escape defense.
Impact: A regression in nproc limit enforcement (e.g., sandbox-rlimits.sh logic error, Dockerfile hook installation bug) would not be caught until someone manually runs the e2e scenario. The unit test provided push-frequency coverage on Linux runners.
Recommended action: Either: (a) add sandbox-rlimits-connect to a scheduled/nightly workflow (e.g., nightly-e2e.yaml), or (b) retain a lightweight unit test in sandbox-rlimit-hooks.test.ts that validates nproc enforcement by spawning children until EAGAIN, without Docker/connect overhead. Option (b) is lower cost and restores push-frequency coverage.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Check .github/workflows/e2e-vitest-scenarios.yaml: sandbox-rlimits-connect-vitest job runs only when explicitly selected via inputs.jobs or inputs.scenarios. Not present in nightly-e2e.yaml, regression-e2e.yaml, or pr.yaml.
Missing regression test: Scheduled CI run of sandbox-rlimits-connect e2e scenario, or a retained unit test that sources the rlimit shim and spawns processes until nproc denial (without full Docker replay).
Done when: The risk is fixed or explicitly justified in the PR. Verification: Check .github/workflows/e2e-vitest-scenarios.yaml: sandbox-rlimits-connect-vitest job runs only when explicitly selected via inputs.jobs or inputs.scenarios. Not present in nightly-e2e.yaml, regression-e2e.yaml, or pr.yaml.
Evidence: Deleted test used forkStormIt = process.platform === 'linux' ? it : it.skip; ran on WSL Ubuntu in platform-vitest-main.yaml. E2e test gated behind two opt-in env vars and workflow_dispatch.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

`PRA-2` Improvement — Dead code correctly removed: FORK_STORM constants and currentUserProcessCount helper

Location: test/sandbox-rlimit-hooks.test.ts:1
Category: security
Problem: Constants FORK_STORM_LIMIT_HEADROOM=512, FORK_STORM_SAFETY_HEADROOM=128, and currentUserProcessCount() helper (ran `ps -u $(id -u) -o pid= | wc -l`) were only used by the retired fork storm test. No other references in codebase. Clean removal.
Impact: No functional impact. Reduces test file size and eliminates unused code.
Suggested action: No action needed. Dead code correctly removed.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: grep -r FORK_STORM_LIMIT_HEADROOM && grep -r currentUserProcessCount --include="*.ts" --include="*.sh" — no matches expected.
Missing regression test: None — dead code removal.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: PR diff shows deletion of these constants and function; grep confirms no other references.

`PRA-3` Improvement — Remaining unit tests cover static rlimit mechanics but not dynamic fork denial under load

Location: test/sandbox-rlimit-hooks.test.ts:292
Category: tests
Problem: Four remaining tests cover: POSIX sh rlimit safety, connect shell limit enforcement, shadowed ulimit bypass, silent verification, and stale base replay for OpenClaw/Hermes. All verify static limit values and hook installation. None spawn child processes to hit the nproc limit and verify denial. The fork storm test was the only one exercising process-spawn denial under load.
Impact: Static checks pass even if the nproc limit is set but not actually enforced against fork(). The e2e test covers this but only on-demand.
Suggested action: Add a focused unit test in sandbox-rlimit-hooks.test.ts that sources the rlimit shim (rlimitShim helper) and spawns child processes in a loop until EAGAIN/Resource temporarily unavailable, asserting denial occurs at or below the configured nproc limit. This restores push-frequency coverage for the specific threat model without Docker overhead.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Review remaining test cases in sandbox-rlimit-hooks.test.ts — none call spawn/fork or check for process creation denial.
Missing regression test: Lightweight unit test that sources the rlimit shim and attempts to spawn children until EAGAIN, asserting denial occurs at or below configured nproc limit.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: Remaining tests: expectSystemRlimitHookEnforcesLimits (checks ulimit -u value), expectSystemRlimitHookBypassesShadowedUlimit, expectSystemRlimitHookIsSilentWhenVerificationFails, expectRlimitLibIsPosixShSafe, expectRlimitLibRejectsUnboundedPosixShNoFile, expectUnsupportedNprocDoesNotMaskPosixShNoFile — all static limit checks.

Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

PRA-T1 Remaining unit tests cover static rlimit mechanics but not dynamic fork denial under load — Add a focused unit test in sandbox-rlimit-hooks.test.ts that sources the rlimit shim (rlimitShim helper) and spawns child processes in a loop until EAGAIN/Resource temporarily unavailable, asserting denial occurs at or below the configured nproc limit. This restores push-frequency coverage for the specific threat model without Docker overhead.
PRA-T2 Acceptance clause — Keep deterministic local coverage for hook installation and limit enforcement; keep the destructive fork-denial assertion in sandbox-rlimits-connect — add test evidence or identify existing coverage. Four deterministic unit tests retained for hook installation and limit enforcement. Fork-denial assertion moved to e2e but e2e runs only on-demand — no push-frequency coverage for dynamic fork denial.

Since last review details

Current findings, using the urgency labels above:

`PRA-1` Resolve/justify — Fork storm unit test removed; e2e replacement runs only on explicit opt-in

Location: test/sandbox-rlimit-hooks.test.ts:1
Category: security
Problem: The deleted unit test (forkStormIt) exercised nproc rlimit fork-bomb protection on every push to main via platform-vitest-main.yaml (macOS skipped, WSL Ubuntu ran it). Its replacement e2e test (sandbox-rlimits-connect) requires NEMOCLAW_E2E_CONNECT_RLIMITS=1 and NEMOCLAW_RUN_E2E_SCENARIOS=1 — only triggered via workflow_dispatch or manual selection in e2e-vitest-scenarios.yaml. Not included in nightly-e2e.yaml, regression-e2e.yaml, or pr.yaml. This reduces regression detection frequency for a primary sandbox escape defense.
Impact: A regression in nproc limit enforcement (e.g., sandbox-rlimits.sh logic error, Dockerfile hook installation bug) would not be caught until someone manually runs the e2e scenario. The unit test provided push-frequency coverage on Linux runners.
Recommended action: Either: (a) add sandbox-rlimits-connect to a scheduled/nightly workflow (e.g., nightly-e2e.yaml), or (b) retain a lightweight unit test in sandbox-rlimit-hooks.test.ts that validates nproc enforcement by spawning children until EAGAIN, without Docker/connect overhead. Option (b) is lower cost and restores push-frequency coverage.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Check .github/workflows/e2e-vitest-scenarios.yaml: sandbox-rlimits-connect-vitest job runs only when explicitly selected via inputs.jobs or inputs.scenarios. Not present in nightly-e2e.yaml, regression-e2e.yaml, or pr.yaml.
Missing regression test: Scheduled CI run of sandbox-rlimits-connect e2e scenario, or a retained unit test that sources the rlimit shim and spawns processes until nproc denial (without full Docker replay).
Done when: The risk is fixed or explicitly justified in the PR. Verification: Check .github/workflows/e2e-vitest-scenarios.yaml: sandbox-rlimits-connect-vitest job runs only when explicitly selected via inputs.jobs or inputs.scenarios. Not present in nightly-e2e.yaml, regression-e2e.yaml, or pr.yaml.
Evidence: Deleted test used forkStormIt = process.platform === 'linux' ? it : it.skip; ran on WSL Ubuntu in platform-vitest-main.yaml. E2e test gated behind two opt-in env vars and workflow_dispatch.

`PRA-2` Improvement — Dead code correctly removed: FORK_STORM constants and currentUserProcessCount helper

Location: test/sandbox-rlimit-hooks.test.ts:1
Category: security
Problem: Constants FORK_STORM_LIMIT_HEADROOM=512, FORK_STORM_SAFETY_HEADROOM=128, and currentUserProcessCount() helper (ran `ps -u $(id -u) -o pid= | wc -l`) were only used by the retired fork storm test. No other references in codebase. Clean removal.
Impact: No functional impact. Reduces test file size and eliminates unused code.
Suggested action: No action needed. Dead code correctly removed.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: grep -r FORK_STORM_LIMIT_HEADROOM && grep -r currentUserProcessCount --include="*.ts" --include="*.sh" — no matches expected.
Missing regression test: None — dead code removal.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: PR diff shows deletion of these constants and function; grep confirms no other references.

`PRA-3` Improvement — Remaining unit tests cover static rlimit mechanics but not dynamic fork denial under load

Location: test/sandbox-rlimit-hooks.test.ts:292
Category: tests
Problem: Four remaining tests cover: POSIX sh rlimit safety, connect shell limit enforcement, shadowed ulimit bypass, silent verification, and stale base replay for OpenClaw/Hermes. All verify static limit values and hook installation. None spawn child processes to hit the nproc limit and verify denial. The fork storm test was the only one exercising process-spawn denial under load.
Impact: Static checks pass even if the nproc limit is set but not actually enforced against fork(). The e2e test covers this but only on-demand.
Suggested action: Add a focused unit test in sandbox-rlimit-hooks.test.ts that sources the rlimit shim (rlimitShim helper) and spawns child processes in a loop until EAGAIN/Resource temporarily unavailable, asserting denial occurs at or below the configured nproc limit. This restores push-frequency coverage for the specific threat model without Docker overhead.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Review remaining test cases in sandbox-rlimit-hooks.test.ts — none call spawn/fork or check for process creation denial.
Missing regression test: Lightweight unit test that sources the rlimit shim and attempts to spawn children until EAGAIN, asserting denial occurs at or below configured nproc limit.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: Remaining tests: expectSystemRlimitHookEnforcesLimits (checks ulimit -u value), expectSystemRlimitHookBypassesShadowedUlimit, expectSystemRlimitHookIsSilentWhenVerificationFails, expectRlimitLibIsPosixShSafe, expectRlimitLibRejectsUnboundedPosixShNoFile, expectUnsupportedNprocDoesNotMaskPosixShNoFile — all static limit checks.

Workflow run details

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.

cv · 2026-06-27T07:29:09Z

Advisor follow-up (PRA-T1): the #2173 acceptance clause is owned by test/e2e-scenario/live/sandbox-rlimits-connect.test.ts. Its connect script launches seq 1 5000 sleeper processes, captures the fork log, and asserts Resource temporarily unavailable|fork: retry|fork; it also verifies numeric login/interactive nproc and nofile limits. That isolated scenario passed during #5865 (run 28252272339). The remaining four deterministic local hook tests and the full CLI coverage suite also pass on this branch with 16 workers. Adding another host-local fork test would reintroduce the resource contention this PR removes.

cv · 2026-06-27T07:30:10Z

Nemotron follow-up (PRA-1, PRA-T1, PRA-T2): retaining a “lightweight” fork-until-EAGAIN unit test is not safe in a shared test runner. RLIMIT_NPROC is enforced against the real UID, so proving denial necessarily consumes that UID’s process budget and competes with parallel Vitest workers; lowering the headroom only makes the result dependent on unrelated processes. The deterministic local test still verifies that the hook installs numeric nproc <= 4096 / nofile <= 65536 limits and that both limits cannot be raised. The kernel-level fork-denial acceptance remains in the isolated sandbox scenario, where it can safely create 5,000 children in the sandbox process namespace. That scenario is explicit-only by design because it is destructive, and the E2E advisor selects it when rlimit runtime paths change (as it did in #5865). Scheduling that cloud scenario nightly is a separate cost/cadence decision and should not be smuggled into this test-cleanup PR.

## Summary Retire the host-local fork-storm test now that the isolated live Vitest scenario owns the exact 5,000-process acceptance check. This prevents the ordinary CLI suite from creating hundreds of sleeper processes that compete with parallel Vitest workers. ## Related Issue Refs NVIDIA#2173 and follow-up coverage from NVIDIA#5865. ## Changes - Delete the redundant fork-storm test and its private process-count/probe helpers from `test/sandbox-rlimit-hooks.test.ts`. - Keep deterministic local coverage for hook installation and limit enforcement; keep the destructive fork-denial assertion in `sandbox-rlimits-connect`. ## 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 - [x] Existing tests cover changed behavior — justification: the live `sandbox-rlimits-connect` scenario performs the exact 5,000-process fork-denial acceptance check, while four deterministic local tests retain hook and limit coverage. - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: test ownership changes only; sandbox behavior is unchanged. - [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: verified that the isolated live scenario retains the exact security acceptance assertion and the local suite retains deterministic enforcement checks. - [ ] 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] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [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) ---  Signed-off-by: Carlos Villela <cvillela@nvidia.com>  ## Summary by CodeRabbit * **Tests** * Updated the sandbox rlimit test suite by removing an obsolete fork-storm scenario and related helper checks. * Kept the remaining replay coverage for rlimit hook behavior intact.  Signed-off-by: Carlos Villela <cvillela@nvidia.com>

test(sandbox): retire local fork-storm duplicate

866cff3

Signed-off-by: Carlos Villela <cvillela@nvidia.com>

cv self-assigned this Jun 27, 2026

cv merged commit 092729e into main Jun 27, 2026
47 checks passed

cv deleted the codex/retire-local-fork-storm branch June 27, 2026 07:32

cv added the v0.0.69 label Jun 27, 2026

wscurran added the area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery label Aug 1, 2026

Uh oh!

Conversation

cv commented Jun 27, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Related Issue

Changes

Type of Change

Quality Gates

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Uh oh!

github-code-quality Bot commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Overview

TypeScript / code-coverage/plugin

TypeScript / code-coverage/cli

Uh oh!

github-actions Bot commented Jun 27, 2026

E2E Advisor Recommendation

E2E Recommendation Advisor

Required E2E

Optional E2E

New E2E recommendations

Uh oh!

github-actions Bot commented Jun 27, 2026

Vitest E2E Scenario Recommendation

Vitest E2E Scenario Advisor

Required Vitest E2E scenarios

Optional Vitest E2E scenarios

Relevant changed files

Uh oh!

github-actions Bot commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Advisor — No blocking findings

Action checklist

Uh oh!

github-actions Bot commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Advisor (Nemotron Ultra) — Changes requested

Action checklist

Findings index

⚠️ Resolve or justify before merge

PRA-1 Resolve/justify — Fork storm unit test removed; e2e replacement runs only on explicit opt-in

💡 In-scope improvements

PRA-2 Improvement — Dead code correctly removed: FORK_STORM constants and currentUserProcessCount helper

PRA-3 Improvement — Remaining unit tests cover static rlimit mechanics but not dynamic fork denial under load

PRA-1 Resolve/justify — Fork storm unit test removed; e2e replacement runs only on explicit opt-in

PRA-2 Improvement — Dead code correctly removed: FORK_STORM constants and currentUserProcessCount helper

PRA-3 Improvement — Remaining unit tests cover static rlimit mechanics but not dynamic fork denial under load

Uh oh!

cv commented Jun 27, 2026

Uh oh!

cv commented Jun 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cv commented Jun 27, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 27, 2026 •

edited

Loading

github-code-quality Bot commented Jun 27, 2026 •

edited

Loading

github-actions Bot commented Jun 27, 2026 •

edited

Loading

github-actions Bot commented Jun 27, 2026 •

edited

Loading

`PRA-1` Resolve/justify — Fork storm unit test removed; e2e replacement runs only on explicit opt-in

`PRA-2` Improvement — Dead code correctly removed: FORK_STORM constants and currentUserProcessCount helper

`PRA-3` Improvement — Remaining unit tests cover static rlimit mechanics but not dynamic fork denial under load

`PRA-1` Resolve/justify — Fork storm unit test removed; e2e replacement runs only on explicit opt-in

`PRA-2` Improvement — Dead code correctly removed: FORK_STORM constants and currentUserProcessCount helper

`PRA-3` Improvement — Remaining unit tests cover static rlimit mechanics but not dynamic fork denial under load