fix(logs): honor tail and since flags#2825
Merged
Merged
Conversation
Fixes NVIDIA#2755 Signed-off-by: Deepak Jain <deepujain@gmail.com>
Contributor
📝 WalkthroughWalkthroughAdds --tail/-n, --since and expanded --help support for ChangesLogs feature — options, plumbing, and behavior
Sequence Diagram(s)sequenceDiagram
participant CLI as nemoClaw CLI
participant Bridge as Runtime Bridge
participant OpenShell as OpenShell (sandbox logs)
participant Gateway as OpenClaw Gateway (tail probe)
CLI->>Bridge: sandboxLogs(name, {follow, lines, since})
alt since not provided
Bridge->>Gateway: sandbox exec ... tail -n <lines> /tmp/gateway.log
Gateway-->>Bridge: gateway tail output
end
Bridge->>OpenShell: logs <name> -n <lines> --source all [--since <duration>] [--tail]
OpenShell-->>Bridge: sandbox logs output (stream or one-shot)
Bridge-->>CLI: stream/print combined outputs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/cli.test.ts (1)
724-767: ⚡ Quick winAdd a malformed
-ntest to close the alias-validation gap.Line 724 adds the happy path for
-n, but only--tailhas an explicit malformed-input test right now. Adding one negative-ncase would guard alias-specific regressions.Proposed test addition
it("passes -n line count to both log sources", () => { const setup = createLogsTestSetup("nemoclaw-cli-logs-n-"); const r = setup.runLogs("alpha logs -n 25"); @@ ]); }); + it("rejects -n without a line count", () => { + const setup = createLogsTestSetup("nemoclaw-cli-logs-n-missing-"); + const r = setup.runLogs("alpha logs -n 2>&1"); + + expect(r.code).toBe(1); + expect(r.out).toContain("positive line count"); + expect(setup.readCalls()).toEqual([]); + }); + it("passes --since to OpenShell logs without an unfiltered gateway tail", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.ts` around lines 724 - 767, Add a negative test for the -n alias similar to the existing --tail test: create a test named like "rejects -n without a line count" that uses createLogsTestSetup and calls setup.runLogs("alpha logs -n 2>&1"), then assert r.code === 1, r.out contains the same error string used for --tail ("--tail requires a positive line count"), and setup.readCalls() returns an empty array; refer to createLogsTestSetup, runLogs, and the existing "--tail" test as templates to place and structure the new spec.src/nemoclaw.ts (1)
4565-4570: Optional: run targeted E2E workflows for lifecycle safetySince this change lives in
src/nemoclaw.ts, it’s worth running the two recommended nightly E2E jobs before merge to catch any unexpected sandbox command interactions.As per coding guidelines: for
src/nemoclaw.ts, runsandbox-survival-e2eandsandbox-operations-e2eselectively viagh workflow run nightly-e2e.yaml --ref <branch> -f jobs=sandbox-survival-e2e,sandbox-operations-e2e.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 4565 - 4570, This change touches src/nemoclaw.ts (around the sandboxLogs / parseSandboxLogsArgs usage), so before merging run the two targeted nightly E2E jobs for lifecycle safety: trigger the GitHub Actions workflow for sandbox-survival-e2e and sandbox-operations-e2e on your branch (use the nightly-e2e workflow with the jobs filter for those two jobs and the current branch/ref) and confirm both pass to catch any sandbox command regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/nemoclaw.ts`:
- Around line 4565-4570: This change touches src/nemoclaw.ts (around the
sandboxLogs / parseSandboxLogsArgs usage), so before merging run the two
targeted nightly E2E jobs for lifecycle safety: trigger the GitHub Actions
workflow for sandbox-survival-e2e and sandbox-operations-e2e on your branch (use
the nightly-e2e workflow with the jobs filter for those two jobs and the current
branch/ref) and confirm both pass to catch any sandbox command regressions.
In `@test/cli.test.ts`:
- Around line 724-767: Add a negative test for the -n alias similar to the
existing --tail test: create a test named like "rejects -n without a line count"
that uses createLogsTestSetup and calls setup.runLogs("alpha logs -n 2>&1"),
then assert r.code === 1, r.out contains the same error string used for --tail
("--tail requires a positive line count"), and setup.readCalls() returns an
empty array; refer to createLogsTestSetup, runLogs, and the existing "--tail"
test as templates to place and structure the new spec.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 22cf294a-92db-433e-b978-940b45789a22
📒 Files selected for processing (3)
src/lib/command-registry.tssrc/nemoclaw.tstest/cli.test.ts
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Contributor
Author
|
Added the missing |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/cli.test.ts (1)
746-757: ⚡ Quick winAdd a
--follow --sincecombo test to lock in gateway-skip behaviorLine 746 covers
--sincein non-follow mode, but the streaming branch is also sensitive here. A focused combo test would prevent regressions where gateway tailing is accidentally reintroduced for follow streams.💡 Suggested test addition
+ it("skips OpenClaw source for logs --follow --since and forwards both flags to OpenShell", () => { + const setup = createLogsTestSetup("nemoclaw-cli-logs-follow-since-"); + const r = setup.runLogs("alpha logs --follow --since 5m"); + + const calls = setup.readCalls(); + expect(r.code).toBe(0); + expect(calls).toContain("settings set alpha --key ocsf_json_enabled --value true"); + expect(calls).toContain("logs alpha -n 200 --source all --since 5m --tail"); + expect(calls.some((call) => call.startsWith("sandbox exec -n alpha"))).toBe(false); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.ts` around lines 746 - 757, Add a new test that mirrors the existing "passes --since to OpenShell logs without an unfiltered gateway tail" but uses the follow flag to lock in gateway-skip behavior: create a test (e.g., "passes --follow --since to OpenShell logs without an unfiltered gateway tail") that calls createLogsTestSetup("nemoclaw-cli-logs-since-follow-") and invokes setup.runLogs("alpha logs --follow --since 5m"), then assert r.code === 0, assert the expected preflight calls include "settings set alpha --key ocsf_json_enabled --value true" and "logs alpha -n 200 --source all --since 5m", and assert no call startsWith("sandbox exec -n alpha") (using the same readCalls/assert pattern as the existing test) so the follow+since combo does not reintroduce gateway tailing.
🤖 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.ts`:
- Around line 746-757: Add a new test that mirrors the existing "passes --since
to OpenShell logs without an unfiltered gateway tail" but uses the follow flag
to lock in gateway-skip behavior: create a test (e.g., "passes --follow --since
to OpenShell logs without an unfiltered gateway tail") that calls
createLogsTestSetup("nemoclaw-cli-logs-since-follow-") and invokes
setup.runLogs("alpha logs --follow --since 5m"), then assert r.code === 0,
assert the expected preflight calls include "settings set alpha --key
ocsf_json_enabled --value true" and "logs alpha -n 200 --source all --since 5m",
and assert no call startsWith("sandbox exec -n alpha") (using the same
readCalls/assert pattern as the existing test) so the follow+since combo does
not reintroduce gateway tailing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 966ee038-e7f4-4def-a5a4-416b42e918c7
📒 Files selected for processing (1)
test/cli.test.ts
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Contributor
Author
|
Added the |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/cli.test.ts (1)
737-745: ⚡ Quick winExpand the remaining negative-path coverage.
You already cover missing
-n/--tailvalues and unknown flags, but the new suite still misses the--sincemissing-value branch and malformed line-count inputs like0or non-numeric values. One small regression case for each would close that gap.Also applies to: 770-787
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.ts` around lines 737 - 745, Add tests to expand negative-path coverage: using the existing test helpers createLogsTestSetup and its runLogs method, add cases that call "alpha logs --since 2>&1" to assert exit code 1 and an error message about "--since requires a timestamp" (or similar), and add separate tests calling "alpha logs -n 0 2>&1" and "alpha logs -n foo 2>&1" to assert exit code 1, the "-n requires a positive line count" message, and that no read calls were made (setup.readCalls() === []). Place these alongside the existing "rejects -n without a line count" test so the suite covers missing --since and malformed/zero -n inputs.
🤖 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.ts`:
- Around line 737-745: Add tests to expand negative-path coverage: using the
existing test helpers createLogsTestSetup and its runLogs method, add cases that
call "alpha logs --since 2>&1" to assert exit code 1 and an error message about
"--since requires a timestamp" (or similar), and add separate tests calling
"alpha logs -n 0 2>&1" and "alpha logs -n foo 2>&1" to assert exit code 1, the
"-n requires a positive line count" message, and that no read calls were made
(setup.readCalls() === []). Place these alongside the existing "rejects -n
without a line count" test so the suite covers missing --since and
malformed/zero -n inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4d3afef3-02af-462a-83c3-ae79284ec334
📒 Files selected for processing (1)
test/cli.test.ts
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Contributor
Author
|
Expanded the logs parser negative-path tests for missing |
wscurran
added
bug
Closed
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
force-pushed
the
fix/2755-logs-flags
branch
from
b44c2c7 to
2a4f0b0
Compare
ericksoa
approved these changes
May 2, 2026
Contributor
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Approved after re-review. Required checks are green; logs flag parsing/build/typecheck/focused CLI validation passed locally, and the full nightly validation branch had no PR-required failures.
12 tasks
This was referenced May 4, 2026
miyoungc
added a commit
that referenced
this pull request
May 5, 2026
## Summary Catch up the docs for user-facing changes that landed over the weekend and today, so the published guidance matches current installer, onboarding, status, logs, local inference, rebuild backup behavior, the next docs version selector, and refreshed generated user skills. ## Related Issue None. ## Changes - Document WSL Windows-host Ollama onboarding actions, including use, start, restart, install, and `host.docker.internal` model pulls from #2800; clarify that onboard owns Ollama install and model pulls from #2952. - Document installer fail-fast behavior for non-TTY third-party software acceptance from #2706. - Update sandbox-name guidance and Brev deploy validation behavior from #2948. - Add `nemoclaw <name> logs --tail/--since` coverage from #2825. - Add global `nemoclaw status --json` coverage from #2822. - Document verified-gateway status behavior and non-zero degraded exits from #2884. - Clarify that rebuild stops before deleting the original sandbox when backup fails, including unreadable or root-owned state paths. - Bump docs switcher metadata from 0.0.33 to 0.0.34 without changing package versions or creating release tags. - Regenerate `.agents/skills/nemoclaw-user-*` from docs, including the new `nemoclaw-user-manage-sandboxes` generated skill and removal of the stale `nemoclaw-user-workspace` output. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Verification - [ ] `npx prek run --all-files` passes - [ ] `npm test` passes - [ ] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [x] Docs updated for user-facing behavior changes - [x] `make docs` builds without warnings (doc changes only) - [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) --- <!-- DCO sign-off required by CI. Run: git config user.name && git config user.email --> Signed-off-by: Miyoung Choi <miyoungc@nvidia.com> Made with [Cursor](https://cursor.com) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Expanded Ollama/local inference guidance with detailed WSL and Windows-host workflows, proxy/token behavior, and onboarding options * Standardized sandbox name validation and updated troubleshooting, CLI, and deploy docs to surface the rules and validation timing * Added/rewrote Manage Sandboxes, policy management, backup/restore, messaging channels, workspace persistence, and CLI selection guides * Refreshed quickstart/Hermes guidance, skill mappings, and bumped docs version to 0.0.34 <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
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
3 participants
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
Fixes #2755.
This teaches
nemoclaw <name> logsto parse the documented log flags instead of silently discarding them:--tail <lines>and-n <lines>now drive the line count sent to both log sources.--since <duration>is forwarded toopenshell logsand skips the unfiltered gateway-log tail.--helpprints usage and exits without fetching logs.Validation
npm run build:clinpm run typecheck:clinpm test -- test/cli.test.tsnpm install --ignore-scriptsandnpm run buildinnemoclaw/npm test -- test/ssrf-parity.test.ts nemoclaw/src/commands/migration-state.test.tsFull
npm testwas also rerun after the nested build; the changed CLI coverage passes, while the existing installer/uninstall suites still fail in this local worktree.Summary by CodeRabbit
New Features
Documentation
Tests
Signed-off-by: Aaron Erickson aerickson@nvidia.com