fix(cli): close non-terminal exec stdin by default - #6394
Closed
chengjiew wants to merge 1 commit into
Closed
Conversation
Fixes #6319 Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds stdin inheritance control to sandbox exec: a new module decides whether stdin should be inherited based on TTY detection or explicit request, exec.ts wires this into spawning, a new ChangesStdin forwarding feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant execSandbox
participant runSandboxExecChild
participant defaultSandboxExecSpawner
participant buildSandboxExecStdio
CLI->>execSandbox: run(options with stdin flag)
execSandbox->>runSandboxExecChild: spawnChild(binary, args, options)
runSandboxExecChild->>defaultSandboxExecSpawner: options
defaultSandboxExecSpawner->>buildSandboxExecStdio: options, stdinIsTty
buildSandboxExecStdio-->>defaultSandboxExecSpawner: "inherit" or ["ignore","inherit","inherit"]
defaultSandboxExecSpawner-->>execSandbox: spawned child with resolved stdio
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Code Coverage is in Public Preview. Learn more and provide us with your feedback. |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-6394.docs.buildwithfern.com/nemoclaw |
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 0 in-scope improvements
|
Contributor
PR Review Advisor (Nemotron Ultra) — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Findings index
Review findings by urgency: 0 required fixes, 0 items to resolve/justify, 4 in-scope improvements
|
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/reference/commands-nemohermes.mdx`:
- Around line 576-596: The stdin flag description is imprecise: `--no-stdin`
does not just “close terminal stdin explicitly,” it always forces stdin off via
`ExecOptions`/`stdin: false` and maps to the ignore/inherit/inherit behavior
regardless of terminal state. Update the wording in the `commands-nemohermes`
docs near the `--stdin` / `--no-stdin` entry and the introductory paragraph to
say `--stdin` forwards an intentional pipe, while `--no-stdin` unconditionally
closes stdin. Keep the existing default behavior description for inherited
terminal stdin and non-terminal stdin closure.
In `@docs/reference/commands.mdx`:
- Around line 706-726: Update the stdin flag description in commands.mdx to
match shouldInheritSandboxExecStdin in src/lib/actions/sandbox/exec-stdio.ts:
explicit --no-stdin always closes stdin, not just terminal stdin. Adjust the
wording in the exec docs table and the surrounding prose so it says default
behavior only inherits terminal stdin, while --no-stdin forces stdin closure
regardless of TTY state; this source should then regenerate the Hermes variant
correctly.
🪄 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: 5cfb61fb-8a3a-4655-aa72-153f0efec0b2
📒 Files selected for processing (10)
docs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxsrc/commands/sandbox/exec.test.tssrc/commands/sandbox/exec.tssrc/lib/actions/sandbox/exec-openclaw-permission-cleanup.test.tssrc/lib/actions/sandbox/exec-policy-hint-emission.tssrc/lib/actions/sandbox/exec-stdio.test.tssrc/lib/actions/sandbox/exec-stdio.tssrc/lib/actions/sandbox/exec.multiline-guard.test.tssrc/lib/actions/sandbox/exec.ts
Comment on lines
+576
to
+596
| By default, NemoClaw inherits caller stdin only when it is a terminal. | ||
| Non-terminal or unavailable stdin is closed so SSH, CI, and other one-shot commands cannot wait on an inherited pipe. | ||
| Pass `--stdin` to forward an intentional pipe, or `--no-stdin` to close terminal stdin explicitly. | ||
|
|
||
| ```bash | ||
| printf 'hello\n' | nemohermes my-assistant exec --stdin -- cat | ||
| ssh dgx-spark 'nemohermes my-assistant exec --no-stdin -- pwd' | ||
| ``` | ||
|
|
||
| The OpenShell exec endpoint rejects any command argument (the values after `--`) that contains a newline or carriage return, so multi-line commands such as a `bash` heredoc cannot be passed through `exec`. | ||
| NemoClaw detects this before dispatch, names the offending argument position, and exits with status `2` instead of surfacing the lower-level OpenShell `InvalidArgument` error. | ||
| Join the statements with semicolons (`nemohermes <name> exec -- bash -lc "cmd1; cmd2"`). | ||
| Pipe the script into the sandbox shell over stdin (`printf 'cmd1\ncmd2\n' | nemohermes <name> exec -- bash`). | ||
| Pipe the script into the sandbox shell over stdin (`printf 'cmd1\ncmd2\n' | nemohermes <name> exec --stdin -- bash`). | ||
| Or write the script to a file in the sandbox and run it (`nemohermes <name> exec -- bash <script-path>`). | ||
|
|
||
| | Flag | Description | | ||
| |------|-------------| | ||
| | `--workdir <dir>` | Working directory inside the sandbox. The directory is checked before the command runs; if it does not exist, NemoClaw reports `error: --workdir: <dir> does not exist inside the sandbox` and exits with status `1` without invoking the inner command. | | ||
| | `--tty` / `--no-tty` | Allocate a pseudo-terminal; defaults to auto-detection (on when stdin and stdout are terminals) | | ||
| | `--timeout <seconds>` | Timeout in seconds (`0` means no timeout) | | ||
| | `--stdin` / `--no-stdin` | Force caller stdin forwarding or closure (default: inherit terminal stdin; close non-terminal or unavailable stdin). | |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Imprecise wording: "close terminal stdin explicitly."
--no-stdin forces stdin: false and always maps to ["ignore", "inherit", "inherit"] regardless of whether stdin is a terminal — it closes stdin unconditionally, not just terminal stdin. The current phrasing could mislead users into thinking --no-stdin is a no-op for piped/non-terminal stdin.
📝 Proposed wording fix
-Pass `--stdin` to forward an intentional pipe, or `--no-stdin` to close terminal stdin explicitly.
+Pass `--stdin` to forward an intentional pipe, or `--no-stdin` to close stdin explicitly.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| By default, NemoClaw inherits caller stdin only when it is a terminal. | |
| Non-terminal or unavailable stdin is closed so SSH, CI, and other one-shot commands cannot wait on an inherited pipe. | |
| Pass `--stdin` to forward an intentional pipe, or `--no-stdin` to close terminal stdin explicitly. | |
| ```bash | |
| printf 'hello\n' | nemohermes my-assistant exec --stdin -- cat | |
| ssh dgx-spark 'nemohermes my-assistant exec --no-stdin -- pwd' | |
| ``` | |
| The OpenShell exec endpoint rejects any command argument (the values after `--`) that contains a newline or carriage return, so multi-line commands such as a `bash` heredoc cannot be passed through `exec`. | |
| NemoClaw detects this before dispatch, names the offending argument position, and exits with status `2` instead of surfacing the lower-level OpenShell `InvalidArgument` error. | |
| Join the statements with semicolons (`nemohermes <name> exec -- bash -lc "cmd1; cmd2"`). | |
| Pipe the script into the sandbox shell over stdin (`printf 'cmd1\ncmd2\n' | nemohermes <name> exec -- bash`). | |
| Pipe the script into the sandbox shell over stdin (`printf 'cmd1\ncmd2\n' | nemohermes <name> exec --stdin -- bash`). | |
| Or write the script to a file in the sandbox and run it (`nemohermes <name> exec -- bash <script-path>`). | |
| | Flag | Description | | |
| |------|-------------| | |
| | `--workdir <dir>` | Working directory inside the sandbox. The directory is checked before the command runs; if it does not exist, NemoClaw reports `error: --workdir: <dir> does not exist inside the sandbox` and exits with status `1` without invoking the inner command. | | |
| | `--tty` / `--no-tty` | Allocate a pseudo-terminal; defaults to auto-detection (on when stdin and stdout are terminals) | | |
| | `--timeout <seconds>` | Timeout in seconds (`0` means no timeout) | | |
| | `--stdin` / `--no-stdin` | Force caller stdin forwarding or closure (default: inherit terminal stdin; close non-terminal or unavailable stdin). | | |
| By default, NemoClaw inherits caller stdin only when it is a terminal. | |
| Non-terminal or unavailable stdin is closed so SSH, CI, and other one-shot commands cannot wait on an inherited pipe. | |
| Pass `--stdin` to forward an intentional pipe, or `--no-stdin` to close stdin explicitly. | |
🤖 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 576 - 596, The stdin
flag description is imprecise: `--no-stdin` does not just “close terminal stdin
explicitly,” it always forces stdin off via `ExecOptions`/`stdin: false` and
maps to the ignore/inherit/inherit behavior regardless of terminal state. Update
the wording in the `commands-nemohermes` docs near the `--stdin` / `--no-stdin`
entry and the introductory paragraph to say `--stdin` forwards an intentional
pipe, while `--no-stdin` unconditionally closes stdin. Keep the existing default
behavior description for inherited terminal stdin and non-terminal stdin
closure.
Comment on lines
+706
to
+726
| By default, NemoClaw inherits caller stdin only when it is a terminal. | ||
| Non-terminal or unavailable stdin is closed so SSH, CI, and other one-shot commands cannot wait on an inherited pipe. | ||
| Pass `--stdin` to forward an intentional pipe, or `--no-stdin` to close terminal stdin explicitly. | ||
|
|
||
| ```bash | ||
| printf 'hello\n' | $$nemoclaw my-assistant exec --stdin -- cat | ||
| ssh dgx-spark '$$nemoclaw my-assistant exec --no-stdin -- pwd' | ||
| ``` | ||
|
|
||
| The OpenShell exec endpoint rejects any command argument (the values after `--`) that contains a newline or carriage return, so multi-line commands such as a `bash` heredoc cannot be passed through `exec`. | ||
| NemoClaw detects this before dispatch, names the offending argument position, and exits with status `2` instead of surfacing the lower-level OpenShell `InvalidArgument` error. | ||
| Join the statements with semicolons (`$$nemoclaw <name> exec -- bash -lc "cmd1; cmd2"`). | ||
| Pipe the script into the sandbox shell over stdin (`printf 'cmd1\ncmd2\n' | $$nemoclaw <name> exec -- bash`). | ||
| Pipe the script into the sandbox shell over stdin (`printf 'cmd1\ncmd2\n' | $$nemoclaw <name> exec --stdin -- bash`). | ||
| Or write the script to a file in the sandbox and run it (`$$nemoclaw <name> exec -- bash <script-path>`). | ||
|
|
||
| | Flag | Description | | ||
| |------|-------------| | ||
| | `--workdir <dir>` | Working directory inside the sandbox. The directory is checked before the command runs; if it does not exist, NemoClaw reports `error: --workdir: <dir> does not exist inside the sandbox` and exits with status `1` without invoking the inner command. | | ||
| | `--tty` / `--no-tty` | Allocate a pseudo-terminal; defaults to auto-detection (on when stdin and stdout are terminals) | | ||
| | `--timeout <seconds>` | Timeout in seconds (`0` means no timeout) | | ||
| | `--stdin` / `--no-stdin` | Force caller stdin forwarding or closure (default: inherit terminal stdin; close non-terminal or unavailable stdin). | |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Same imprecise wording: "close terminal stdin explicitly."
Per shouldInheritSandboxExecStdin in src/lib/actions/sandbox/exec-stdio.ts, an explicit --no-stdin bypasses TTY detection and always closes stdin, not just terminal stdin. Since this page is the source that commands-nemohermes.mdx is regenerated from, fixing the wording here should also propagate to the Hermes variant.
📝 Proposed wording fix
-Pass `--stdin` to forward an intentional pipe, or `--no-stdin` to close terminal stdin explicitly.
+Pass `--stdin` to forward an intentional pipe, or `--no-stdin` to close stdin explicitly.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| By default, NemoClaw inherits caller stdin only when it is a terminal. | |
| Non-terminal or unavailable stdin is closed so SSH, CI, and other one-shot commands cannot wait on an inherited pipe. | |
| Pass `--stdin` to forward an intentional pipe, or `--no-stdin` to close terminal stdin explicitly. | |
| ```bash | |
| printf 'hello\n' | $$nemoclaw my-assistant exec --stdin -- cat | |
| ssh dgx-spark '$$nemoclaw my-assistant exec --no-stdin -- pwd' | |
| ``` | |
| The OpenShell exec endpoint rejects any command argument (the values after `--`) that contains a newline or carriage return, so multi-line commands such as a `bash` heredoc cannot be passed through `exec`. | |
| NemoClaw detects this before dispatch, names the offending argument position, and exits with status `2` instead of surfacing the lower-level OpenShell `InvalidArgument` error. | |
| Join the statements with semicolons (`$$nemoclaw <name> exec -- bash -lc "cmd1; cmd2"`). | |
| Pipe the script into the sandbox shell over stdin (`printf 'cmd1\ncmd2\n' | $$nemoclaw <name> exec -- bash`). | |
| Pipe the script into the sandbox shell over stdin (`printf 'cmd1\ncmd2\n' | $$nemoclaw <name> exec --stdin -- bash`). | |
| Or write the script to a file in the sandbox and run it (`$$nemoclaw <name> exec -- bash <script-path>`). | |
| | Flag | Description | | |
| |------|-------------| | |
| | `--workdir <dir>` | Working directory inside the sandbox. The directory is checked before the command runs; if it does not exist, NemoClaw reports `error: --workdir: <dir> does not exist inside the sandbox` and exits with status `1` without invoking the inner command. | | |
| | `--tty` / `--no-tty` | Allocate a pseudo-terminal; defaults to auto-detection (on when stdin and stdout are terminals) | | |
| | `--timeout <seconds>` | Timeout in seconds (`0` means no timeout) | | |
| | `--stdin` / `--no-stdin` | Force caller stdin forwarding or closure (default: inherit terminal stdin; close non-terminal or unavailable stdin). | | |
| By default, NemoClaw inherits caller stdin only when it is a terminal. | |
| Non-terminal or unavailable stdin is closed so SSH, CI, and other one-shot commands cannot wait on an inherited pipe. | |
| Pass `--stdin` to forward an intentional pipe, or `--no-stdin` to close stdin explicitly. | |
🤖 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.mdx` around lines 706 - 726, Update the stdin flag
description in commands.mdx to match shouldInheritSandboxExecStdin in
src/lib/actions/sandbox/exec-stdio.ts: explicit --no-stdin always closes stdin,
not just terminal stdin. Adjust the wording in the exec docs table and the
surrounding prose so it says default behavior only inherits terminal stdin,
while --no-stdin forces stdin closure regardless of TTY state; this source
should then regenerate the Hermes variant correctly.
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
1 participant
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
nemoclaw <sandbox> exec, preventing live pipes/SSH stdin from hanging one-shot commands.--stdin/--no-stdincontrols so intentional piped scripts still work.Related Issue
Fixes #6319
Verification
npx vitest run --project cli src/commands/sandbox/exec.test.ts src/lib/actions/sandbox/exec-stdio.test.ts src/lib/actions/sandbox/exec.multiline-guard.test.ts src/lib/actions/sandbox/exec-openclaw-permission-cleanup.test.ts— 4 files / 47 tests passed.npm run build:cli— passed.npm run typecheck:cli— passed.npm run docs:check-agent-variants— passed.git diff --cached --check— passed before commit.c8f0cf7503191de39358c98a081f6a856127d1b5.Linux Reproduction
Validated on
aits@aits-log-worker-6with Ready sandboxrepro-6182-live.Baseline
nemoclaw v0.0.74-13-gf0d25491c:Fixed PR head
nemoclaw v0.0.74-37-gd73686641:Signed-off-by: Chengjie Wang chengjiew@nvidia.com
Summary by CodeRabbit
New Features
--stdinand--no-stdinoptions to sandbox exec commands.Documentation