fix(cli): forward exec help flags after separator - #4192
Conversation
wscurran
added
fix
refactor
|
✨ Thanks for submitting this detailed PR about forwarding exec help flags after the separator and refactoring computeExitCode. This proposes a fix for the CLI and improves exit-code semantics. |
wscurran
added
area: cli
…commands Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
… as an exec flag Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…ntly Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…tics Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
1a0b515 to
0b048e9
Compare
📝 WalkthroughWalkthroughThis PR refactors the sandbox exec command into a new oclif command module with simplified exit-code computation, removing workdir validation logic. It adds tests for CLI dispatch resolution and the new exec command, updates command-registry contract test expectations, and introduces new nemoclaw/openshell CLI reference documentation. ChangesSandbox Exec Refactor and Dispatch Tests
Estimated code review effort: 3 (Moderate) | ~30 minutes CLI Reference Documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Rebased onto current Branch — sent via hermes-pr-bot. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.agents/skills/nemoclaw-user-reference/references/commands.md:
- Around line 287-310: The commands reference is missing the new one-off
`nemoclaw <name> exec` section between `connect` and `recover`, so update this
document to include that command in the same style as the surrounding entries.
Use the existing `connect` and `recover` headings as anchors, and add the `exec`
usage, purpose, and any key flags/behavior so the skill copy matches the public
docs.
In `@src/lib/actions/sandbox/exec.ts`:
- Around line 50-55: The exit handling in exitWithSpawnResult() is printing the
PATH/install hint for every spawn failure, but that hint should only appear for
ENOENT. Update the error branch to inspect the underlying spawn error code from
computeExitCode(result) (or the SpawnLikeResult data it derives from) and only
emit the “Ensure 'openshell' is installed and on PATH.” message when the failure
is ENOENT; keep other failures like EACCES/ENOEXEC limited to the actual error
message. Reference exitWithSpawnResult() and computeExitCode() to locate the
change.
In `@src/lib/commands/sandbox/exec.test.ts`:
- Around line 20-30: The current SandboxExecCommand.run test only covers
ordinary argv forwarding and does not protect the fixed help-flag behavior after
the separator. Add a regression test in exec.test.ts using
SandboxExecCommand.run that passes a command like "-- --help" (and/or "-h")
after the separator, then assert execSandboxMock receives those flags unchanged
in the inner argv. Keep the check focused on forwarding behavior, not parser
internals, and reference SandboxExecCommand.run and execSandboxMock when adding
the new case.
In `@test/package-contract/cli/command-registry.test.ts`:
- Around line 67-70: The aggregate visible-command count in command-registry
tests is too broad to catch a broken `withCommandDisplay()` registration for the
new `nemoclaw <name> exec` help entry. Add one explicit assertion in
`visibleCommands()`-based coverage that this command appears as a visible
command, using the relevant registry/setup symbols in
`command-registry.test.ts`, so the test directly verifies the new public
entrypoint instead of only relying on totals.
🪄 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: 64b75076-b863-43e9-85e4-ac6b74617edc
📒 Files selected for processing (11)
.agents/skills/nemoclaw-user-reference/references/cli-selection-guide.md.agents/skills/nemoclaw-user-reference/references/commands.mddocs/reference/cli-selection-guide.mddocs/reference/commands.mdsrc/commands/sandbox/exec.tssrc/lib/actions/sandbox/exec.test.tssrc/lib/actions/sandbox/exec.tssrc/lib/cli/oclif-dispatch.test.tssrc/lib/commands/sandbox/exec.test.tssrc/lib/commands/sandbox/exec.tstest/package-contract/cli/command-registry.test.ts
| ### `nemoclaw <name> connect` | ||
|
|
||
| Connect to a sandbox by name. | ||
| If the sandbox is not yet in the `Ready` phase, `connect` polls `openshell sandbox list` every few seconds and prints the current phase. This gives you progress output right after onboarding, when the 2.4 GB image is still pulling, instead of a silent hang. | ||
| Control the wait budget with `NEMOCLAW_CONNECT_TIMEOUT` (integer seconds, default `120`). When the deadline expires, `connect` exits non-zero with the last-seen phase. | ||
|
|
||
| On a TTY, a one-shot hint prints before dropping into the sandbox shell. | ||
| The hint is agent-aware. It names the correct TUI command for the sandbox's agent and reminds you to use `/exit` to leave the chat before `exit` returns you to the host shell. | ||
| Set `NEMOCLAW_NO_CONNECT_HINT=1` to suppress the hint in scripted workflows. | ||
| If the sandbox is running an outdated agent version, a non-blocking warning prints before connecting with a `nemoclaw <name> rebuild` hint. | ||
| If another terminal is already connected to the sandbox, `connect` prints a note with the number of existing sessions before proceeding. Multiple concurrent sessions are allowed. | ||
|
|
||
| After a host reboot, the OpenShell gateway rotates its SSH host keys. | ||
| `connect` detects the resulting identity drift, prunes stale `openshell-*` entries from `~/.ssh/known_hosts`, and retries automatically. | ||
| You no longer need to re-run `nemoclaw onboard` after a reboot in this case. | ||
|
|
||
| ```console | ||
| $ nemoclaw my-assistant connect [--probe-only] | ||
| ``` | ||
|
|
||
| The `--probe-only` flag verifies the sandbox is reachable over SSH and exits without opening a shell. | ||
| Use it for health checks and scripted readiness probes. | ||
|
|
||
| ### `nemoclaw <name> recover` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the new nemoclaw <name> exec section here.
This skill copy skips the new one-off command that the public docs now document, so readers miss the supported path between connect and recover.
🤖 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 @.agents/skills/nemoclaw-user-reference/references/commands.md around lines
287 - 310, The commands reference is missing the new one-off `nemoclaw <name>
exec` section between `connect` and `recover`, so update this document to
include that command in the same style as the surrounding entries. Use the
existing `connect` and `recover` headings as anchors, and add the `exec` usage,
purpose, and any key flags/behavior so the skill copy matches the public docs.
| function exitWithSpawnResult(result: SpawnLikeResult): never { | ||
| const { code, errorMessage } = computeExitCode(result); | ||
| if (errorMessage) { | ||
| console.error(` Failed to invoke openshell: ${errorMessage}`); | ||
| console.error(" Ensure 'openshell' is installed and on PATH."); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Only print the PATH hint for ENOENT.
getOpenshellBinary() already exits when openshell cannot be resolved, so this branch mostly represents execution failures like EACCES/ENOEXEC. Always telling users to reinstall or fix PATH points them at the wrong root cause.
Suggested change
function exitWithSpawnResult(result: SpawnLikeResult): never {
const { code, errorMessage } = computeExitCode(result);
if (errorMessage) {
console.error(` Failed to invoke openshell: ${errorMessage}`);
- console.error(" Ensure 'openshell' is installed and on PATH.");
+ if ((result.error as NodeJS.ErrnoException | undefined)?.code === "ENOENT") {
+ console.error(" Ensure 'openshell' is installed and on PATH.");
+ }
}
process.exit(code);
}Based on the related getOpenshellBinary() context, missing binaries already terminate before this branch runs.
📝 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.
| function exitWithSpawnResult(result: SpawnLikeResult): never { | |
| const { code, errorMessage } = computeExitCode(result); | |
| if (errorMessage) { | |
| console.error(` Failed to invoke openshell: ${errorMessage}`); | |
| console.error(" Ensure 'openshell' is installed and on PATH."); | |
| } | |
| function exitWithSpawnResult(result: SpawnLikeResult): never { | |
| const { code, errorMessage } = computeExitCode(result); | |
| if (errorMessage) { | |
| console.error(` Failed to invoke openshell: ${errorMessage}`); | |
| if ((result.error as NodeJS.ErrnoException | undefined)?.code === "ENOENT") { | |
| console.error(" Ensure 'openshell' is installed and on PATH."); | |
| } | |
| } | |
| process.exit(code); | |
| } |
🤖 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 `@src/lib/actions/sandbox/exec.ts` around lines 50 - 55, The exit handling in
exitWithSpawnResult() is printing the PATH/install hint for every spawn failure,
but that hint should only appear for ENOENT. Update the error branch to inspect
the underlying spawn error code from computeExitCode(result) (or the
SpawnLikeResult data it derives from) and only emit the “Ensure 'openshell' is
installed and on PATH.” message when the failure is ENOENT; keep other failures
like EACCES/ENOEXEC limited to the actual error message. Reference
exitWithSpawnResult() and computeExitCode() to locate the change.
| it("forwards everything after -- as the inner command argv", async () => { | ||
| await SandboxExecCommand.run( | ||
| ["alpha", "--", "openclaw", "agent", "--agent", "main", "-m", "hi"], | ||
| rootDir, | ||
| ); | ||
| expect(execSandboxMock).toHaveBeenCalledWith( | ||
| "alpha", | ||
| ["openclaw", "agent", "--agent", "main", "-m", "hi"], | ||
| { workdir: undefined, tty: null, timeoutSeconds: undefined }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a -- --help regression case.
This assertion uses --agent, which never exercises the special-case that broke here. A parser can still swallow --help or -h after the separator while this test stays green, so the PR's stated fix is not locked in.
Suggested test
it("forwards everything after -- as the inner command argv", async () => {
await SandboxExecCommand.run(
["alpha", "--", "openclaw", "agent", "--agent", "main", "-m", "hi"],
rootDir,
);
expect(execSandboxMock).toHaveBeenCalledWith(
"alpha",
["openclaw", "agent", "--agent", "main", "-m", "hi"],
{ workdir: undefined, tty: null, timeoutSeconds: undefined },
);
});
+
+ it("forwards help flags after -- as the inner command argv", async () => {
+ await SandboxExecCommand.run(["alpha", "--", "--help"], rootDir);
+ expect(execSandboxMock).toHaveBeenCalledWith("alpha", ["--help"], {
+ workdir: undefined,
+ tty: null,
+ timeoutSeconds: undefined,
+ });
+ });As per path instructions, "Review tests for behavioral confidence rather than implementation lock-in," and the PR objective here is specifically forwarding exec help flags after the separator.
📝 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.
| it("forwards everything after -- as the inner command argv", async () => { | |
| await SandboxExecCommand.run( | |
| ["alpha", "--", "openclaw", "agent", "--agent", "main", "-m", "hi"], | |
| rootDir, | |
| ); | |
| expect(execSandboxMock).toHaveBeenCalledWith( | |
| "alpha", | |
| ["openclaw", "agent", "--agent", "main", "-m", "hi"], | |
| { workdir: undefined, tty: null, timeoutSeconds: undefined }, | |
| ); | |
| }); | |
| it("forwards everything after -- as the inner command argv", async () => { | |
| await SandboxExecCommand.run( | |
| ["alpha", "--", "openclaw", "agent", "--agent", "main", "-m", "hi"], | |
| rootDir, | |
| ); | |
| expect(execSandboxMock).toHaveBeenCalledWith( | |
| "alpha", | |
| ["openclaw", "agent", "--agent", "main", "-m", "hi"], | |
| { workdir: undefined, tty: null, timeoutSeconds: undefined }, | |
| ); | |
| }); | |
| it("forwards help flags after -- as the inner command argv", async () => { | |
| await SandboxExecCommand.run(["alpha", "--", "--help"], rootDir); | |
| expect(execSandboxMock).toHaveBeenCalledWith("alpha", ["--help"], { | |
| workdir: undefined, | |
| tty: null, | |
| timeoutSeconds: undefined, | |
| }); | |
| }); |
🤖 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 `@src/lib/commands/sandbox/exec.test.ts` around lines 20 - 30, The current
SandboxExecCommand.run test only covers ordinary argv forwarding and does not
protect the fixed help-flag behavior after the separator. Add a regression test
in exec.test.ts using SandboxExecCommand.run that passes a command like "--
--help" (and/or "-h") after the separator, then assert execSandboxMock receives
those flags unchanged in the inner argv. Keep the check focused on forwarding
behavior, not parser internals, and reference SandboxExecCommand.run and
execSandboxMock when adding the new case.
Source: Path instructions
| it("should exclude 11 hidden commands (48 visible)", () => { | ||
| // 5 hidden global (help, --help, -h, --version, -v) + | ||
| // 6 hidden sandbox (shields×3, config get/set/rotate-token) | ||
| expect(visibleCommands()).toHaveLength(48); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep one direct contract for the new nemoclaw <name> exec help entry.
After removing the discovery-coverage block, this file only checks aggregate counts/tokens. A broken withCommandDisplay() registration for the new exec command could still pass if another command keeps those totals stable, so please add one explicit visible-command assertion for this entry.
Suggested test
describe("visibleCommands()", () => {
it("should exclude 11 hidden commands (48 visible)", () => {
// 5 hidden global (help, --help, -h, --version, -v) +
// 6 hidden sandbox (shields×3, config get/set/rotate-token)
expect(visibleCommands()).toHaveLength(48);
});
+
+ it("includes sandbox exec in visible sandbox help", () => {
+ expect(
+ visibleCommands().find((c) => c.usage === "nemoclaw <name> exec"),
+ ).toMatchObject({
+ group: "Sandbox Management",
+ scope: "sandbox",
+ });
+ });
it("no visible command has hidden=true", () => {
for (const cmd of visibleCommands()) {
expect(cmd.hidden).not.toBe(true);
}As per path instructions, "Tests must prove that public entrypoints reach the new path and that the old path is deleted or cannot execute."
📝 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.
| it("should exclude 11 hidden commands (48 visible)", () => { | |
| // 5 hidden global (help, --help, -h, --version, -v) + | |
| // 6 hidden sandbox (shields×3, config get/set/rotate-token) | |
| expect(visibleCommands()).toHaveLength(48); | |
| it("should exclude 11 hidden commands (48 visible)", () => { | |
| // 5 hidden global (help, --help, -h, --version, -v) + | |
| // 6 hidden sandbox (shields×3, config get/set/rotate-token) | |
| expect(visibleCommands()).toHaveLength(48); | |
| }); | |
| it("includes sandbox exec in visible sandbox help", () => { | |
| expect( | |
| visibleCommands().find((c) => c.usage === "nemoclaw <name> exec"), | |
| ).toMatchObject({ | |
| group: "Sandbox Management", | |
| scope: "sandbox", | |
| }); | |
| }); |
🤖 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 `@test/package-contract/cli/command-registry.test.ts` around lines 67 - 70, The
aggregate visible-command count in command-registry tests is too broad to catch
a broken `withCommandDisplay()` registration for the new `nemoclaw <name> exec`
help entry. Add one explicit assertion in `visibleCommands()`-based coverage
that this command appears as a visible command, using the relevant
registry/setup symbols in `command-registry.test.ts`, so the test directly
verifies the new public entrypoint instead of only relying on totals.
Source: Path instructions
|
Closing as superseded by #3667 and the current main exec path, which already carries the command-after-separator and forwarded help-flag behavior this PR set out to add. The branch is now substantially behind and conflicts with the evolved command registry, so reviving it would require a redesign rather than a narrow rebase. Thank you, @Kartalops, for identifying and working through this CLI behavior; that contribution and context remain preserved here. |
4 participants
Description
Forward exec help flags after separator, refactor computeExitCode and unit-test exit-code semantics.
Changes
fix(cli): forward exec help flags after separatorrefactor(exec): extract computeExitCode and unit-test exit-code semanticsdocs(exec): wrap HOME=/sandbox in inline code per style guideTesting
Unit tests for exit-code semantics added.
Summary by CodeRabbit
New Features
sandbox:execcommand to run a single command inside an active sandbox with support for working directory, timeout, and TTY options.Documentation
Bug Fixes