feat(cli): add sandbox sessions subcommand group#4570
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
📝 WalkthroughWalkthroughAdds a sessions CLI surface and actions: validators, passthrough list/cleanup, gateway-backed reset, trajectory export (with optional host download), sandbox download, CLI display wiring, docs, unit tests, an E2E script, and a nightly CI job. ChangesSessions CLI feature and testing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-4570.docs.buildwithfern.com/nemoclaw |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 @.github/workflows/nightly-e2e.yaml:
- Around line 510-528: Add the new sessions-cli-e2e job to the workflow_dispatch
"Valid:" jobs list and update coderabbit path_instructions: update the
inputs.jobs.description block (the user-facing valid jobs list referenced around
inputs.jobs.description) to include "sessions-cli-e2e" so manual dispatch docs
reflect the selectable job, and add corresponding path_instructions for the
source files covered by sessions-cli-e2e in .coderabbit.yaml so the new E2E job
is represented in coderabbit's mapping.
In `@docs/reference/commands.mdx`:
- Around line 842-849: Replace the fenced block labeled as `console` with a
copyable shell fence (e.g., `bash` or `sh`) and remove the leading `$` prompt
characters from each example line so the examples like "nemoclaw my-assistant
sessions list", "nemoclaw my-assistant sessions cleanup --dry-run", "nemoclaw
my-assistant sessions rm main", "nemoclaw my-assistant sessions rm main
agent:main:telegram:thread", and "nemoclaw my-assistant sessions download main
--out ./out/" are plain, runnable commands inside the fence.
- Around line 834-841: Add explicit "###" markdown subcommand headings for the
sessions commands so the docs match the CLI help: create headings titled "###
sessions", "### sessions list", "### sessions cleanup", "### sessions rm", and
"### sessions download" and move or duplicate the corresponding table rows'
descriptions under each heading (preserve the forwarded flags and usage examples
like `sessions rm <agent> [<sessionKey>]` and `sessions download <agent>
[<sessionKey>] [--out <dir>]`) so the content under each heading exactly
documents the purpose and flags as in the table; ensure heading text matches the
subcommand names used in the help output.
In `@docs/reference/session-storage.mdx`:
- Around line 94-98: The final "## Related References" section should be
replaced with a standardized "## Next Steps" section; update the heading text
from "Related References" to "Next Steps" and keep the existing link items
(OpenClaw — Session management deep dive, Commands Reference: `nemoclaw <name>
sessions`, Best Practices: workspace and credential storage) under that new
heading so the page conforms to the guideline requiring a bottom "Next Steps"
section; ensure the link labels and destinations remain unchanged while only
changing the heading text and preserving list order.
- Around line 1-12: Add the required frontmatter fields and an explicit H1 that
matches the page title: update the top YAML/frontmatter to include topics, tags,
difficulty, audience, and status keys (populate with appropriate values for this
reference page) and add an H1 heading whose text exactly matches the title value
"Session Storage Layout" (e.g., "# Session Storage Layout") immediately after
the frontmatter; ensure the frontmatter title and the H1 are identical and that
the new keys follow the same naming/format conventions used by other docs pages.
In `@test/e2e/test-sessions-cli.sh`:
- Around line 209-214: The dependent session tests are not actually gated by
seed success; replace the current loose "seed_session || skip ..." logic with an
explicit conditional: run seed_session and if it succeeds then execute
test_sessions_list_json, test_sessions_cleanup_dry_run, test_sessions_download,
test_sessions_rm_agent, and test_sessions_list_empty_after_rm; otherwise call
skip with the existing message and do not run those test_* functions. Locate the
seed invocation (seed_session) and wrap the subsequent test function calls in an
if/then/else block (or equivalent short-circuit that prevents execution) to
ensure the dependent tests only run on successful seeding.
🪄 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: b5a923ef-f4f9-432d-990a-773c9bacb4d1
📒 Files selected for processing (20)
.github/workflows/nightly-e2e.yamldocs/index.ymldocs/reference/commands.mdxdocs/reference/session-storage.mdxsrc/commands/sandbox/sessions.tssrc/commands/sandbox/sessions/cleanup.tssrc/commands/sandbox/sessions/download.tssrc/commands/sandbox/sessions/list.tssrc/commands/sandbox/sessions/rm.tssrc/lib/actions/sandbox/sessions/download.tssrc/lib/actions/sandbox/sessions/passthrough.tssrc/lib/actions/sandbox/sessions/paths.test.tssrc/lib/actions/sandbox/sessions/paths.tssrc/lib/actions/sandbox/sessions/rm.tssrc/lib/actions/sandbox/sessions/store.test.tssrc/lib/actions/sandbox/sessions/store.tssrc/lib/cli/command-registry.test.tssrc/lib/cli/public-display-defaults.tstest/cli.test.tstest/e2e/test-sessions-cli.sh
PR Review AdvisorFindings: 3 needs attention, 5 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com> # Conflicts: # docs/reference/commands.mdx # src/lib/cli/command-registry.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/actions/sandbox/sessions/rm.ts (1)
60-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLock check is racy because probe and delete happen in separate execs.
rmcan still race an active writer if a lock appears after the probe but before the delete/update script runs. Move lock detection and deletion into one shell transaction (or re-check immediately before delete in the same script) so the refusal guarantee is actually enforced.Suggested direction
- const probe = captureOpenshell([...lock probe...]); - if (lockCount > 0 && !force) failOnActiveLocks(...); - const result = captureOpenshell([...delete script...]); + const result = captureOpenshell([...single script that: + 1) checks locks + 2) exits non-zero when locks > 0 and !force + 3) performs delete + sessions.json write + 4) prints LOCKS and REMOVED for parsing + ...]);Also applies to: 82-93, 127-139, 153-164
🤖 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/sessions/rm.ts` around lines 60 - 76, The current two-step approach using captureOpenshell (probe) then a separate delete exec is racy; move the lock detection and removal into a single shell invocation so detection and delete happen atomically. Update the code that builds the sandbox "exec" command (the captureOpenshell call that references sandboxName and sessionsDir) to perform the existence check, count locks, re-check for locks immediately before any rm/update, and abort if locks are present — i.e., combine the probe and delete logic into one shell script (or use an atomic strategy like creating a temp marker and using POSIX file tests/flock within the same command) so the refusal guarantee is enforced.
🧹 Nitpick comments (1)
docs/reference/session-storage.mdx (1)
89-96: 💤 Low valueUse fewer em dashes in this paragraph (LLM pattern detected).
This paragraph now has multiple em dashes; use commas/periods for one of them.
As per coding guidelines, “Excessive em dashes. One per paragraph is fine; multiple per paragraph ... should be flagged.”
🤖 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 `@docs/reference/session-storage.mdx` around lines 89 - 96, The paragraph describing `sessions rm` uses multiple em dashes; revise it to use at most one em dash by replacing other dashes with commas or sentence breaks, e.g., keep a single em dash for a parenthetical emphasis (if needed) and convert the clause about refusing and exiting non-zero into a separate sentence or use commas, ensure references like `sessions rm`, `*.jsonl.lock`, `--force`, and `<sessionId>.reset.<timestamp>.jsonl` remain intact and unchanged while correcting punctuation and sentence boundaries to remove the LLM-style excessive em dashes.
🤖 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.
Outside diff comments:
In `@src/lib/actions/sandbox/sessions/rm.ts`:
- Around line 60-76: The current two-step approach using captureOpenshell
(probe) then a separate delete exec is racy; move the lock detection and removal
into a single shell invocation so detection and delete happen atomically. Update
the code that builds the sandbox "exec" command (the captureOpenshell call that
references sandboxName and sessionsDir) to perform the existence check, count
locks, re-check for locks immediately before any rm/update, and abort if locks
are present — i.e., combine the probe and delete logic into one shell script (or
use an atomic strategy like creating a temp marker and using POSIX file
tests/flock within the same command) so the refusal guarantee is enforced.
---
Nitpick comments:
In `@docs/reference/session-storage.mdx`:
- Around line 89-96: The paragraph describing `sessions rm` uses multiple em
dashes; revise it to use at most one em dash by replacing other dashes with
commas or sentence breaks, e.g., keep a single em dash for a parenthetical
emphasis (if needed) and convert the clause about refusing and exiting non-zero
into a separate sentence or use commas, ensure references like `sessions rm`,
`*.jsonl.lock`, `--force`, and `<sessionId>.reset.<timestamp>.jsonl` remain
intact and unchanged while correcting punctuation and sentence boundaries to
remove the LLM-style excessive em dashes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f18f44e7-6462-4be0-92d7-1d01a8d542ad
📒 Files selected for processing (10)
.coderabbit.yaml.github/workflows/nightly-e2e.yamldocs/reference/commands.mdxdocs/reference/session-storage.mdxsrc/commands/sandbox/sessions/rm.tssrc/lib/actions/sandbox/sessions/rm.tssrc/lib/cli/command-registry.test.tssrc/lib/cli/public-display-defaults.tstest/cli.test.tstest/e2e/test-sessions-cli.sh
✅ Files skipped from review due to trivial changes (1)
- docs/reference/commands.mdx
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/nightly-e2e.yaml
- src/commands/sandbox/sessions/rm.ts
- src/lib/cli/public-display-defaults.ts
- test/e2e/test-sessions-cli.sh
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/actions/sandbox/sessions/rm.ts (1)
35-51: Run the targeted sessions E2E workflow before merge.Given this path controls lock detection and
sessions.jsonrewrites, runsessions-cli-e2eto validate end-to-end safety (list,cleanup --dry-run,download,rm) on this branch.As per coding guidelines:
src/lib/actions/sandbox/sessions/**changes should runsessions-cli-e2eviagh workflow run nightly-e2e.yaml --ref <branch> -f jobs=sessions-cli-e2e.🤖 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/sessions/rm.ts` around lines 35 - 51, This change to rmSandboxSessions (and the related functions wipeWholeAgent and removeSingleSession) affects lock detection and sessions.json rewrites for agent sessions; before merging, run the sessions E2E workflow to validate list/cleanup/download/rm flows by executing the sessions-cli-e2e job via GitHub Actions: run gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=sessions-cli-e2e and verify the sessions-cli-e2e job passes to ensure end-to-end safety for these session removal code paths.
🤖 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.
Nitpick comments:
In `@src/lib/actions/sandbox/sessions/rm.ts`:
- Around line 35-51: This change to rmSandboxSessions (and the related functions
wipeWholeAgent and removeSingleSession) affects lock detection and sessions.json
rewrites for agent sessions; before merging, run the sessions E2E workflow to
validate list/cleanup/download/rm flows by executing the sessions-cli-e2e job
via GitHub Actions: run gh workflow run nightly-e2e.yaml --ref <branch> -f
jobs=sessions-cli-e2e and verify the sessions-cli-e2e job passes to ensure
end-to-end safety for these session removal code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 46191472-17cc-4a21-8855-f0454171a453
📒 Files selected for processing (7)
.github/workflows/nightly-e2e.yamldocs/reference/session-storage.mdxsrc/lib/actions/sandbox/sessions/rm.tssrc/lib/cli/public-display-defaults.tssrc/lib/cli/public-display-layout.tssrc/lib/cli/public-display-sessions.tstest/cli.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/cli/public-display-layout.ts
- docs/reference/session-storage.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/cli/public-display-defaults.ts
- .github/workflows/nightly-e2e.yaml
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
docs/reference/commands.mdx (4)
885-885: ⚡ Quick winSplit table cell into separate sentences.
The description contains two independent clauses separated by a semicolon.
Split them for consistency with the one-sentence-per-line guideline.Suggested rewrite
-| `--reason` | `reset` (default) keeps the archive trail bound to the key; `new` registers a brand-new session id without binding the prior transcript. | +| `--reason` | `reset` (default) keeps the archive trail bound to the key. `new` registers a brand-new session id without binding the prior transcript. |As per coding guidelines, "One sentence per line in source (makes diffs readable)."
🤖 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 `@docs/reference/commands.mdx` at line 885, The table cell describing `--reason` currently uses a semicolon to join two independent clauses; replace the single semicolon-separated sentence with two sentences so each clause is on its own line. Locate the `--reason` table cell and change "reset (default) keeps the archive trail bound to the key; new registers a brand-new session id without binding the prior transcript." into two sentences like "reset (default) keeps the archive trail bound to the key. new registers a brand-new session id without binding the prior transcript." so each sentence is on its own line.
859-859: ⚡ Quick winUse active voice.
"Files ... are not touched" is passive.
Rewrite with the command as the subject.Active voice alternative
-Files whose names only share a string prefix with the resolved `sessionId` are not touched. +The command does not touch files whose names only share a string prefix with the resolved `sessionId`.As per coding guidelines, "Active voice required. Flag passive constructions."
🤖 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 `@docs/reference/commands.mdx` at line 859, Rewrite the passive sentence so the command is the subject performing the action: change "Files whose names only share a string prefix with the resolved `sessionId` are not touched." to an active-voice sentence where `sessions download` (or "the command") explicitly does not copy or touch those files; keep the same meaning and mention `sessions download`, `sessionId`, and the filename patterns (`<sessionId>.*`, `<sessionId>-topic-*`) so readers can locate the referenced symbols.
877-877: ⚡ Quick winSplit sentences onto separate lines.
Line 877 contains two sentences.
One sentence per line improves diff clarity.Suggested split
-Goes through `openshell sandbox exec` to `openclaw gateway call sessions.reset`, so the gateway owns the archival of the prior transcript (`<sessionId>.reset.<timestamp>.jsonl`), the lock, and the lifecycle event emission. The NemoClaw host never edits `sessions.json`. +Goes through `openshell sandbox exec` to `openclaw gateway call sessions.reset`, so the gateway owns the archival of the prior transcript (`<sessionId>.reset.<timestamp>.jsonl`), the lock, and the lifecycle event emission. +The NemoClaw host never edits `sessions.json`.As per coding guidelines, "One sentence per line in source (makes diffs readable)."
🤖 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 `@docs/reference/commands.mdx` at line 877, The paragraph "Goes through `openshell sandbox exec` to `openclaw gateway call sessions.reset`, so the gateway owns the archival of the prior transcript (`<sessionId>.reset.<timestamp>.jsonl`), the lock, and the lifecycle event emission. The NemoClaw host never edits `sessions.json`." should be split into two separate lines—one containing the sentence about openshell/openclaw and archival/lock/lifecycle, and the next line containing "The NemoClaw host never edits `sessions.json`"—so each sentence is on its own line to improve diff readability.
858-860: ⚡ Quick winSplit sentences onto separate lines.
Line 858 has two independent clauses separated by a semicolon, and line 859 contains two sentences.
One sentence per line makes diffs cleaner.Suggested split
Line 858:
-`sessions reset` goes through `openshell sandbox exec` to `openclaw gateway call sessions.reset`, so the OpenClaw gateway performs the archive-then-recreate; the NemoClaw host never edits `sessions.json` directly. Sessions stay lock-safe because the gateway is the writer. +`sessions reset` goes through `openshell sandbox exec` to `openclaw gateway call sessions.reset`, so the OpenClaw gateway performs the archive-then-recreate. +The NemoClaw host never edits `sessions.json` directly. +Sessions stay lock-safe because the gateway is the writer.Line 859:
-`sessions download` reads `sessions.json` over `openshell sandbox exec`, resolves the supplied `<sessionKey>` to its current `sessionId`, and copies the owned filename shapes for that session: `<sessionId>.*` (transcript, trajectory, lock, reset archives) and `<sessionId>-topic-*` (topic transcripts). Files whose names only share a string prefix with the resolved `sessionId` are not touched. +`sessions download` reads `sessions.json` over `openshell sandbox exec`, resolves the supplied `<sessionKey>` to its current `sessionId`, and copies the owned filename shapes for that session: `<sessionId>.*` (transcript, trajectory, lock, reset archives) and `<sessionId>-topic-*` (topic transcripts). +The command does not touch files whose names only share a string prefix with the resolved `sessionId`.As per coding guidelines, "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
🤖 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 `@docs/reference/commands.mdx` around lines 858 - 860, The paragraph describing the behavior of the sessions commands contains multiple sentences on the same lines; update the doc text so each sentence is on its own line for clearer diffs: split the line that mentions "sessions reset goes through openshell sandbox exec to openclaw gateway call sessions.reset, so the OpenClaw gateway performs the archive-then-recreate; the NemoClaw host never edits sessions.json directly." into separate lines for each independent clause and likewise split the line describing "sessions download reads sessions.json over openshell sandbox exec, resolves the supplied <sessionKey> to its current <sessionId>, and copies..." into one sentence per line (preserve references to sessions reset, openshell sandbox exec, openclaw gateway call sessions.reset, sessions download, sessions.json, <sessionKey>, and <sessionId> so meaning is unchanged).
🤖 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 `@test/e2e/test-sessions-cli.sh`:
- Around line 185-199: The test_sessions_list_after_reset currently only checks
for valid JSON but does not assert that the rebound session key is present;
update the function test_sessions_list_after_reset (TC-SESS-05) to parse the
JSON output of the nemoclaw "$SANDBOX_NAME" sessions list --json command and
assert that the expected rebound session key (use the actual key name used
elsewhere, e.g., rebound_session_key or the variable holding it) is present in
at least one session object, failing the test and dumping $out if the key is
missing.
---
Nitpick comments:
In `@docs/reference/commands.mdx`:
- Line 885: The table cell describing `--reason` currently uses a semicolon to
join two independent clauses; replace the single semicolon-separated sentence
with two sentences so each clause is on its own line. Locate the `--reason`
table cell and change "reset (default) keeps the archive trail bound to the key;
new registers a brand-new session id without binding the prior transcript." into
two sentences like "reset (default) keeps the archive trail bound to the key.
new registers a brand-new session id without binding the prior transcript." so
each sentence is on its own line.
- Line 859: Rewrite the passive sentence so the command is the subject
performing the action: change "Files whose names only share a string prefix with
the resolved `sessionId` are not touched." to an active-voice sentence where
`sessions download` (or "the command") explicitly does not copy or touch those
files; keep the same meaning and mention `sessions download`, `sessionId`, and
the filename patterns (`<sessionId>.*`, `<sessionId>-topic-*`) so readers can
locate the referenced symbols.
- Line 877: The paragraph "Goes through `openshell sandbox exec` to `openclaw
gateway call sessions.reset`, so the gateway owns the archival of the prior
transcript (`<sessionId>.reset.<timestamp>.jsonl`), the lock, and the lifecycle
event emission. The NemoClaw host never edits `sessions.json`." should be split
into two separate lines—one containing the sentence about openshell/openclaw and
archival/lock/lifecycle, and the next line containing "The NemoClaw host never
edits `sessions.json`"—so each sentence is on its own line to improve diff
readability.
- Around line 858-860: The paragraph describing the behavior of the sessions
commands contains multiple sentences on the same lines; update the doc text so
each sentence is on its own line for clearer diffs: split the line that mentions
"sessions reset goes through openshell sandbox exec to openclaw gateway call
sessions.reset, so the OpenClaw gateway performs the archive-then-recreate; the
NemoClaw host never edits sessions.json directly." into separate lines for each
independent clause and likewise split the line describing "sessions download
reads sessions.json over openshell sandbox exec, resolves the supplied
<sessionKey> to its current <sessionId>, and copies..." into one sentence per
line (preserve references to sessions reset, openshell sandbox exec, openclaw
gateway call sessions.reset, sessions download, sessions.json, <sessionKey>, and
<sessionId> so meaning is unchanged).
🪄 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: e628be25-aea6-4611-b7a2-104a1cfbd477
📒 Files selected for processing (10)
.coderabbit.yaml.github/workflows/nightly-e2e.yamldocs/reference/commands.mdxdocs/reference/session-storage.mdxsrc/commands/sandbox/sessions/reset.tssrc/lib/actions/sandbox/sessions/reset.tssrc/lib/cli/command-registry.test.tssrc/lib/cli/public-display-sessions.tstest/cli.test.tstest/e2e/test-sessions-cli.sh
✅ Files skipped from review due to trivial changes (2)
- src/lib/cli/command-registry.test.ts
- docs/reference/session-storage.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/cli/public-display-sessions.ts
- .coderabbit.yaml
- .github/workflows/nightly-e2e.yaml
| # ── TC-SESS-05: list after reset still surfaces the rebound key ────────────── | ||
| test_sessions_list_after_reset() { | ||
| section "TC-SESS-05: sessions list --json after reset still surfaces the key" | ||
| local out | ||
| out="$(nemoclaw "$SANDBOX_NAME" sessions list --json 2>&1)" || { | ||
| fail "TC-SESS-05: sessions list --json exited non-zero after reset" | ||
| info "$out" | ||
| return 1 | ||
| } | ||
| if ! printf '%s' "$out" | python3 -c "import json,sys; v=json.loads(sys.stdin.read())" 2>/dev/null; then | ||
| fail "TC-SESS-05: sessions list --json after reset did not return parseable JSON" | ||
| info "$out" | ||
| return 1 | ||
| fi | ||
| pass "TC-SESS-05: sessions list --json after reset returned valid JSON" |
There was a problem hiding this comment.
Assert the rebound session key in TC-SESS-05.
Line 194 only validates that output is JSON; it does not verify the rebound key is present, so this test can pass even if reset behavior regresses.
Suggested patch
test_sessions_list_after_reset() {
section "TC-SESS-05: sessions list --json after reset still surfaces the key"
local out
+ local key="agent:main:main"
out="$(nemoclaw "$SANDBOX_NAME" sessions list --json 2>&1)" || {
fail "TC-SESS-05: sessions list --json exited non-zero after reset"
info "$out"
return 1
}
- if ! printf '%s' "$out" | python3 -c "import json,sys; v=json.loads(sys.stdin.read())" 2>/dev/null; then
- fail "TC-SESS-05: sessions list --json after reset did not return parseable JSON"
+ if ! printf '%s' "$out" | python3 - "$key" <<'PY' 2>/dev/null
+import json, sys
+key = sys.argv[1]
+data = json.loads(sys.stdin.read())
+
+def has_key(node):
+ if isinstance(node, dict):
+ return node.get("key") == key or any(has_key(v) for v in node.values())
+ if isinstance(node, list):
+ return any(has_key(v) for v in node)
+ return False
+
+sys.exit(0 if has_key(data) else 1)
+PY
+ then
+ fail "TC-SESS-05: sessions list --json after reset did not include '${key}'"
info "$out"
return 1
fi
- pass "TC-SESS-05: sessions list --json after reset returned valid JSON"
+ pass "TC-SESS-05: sessions list --json after reset still includes '${key}'"
}🤖 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/e2e/test-sessions-cli.sh` around lines 185 - 199, The
test_sessions_list_after_reset currently only checks for valid JSON but does not
assert that the rebound session key is present; update the function
test_sessions_list_after_reset (TC-SESS-05) to parse the JSON output of the
nemoclaw "$SANDBOX_NAME" sessions list --json command and assert that the
expected rebound session key (use the actual key name used elsewhere, e.g.,
rebound_session_key or the variable holding it) is present in at least one
session object, failing the test and dumping $out if the key is missing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib/cli/public-display-sessions.ts`:
- Around line 35-36: Update the CLI command flags in public-display-sessions.ts
to use the canonical sessionKey placeholder: change the flags string that
currently reads "<agent> <session> [--reason new|reset]" to "<agent>
<sessionKey> [--reason new|reset]" so help text aligns with the rest of the docs
and the sessionKey terminology used elsewhere (look for the flags property on
the command definition in public-display-sessions.ts).
🪄 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: 2d59f907-879b-46fc-9a1c-3a25a8722a1b
📒 Files selected for processing (10)
.coderabbit.yaml.github/workflows/nightly-e2e.yamldocs/reference/commands.mdxdocs/reference/session-storage.mdxsrc/commands/sandbox/sessions/reset.tssrc/lib/actions/sandbox/sessions/reset.tssrc/lib/cli/command-registry.test.tssrc/lib/cli/public-display-sessions.tstest/cli.test.tstest/e2e/test-sessions-cli.sh
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/reference/commands.mdx
- .coderabbit.yaml
- src/commands/sandbox/sessions/reset.ts
- .github/workflows/nightly-e2e.yaml
- src/lib/actions/sandbox/sessions/reset.ts
- test/cli.test.ts
- test/e2e/test-sessions-cli.sh
…host Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.coderabbit.yaml (1)
261-266: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUpdate the command list to match the actual implementation.
The description mentions
rm|download(line 263-264), but the actual commands tested areresetandexport-trajectory(lines 270-272). The PR objectives confirm thatdownloadwas replaced withexport-trajectoryduring development.📝 Proposed fix
- path: "src/lib/actions/sandbox/sessions/**" instructions: &e2e-sessions-cli | This file is part of the host-side OpenClaw sessions management - surface (`nemoclaw <name> sessions list|cleanup|rm|download`). + surface (`nemoclaw <name> sessions list|cleanup|reset|export-trajectory`). Changes affect session store reads, owned-shape file matching, write-lock detection, and `sessions.json` rewrites inside a running🤖 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 @.coderabbit.yaml around lines 261 - 266, Update the documented command list under the instructions anchor (&e2e-sessions-cli) so it reflects the actual implemented commands: replace the stale "rm|download" mention with the current "reset|export-trajectory" verbs (or otherwise enumerate the real commands used by the implementation and tests) so the description matches what nemoclaw implements and the PR objectives.
🧹 Nitpick comments (5)
docs/reference/commands.mdx (4)
859-859: ⚡ Quick winFormat commands and flags as inline code.
Line 859 mentions
openclaw sessions export-trajectory --json,--save-host, andopenshell sandbox downloadwithout backticks.As per coding guidelines, "CLI commands, file paths, flags, parameter names, and values must use inline
codeformatting."Suggested fix
-`sessions export-trajectory` runs `openclaw sessions export-trajectory --json` inside the sandbox so OpenClaw owns the redaction pipeline and the on-disk bundle shape; pass `--save-host <dir>` to additionally copy the resolved bundle directory to the host via `openshell sandbox download`.(Note: Apply backticks to
openclaw sessions export-trajectory --json,--save-host <dir>, andopenshell sandbox download)🤖 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 `@docs/reference/commands.mdx` at line 859, Update the documentation sentence to format the CLI commands and flags as inline code: wrap "openclaw sessions export-trajectory --json", the flag " --save-host <dir>", and the command "openshell sandbox download" in backticks so they appear as inline code in the docs; ensure spacing and punctuation remain correct after adding backticks.
846-846: ⚡ Quick winFormat command names as inline code.
The description mentions
openclaw sessions export-trajectoryandopenshell sandbox downloadbut they are not formatted with backticks.As per coding guidelines, "CLI commands, file paths, flags, parameter names, and values must use inline
codeformatting."Suggested fix
-| `sessions export-trajectory <agent> <sessionKey> [--output <name>] [--workspace <dir>] [--save-host <dir>] [--json]` | Run openclaw sessions export-trajectory inside the sandbox and (optionally) copy the resulting bundle to the host via openshell sandbox download. | +| `sessions export-trajectory <agent> <sessionKey> [--output <name>] [--workspace <dir>] [--save-host <dir>] [--json]` | Run `openclaw sessions export-trajectory` inside the sandbox and (optionally) copy the resulting bundle to the host via `openshell sandbox download`. |🤖 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 `@docs/reference/commands.mdx` at line 846, The command descriptions need inline code formatting: edit the table row containing sessions export-trajectory and wrap the two CLI commands `openclaw sessions export-trajectory` and `openshell sandbox download` in backticks so they appear as inline code; ensure any other CLI tokens in that same line (flags like --output, --workspace, --save-host, --json are already formatted) remain unchanged.
897-897: ⚡ Quick winFormat file path as inline code.
The path
.openclaw/trajectory-exportsin the--outputflag description should use backticks.As per coding guidelines, "CLI commands, file paths, flags, parameter names, and values must use inline
codeformatting."Suggested fix
-| `--output <name>` | Bundle directory name inside the sandbox's .openclaw/trajectory-exports. OpenClaw assigns a name when omitted. | +| `--output <name>` | Bundle directory name inside the sandbox's `.openclaw/trajectory-exports`. OpenClaw assigns a name when omitted. |🤖 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 `@docs/reference/commands.mdx` at line 897, Update the `--output <name>` flag description so the file path `.openclaw/trajectory-exports` is formatted as inline code; locate the text containing `--output <name>` and replace the plain path with backticked ` .openclaw/trajectory-exports ` (i.e., `\`.openclaw/trajectory-exports\``) so it follows the guideline that CLI commands, file paths, flags, parameter names, and values use inline `code` formatting.
889-889: ⚡ Quick winFormat commands and paths as inline code.
Line 889 mentions
openclaw sessions export-trajectory --json,openshell sandbox exec, and the bundle layout.openclaw/trajectory-exports/without backticks.As per coding guidelines, "CLI commands, file paths, flags, parameter names, and values must use inline
codeformatting."Suggested fix
-Runs `openclaw sessions export-trajectory --json` inside the sandbox via `openshell sandbox exec`, so OpenClaw owns the redaction pipeline and the bundle layout (`.openclaw/trajectory-exports/<output>/`).(Note: The command, exec wrapper, and path should all have backticks)
🤖 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 `@docs/reference/commands.mdx` at line 889, Update the documentation sentence to format CLI commands, wrapper and path as inline code: wrap openclaw sessions export-trajectory --json, openshell sandbox exec, and the bundle path .openclaw/trajectory-exports/<output>/ in backticks so they appear as `openclaw sessions export-trajectory --json`, `openshell sandbox exec`, and `/.openclaw/trajectory-exports/<output>/` respectively; ensure flags and the path segment `<output>` remain inside the code spans to satisfy the guideline for commands, flags, and file paths.src/lib/cli/public-display-sessions.ts (1)
43-43: ⚡ Quick winUse canonical
<sessionKey>placeholder.Line 43 uses
<session>, but the CLI and docs describe this argument as a canonicalsessionKey(per line 838 incommands.mdx: "sessionKey arguments are the canonical keys as stored insessions.json"). Use<sessionKey>to keep help text consistent with the rest of the sessions command surface.Suggested fix
- flags: "<agent> <session> [--output <name>] [--workspace <dir>] [--save-host <dir>] [--json]", + flags: "<agent> <sessionKey> [--output <name>] [--workspace <dir>] [--save-host <dir>] [--json]",🤖 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/cli/public-display-sessions.ts` at line 43, Update the CLI help flags in src/lib/cli/public-display-sessions.ts to use the canonical placeholder '<sessionKey>' instead of '<session>' so the flags string (the flags property for the public-display-sessions command) matches the rest of the sessions surface and the documentation (commands.mdx); locate the flags definition where it reads 'flags: "<agent> <session> [...]"' and replace the '<session>' token with '<sessionKey>' preserving the rest of the flag syntax and options.
🤖 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.
Outside diff comments:
In @.coderabbit.yaml:
- Around line 261-266: Update the documented command list under the instructions
anchor (&e2e-sessions-cli) so it reflects the actual implemented commands:
replace the stale "rm|download" mention with the current
"reset|export-trajectory" verbs (or otherwise enumerate the real commands used
by the implementation and tests) so the description matches what nemoclaw
implements and the PR objectives.
---
Nitpick comments:
In `@docs/reference/commands.mdx`:
- Line 859: Update the documentation sentence to format the CLI commands and
flags as inline code: wrap "openclaw sessions export-trajectory --json", the
flag " --save-host <dir>", and the command "openshell sandbox download" in
backticks so they appear as inline code in the docs; ensure spacing and
punctuation remain correct after adding backticks.
- Line 846: The command descriptions need inline code formatting: edit the table
row containing sessions export-trajectory and wrap the two CLI commands
`openclaw sessions export-trajectory` and `openshell sandbox download` in
backticks so they appear as inline code; ensure any other CLI tokens in that
same line (flags like --output, --workspace, --save-host, --json are already
formatted) remain unchanged.
- Line 897: Update the `--output <name>` flag description so the file path
`.openclaw/trajectory-exports` is formatted as inline code; locate the text
containing `--output <name>` and replace the plain path with backticked `
.openclaw/trajectory-exports ` (i.e., `\`.openclaw/trajectory-exports\``) so it
follows the guideline that CLI commands, file paths, flags, parameter names, and
values use inline `code` formatting.
- Line 889: Update the documentation sentence to format CLI commands, wrapper
and path as inline code: wrap openclaw sessions export-trajectory --json,
openshell sandbox exec, and the bundle path
.openclaw/trajectory-exports/<output>/ in backticks so they appear as `openclaw
sessions export-trajectory --json`, `openshell sandbox exec`, and
`/.openclaw/trajectory-exports/<output>/` respectively; ensure flags and the
path segment `<output>` remain inside the code spans to satisfy the guideline
for commands, flags, and file paths.
In `@src/lib/cli/public-display-sessions.ts`:
- Line 43: Update the CLI help flags in src/lib/cli/public-display-sessions.ts
to use the canonical placeholder '<sessionKey>' instead of '<session>' so the
flags string (the flags property for the public-display-sessions command)
matches the rest of the sessions surface and the documentation (commands.mdx);
locate the flags definition where it reads 'flags: "<agent> <session> [...]"'
and replace the '<session>' token with '<sessionKey>' preserving the rest of the
flag syntax and options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ebb3500b-7a7d-4195-9e91-228638a6725e
📒 Files selected for processing (12)
.coderabbit.yaml.github/workflows/nightly-e2e.yamldocs/reference/commands.mdxdocs/reference/session-storage.mdxsrc/commands/sandbox/sessions/export-trajectory.tssrc/lib/actions/sandbox/sessions/export-trajectory.tssrc/lib/actions/sandbox/sessions/paths.test.tssrc/lib/actions/sandbox/sessions/paths.tssrc/lib/cli/command-registry.test.tssrc/lib/cli/public-display-sessions.tstest/cli.test.tstest/e2e/test-sessions-cli.sh
💤 Files with no reviewable changes (2)
- src/lib/actions/sandbox/sessions/paths.test.ts
- src/lib/actions/sandbox/sessions/paths.ts
✅ Files skipped from review due to trivial changes (1)
- docs/reference/session-storage.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/cli/command-registry.test.ts
- test/e2e/test-sessions-cli.sh
…ndling Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
| expect(fs.existsSync(hostDest)).toBe(true); | ||
| const calls = fs.readFileSync(openshellLog, "utf8"); | ||
| expect(calls).toMatch( | ||
| new RegExp(`sandbox download alpha /sandbox/workspace/notes\\.md ${hostDest.replace(/\//g, "\\/")}`), |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib/actions/sandbox/download.ts`:
- Around line 24-27: The helper downloadFromSandbox should not terminate the
process; replace the current process.exit call in the sandbox path validation
with a thrown error so the CLI boundary can decide exit behavior. In the
downloadFromSandbox function, where you check opts.sandboxPath (the block that
currently does console.error(...) then process.exit(1)), remove the
process.exit(1) and instead throw a descriptive Error (e.g., "No sandbox path
provided; refusing to invoke `openshell sandbox download`.") so upstream callers
can handle/log/exit as appropriate.
🪄 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: 3c06c355-6484-4456-af4f-c54e569ff41e
📒 Files selected for processing (11)
.coderabbit.yamldocs/reference/commands.mdxdocs/reference/session-storage.mdxsrc/commands/sandbox/download.tssrc/lib/actions/sandbox/download.tssrc/lib/actions/sandbox/sessions/paths.tssrc/lib/actions/sandbox/sessions/reset.tssrc/lib/cli/command-registry.test.tssrc/lib/cli/public-display-defaults.tssrc/lib/cli/public-display-sessions.tstest/cli.test.ts
✅ Files skipped from review due to trivial changes (2)
- docs/reference/commands.mdx
- docs/reference/session-storage.mdx
🚧 Files skipped from review as they are similar to previous changes (4)
- .coderabbit.yaml
- src/lib/cli/public-display-sessions.ts
- src/lib/actions/sandbox/sessions/reset.ts
- src/lib/cli/public-display-defaults.ts
| if (!opts.sandboxPath || opts.sandboxPath.trim().length === 0) { | ||
| console.error(" No sandbox path provided; refusing to invoke `openshell sandbox download`."); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Avoid terminating the process inside the action helper.
downloadFromSandbox() should throw and let the CLI boundary decide exit behavior; process.exit(1) here can short-circuit upstream handling and reuse paths.
Proposed fix
if (!opts.sandboxPath || opts.sandboxPath.trim().length === 0) {
- console.error(" No sandbox path provided; refusing to invoke `openshell sandbox download`.");
- process.exit(1);
+ throw new Error("No sandbox path provided; refusing to invoke `openshell sandbox download`.");
}Based on learnings: "avoid adding defensive error handling around internal helper logic... only add validation/error handling at system boundaries."
📝 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.
| if (!opts.sandboxPath || opts.sandboxPath.trim().length === 0) { | |
| console.error(" No sandbox path provided; refusing to invoke `openshell sandbox download`."); | |
| process.exit(1); | |
| } | |
| if (!opts.sandboxPath || opts.sandboxPath.trim().length === 0) { | |
| throw new Error("No sandbox path provided; refusing to invoke `openshell sandbox download`."); | |
| } |
🤖 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/download.ts` around lines 24 - 27, The helper
downloadFromSandbox should not terminate the process; replace the current
process.exit call in the sandbox path validation with a thrown error so the CLI
boundary can decide exit behavior. In the downloadFromSandbox function, where
you check opts.sandboxPath (the block that currently does console.error(...)
then process.exit(1)), remove the process.exit(1) and instead throw a
descriptive Error (e.g., "No sandbox path provided; refusing to invoke
`openshell sandbox download`.") so upstream callers can handle/log/exit as
appropriate.
Summary
Adds a
nemoclaw <name> sessions {list,cleanup,rm,download}host-side subcommand group that wraps OpenClaw's in-sandboxsessions list/sessions cleanupand adds NemoClaw-siderm(per-key or whole-agent wipe) anddownload(per-key or whole-agent host copy). All in-sandbox work goes throughopenshell sandbox exec. Documents the on-disk OpenClaw session storage layout in a newSession Storage Layoutreference page so audit/replay tooling no longer has to reverse-engineer the paths.Related Issue
Resolves #834.
Fixes #3978.
Resolves #3979.
Changes
src/lib/actions/sandbox/sessions/: new action module split intopaths.ts(path resolution + agent-id, session-key, session-id validators),store.ts(sessions.jsonparse + key→sessionId resolver),passthrough.ts(forwards toopenclaw sessions [verb] [args...]viaopenshell sandbox exec),rm.ts(whole-agent and per-key wipe; resets/editssessions.jsonin place),download.ts(whole-agent or per-key host copy viaopenshell sandbox download).src/commands/sandbox/sessions.ts+src/commands/sandbox/sessions/{list,cleanup,rm,download}.ts: five new oclif command adapters. Pass-through verbs skipthis.parse()so arbitrary OpenClaw flags forward verbatim;rmanddownloaduse strict args with<agent>required and[<session>]optional plus--out <dir>ondownload.src/lib/actions/sandbox/sessions/{paths,store}.test.ts: 15 unit tests covering path resolution, agent-id / session-key / session-id validation rejection of shell metacharacters, andsessions.jsonparse + key resolution edge cases.test/cli.test.ts: four CLI integration tests covering the pass-through wiring (sessions list --jsonreachesopenshell sandbox exec --name <name> -- openclaw sessions list --json), the whole-agent wipe, the per-key wipe (withsessions.jsonread + edit), and the unknown-key error message.test/e2e/test-sessions-cli.sh+.github/workflows/nightly-e2e.yamlsessions-cli-e2ejob: end-to-end coverage that onboards a sandbox, seeds a session, exercisessessions list,sessions cleanup --dry-run,sessions download,sessions rm, then re-asserts the list is empty.docs/reference/session-storage.mdx+docs/index.yml: new reference page documenting the per-agentsessions/layout (sessions.json,<sessionId>.jsonl,.trajectory.jsonl,.jsonl.lock,-topic-,.reset.<ts>.), what each file is for, and which commands consume them. Closes the documentation gap in [docs] Agent session JSONL / trajectory paths are stable but undocumented #3978.docs/reference/commands.mdx: new### nemoclaw <name> sessionssection with the five subcommand grammar, example invocations, and the Gateway-idleness caveat forrm.src/lib/cli/public-display-defaults.ts+src/lib/cli/command-registry.test.ts: register display metadata for the five new sandbox commands and update the hardcoded command-count assertions (63→68, 35→40, 51→56, 23→24 action tokens).Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
Documentation
Tests