fix(sandbox): keep rlimit shell hooks quiet - #5865
Merged
Merged
Conversation
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughShell-init rlimit verification now ignores failures, generated sandbox provisioning shims and shell-hook tests were updated accordingly, a new explicit-only ChangesSandbox rlimit hook suppression
E2E workflow wiring for sandbox-rlimits connect
Messaging channel availability
Sequence Diagram(s)sequenceDiagram
participant dispatch as "workflow_dispatch"
participant workflow as "e2e-vitest-scenarios"
participant job as "sandbox-rlimits-connect-vitest"
participant report as "report-to-pr"
dispatch->>workflow: select sandbox-rlimits-connect-vitest
workflow->>job: run live Vitest job
job->>report: provide job result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
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 |
Contributor
PR Review Advisor — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 28248224270
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 28247964531
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 28249358493
|
cv
approved these changes
Jun 26, 2026
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 28250464279
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/e2e-scenarios/workflow-boundary.mts (1)
7805-7849: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidation block is correct; consider extracting it into a dedicated validator for consistency.
The block faithfully mirrors the workflow job (needs, explicit-only
if, env opt-ins, artifact dir, run step, secret wiring). Most comparable jobs use a dedicatedvalidateXxxVitestJob(errors, jobs)helper rather than an inline block; extracting this would keepvalidateE2eVitestScenariosWorkflowBoundaryuniform and easier to scan. Optional given the inline jetson precedent just above.🤖 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/e2e-scenarios/workflow-boundary.mts` around lines 7805 - 7849, The sandbox-rlimits-connect-vitest validation is correct but should be moved out of the inline block for consistency with the other validators. Extract this logic from validateE2eVitestScenariosWorkflowBoundary into a dedicated helper such as validateSandboxRlimitConnectVitestJob(errors, jobs), and keep the same checks for needs, explicitOnlyFreeStandingJobIf, env opt-ins, artifact dir, Run sandbox rlimit connect live test, and NVIDIA_INFERENCE_API_KEY wiring.
🤖 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 `@tools/e2e-scenarios/workflow-boundary.mts`:
- Around line 7805-7849: The sandbox-rlimits-connect-vitest validation is
correct but should be moved out of the inline block for consistency with the
other validators. Extract this logic from
validateE2eVitestScenariosWorkflowBoundary into a dedicated helper such as
validateSandboxRlimitConnectVitestJob(errors, jobs), and keep the same checks
for needs, explicitOnlyFreeStandingJobIf, env opt-ins, artifact dir, Run sandbox
rlimit connect live test, and NVIDIA_INFERENCE_API_KEY wiring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d394fcea-fca4-494e-9a37-9dbbdfce2992
📒 Files selected for processing (3)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/workflow-boundary.mts
wscurran
added
area: e2e
Contributor
Contributor
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 28251663965
|
Contributor
Vitest E2E Scenario Results —
|
| Job | Result |
|---|---|
| sandbox-rlimits-connect-vitest |
Contributor
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 28252272339
|
2 tasks
21 tasks
cv
added a commit
that referenced
this pull request
Jun 27, 2026
<!-- markdownlint-disable MD041 --> ## 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 - [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 <!-- Check all that apply. For any "covered by existing tests", "not applicable", or waiver entry, add a brief justification on the same line or in the Changes section. --> - [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 <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Carlos Villela <cvillela@nvidia.com>
21 tasks
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
## Summary - keep connect/login shell rlimit hooks quiet and best-effort so startup diagnostics do not pollute command, agent, or inference response streams - preserve loud PID 1 entrypoint rlimit enforcement while shell hooks use `verify_resource_limits --quiet || true` - add regression coverage proving shell hooks remain silent when verification fails ## Root cause PR NVIDIA#5682 added `verify_resource_limits` to `/etc/profile.d` and `/etc/bash.bashrc` hooks. When a shell cannot report/enforce a limit, that non-quiet verification emits `[SECURITY] ...` diagnostics on stderr before user commands run. Nightly E2E jobs then captured those diagnostics in API/model probes and failed assertions expecting clean responses. ## Test plan - `./node_modules/.bin/vitest run test/sandbox-rlimit-hooks.test.ts test/sandbox-provisioning.test.ts test/sandbox-build-context.test.ts test/sandbox-init.test.ts` - targeted nightly E2E dispatch pending for the rlimit-output-contamination failures Refs NVIDIA#5682 / NVIDIA#2173. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a new explicit test job for the rlimit connect scenario, making it selectable on demand in CI. * Improved shell startup behavior so resource-limit checks no longer interrupt builds or login shells when verification fails. * **Bug Fixes** * Reduced noisy security diagnostics during shell startup. * Messaging channel detection now considers built-in channel availability more consistently. * **Tests** * Expanded coverage for silent rlimit verification failures and the new workflow/job selection path. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## 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 <!-- Check all that apply. For any "covered by existing tests", "not applicable", or waiver entry, add a brief justification on the same line or in the Changes section. --> - [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 <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Carlos Villela <cvillela@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
verify_resource_limits --quiet || trueRoot cause
PR #5682 added
verify_resource_limitsto/etc/profile.dand/etc/bash.bashrchooks. When a shell cannot report/enforce a limit, that non-quiet verification emits[SECURITY] ...diagnostics on stderr before user commands run. Nightly E2E jobs then captured those diagnostics in API/model probes and failed assertions expecting clean responses.Test plan
./node_modules/.bin/vitest run test/sandbox-rlimit-hooks.test.ts test/sandbox-provisioning.test.ts test/sandbox-build-context.test.ts test/sandbox-init.test.tsRefs #5682 / #2173.
Summary by CodeRabbit
New Features
Bug Fixes
Tests