-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix(cli): close non-terminal exec stdin by default #6394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -703,17 +703,27 @@ The exit code is the remote command's exit code. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </AgentOnly> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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). | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+706
to
+726
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Same imprecise wording: "close terminal stdin explicitly." Per 📝 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ### `$$nemoclaw <name> agent` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1083,14 +1093,19 @@ The command exits with the remote command's exit code. | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| </AgentOnly> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $$nemoclaw my-assistant exec [--workdir <dir>] [--tty|--no-tty] [--timeout <s>] -- <cmd> [args...] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $$nemoclaw my-assistant exec [--workdir <dir>] [--tty|--no-tty] [--timeout <s>] [--stdin|--no-stdin] -- <cmd> [args...] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| | Flag | Description | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| |------|-------------| | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| | `--workdir <dir>` | Set the 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 or disable a pseudo-terminal; defaults to auto-detection | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| | `--timeout <s>` | Timeout in seconds. Use `0` for no timeout | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| | `--stdin`, `--no-stdin` | Force caller stdin forwarding or closure (default: inherit terminal stdin; close non-terminal or unavailable stdin). | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ### `$$nemoclaw <name> logs` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import { describe, expect, it } from "vitest"; | ||
| import { buildSandboxExecStdio, shouldInheritSandboxExecStdin } from "./exec-stdio"; | ||
|
|
||
| describe("buildSandboxExecStdio", () => { | ||
| it("inherits terminal stdin by default", () => { | ||
| expect(buildSandboxExecStdio({}, true)).toBe("inherit"); | ||
| }); | ||
|
|
||
| it("closes non-terminal or unknown stdin by default", () => { | ||
| expect(buildSandboxExecStdio({}, false)).toEqual(["ignore", "inherit", "inherit"]); | ||
| expect(buildSandboxExecStdio({}, undefined)).toEqual(["ignore", "inherit", "inherit"]); | ||
| }); | ||
|
|
||
| it("honors explicit flags over terminal detection", () => { | ||
| expect(buildSandboxExecStdio({ stdin: true }, false)).toBe("inherit"); | ||
| expect(buildSandboxExecStdio({ stdin: true }, undefined)).toBe("inherit"); | ||
| expect(buildSandboxExecStdio({ stdin: false }, true)).toEqual(["ignore", "inherit", "inherit"]); | ||
| }); | ||
| }); | ||
|
|
||
| describe("shouldInheritSandboxExecStdin", () => { | ||
| it("lets explicit --stdin and --no-stdin win", () => { | ||
| expect(shouldInheritSandboxExecStdin(true, false)).toBe(true); | ||
| expect(shouldInheritSandboxExecStdin(false, true)).toBe(false); | ||
| }); | ||
|
|
||
| it("inherits only a positively identified TTY when no flag is present", () => { | ||
| expect(shouldInheritSandboxExecStdin(undefined, true)).toBe(true); | ||
| expect(shouldInheritSandboxExecStdin(undefined, false)).toBe(false); | ||
| expect(shouldInheritSandboxExecStdin(undefined, undefined)).toBe(false); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import type { StdioOptions } from "node:child_process"; | ||
| import { isStdinTty } from "../../core/stdin"; | ||
| import type { SandboxExecOptions } from "./exec"; | ||
|
|
||
| export function shouldInheritSandboxExecStdin( | ||
| requested: boolean | undefined, | ||
| stdinIsTty: boolean | undefined, | ||
| ): boolean { | ||
| if (typeof requested === "boolean") return requested; | ||
| return stdinIsTty === true; | ||
| } | ||
|
|
||
| export function buildSandboxExecStdio( | ||
| options: SandboxExecOptions = {}, | ||
| stdinIsTty: boolean | undefined = isStdinTty(), | ||
| ): StdioOptions { | ||
| return shouldInheritSandboxExecStdin(options.stdin, stdinIsTty) | ||
| ? "inherit" | ||
| : ["ignore", "inherit", "inherit"]; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Imprecise wording: "close terminal stdin explicitly."
--no-stdinforcesstdin: falseand 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-stdinis a no-op for piped/non-terminal stdin.📝 Proposed wording fix
📝 Committable suggestion
🤖 Prompt for AI Agents