fix(cli): close non-terminal sandbox exec stdin by default - #5388
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a new --stdin/--no-stdin CLI flag, threads a stdin?: boolean option through SandboxExecOptions, maps it to spawnSync stdio via buildSandboxExecStdio, updates exec command parsing and tests, and updates reference docs including nemohermes entries. ChangesSandbox stdin control feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/reference/commands-nemohermes.mdx (1)
528-535: ⚡ Quick winUse active voice in stdin forwarding descriptions.
Lines 528 and 761 use passive voice ("Caller stdin is forwarded by default"). Rewrite to active voice for consistency with the documentation style guide:
- "NemoClaw forwards caller stdin by default for compatibility with terminal, pipe, and SSH workflows."
instead of "Caller stdin is forwarded by default for compatibility with terminal, pipe, and SSH workflows."
Also applies to: 761-763
🤖 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 `@docs/reference/commands-nemohermes.mdx` around lines 528 - 535, Replace the passive-voice sentence "Caller stdin is forwarded by default for compatibility with terminal, pipe, and SSH workflows." in docs/reference/commands-nemohermes.mdx with an active-voice sentence; specifically change it to "NemoClaw forwards caller stdin by default for compatibility with terminal, pipe, and SSH workflows." Make this same replacement at both occurrences (the one around lines 528–535 and the other around lines 761–763) so the file consistently uses active voice.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 `@docs/reference/commands-nemohermes.mdx`:
- Around line 528-535: Replace the passive-voice sentence "Caller stdin is
forwarded by default for compatibility with terminal, pipe, and SSH workflows."
in docs/reference/commands-nemohermes.mdx with an active-voice sentence;
specifically change it to "NemoClaw forwards caller stdin by default for
compatibility with terminal, pipe, and SSH workflows." Make this same
replacement at both occurrences (the one around lines 528–535 and the other
around lines 761–763) so the file consistently uses active voice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 81828124-6f04-468d-af48-7b377cca23a2
📒 Files selected for processing (6)
docs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxsrc/commands/sandbox/exec.test.tssrc/commands/sandbox/exec.tssrc/lib/actions/sandbox/exec.test.tssrc/lib/actions/sandbox/exec.ts
Signed-off-by: HwangJohn <angelic805@gmail.com>
Signed-off-by: HwangJohn <angelic805@gmail.com>
2dd280b to
0006d1a
Compare
|
Thanks again for the approval. I fixed the signed-commit requirement and the current commits are now GitHub-verified. The remaining required checks appear to be waiting on maintainer workflow approval for the fork PR. Could you approve running the workflows when convenient? |
Signed-off-by: HwangJohn <angelic805@gmail.com>
|
Pushed a signed follow-up commit to sync the generated Validation on DGX Spark/Linux:
The fork PR workflows appear to be awaiting maintainer approval again after this push. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/reference/commands-nemohermes.mdx (1)
442-442: ⚡ Quick winStandardize
--stdin/--no-stdinflag descriptions across both exec sections.Both table entries mix a complete sentence with a label-value pattern and violate the guideline that colons should only introduce lists. As per coding guidelines, documentation sentences must end with periods. Combine into a single sentence with a parenthetical for consistency with the table style elsewhere in the file (see line 617:
Timeout in seconds. Use '0' for no timeout).
docs/reference/commands-nemohermes.mdx#L442-L442: ChangeForce caller stdin forwarding or closure. Default: forward stdintoForce caller stdin forwarding or closure (default: forward stdin).docs/reference/commands-nemohermes.mdx#L618-L618: ChangeForce caller stdin forwarding or closure. Default: forward stdintoForce caller stdin forwarding or closure (default: forward stdin).🤖 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 `@docs/reference/commands-nemohermes.mdx` at line 442, Update the two table entries to use a single consistent sentence with a parenthetical default: in docs/reference/commands-nemohermes.mdx at 442-442 change the cell text from "Force caller stdin forwarding or closure. Default: forward stdin" to "Force caller stdin forwarding or closure (default: forward stdin)." and in the same file at 618-618 change the cell text from "Force caller stdin forwarding or closure. Default: forward stdin" to "Force caller stdin forwarding or closure (default: forward stdin)."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 `@docs/reference/commands-nemohermes.mdx`:
- Line 442: Update the two table entries to use a single consistent sentence
with a parenthetical default: in docs/reference/commands-nemohermes.mdx at
442-442 change the cell text from "Force caller stdin forwarding or closure.
Default: forward stdin" to "Force caller stdin forwarding or closure (default:
forward stdin)." and in the same file at 618-618 change the cell text from
"Force caller stdin forwarding or closure. Default: forward stdin" to "Force
caller stdin forwarding or closure (default: forward stdin)."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7296f819-aa3b-4213-93bf-423c9f8fec64
📒 Files selected for processing (1)
docs/reference/commands-nemohermes.mdx
Signed-off-by: HwangJohn <angelic805@gmail.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com> # Conflicts: # src/lib/actions/sandbox/exec.test.ts # src/lib/actions/sandbox/exec.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
E2E Target Results — ✅ All requested jobs passedRun: 28813941726
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
E2E Target Results — ✅ All requested jobs passedRun: 28817029378
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com> # Conflicts: # src/lib/actions/sandbox/exec.multiline-guard.test.ts # src/lib/actions/sandbox/exec.ts
E2E Target Results — ✅ All requested jobs passedRun: 28829674160
|
<!-- markdownlint-disable MD041 --> ## Summary Prepares the user-facing documentation for NemoClaw v0.0.76 and closes the release-prep documentation gate. It adds the release highlights, documents the arm64 Local NIM warning and expanded image cleanup behavior, and fixes agent-specific command headings in generated guides. ## Changes - Add the v0.0.76 release-notes section and move the shared-gateway route containment entry out of the v0.0.74 history where it was incorrectly placed. - Document the advisory Linux arm64 Local NIM manifest warning in the canonical platform matrix and local-inference guidance. - Document that `gc` scans both gateway-built and locally prebuilt sandbox image repositories. - Keep OpenClaw and Hermes session headings out of the generated Deep Agents command guide. - Add a focused variant regression test for the agent-specific session headings. ### Source summary | Merged sources | Documentation coverage | | --- | --- | | [#6414](#6414), [#6418](#6418), [#6416](#6416), [#6344](#6344) | v0.0.76 release notes and the Deep Agents quickstart/inference routes | | [#6340](#6340) | v0.0.76 release notes and existing Deep Agents observability guidance | | [#6338](#6338), [#6378](#6378), [#6297](#6297) | v0.0.76 release notes and existing inference/troubleshooting guidance | | [#6362](#6362) | v0.0.76 release notes and existing lifecycle, command, and credential guidance | | [#6330](#6330), [#6307](#6307), [#6008](#6008) | v0.0.76 release notes and existing security, troubleshooting, and command guidance | | [#6382](#6382) | v0.0.76 release notes and existing MCP/command guidance | | [#6326](#6326), [#5868](#5868), [#5539](#5539) | v0.0.76 release notes, platform matrix, inference options, and local-inference guidance | | [#6396](#6396), [#6390](#6390), [#6007](#6007) | v0.0.76 release notes and existing messaging guidance | | [#5388](#5388), [#6249](#6249), [#6303](#6303), [#6306](#6306) | v0.0.76 release notes and command/lifecycle guidance | ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] 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: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] 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: - [ ] 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 — `npx vitest run --project integration test/generate-platform-docs.test.ts test/agent-variant-docs.test.ts test/sync-agent-variant-docs.test.ts` (3 files, 29 tests passed) - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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) — completed with 0 errors and 2 pre-existing Fern warnings - [x] 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: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added v0.0.76 release notes content, and removed an older conflicting bullet from the surrounding release history. * Expanded Local NVIDIA NIM guidance across inference/provider docs, including an advisory for Linux arm64 DGX Spark/DGX Station hosts when a matching `linux/arm64` image manifest is unavailable. * Updated the command reference for correct session-section rendering and clarified `gc` image cleanup sources. * **Tests** * Added coverage ensuring Deep Agents omits sessions headings while Hermes includes them. * **CI** * Refreshed Local NVIDIA NIM provider notes used in the platform matrix. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Fixes sandbox exec hangs caused by forwarding a live non-terminal stdin that never reaches EOF. NemoClaw now inherits stdin by default only for terminals, closes non-terminal or unavailable stdin, and keeps intentional pipes available through `--stdin`. ## Related Issue Fixes NVIDIA#6319 ## Changes - Merge current `main` without rewriting the contributor's commits or credit. - Add public `--stdin` / `--no-stdin` controls, with explicit flags taking precedence over TTY detection. - Thread `stdin?: boolean` through sandbox exec and select inherited versus ignored stdin in the async production spawner. - Keep the stdio policy and its focused tests in companion modules so the existing exec action/test files stay within growth limits. - Preserve signal forwarding, remote exit codes, cleanup, workdir and multiline guards, and denial-adjacent policy hints. - Update NemoClaw and NemoHermes command references so piped scripts opt in with `--stdin`. - Add parser, default-selection, production-stdio, multiline-guidance, signal, cleanup, exit-code, and policy-hint coverage. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] 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: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] 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: fresh maintainer review requested on the new semantics; the prior approval predates this head and does not satisfy this gate. - [ ] 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 — GitHub reports all 10 PR commits verified at `d73686641e828025789dedf581099b9f23e4b988`. - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable — `npm run check:diff` passed at `d73686641e828025789dedf581099b9f23e4b988`. - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — the sandbox-exec command/action/policy/cleanup/stdio plus runtime-environment matrix passed 11 files / 160 tests; `npm run build:cli`, CLI typecheck, and `npm run docs:strict` also passed. - [x] Applicable broad gate passed — exact-head required CI run `28829324545` passed (including the successful same-head retry of one unrelated timing flake), and advisor-selected E2E run `28829674160` passed 3/3 jobs. - [ ] Quality Gates section completed with required justifications or waivers — fresh sensitive-path review is pending. - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — `npm run docs:strict` found 0 errors and two pre-existing hidden Fern warnings. - [x] 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) The exact-head isolated OpenShell 0.0.72 proof `2026-07-06T23-01-11-471Z-pid-7719` passed: a held-open non-TTY FIFO returned promptly by default, `/dev/null` reached EOF, `--stdin` forwarded a finite two-line script, remote exit status `37` propagated, and a parent-only SIGTERM produced NemoClaw exit `143`. OpenShell 0.0.72 has no remote exec cancellation operation, so the harness recorded its pre-existing detached-command behavior separately and removed the uniquely tagged process after identity verification. Both exact-head PR advisors (`28829423941`) returned `merge_as_is` with no blocker or warning. The E2E advisor (`28829422926`) selected `sessions-agents-cli`, `sandbox-operations`, and `shields-config`; all three passed. Fresh human approval remains required because the existing approval predates this head and its stdin semantics. --- Signed-off-by: HwangJohn <angelic805@gmail.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> --------- Signed-off-by: HwangJohn <angelic805@gmail.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Prepares the user-facing documentation for NemoClaw v0.0.76 and closes the release-prep documentation gate. It adds the release highlights, documents the arm64 Local NIM warning and expanded image cleanup behavior, and fixes agent-specific command headings in generated guides. ## Changes - Add the v0.0.76 release-notes section and move the shared-gateway route containment entry out of the v0.0.74 history where it was incorrectly placed. - Document the advisory Linux arm64 Local NIM manifest warning in the canonical platform matrix and local-inference guidance. - Document that `gc` scans both gateway-built and locally prebuilt sandbox image repositories. - Keep OpenClaw and Hermes session headings out of the generated Deep Agents command guide. - Add a focused variant regression test for the agent-specific session headings. ### Source summary | Merged sources | Documentation coverage | | --- | --- | | [NVIDIA#6414](NVIDIA#6414), [NVIDIA#6418](NVIDIA#6418), [NVIDIA#6416](NVIDIA#6416), [NVIDIA#6344](NVIDIA#6344) | v0.0.76 release notes and the Deep Agents quickstart/inference routes | | [NVIDIA#6340](NVIDIA#6340) | v0.0.76 release notes and existing Deep Agents observability guidance | | [NVIDIA#6338](NVIDIA#6338), [NVIDIA#6378](NVIDIA#6378), [NVIDIA#6297](NVIDIA#6297) | v0.0.76 release notes and existing inference/troubleshooting guidance | | [NVIDIA#6362](NVIDIA#6362) | v0.0.76 release notes and existing lifecycle, command, and credential guidance | | [NVIDIA#6330](NVIDIA#6330), [NVIDIA#6307](NVIDIA#6307), [NVIDIA#6008](NVIDIA#6008) | v0.0.76 release notes and existing security, troubleshooting, and command guidance | | [NVIDIA#6382](NVIDIA#6382) | v0.0.76 release notes and existing MCP/command guidance | | [NVIDIA#6326](NVIDIA#6326), [NVIDIA#5868](NVIDIA#5868), [NVIDIA#5539](NVIDIA#5539) | v0.0.76 release notes, platform matrix, inference options, and local-inference guidance | | [NVIDIA#6396](NVIDIA#6396), [NVIDIA#6390](NVIDIA#6390), [NVIDIA#6007](NVIDIA#6007) | v0.0.76 release notes and existing messaging guidance | | [NVIDIA#5388](NVIDIA#5388), [NVIDIA#6249](NVIDIA#6249), [NVIDIA#6303](NVIDIA#6303), [NVIDIA#6306](NVIDIA#6306) | v0.0.76 release notes and command/lifecycle guidance | ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] 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: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] 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: - [ ] 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 — `npx vitest run --project integration test/generate-platform-docs.test.ts test/agent-variant-docs.test.ts test/sync-agent-variant-docs.test.ts` (3 files, 29 tests passed) - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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) — completed with 0 errors and 2 pre-existing Fern warnings - [x] 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: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added v0.0.76 release notes content, and removed an older conflicting bullet from the surrounding release history. * Expanded Local NVIDIA NIM guidance across inference/provider docs, including an advisory for Linux arm64 DGX Spark/DGX Station hosts when a matching `linux/arm64` image manifest is unavailable. * Updated the command reference for correct session-section rendering and clarified `gc` image cleanup sources. * **Tests** * Added coverage ensuring Deep Agents omits sessions headings while Hermes includes them. * **CI** * Refreshed Local NVIDIA NIM provider notes used in the platform matrix. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Summary
Fixes sandbox exec hangs caused by forwarding a live non-terminal stdin that never reaches EOF. NemoClaw now inherits stdin by default only for terminals, closes non-terminal or unavailable stdin, and keeps intentional pipes available through
--stdin.Related Issue
Fixes #6319
Changes
mainwithout rewriting the contributor's commits or credit.--stdin/--no-stdincontrols, with explicit flags taking precedence over TTY detection.stdin?: booleanthrough sandbox exec and select inherited versus ignored stdin in the async production spawner.--stdin.Type of Change
Quality Gates
Verification
Verifiedin GitHub — GitHub reports all 10 PR commits verified atd73686641e828025789dedf581099b9f23e4b988.pre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailable —npm run check:diffpassed atd73686641e828025789dedf581099b9f23e4b988.npm run build:cli, CLI typecheck, andnpm run docs:strictalso passed.28829324545passed (including the successful same-head retry of one unrelated timing flake), and advisor-selected E2E run28829674160passed 3/3 jobs.npm run docsbuilds without warnings (doc changes only) —npm run docs:strictfound 0 errors and two pre-existing hidden Fern warnings.The exact-head isolated OpenShell 0.0.72 proof
2026-07-06T23-01-11-471Z-pid-7719passed: a held-open non-TTY FIFO returned promptly by default,/dev/nullreached EOF,--stdinforwarded a finite two-line script, remote exit status37propagated, and a parent-only SIGTERM produced NemoClaw exit143. OpenShell 0.0.72 has no remote exec cancellation operation, so the harness recorded its pre-existing detached-command behavior separately and removed the uniquely tagged process after identity verification.Both exact-head PR advisors (
28829423941) returnedmerge_as_iswith no blocker or warning. The E2E advisor (28829422926) selectedsessions-agents-cli,sandbox-operations, andshields-config; all three passed. Fresh human approval remains required because the existing approval predates this head and its stdin semantics.Signed-off-by: HwangJohn angelic805@gmail.com
Signed-off-by: Aaron Erickson aerickson@nvidia.com