fix: stream sandbox logs via tail (Fixes #253)#1039
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:
📝 WalkthroughWalkthroughUpdated CLI routing tests for the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/cli.test.js (1)
141-148: Harden log-routing assertions to avoid false positives.
toContain(...)can still pass if extra unintended args are appended. Consider asserting exact argv tokens (and explicitly no-fin non-follow mode).Proposed test tightening
- expect(fs.readFileSync(markerFile, "utf8")).toContain("sandbox connect alpha -- tail -n 200 /tmp/gateway.log"); + const argv = fs.readFileSync(markerFile, "utf8").trim().split(/\s+/); + expect(argv).toEqual(["sandbox", "connect", "alpha", "--", "tail", "-n", "200", "/tmp/gateway.log"]); + expect(argv).not.toContain("-f"); ... - expect(fs.readFileSync(markerFile, "utf8")).toContain("sandbox connect alpha -- tail -n 200 -f /tmp/gateway.log"); + const followArgv = fs.readFileSync(markerFile, "utf8").trim().split(/\s+/); + expect(followArgv).toEqual(["sandbox", "connect", "alpha", "--", "tail", "-n", "200", "-f", "/tmp/gateway.log"]);Also applies to: 184-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.js` around lines 141 - 148, The test currently uses expect(...).toContain(...) which can give false positives; update the assertion to parse the recorded command from markerFile (read via fs.readFileSync(markerFile, "utf8")), split it into argv tokens (e.g., by whitespace while preserving quoted tokens) and assert the token array equals the exact expected argv array for the non-follow case (e.g., ["sandbox","connect","alpha","--","tail","-n","200","/tmp/gateway.log"]) and also explicitly assert that the "-f" flag is not present; apply the same tightening to the other similar assertion around the runWithEnv usage at the later block (the 184-190 case) so both tests validate exact tokens rather than substring containment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/cli.test.js`:
- Around line 141-148: The test currently uses expect(...).toContain(...) which
can give false positives; update the assertion to parse the recorded command
from markerFile (read via fs.readFileSync(markerFile, "utf8")), split it into
argv tokens (e.g., by whitespace while preserving quoted tokens) and assert the
token array equals the exact expected argv array for the non-follow case (e.g.,
["sandbox","connect","alpha","--","tail","-n","200","/tmp/gateway.log"]) and
also explicitly assert that the "-f" flag is not present; apply the same
tightening to the other similar assertion around the runWithEnv usage at the
later block (the 184-190 case) so both tests validate exact tokens rather than
substring containment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d90c901d-27cb-48e5-a728-82314e810cfe
📒 Files selected for processing (2)
bin/nemoclaw.jstest/cli.test.js
|
Tightened the sandbox logs assertions to check exact argv tokens now, including an explicit no--f check for the non-follow path. Reran the CLI test file plus the full suite, and both are green here. |
0089a0e to
66d3188
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/cli.test.js (1)
159-201: Consolidate duplicate plain-logs coverage.Line 159 introduces a second
it("passes plain logs through without the tail flag", ...)test, while the same scenario/title already exists at Line 444. Keeping both makes failures harder to triage and duplicates setup logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.js` around lines 159 - 201, Remove the duplicate test case titled "passes plain logs through without the tail flag" in test/cli.test.js (the block that calls runWithEnv("alpha logs", ...) and writes markerFile) to avoid redundant coverage; keep only one instance of this scenario (the other at line ~444) or consolidate any unique assertions into the remaining test, ensuring references to runWithEnv, markerFile, and the openshell stub are preserved where applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/cli.test.js`:
- Around line 193-201: The test currently uses substring checks on the recorded
argv which can miss leaked flags; update the assertions to parse the recorded
marker (read via fs.readFileSync(markerFile, "utf8")) into tokens (e.g., split
on whitespace) and assert the token array equals the expected argv token list
for follow and non-follow modes, and explicitly assert that the '--follow' token
is absent in the non-follow case (and present in the follow case if applicable);
apply the same strengthened tokenized assertions to the other test block
referenced (the block around lines 237-244) and use runWithEnv and markerFile to
locate the recorded argv for both checks.
---
Nitpick comments:
In `@test/cli.test.js`:
- Around line 159-201: Remove the duplicate test case titled "passes plain logs
through without the tail flag" in test/cli.test.js (the block that calls
runWithEnv("alpha logs", ...) and writes markerFile) to avoid redundant
coverage; keep only one instance of this scenario (the other at line ~444) or
consolidate any unique assertions into the remaining test, ensuring references
to runWithEnv, markerFile, and the openshell stub are preserved where
applicable.
🪄 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: Pro
Run ID: 5ebfba9a-af11-4cd4-9c98-1ec91fe78d42
📒 Files selected for processing (1)
test/cli.test.js
| const r = runWithEnv("alpha logs", { | ||
| HOME: home, | ||
| PATH: `${localBin}:${process.env.PATH || ""}`, | ||
| }); | ||
|
|
||
| expect(r.code).toBe(0); | ||
| expect(fs.readFileSync(markerFile, "utf8")).toContain("logs alpha"); | ||
| expect(fs.readFileSync(markerFile, "utf8")).not.toContain("--tail"); | ||
| }); |
There was a problem hiding this comment.
Strengthen argv assertions to catch leaked flags.
These checks currently rely on substring matching, so they can still pass if unexpected args are forwarded (for example, --follow alongside --tail). Prefer tokenized argv equality and explicitly assert --follow is absent in follow mode.
Proposed assertion hardening
- expect(fs.readFileSync(markerFile, "utf8")).toContain("logs alpha");
- expect(fs.readFileSync(markerFile, "utf8")).not.toContain("--tail");
+ const plainArgs = fs.readFileSync(markerFile, "utf8").trim().split(/\s+/);
+ expect(plainArgs).toEqual(["logs", "alpha"]);
+ expect(plainArgs).not.toContain("--tail");
@@
- expect(fs.readFileSync(markerFile, "utf8")).toContain("logs alpha --tail");
+ const followArgs = fs.readFileSync(markerFile, "utf8").trim().split(/\s+/);
+ expect(followArgs).toEqual(["logs", "alpha", "--tail"]);
+ expect(followArgs).not.toContain("--follow");Also applies to: 237-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cli.test.js` around lines 193 - 201, The test currently uses substring
checks on the recorded argv which can miss leaked flags; update the assertions
to parse the recorded marker (read via fs.readFileSync(markerFile, "utf8")) into
tokens (e.g., split on whitespace) and assert the token array equals the
expected argv token list for follow and non-follow modes, and explicitly assert
that the '--follow' token is absent in the non-follow case (and present in the
follow case if applicable); apply the same strengthened tokenized assertions to
the other test block referenced (the block around lines 237-244) and use
runWithEnv and markerFile to locate the recorded argv for both checks.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/cli.test.js (1)
203-235: Test setup looks correct; consider adding--versionhandler for robustness.The test correctly validates the
--follow→--tailtranslation. However, like the plain-logs test, this stub doesn't handle the--versioncall thatsandboxLogsmakes. Currently this works because a missing version response causes the version check to be skipped, but it would be more robust to mirror the approach in the test at Line 467-480 which includes the version response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.js` around lines 203 - 235, The openshell stub used in the "passes --follow through to openshell logs" test should also handle the `--version` probe that sandboxLogs issues: update the script written to path.join(localBin, "openshell") (the stub that currently writes args to marker_file) to detect when `--version` (or `-v` if applicable) is present and print a version string and exit 0 before the normal arg-capturing behavior; keep the existing marker_file behavior for other invocations so the test still validates translation of `--follow` to `--tail`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/cli.test.js`:
- Around line 159-160: The test named "passes plain logs through without the
tail flag" is duplicated and uses the wrong temp dir prefix
"nemoclaw-cli-logs-follow-" (copy-paste from the follow test); either delete
this duplicate or rename it to a distinct scenario name and adjust its
assertions, and change the temp directory prefix to a descriptive one (e.g.,
"nemoclaw-cli-logs-plain-") so it matches the test intent; locate the spec by
the exact test name string and update the mkdtempSync call and any related
assertions or flags to reflect the unique scenario (or remove the whole
duplicate block if the other test covers the intended behavior).
---
Nitpick comments:
In `@test/cli.test.js`:
- Around line 203-235: The openshell stub used in the "passes --follow through
to openshell logs" test should also handle the `--version` probe that
sandboxLogs issues: update the script written to path.join(localBin,
"openshell") (the stub that currently writes args to marker_file) to detect when
`--version` (or `-v` if applicable) is present and print a version string and
exit 0 before the normal arg-capturing behavior; keep the existing marker_file
behavior for other invocations so the test still validates translation of
`--follow` to `--tail`.
🪄 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: Pro
Run ID: 2f4a1a9f-1acd-4502-9920-a1e7e44724fa
📒 Files selected for processing (1)
test/cli.test.js
| it("passes plain logs through without the tail flag", () => { | ||
| const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-cli-logs-follow-")); |
There was a problem hiding this comment.
Duplicate test name and inconsistent temp directory.
This test has the same name as the test at Line 444 ("passes plain logs through without the tail flag"). Duplicate test names make test reports confusing and obscure which test is canonical.
Additionally, the temp directory name "nemoclaw-cli-logs-follow-" is inconsistent with the test's purpose—it appears to be a copy-paste artifact from the follow test setup.
The test at Line 444 appears more complete (includes --version handling), so consider removing this duplicate or renaming it to test a distinct scenario.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cli.test.js` around lines 159 - 160, The test named "passes plain logs
through without the tail flag" is duplicated and uses the wrong temp dir prefix
"nemoclaw-cli-logs-follow-" (copy-paste from the follow test); either delete
this duplicate or rename it to a distinct scenario name and adjust its
assertions, and change the temp directory prefix to a descriptive one (e.g.,
"nemoclaw-cli-logs-plain-") so it matches the test intent; locate the spec by
the exact test name string and update the mkdtempSync call and any related
assertions or flags to reflect the unique scenario (or remove the whole
duplicate block if the other test covers the intended behavior).
66d3188 to
b7f3ba8
Compare
|
Verified the finding. The branch had a duplicate plain-logs spec and the earlier copy used a follow-style temp dir prefix from a copy/paste. I removed the duplicate and kept the later, more complete plain-logs test. Focused CLI tests and the full suite both pass locally. |
|
I ported this branch across the JS→TS migration and merged the latest Validation rerun:
|
ece9fde to
fc70e1c
Compare
|
Rebased this onto the latest main and finished the TS-side port for the logs fallback. The CLI now routes |
|
Thanks for the sandbox logs fix. The codebase has changed significantly since March 28 — including a full TypeScript migration and rework of the logs streaming path — so this will need a rebase on |
fc70e1c to
7771ffb
Compare
|
Rebased this onto the latest main again. The logs fallback still routes through sandbox connect tail on top of the current TS entrypoint, and npm run build:cli plus npm test -- test/cli.test.ts pass locally. |
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Fixes NVIDIA#253 Signed-off-by: Deepak Jain <deepujain@gmail.com>
7771ffb to
e9b92e7
Compare
|
Rebased on current main and finished the logs fallback port on the TS entrypoint. The logs path now goes through sandbox connect tail, and the focused logs CLI cases pass locally. |
Fixes NVIDIA#253 Signed-off-by: Deepak Jain <deepujain@gmail.com>
|
Pushed a cleanup pass for the current branch head. The wrapper executable bit is restored, the diff is back to the TS entrypoint and test file only, and the PR body now has the DCO sign-off. |
ericksoa
left a comment
There was a problem hiding this comment.
Thanks for pushing through multiple rebases on this one, @deepujain — the approach is solid and CI is fully green.
One issue to fix before merge:
isMissingSandboxDeleteResult check is dead code with stdio: "inherit"
const result = runOpenshell(args, {
stdio: "inherit", // ← stdout/stderr go directly to terminal
ignoreError: true,
});
if (result.status !== 0 && !isMissingSandboxDeleteResult(`${result.stdout || ""}${result.stderr || ""}`)) {With stdio: "inherit", spawnSync doesn't capture output — result.stdout and result.stderr are both null. The concatenation always yields "", so isMissingSandboxDeleteResult("") never matches, making this guard a no-op.
Fix (pick one):
- Use
stdio: ["inherit", "pipe", "pipe"]if you need to inspect stderr for the missing-sandbox case, or - Remove the
isMissingSandboxDeleteResultcheck entirely and just print the error unconditionally on non-zero exit — simpler, and the user already seestail's error directly on their terminal via the inherited stdio.
Minor suggestion: the hardcoded /tmp/gateway.log path in buildSandboxLogsArgs could be a named constant (e.g., GATEWAY_LOG_PATH) to make it easier to find/update if OpenClaw ever changes the log location. Not a blocker.
Also, the status: rebase label looks stale — the branch is current with main.
Fixes NVIDIA#253 Signed-off-by: Deepak Jain <deepujain@gmail.com>
|
The dead missing-sandbox guard is gone from sandboxLogs, so the nonzero path now matches the inherited-stdio behavior. |
|
Hey @deepujain, thanks for tackling this and for the multiple rebases through the TS migration. The design intent (tunnel into the sandbox and tail the gateway log) is right, and removing the Wrong OpenShell subcommand. The PR uses Why the tests pass despite this. The mock openshell in Suggested fix — swap function buildSandboxLogsArgs(sandboxName, follow) {
- const args = ["sandbox", "connect", sandboxName, "--", "tail", "-n", "200"];
+ const args = ["sandbox", "exec", "-n", sandboxName, "--", "tail", "-n", "200"];
if (follow) {
args.push("-f");
}
args.push("/tmp/gateway.log");
return args;
}Before merge, worth manually confirming against a real openshell: both |
Fixes NVIDIA#253 Signed-off-by: Deepak Jain <deepujain@gmail.com>
|
Swapped the logs path over to |
Summary
nemoclaw <sandbox> logsno longer depends on the unsupportedopenshell sandbox logssubcommand. It now reaches the gateway log by connecting into the sandbox and runningtaildirectly, which keeps the logs flow working on affected OpenShell versions.Fixes #253
Changes
src/nemoclaw.ts- routeslogsthroughopenshell sandbox connect <name> -- tail -n 200 /tmp/gateway.log, and adds-fwhen--followis requested.test/cli.test.ts- covers the plain and--followlogs paths against the current TS entrypoint.Testing
npm run build:clinpx vitest run test/cli.test.ts -t "routes logs to sandbox connect tailing the gateway log|passes --follow through to tail inside sandbox connect|passes plain logs through without the tail flag"Signed-off-by: Deepak Jain deepujain@gmail.com