fix(cli): include OpenShell audit events in logs#2590
Merged
ericksoa merged 2 commits intoNVIDIA:mainfrom Apr 29, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds richer child-process result metadata and reworks Changes
Sequence DiagramsequenceDiagram
participant CLI as "nemoclaw CLI"
participant Settings as "OpenShell Settings"
participant Gateway as "OpenClaw Gateway"
participant OpenShell as "OpenShell Logs"
participant Parent as "Parent Process"
CLI->>Settings: set ocsf_json_enabled = true
Settings-->>CLI: OK / error
alt follow=true
rect rgba(100,200,150,0.5)
par Gateway stream
CLI->>Gateway: sandbox exec tail -n 200 -f /tmp/gateway.log
Gateway-->>CLI: stream events
and OpenShell stream
CLI->>OpenShell: openshell logs <sandbox> -n 200 --source all --tail
OpenShell-->>CLI: stream events
end
end
rect rgba(200,100,100,0.5)
par Monitor children
Gateway-->>Parent: exit / error (status, signal, error)
and
OpenShell-->>Parent: exit / error (status, signal, error)
end
end
Parent->>CLI: aggregate statuses, map signals to exit code (with forced-exit fallback)
else follow=false
CLI->>Gateway: sandbox exec tail -n 200 /tmp/gateway.log (probe with timeout)
Gateway-->>CLI: output OR "OpenClaw log source unavailable (ETIMEDOUT)"
CLI->>OpenShell: openshell logs <sandbox> -n 200 --source all
OpenShell-->>CLI: last entries
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![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: 1
🧹 Nitpick comments (1)
test/cli.test.ts (1)
331-425: These two log-routing tests currently assert the same behavior.Both cases set up the same stub and expect the same three calls, so they’ll fail and pass together. Consider collapsing them or extracting a helper so the second case can focus only on the audit-specific contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.ts` around lines 331 - 425, Both tests "routes logs to OpenClaw and OpenShell log sources" and "enables OpenShell audit events before reading logs" duplicate the same setup and identical assertions; extract the shared setup into a helper (e.g., createLogsTestSetup) that creates HOME/localBin/registryDir, writes sandboxes.json and the openshell stub and calls runWithEnv, then reuse it in both tests; change the second test ("enables OpenShell audit events before reading logs") to only assert the audit-specific contract (instead of repeating the three-call equality) by reading the marker file and asserting the presence of the audit-enabling call (e.g., check that calls includes the settings write that enables OpenShell audit) using runWithEnv, markerFile, and the existing openshell stub utilities.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nemoclaw.ts`:
- Around line 1896-1901: The enableSandboxAuditLogs function currently swallows
failures from runOpenshell, so update it to check the operation result (from
runOpenshell) and surface errors: call runOpenshell(["settings", "set", ...])
and if it returns a non-zero exit code or an error object, log or throw a
descriptive error via processLogger/error handling (include sandboxName and the
command and any stderr), or propagate the error so callers can handle it;
reference the enableSandboxAuditLogs function and the runOpenshell invocation
when making this change.
---
Nitpick comments:
In `@test/cli.test.ts`:
- Around line 331-425: Both tests "routes logs to OpenClaw and OpenShell log
sources" and "enables OpenShell audit events before reading logs" duplicate the
same setup and identical assertions; extract the shared setup into a helper
(e.g., createLogsTestSetup) that creates HOME/localBin/registryDir, writes
sandboxes.json and the openshell stub and calls runWithEnv, then reuse it in
both tests; change the second test ("enables OpenShell audit events before
reading logs") to only assert the audit-specific contract (instead of repeating
the three-call equality) by reading the marker file and asserting the presence
of the audit-enabling call (e.g., check that calls includes the settings write
that enables OpenShell audit) using runWithEnv, markerFile, and the existing
openshell stub utilities.
🪄 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: 08725020-6df4-43ef-8af9-5781a3fc15dc
📒 Files selected for processing (2)
src/nemoclaw.tstest/cli.test.ts
yimoj
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.ts`:
- Around line 93-139: The tests currently only assert that the fake openshell
was invoked (via argv recorded in markerFile) but do not verify that the child
process stdout/stderr are forwarded to the CLI; update createLogsTestSetup to
make the fake openshell emit distinct log lines (e.g., add lines in
openshellLines that print identifiable stdout and stderr messages), then change
the tests that call runLogs (and the --follow variants) to assert that the CLI
output contains those distinct log lines (use markerFile for invocation checks
only and assert runLogs() stdout/stderr contains the emitted messages); refer to
createLogsTestSetup, the openshell script content, markerFile, and runLogs to
locate and modify the test harness and assertions.
- Around line 401-416: The test only checks the warning text after a failed
OpenShell settings set but doesn't assert that log collection still proceeds;
update the test using the createLogsTestSetup(...) and runLogs(...) result (r)
to assert that actual log lines from the fallback log source are present (e.g.,
expect(r.out).toContain(<expected log line>)) or assert a success indicator that
the other log reader was invoked, so the test fails if code returns early after
settings failure; locate assertions around r (from runLogs) and add a check for
an actual log entry produced by the normal collection path.
🪄 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: 3367306a-c05b-4117-b6c7-e410ed2a987b
📒 Files selected for processing (2)
src/nemoclaw.tstest/cli.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nemoclaw.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/cli.test.ts (1)
507-516: Make the follow-source polling window deadline-based to reduce CI flakes.Line 507 uses a fixed ~2s polling budget (20 × 100ms). Under slower CI load, this can intermittently fail before both subprocesses register. Prefer a deadline tied to test timeout.
♻️ Suggested reliability tweak
- for (let attempt = 0; attempt < 20; attempt += 1) { + const deadline = Date.now() + Math.min(Number(process.env.NEMOCLAW_TEST_TIMEOUT || 10000), 10000); + while (Date.now() < deadline) { calls = readCalls(); if ( calls.includes("logs alpha -n 200 --source all --tail") && calls.includes("sandbox exec -n alpha -- tail -n 200 -f /tmp/gateway.log") ) { break; } await new Promise((resolve) => setTimeout(resolve, 100)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.ts` around lines 507 - 516, The fixed 20-iteration loop that polls readCalls() uses a hardcoded ~2s window which flakes under slow CI; replace it with a deadline-based loop using a deadline = Date.now() + timeoutMs (where timeoutMs is derived from the test's timeout or a configurable constant, e.g., jest.getTimeout() or a testTimeoutMs variable) and loop while (Date.now() < deadline) { calls = readCalls(); if (calls.includes(... ) && calls.includes(...)) break; await sleep(100); } so the polling window scales to the test timeout and still checks the same command strings used in the current checks.
🤖 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 507-516: The fixed 20-iteration loop that polls readCalls() uses a
hardcoded ~2s window which flakes under slow CI; replace it with a
deadline-based loop using a deadline = Date.now() + timeoutMs (where timeoutMs
is derived from the test's timeout or a configurable constant, e.g.,
jest.getTimeout() or a testTimeoutMs variable) and loop while (Date.now() <
deadline) { calls = readCalls(); if (calls.includes(... ) &&
calls.includes(...)) break; await sleep(100); } so the polling window scales to
the test timeout and still checks the same command strings used in the current
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 709ab13a-27fb-41ce-a450-7b84b45ad2b4
📒 Files selected for processing (1)
test/cli.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/cli.test.ts (1)
1023-1024: Optional: avoid duplicate marker-file reads in the same assertion block.Line 1023 and Line 1024 both call
readRecordedArgs(markerFile). Caching once makes the test slightly clearer and avoids redundant I/O.♻️ Suggested cleanup
- expect(readRecordedArgs(markerFile)).toEqual(["logs", "alpha", "-n", "200", "--source", "all"]); - expect(readRecordedArgs(markerFile)).not.toContain("--tail"); + const recordedArgs = readRecordedArgs(markerFile); + expect(recordedArgs).toEqual(["logs", "alpha", "-n", "200", "--source", "all"]); + expect(recordedArgs).not.toContain("--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 1023 - 1024, Cache the result of readRecordedArgs(markerFile) into a local variable and use that variable for both assertions instead of calling readRecordedArgs(markerFile) twice; update the test block around the two expect calls (the ones asserting equals ["logs","alpha","-n","200","--source","all"] and not.toContain("--tail")) to reference the cached array variable so the markerFile is read once.
🤖 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 1023-1024: Cache the result of readRecordedArgs(markerFile) into a
local variable and use that variable for both assertions instead of calling
readRecordedArgs(markerFile) twice; update the test block around the two expect
calls (the ones asserting equals ["logs","alpha","-n","200","--source","all"]
and not.toContain("--tail")) to reference the cached array variable so the
markerFile is read once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2383aece-fc7e-44f1-a383-18e2770a041f
📒 Files selected for processing (1)
test/cli.test.ts
yimoj
force-pushed
the
fix/2512-logs-policy-denials
branch
from
0fc8faa to
dd65996
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/nemoclaw.ts (1)
1828-1837: Consider deduplicatingspawnoptions for the two follow-mode sources.Both child-process spawns use identical
{ cwd, env, stdio }; extracting a small helper/const will reduce drift in future edits.♻️ Optional cleanup
function streamSandboxFollowLogs(sandboxName: string): void { const openclawArgs = buildSandboxOpenclawGatewayLogsArgs(sandboxName, true); const openshellArgs = buildSandboxLogsArgs(sandboxName, true); - const openclaw = spawn(getOpenshellBinary(), openclawArgs, { - cwd: ROOT, - env: process.env, - stdio: "inherit", - }); - const openshell = spawn(getOpenshellBinary(), openshellArgs, { - cwd: ROOT, - env: process.env, - stdio: "inherit", - }); + const spawnOpts = { cwd: ROOT, env: process.env, stdio: "inherit" as const }; + const openclaw = spawn(getOpenshellBinary(), openclawArgs, spawnOpts); + const openshell = spawn(getOpenshellBinary(), openshellArgs, spawnOpts);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 1828 - 1837, Both spawn() calls for openclaw and openshell repeat the same options; extract a shared constant or helper (e.g., followModeSpawnOpts or makeSpawnOptions()) that returns the object { cwd: ROOT, env: process.env, stdio: "inherit" } and then call spawn(getOpenshellBinary(), openclawArgs, followModeSpawnOpts) and spawn(getOpenshellBinary(), openshellArgs, followModeSpawnOpts) to remove duplication and prevent drift between openclaw and openshell invocations.
🤖 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 1828-1837: Both spawn() calls for openclaw and openshell repeat
the same options; extract a shared constant or helper (e.g., followModeSpawnOpts
or makeSpawnOptions()) that returns the object { cwd: ROOT, env: process.env,
stdio: "inherit" } and then call spawn(getOpenshellBinary(), openclawArgs,
followModeSpawnOpts) and spawn(getOpenshellBinary(), openshellArgs,
followModeSpawnOpts) to remove duplication and prevent drift between openclaw
and openshell invocations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d23fccc1-96ea-4b64-8c8b-613d69d6e05b
📒 Files selected for processing (2)
src/nemoclaw.tstest/cli.test.ts
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nemoclaw.ts`:
- Around line 1799-1800: The new synchronous OpenShell probes (calls to
runOpenshell and the caller runOpenclawGatewayLogs) currently block
indefinitely; add a configurable timeout to these probes, pass the timeout into
runOpenshell (or overload it) and ensure the caller (runOpenclawGatewayLogs and
the other locations where runOpenshell is invoked) aborts the probe when the
timeout elapses, logs the timeout event, and returns a sensible fallback so
nemoclaw ... logs can continue to the denial stream; make the timeout
configurable (with a reasonable default) and implement proper
cancellation/cleanup in the runOpenshell implementation so hanging child
processes/streams are terminated.
🪄 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: 58d9eeba-7ff0-4a1e-9097-d25aa057f065
📒 Files selected for processing (2)
src/nemoclaw.tstest/cli.test.ts
wscurran
added
NemoClaw CLI
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/openshell.test.ts (1)
101-131: Add a timeout regression forcaptureOpenshellCommand().These cases exercise timeout passthrough and
runOpenshellCommand(), but the newcaptureOpenshellCommand()ETIMEDOUT+ignoreErrorbranch still has no direct assertion. If that path regresses, the log/connect callers will only catch it indirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/openshell.test.ts` around lines 101 - 131, The test suite is missing a direct assertion that captureOpenshellCommand correctly handles ETIMEDOUT with ignoreError the same way runOpenshellCommand does; add a new test that calls captureOpenshellCommand("openshell", ["status"], { ignoreError: true, timeout: 1, spawnSyncImpl: stubSpawnSync({ status: null, stdout: "", stderr: "", error: timeoutError(), signal: "SIGTERM" }), exit: exitWithCode }) and assert the returned result has status === null, signal === "SIGTERM", and error.message contains "ETIMEDOUT" so regressions in captureOpenshellCommand's timeout + ignoreError branch are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/openshell.ts`:
- Around line 123-129: The current branch that handles result.error and
isIgnoredTimeout(result.error, opts) normalizes timeout failures into status: 1
and drops the original timeout metadata; instead, preserve the original timeout
shape by returning the original result fields (including signal and error) while
still computing output the same way (combining result.stdout and conditional
result.stderr per opts.ignoreError). Update the branch in openshell.ts where
isIgnoredTimeout is checked so it returns the unmodified result (or a
shallow-cloned result with output adjusted) rather than overwriting status and
losing signal/error; refer to isIgnoredTimeout, result, opts and the
captureOpenshell caller to guide the change.
---
Nitpick comments:
In `@src/lib/openshell.test.ts`:
- Around line 101-131: The test suite is missing a direct assertion that
captureOpenshellCommand correctly handles ETIMEDOUT with ignoreError the same
way runOpenshellCommand does; add a new test that calls
captureOpenshellCommand("openshell", ["status"], { ignoreError: true, timeout:
1, spawnSyncImpl: stubSpawnSync({ status: null, stdout: "", stderr: "", error:
timeoutError(), signal: "SIGTERM" }), exit: exitWithCode }) and assert the
returned result has status === null, signal === "SIGTERM", and error.message
contains "ETIMEDOUT" so regressions in captureOpenshellCommand's timeout +
ignoreError branch are caught.
🪄 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: 867b1b00-89d2-4100-9f40-0e2bf2c0090e
📒 Files selected for processing (4)
src/lib/openshell.test.tssrc/lib/openshell.tssrc/nemoclaw.tstest/cli.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nemoclaw.ts`:
- Around line 1910-1914: The current sandboxLogs flow blocks starting the follow
stream on the synchronous enableSandboxAuditLogs probe, causing --follow to miss
events if that probe times out or is denied; change sandboxLogs so that when
follow is true it immediately calls streamSandboxFollowLogs(sandboxName) and
then triggers enableSandboxAuditLogs(sandboxName) asynchronously
(fire-and-forget) with a short timeout/retry, instead of awaiting it; ensure the
non-follow path still waits for enableSandboxAuditLogs before printing
non-follow logs, and reuse the same enable logic used elsewhere (e.g., the code
around enableSandboxAuditLogs and any retry helpers) so audit enabling happens
in parallel without delaying stream startup.
- Around line 2016-2024: The SIGINT/SIGTERM handlers set exiting and call
stopChildren(...) but then call process.exit(...) immediately, which prevents
the existing child-exit coordination from running and can leave tailing log
processes alive; update the handlers (the process.once("SIGINT"/"SIGTERM")
blocks that reference exiting and stopChildren) to remove the immediate
process.exit calls and instead wait for the existing child-exit coordination to
complete (e.g., await or listen for the existing children-exited event/promise
used elsewhere in this module) before exiting, with a short fallback timeout to
force exit if children don't terminate; ensure you still call
stopChildren("SIGINT"/"SIGTERM") and preserve setting exiting = true.
🪄 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: e01adeb0-54fb-446f-a164-11cadd8011e9
📒 Files selected for processing (6)
src/nemoclaw.tstest/check-docs-links.test.tstest/cli.test.tstest/e2e/test-skill-agent-e2e.shtest/onboard.test.tsvitest.config.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nemoclaw.ts`:
- Around line 1911-1914: The follow/shutdown path spawns a third child via
enableSandboxAuditLogsInBackground(sandboxName) but
streamSandboxFollowLogs(sandboxName) only monitors the two streaming children,
so ensure the audit-enable child is tracked and signalled during shutdown:
modify enableSandboxAuditLogsInBackground to return the child/process handle (or
a Promise that resolves to it), capture that return where called (in the follow
branch and the other similar places), add that handle to the same
shutdown/cleanup coordination used by streamSandboxFollowLogs (so SIGINT/SIGTERM
waits for or forwards termination to the audit-enable child), and ensure the
parent waits for the child to exit or kills it on early exit to avoid leaving an
orphaned openshell settings set process.
🪄 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: 2c4539a4-d83b-42be-ab05-4cc940c46500
📒 Files selected for processing (2)
src/nemoclaw.tstest/cli.test.ts
Surface OpenShell OCSF audit events (policy denials) alongside existing log streams so users can see which egress requests their sandbox is blocking. Changes: - Enable ocsf_json_enabled on the gateway before reading logs - Add OpenClaw gateway log as a second log source (non-follow and follow) - Bound all log probes with a configurable timeout (NEMOCLAW_LOGS_PROBE_TIMEOUT_MS) - Preserve timeout metadata in openshell capture results - Warn (don't crash) when audit enable or log probes fail - In --follow mode, spawn OpenClaw stream first, enable audit synchronously, then spawn OpenShell stream to avoid blocking and ensure ordering Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
yimoj
force-pushed
the
fix/2512-logs-policy-denials
branch
from
5338a29 to
d8018df
Compare
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/nemoclaw.ts (1)
2058-2060:⚠️ Potential issue | 🟠 MajorStart the OpenShell follow stream before the blocking audit-enable probe.
enableSandboxAuditLogs()still runs synchronously between the twoaddSource(...)calls, sologs --followcan spend up toNEMOCLAW_LOGS_PROBE_TIMEOUT_MSwithout the OpenShell log source attached. Any early denial emitted in that window is still missed by the very stream this fix is adding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 2058 - 2060, The OpenShell follow stream is being attached after enableSandboxAuditLogs(sandboxName) runs, so any early denial emitted during that blocking probe may be missed; move the OpenShell source attachment so addSource("OpenShell log source", openshellArgs) is called before enableSandboxAuditLogs(sandboxName) (i.e., start the OpenShell follow stream prior to invoking the blocking audit-enable probe) while keeping addSource("OpenClaw log source", openclawArgs) where it is.
🧹 Nitpick comments (1)
test/cli.test.ts (1)
541-565: Test name overstates ordering guarantee.The title says OpenClaw starts before audit enable, but Line 559-564 explicitly avoids asserting OpenClaw ordering. Consider renaming for accuracy to reduce future confusion.
Suggested rename
- it("starts OpenClaw logs before enabling audit logs for logs --follow", () => { + it("enables audit logs before starting OpenShell logs for logs --follow", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.ts` around lines 541 - 565, The test title "starts OpenClaw logs before enabling audit logs for logs --follow" overstates an ordering guarantee; update the it(...) description string to accurately reflect what is asserted (e.g. "starts OpenClaw logs when following logs and ensures audit enable precedes OpenShell logs" or similar), leaving the test body and assertions (createLogsTestSetup, setup.runLogs, setup.readCalls, the auditIdx vs openshellIdx checks, and the FAKE_OPENCLAW_LOG_LINE expectation) unchanged so the behavior and checks remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nemoclaw.ts`:
- Around line 1934-1937: The current parsing returns Math.floor(parsed) which
can produce 0 for fractional values like 0.5 and disable timeouts; change the
logic so you compute const parsed = Number(rawValue), then const floored =
Math.floor(parsed) and only accept the parsed value if Number.isFinite(parsed)
&& floored > 0, otherwise return DEFAULT_LOGS_PROBE_TIMEOUT_MS; update the
return expression that currently uses Math.floor(parsed) and reference parsed,
floored, and DEFAULT_LOGS_PROBE_TIMEOUT_MS in the fix.
---
Duplicate comments:
In `@src/nemoclaw.ts`:
- Around line 2058-2060: The OpenShell follow stream is being attached after
enableSandboxAuditLogs(sandboxName) runs, so any early denial emitted during
that blocking probe may be missed; move the OpenShell source attachment so
addSource("OpenShell log source", openshellArgs) is called before
enableSandboxAuditLogs(sandboxName) (i.e., start the OpenShell follow stream
prior to invoking the blocking audit-enable probe) while keeping
addSource("OpenClaw log source", openclawArgs) where it is.
---
Nitpick comments:
In `@test/cli.test.ts`:
- Around line 541-565: The test title "starts OpenClaw logs before enabling
audit logs for logs --follow" overstates an ordering guarantee; update the
it(...) description string to accurately reflect what is asserted (e.g. "starts
OpenClaw logs when following logs and ensures audit enable precedes OpenShell
logs" or similar), leaving the test body and assertions (createLogsTestSetup,
setup.runLogs, setup.readCalls, the auditIdx vs openshellIdx checks, and the
FAKE_OPENCLAW_LOG_LINE expectation) unchanged so the behavior and checks remain
intact.
🪄 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: 0ce5e6cf-dcb2-427f-8459-0c6d55f60599
📒 Files selected for processing (8)
src/lib/openshell.test.tssrc/lib/openshell.tssrc/nemoclaw.tstest/check-docs-links.test.tstest/cli.test.tstest/e2e/test-skill-agent-e2e.shtest/onboard.test.tsvitest.config.ts
✅ Files skipped from review due to trivial changes (3)
- vitest.config.ts
- test/onboard.test.ts
- test/check-docs-links.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/test-skill-agent-e2e.sh
Treat fractional timeout values that floor to zero as invalid so NEMOCLAW_LOGS_PROBE_TIMEOUT_MS cannot accidentally disable spawnSync timeouts. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
ericksoa
approved these changes
Apr 29, 2026
Contributor
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Looks good. I checked the OpenShell log path: openshell logs --source all is OpenShell owned gRPC log output rather than arbitrary sandbox file scraping, and it is the right scope for surfacing policy denials through NemoClaw logs. OpenShell owns the audit/log redaction boundary here, including provider credential placeholder handling, so I do not think NemoClaw should add a second ad hoc redaction layer. The normal caveat is that user generated URL paths or query strings can appear in OpenShell audit telemetry, which is expected observability behavior rather than a new risk from this PR.
13 tasks
miyoungc
added a commit
that referenced
this pull request
Apr 30, 2026
## Summary Refreshes the daily docs from NemoClaw commits merged in the past 24 hours and advances the docs metadata from 0.0.29 to 0.0.31, the next version after tag v0.0.30. The updates cover documented behavior gaps found in the merged PRs listed below. ## Related Issue None. ## Changes - `docs/versions1.json` and `docs/project.json`: bump the preferred docs version to `0.0.31` for daily release preparation after latest tag `v0.0.30`. - `docs/reference/commands.md`: document non-interactive Brave Search validation fallback from #2511 / 9bfe30b, missing `--from <Dockerfile>` path validation from #2597 / 7186834, and `logs` reading OpenShell audit events from #2590 / e225dfb. - `docs/inference/use-local-inference.md`: document local inference reachability retry and host-side fallback from #2453 / 9dbe855, plus compatible-endpoint timeout coverage from #2583 / b4ef3db. - `docs/reference/troubleshooting.md`: document source-install shim fallback from #2520 / 01a177c, TLS gateway trust recovery from #1936 / 24725d2, compatible-endpoint timeout coverage from #2583 / b4ef3db, local reachability diagnostics from #2453 / 9dbe855, and host proxy `NO_PROXY` injection from #2662 / b4df07e. ## 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) Additional verification: - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix nemoclaw-user --dry-run` passed. - `git diff --check` passed. - Pre-push hooks passed through markdownlint, docs-to-skills, JSON checks, gitleaks, and version sync before `Test (skills YAML)` failed because this fresh worktree lacked `vitest/config`. - `npx prek run --all-files` could not run from the fresh worktree because `npx prek` resolved to a missing `prek@*` package; downloading `@j178/prek` was not approved. - `npm test` could not complete from the fresh worktree because dependencies and compiled `dist/lib/*` artifacts were absent. ## AI Disclosure - [x] AI-assisted — tool: OpenAI Codex --- Signed-off-by: Miyoung Choi <miyoungc@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Version updated to 0.0.31 * Local inference onboarding now includes retry logic for container reachability checks * Web search setup failure handling clarified with fallback guidance * Dockerfile path validation timing documented * Logging behavior clarified for concurrent stream reading * New TLS/certificate troubleshooting section added * Install path and proxy configuration troubleshooting updated <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>
DemianHeyGen
pushed a commit
to DemianHeyGen/NemoClaw
that referenced
this pull request
Apr 30, 2026
## Summary Make `nemoclaw <sandbox> logs` include OpenShell audit events so policy denials show up alongside the existing OpenClaw gateway log output. ## Related Issue Fixes NVIDIA#2512 ## Changes - Enable sandbox audit logs before reading logs. - Read both the in-sandbox `/tmp/gateway.log` source and `openshell logs <sandbox> --source all`. - In `--follow` mode, keep the remaining log source running if one source exits. - Add CLI regression coverage for both log sources and follow-mode source shutdown. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [ ] `npx prek run --all-files` passes - [ ] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [ ] 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) Additional verification: - `npm run build:cli` - `npx vitest run test/cli.test.ts -t logs` - `npm run typecheck:cli` - Pre-push hooks passed during `git push --force-with-lease` - E2E repro confirmed denied `curl https://example.com` appears in `nemoclaw repro-2512-e2e logs --follow` ## AI Disclosure - [x] AI-assisted — tool: OpenAI Codex --- Signed-off-by: Yimo Jiang <yimoj@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * `logs --follow` now streams gateway and sandbox logs concurrently and derives CLI exit status from child streams. * **Improvements** * Non-follow log collection probes gateway availability before gathering logs, with a configurable timeout and clearer “unavailable” messaging. * Per-command timeouts honored and follow lifecycle handles signals with a timed forced-exit fallback. * **Bug Fixes** * Still collects logs and emits warnings when enabling sandbox audit logging fails or times out. * **Tests / Chores** * Expanded tests for multi-source streaming, probes/timeouts, signal handling, and minor test/e2e updates. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
DemianHeyGen
pushed a commit
to DemianHeyGen/NemoClaw
that referenced
this pull request
Apr 30, 2026
## Summary Refreshes the daily docs from NemoClaw commits merged in the past 24 hours and advances the docs metadata from 0.0.29 to 0.0.31, the next version after tag v0.0.30. The updates cover documented behavior gaps found in the merged PRs listed below. ## Related Issue None. ## Changes - `docs/versions1.json` and `docs/project.json`: bump the preferred docs version to `0.0.31` for daily release preparation after latest tag `v0.0.30`. - `docs/reference/commands.md`: document non-interactive Brave Search validation fallback from NVIDIA#2511 / 9bfe30b, missing `--from <Dockerfile>` path validation from NVIDIA#2597 / 7186834, and `logs` reading OpenShell audit events from NVIDIA#2590 / e225dfb. - `docs/inference/use-local-inference.md`: document local inference reachability retry and host-side fallback from NVIDIA#2453 / 9dbe855, plus compatible-endpoint timeout coverage from NVIDIA#2583 / b4ef3db. - `docs/reference/troubleshooting.md`: document source-install shim fallback from NVIDIA#2520 / 01a177c, TLS gateway trust recovery from NVIDIA#1936 / 24725d2, compatible-endpoint timeout coverage from NVIDIA#2583 / b4ef3db, local reachability diagnostics from NVIDIA#2453 / 9dbe855, and host proxy `NO_PROXY` injection from NVIDIA#2662 / b4df07e. ## 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) Additional verification: - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix nemoclaw-user --dry-run` passed. - `git diff --check` passed. - Pre-push hooks passed through markdownlint, docs-to-skills, JSON checks, gitleaks, and version sync before `Test (skills YAML)` failed because this fresh worktree lacked `vitest/config`. - `npx prek run --all-files` could not run from the fresh worktree because `npx prek` resolved to a missing `prek@*` package; downloading `@j178/prek` was not approved. - `npm test` could not complete from the fresh worktree because dependencies and compiled `dist/lib/*` artifacts were absent. ## AI Disclosure - [x] AI-assisted — tool: OpenAI Codex --- Signed-off-by: Miyoung Choi <miyoungc@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Version updated to 0.0.31 * Local inference onboarding now includes retry logic for container reachability checks * Web search setup failure handling clarified with fallback guidance * Dockerfile path validation timing documented * Logging behavior clarified for concurrent stream reading * New TLS/certificate troubleshooting section added * Install path and proxy configuration troubleshooting updated <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Miyoung Choi <miyoungc@nvidia.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
Make
nemoclaw <sandbox> logsinclude OpenShell audit events so policy denials show up alongside the existing OpenClaw gateway log output.Related Issue
Fixes #2512
Changes
/tmp/gateway.logsource andopenshell logs <sandbox> --source all.--followmode, keep the remaining log source running if one source exits.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Additional verification:
npm run build:clinpx vitest run test/cli.test.ts -t logsnpm run typecheck:cligit push --force-with-leasecurl https://example.comappears innemoclaw repro-2512-e2e logs --followAI Disclosure
Signed-off-by: Yimo Jiang yimoj@nvidia.com
Summary by CodeRabbit
New Features
logs --follownow streams gateway and sandbox logs concurrently and derives CLI exit status from child streams.Improvements
Bug Fixes
Tests / Chores