Skip to content

fix(cli): recover live sandboxes in list when local registry is empty - #5786

Merged
cv merged 8 commits into
mainfrom
fix/5714-list-registry-recovery
Jun 26, 2026
Merged

cv merged 8 commits into
mainfrom
fix/5714-list-registry-recovery

Conversation

@yimoj

@yimoj yimoj commented Jun 25, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

nemoclaw list printed "No sandboxes registered" while the live OpenShell gateway and container were healthy and nemoclaw <name> status reported Phase: Ready (reported on DGX Station with a Deep Agents sandbox). The list recovery path required a seed — an existing registry entry, an onboard session, or an explicit requested name — before it would probe the gateway. After a local registry loss with no seed, it trusted the empty registry and reported nothing, even though the named-status path could find and reconcile the same sandbox.

Related Issue

Fixes #5714

Changes

  • Widen list recovery for an empty registry. recoverRegistryEntries now attempts recovery whenever the local registry is empty, even with no session/requested-name seed.
  • Bounded, read-only, non-mutating gateway inspection for the unseeded case. Plain nemoclaw list never selects or starts a gateway. It inspects the live sandbox list only when OpenShell is connected to a NemoClaw-managed gateway (nemoclaw or a per-port nemoclaw-<port>) — never a foreign OpenShell gateway. Gateway probes are non-fatal (ignoreProbeErrors), so a hung/timed-out gateway falls back to the empty registry instead of exiting the process, and recovery is gated on a clean (status === 0) sandbox list so error text is never parsed as a sandbox name.
  • Display-only recovery — recovered entries are NOT persisted. The global openshell sandbox list exposes only NAME/CREATED/PHASE, not the agent or gateway binding. Persisting a recovered entry would default agent to openclaw everywhere downstream (state dirs, connect, rebuild, doctor) and permanently misclassify a Deep Agents/Hermes sandbox. Recovered sandboxes are surfaced for the current list only; follow-up named commands reconcile the real agent via the gateway.
  • Honest rendering. Recovered rows show agent/model/provider/GPU as unknown rather than inventing OpenClaw/CPU defaults.
  • getNamedGatewayLifecycleState gains an opt-in ignoreProbeErrors (default keeps existing fatal behavior); captureOpenshell forwards includeStderr so non-fatal probes still capture the gateway status text.

Type of Change

  • Code change (feature, bug fix, or refactor)

Verification

Verified end-to-end through the real worktree CLI (node ./bin/nemoclaw.js …) against a live OpenShell gateway. The local registry was emptied and the onboard session removed to simulate the reporter's registry loss; the on-disk registry is checked after each run.

1. Pre-fix (reporter mismatch) — list blind while a Ready sandbox is live:

$ node ./bin/nemoclaw.js list

  No sandboxes registered. Run `nemoclaw onboard` to get started.

$ node ./bin/nemoclaw.js sbox-4778 status
  Sandbox: sbox-4778
    Model:    nvidia/nemotron-3-super-120b-a12b
    Provider: nvidia-prod
    ...
  Phase: Ready
# docker ps -a → openshell-sbox-4778-… Up   (container healthy, gateway Connected)

2. Post-fix — list rediscovers the live sandboxes (display-only, registry stays empty):

$ node ./bin/nemoclaw.js list

  Recovered 5 sandbox entries from the live OpenShell gateway.

  Sandboxes:
    probe3014x
      agent: unknown  model: unknown  provider: unknown  GPU: unknown  policies: none
    e2e-5468-ollama
      agent: unknown  model: unknown  provider: unknown  GPU: unknown  policies: none
    issue4538-fix
      agent: unknown  model: unknown  provider: unknown  GPU: unknown  policies: none
    sbox-4778
      agent: unknown  model: unknown  provider: unknown  GPU: unknown  policies: none
    dcode5744
      agent: unknown  model: unknown  provider: unknown  GPU: unknown  policies: none

$ node -e 'console.log(Object.keys(require(process.env.HOME+"/.nemoclaw/sandboxes.json").sandboxes))'
[]        # recovered for display only — NOT persisted (agent is unknowable from the gateway list)

(Recovery was also re-verified while OpenShell was connected to a NemoClaw per-port gateway nemoclaw-8092 — connected_other to a NemoClaw-managed name still recovers; a foreign gateway name does not.)

3. Post-fix graceful degradation — gateway unreachable, empty registry → no crash, no bogus names:

$ node ./bin/nemoclaw.js list      # gateway down: Connection refused on :8080
                                   # `openshell sandbox list` → transport error
  No sandboxes registered. Run `nemoclaw onboard` to get started.
$ echo $?
0

Targeted + adjacent unit tests pass (registry-recovery, gateway-runtime, inventory, openshell adapter, repro-2666, gateway/connect drift; ~290 tests), npm run typecheck:cli clean, Biome clean; nemoclaw-start (126) passes with the nemoclaw/ subproject deps installed.

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
  • Targeted tests pass for changed behavior
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed

Signed-off-by: Yimo Jiang yimoj@nvidia.com

Summary by CodeRabbit

  • New Features
    • Improved recovery for empty registries with unseeded sessions via bounded, read-only ephemeral results.
    • Inventory rendering now supports gateway-recovered sandboxes (uses unknown agent/GPU and shows phase when available).
    • Added live sandbox phase parsing with parseLiveSandboxEntries.
  • Bug Fixes
    • Added stderr retention for lifecycle/probe classification (includeStderr) when probe errors are ignored.
    • Prevented transient recovery/display markers (recoveredFromGateway, livePhase) from persisting to disk.
  • Tests
    • Added cases for unseeded vs seeded recovery, probe option behavior, and phase parsing variations.

Supersedes #5771 (reopened from NVIDIA/NemoClaw branch so trusted advisor workflows can run).

yimoj added 3 commits June 25, 2026 05:53
`nemoclaw list` printed "No sandboxes registered" while the live gateway
and container were healthy and `nemoclaw <name> status` reported Ready,
because the list recovery path required a seed (existing registry entry,
onboard session, or requested name) before probing the gateway. After a
local registry loss with no seed, it trusted the empty registry.

Widen recovery so an empty registry always attempts a bounded, read-only
live-gateway inspection — no gateway select/start — whenever OpenShell is
connected to a NemoClaw-managed gateway (`nemoclaw` or `nemoclaw-<port>`),
never a foreign one. Recovered sandboxes are surfaced display-only and are
NOT persisted: the global `openshell sandbox list` exposes only
NAME/CREATED/PHASE, so a persisted entry would default agent to "openclaw"
everywhere downstream and permanently misclassify a Deep Agents/Hermes
sandbox. The renderer shows agent/model/provider/GPU as "unknown" for these
rows rather than inventing OpenClaw/CPU defaults. Gateway probes are
non-fatal (ignoreProbeErrors) so a hung gateway falls back to the empty
registry, and recovery is gated on a clean (status 0) `sandbox list` so
error text is never parsed as sandbox names.

Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Add TSDoc summaries to the functions introduced/changed for #5714
(registry recovery, gateway lifecycle classification, OpenShell capture,
and inventory row projection) to satisfy the CodeRabbit docstring-coverage
pre-merge check. No behavior change.

Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Add TSDoc to the OpenShell runtime adapter helpers and the gateway
status-parsing helpers in the files touched by #5714 to clear the
CodeRabbit docstring-coverage threshold (80%). No behavior change.

Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
@github-code-quality

github-code-quality Bot commented Jun 25, 2026 •

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the fix/5714-list-regist... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/5714-list-regist... 0935ea9 +/-
nemoclaw/src/se...cret-scanner.ts — 100% —
nemoclaw/src/commands/slash.ts — 100% —
nemoclaw/src/li...bprocess-env.ts — 100% —
nemoclaw/src/bl...eprint/state.ts — 98% —
nemoclaw/src/onboard/config.ts — 98% —
nemoclaw/src/bl...int/snapshot.ts — 97% —
nemoclaw/src/bl...print/runner.ts — 95% —
nemoclaw/src/co...ration-state.ts — 94% —
nemoclaw/src/bl...ate-networks.ts — 94% —
nemoclaw/src/index.ts — 94% —

TypeScript / code-coverage/cli

The overall coverage in the fix/5714-list-regist... branch is 47%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/5714-list-regist... 0935ea9 +/-
src/lib/state/o...oard-session.ts — 91% —
src/lib/inference/local.ts — 76% —
src/lib/sandbox/config.ts — 72% —
src/lib/actions...dbox/rebuild.ts — 71% —
src/lib/onboard/preflight.ts — 64% —
src/lib/actions...licy-channel.ts — 60% —
src/lib/state/sandbox.ts — 55% —
src/lib/policy/index.ts — 49% —
src/lib/onboard...er-gpu-patch.ts — 44% —
src/lib/onboard.ts — 19% —

Updated June 25, 2026 19:01 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@coderabbitai

coderabbitai Bot commented Jun 25, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8a9d526e-a82b-4a8c-a1c0-52210287b305

📥 Commits

Reviewing files that changed from the base of the PR and between faab155 and 0935ea9.

📒 Files selected for processing (2)
  • src/lib/runtime-recovery.ts
  • src/lib/state/registry.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/lib/state/registry.ts
  • src/lib/runtime-recovery.ts

📝 Walkthrough

Walkthrough

The PR adds live gateway recovery for empty registry listings, preserves gateway-recovered markers outside disk storage, and updates inventory output to show unknown agent, GPU, and phase values for recovered sandboxes.

Changes

Gateway recovery and inventory display

Layer / File(s) Summary
OpenShell probe plumbing
src/lib/adapters/openshell/runtime.ts, src/lib/gateway-runtime-action.ts, src/lib/gateway-runtime-action.test.ts
OpenShell capture accepts includeStderr, gateway lifecycle probes can ignore probe errors while keeping stderr, and tests cover the probe option behavior.
Live gateway recovery flow
src/lib/runtime-recovery.ts, src/lib/runtime-recovery.test.ts, src/lib/registry-recovery-action.ts, src/lib/registry-recovery-action.test.ts
Live sandbox list parsing now extracts phases, and registry recovery uses read-only or seeded gateway inspection to return persisted entries plus ephemeral recovered sandboxes.
Recovered sandbox marker and inventory output
src/lib/state/registry.ts, src/lib/inventory/index.ts, src/lib/inventory/index.test.ts, test/registry.test.ts
Recovered sandbox entries keep transient display markers out of disk storage, and inventory rendering uses recovered-from-gateway state to show unknown agent, GPU, and phase values.

Sequence Diagram(s)

sequenceDiagram
  participant recoverRegistryEntries
  participant recoverRegistryFromLiveGateway
  participant getNamedGatewayLifecycleState
  participant captureOpenshell
  participant parseLiveSandboxEntries

  recoverRegistryEntries->>recoverRegistryFromLiveGateway: readOnly: !hasRecoverySeed
  recoverRegistryFromLiveGateway->>getNamedGatewayLifecycleState: ignoreProbeErrors: true
  getNamedGatewayLifecycleState->>captureOpenshell: includeStderr: true
  captureOpenshell-->>getNamedGatewayLifecycleState: stderr with Status:/Gateway:
  getNamedGatewayLifecycleState-->>recoverRegistryFromLiveGateway: lifecycle state
  recoverRegistryFromLiveGateway->>parseLiveSandboxEntries: sandbox list output
  parseLiveSandboxEntries-->>recoverRegistryFromLiveGateway: {name, phase}[]
  recoverRegistryFromLiveGateway-->>recoverRegistryEntries: recoveredFromGateway + ephemeralSandboxes
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#5802: Both PRs modify src/lib/adapters/openshell/runtime.ts to extend RunnerOptions with stderr-capture support and forward includeStderr into captureOpenshellCommand.

Suggested reviewers

  • jyaunches
  • cv

Poem

A bunny peeked through gateway dew,
And found the list now telling true.
With phases, paws, and unknown gleam,
The sands came back from live-streamed dream. 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: recovering live sandboxes in list when the local registry is empty.
Linked Issues check ✅ Passed The PR addresses #5714 by probing NemoClaw-managed gateways for an empty registry and rendering recovered sandboxes in list output.
Out of Scope Changes check ✅ Passed The changes stay focused on live sandbox recovery, probe handling, and display-only inventory rendering, with no obvious unrelated additions.
Docstring Coverage ✅ Passed Docstring coverage is 91.18% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/5714-list-registry-recovery

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jun 25, 2026 •

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: sandbox-survival-vitest, gateway-guard-recovery, gateway-health-honest-vitest
Optional E2E: cloud-onboard-vitest, concurrent-gateway-ports-vitest, onboard-resume-vitest, onboard-repair-vitest

Dispatch hint: sandbox-survival-vitest,gateway-guard-recovery,gateway-health-honest-vitest

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • sandbox-survival-vitest (medium): Required because this PR changes gateway/runtime recovery and registry state behavior that can affect sandbox survival across gateway restart and live inference after recovery.
  • gateway-guard-recovery (medium): Required because the PR changes gateway recovery classification and read-only versus mutating recovery paths. This recovery/disruption job exercises real gateway recovery against an onboarded sandbox.
  • gateway-health-honest-vitest (low): Required because getNamedGatewayLifecycleState probe fatality, stderr capture, and unhealthy gateway classification changed. This job validates host CLI gateway health reporting and non-happy-path status behavior.

Optional E2E

  • cloud-onboard-vitest (high): Useful baseline for a clean hosted onboarding plus real registry/gateway/list/status flow after changes to registry serialization and recovery. Not strictly required because the PR does not modify the onboard state machine itself.
  • concurrent-gateway-ports-vitest (high): Useful because the named gateway lifecycle code is per-gateway/per-port sensitive, and the PR changes active-gateway classification and recovery probe options.
  • onboard-resume-vitest (medium): Adjacent confidence for confirmed versus incomplete onboard session handling, since registry recovery now uses onboard session completion state as a seed gate.
  • onboard-repair-vitest (medium): Adjacent confidence for repair behavior when local state/session metadata is stale or incomplete. The mandatory resume/repair rule is not triggered because this PR does not touch the onboard machine live-slice orchestration.

New E2E recommendations

  • empty-registry live gateway recovery (high): No existing E2E appears to directly cover the [DGX Station][CLI&UX] nemoclaw list shows "No sandboxes registered" while nemoclaw <name> status reports Phase=Ready and docker ps confirms the container is Up #5714 acceptance path: onboard a sandbox, remove or empty ~/.nemoclaw/sandboxes.json and ensure plain nemoclaw list performs read-only live gateway discovery, shows the sandbox with live phase Ready, renders agent/GPU as unknown, and does not persist transient recovery markers.
    • Suggested test: Add a live Vitest job/scenario such as empty-registry-live-gateway-recovery under test/e2e-scenario/live/.
  • non-fatal gateway probe timeout during list recovery (medium): The PR relies on ignoreProbeErrors plus stderr capture to keep nemoclaw list from exiting when openshell status or gateway info hangs/fails. Existing E2E coverage does not appear to simulate a timed-out or failing OpenShell status probe while verifying list falls back safely.
    • Suggested test: Add a lightweight live or shimmed CLI E2E that injects a failing/timed-out openshell status during nemoclaw list and asserts no process exit, no bogus sandbox names parsed from error text, and actionable output.

Dispatch hint

  • Workflow: .github/workflows/e2e-vitest-scenarios.yaml
  • jobs input: sandbox-survival-vitest,gateway-guard-recovery,gateway-health-honest-vitest

@github-actions

github-actions Bot commented Jun 25, 2026 •

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: sandbox-survival-vitest, onboard-resume-vitest, onboard-repair-vitest
Optional Vitest E2E scenarios: double-onboard-vitest

Dispatch required Vitest E2E scenarios:

  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=sandbox-survival-vitest
  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=onboard-resume-vitest
  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=onboard-repair-vitest

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • sandbox-survival-vitest: Exercises the changed live registry/list/status and OpenShell gateway-restart boundaries, including nemoclaw list, nemoclaw <name> status, local registry preservation, OpenShell sandbox listing, and cleanup after destroy.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=sandbox-survival-vitest
  • onboard-resume-vitest: Required by the onboarding resume compatibility rule because the PR changes registry/session recovery behavior for persisted onboard-session state and confirmed vs incomplete session handling.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=onboard-resume-vitest
  • onboard-repair-vitest: Required with resume for persisted-session recovery paths because these changes can affect repair/backstop execution and registry recovery from existing onboard state.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=onboard-repair-vitest

Optional Vitest E2E scenarios

  • double-onboard-vitest: Adjacent coverage for multi-sandbox gateway reuse, stale registry preservation, nemoclaw list, and recovery-oriented status/connect/rebuild behavior touched by the gateway and registry-recovery changes.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=double-onboard-vitest

Relevant changed files

  • src/lib/adapters/openshell/runtime.ts
  • src/lib/gateway-runtime-action.ts
  • src/lib/inventory/index.ts
  • src/lib/registry-recovery-action.ts
  • src/lib/runtime-recovery.ts
  • src/lib/state/registry.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
src/lib/gateway-runtime-action.test.ts (1)

81-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the stderr-capture contract in these probe tests.

These cases only verify ignoreError. The production path in getNamedGatewayLifecycleState() also depends on includeStderr staying in lockstep with ignoreProbeErrors; otherwise the lifecycle parser loses the Status:/Gateway: lines and these tests would still pass.

Suggested assertion update
     it("keeps probes fatal (no ignoreError) by default", () => {
       captureSpy.mockReturnValue({ status: 0, output: "Status: Connected\nGateway: nemoclaw\n" });

       gatewayRuntime.getNamedGatewayLifecycleState("nemoclaw");

       for (const [, opts] of captureSpy.mock.calls) {
         expect(opts?.ignoreError).not.toBe(true);
+        expect(opts?.includeStderr).not.toBe(true);
       }
     });

     it("makes probes non-fatal when ignoreProbeErrors is set (`#5714`)", () => {
@@
       expect(captureSpy.mock.calls.length).toBeGreaterThanOrEqual(2);
       for (const [, opts] of captureSpy.mock.calls) {
         expect(opts?.ignoreError).toBe(true);
+        expect(opts?.includeStderr).toBe(true);
       }
     });
🤖 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/gateway-runtime-action.test.ts` around lines 81 - 103, Update the
probe tests for getNamedGatewayLifecycleState so they also assert the
stderr-capture behavior, not just ignoreError. In the default case, verify the
capture calls keep includeStderr enabled alongside the existing no-ignoreError
expectation, and in the ignoreProbeErrors path verify every capture call has
both ignoreError true and includeStderr true. Use the existing
gatewayRuntime.getNamedGatewayLifecycleState and captureSpy assertions to keep
the probe contract locked in.
src/lib/registry-recovery-action.test.ts (1)

245-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the transient recovery marker on the returned sandbox.

This test proves the sandbox is surfaced, but it does not verify that recoverRegistryEntries() sets recoveredFromGateway: true. That marker is what drives the downstream inventory renderer away from the openclaw/CPU sandbox defaults, and the current inventory tests synthesize it manually.

Suggested assertion update
     const result = await recoverRegistryEntries();

     expect(result.recoveredFromGateway).toBe(1);
     const recovered = result.sandboxes.find((s) => s.name === "dcode-station");
     expect(recovered).toBeDefined();
+    expect(recovered?.recoveredFromGateway).toBe(true);
     // Minimal safe entry: no invented agent/model/provider metadata.
     expect(recovered?.model).toBeNull();
     expect(recovered?.provider).toBeNull();
     expect(recovered?.agent).toBeUndefined();
🤖 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/registry-recovery-action.test.ts` around lines 245 - 261, The
`recoverRegistryEntries()` test covers rediscovery of the live sandbox, but it
does not assert the transient recovery marker that downstream rendering depends
on. Update the test around `recoverRegistryEntries`, `result.sandboxes`, and the
recovered `"dcode-station"` entry to verify the returned sandbox is marked as
recovered from the gateway (for example, by asserting the recovery flag is true
on that sandbox or its equivalent field in the recovered entry). Keep the
existing assertions for minimal metadata and add the missing check using the
same symbols so the behavior is validated end to end.
🤖 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/gateway-runtime-action.test.ts`:
- Around line 81-103: Update the probe tests for getNamedGatewayLifecycleState
so they also assert the stderr-capture behavior, not just ignoreError. In the
default case, verify the capture calls keep includeStderr enabled alongside the
existing no-ignoreError expectation, and in the ignoreProbeErrors path verify
every capture call has both ignoreError true and includeStderr true. Use the
existing gatewayRuntime.getNamedGatewayLifecycleState and captureSpy assertions
to keep the probe contract locked in.

In `@src/lib/registry-recovery-action.test.ts`:
- Around line 245-261: The `recoverRegistryEntries()` test covers rediscovery of
the live sandbox, but it does not assert the transient recovery marker that
downstream rendering depends on. Update the test around
`recoverRegistryEntries`, `result.sandboxes`, and the recovered
`"dcode-station"` entry to verify the returned sandbox is marked as recovered
from the gateway (for example, by asserting the recovery flag is true on that
sandbox or its equivalent field in the recovered entry). Keep the existing
assertions for minimal metadata and add the missing check using the same symbols
so the behavior is validated end to end.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 103a3188-3c48-450a-a244-62363abf922b

📥 Commits

Reviewing files that changed from the base of the PR and between e3b8325 and 9f20898.

📒 Files selected for processing (8)
  • src/lib/adapters/openshell/runtime.ts
  • src/lib/gateway-runtime-action.test.ts
  • src/lib/gateway-runtime-action.ts
  • src/lib/inventory/index.test.ts
  • src/lib/inventory/index.ts
  • src/lib/registry-recovery-action.test.ts
  • src/lib/registry-recovery-action.ts
  • src/lib/state/registry.ts

@github-actions

github-actions Bot commented Jun 25, 2026 •

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Recovered list rows still do not show the literal Deep Agents agent requested by #5714.
Open items: 0 required · 1 warning · 0 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 1 still applies · 0 new items found

Action checklist

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify acceptance src/lib/inventory/index.ts:331 Either add a safe authoritative agent source for display-only recovered rows, or explicitly document/accept the narrowed behavior that unseeded `list` can show only name and phase while leaving agent/GPU unknown until a follow-up named command reconciles authoritative metadata.
Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Recovered list rows still do not show the literal Deep Agents agent requested by #5714

  • Location: src/lib/inventory/index.ts:331
  • Category: acceptance
  • Problem: The linked issue's Expected Result says `nemoclaw list` should show `dcode-station` Ready with agent `langchain-deepagents-code`. This patch correctly avoids inventing an OpenClaw default and now carries the live phase, but gateway-recovered rows still render `agent: unknown` because the live OpenShell sandbox list is not treated as an authoritative agent source.
  • Impact: Users regain discoverability and phase consistency, but the literal agent portion of [DGX Station][CLI&UX] nemoclaw list shows "No sandboxes registered" while nemoclaw <name> status reports Phase=Ready and docker ps confirms the container is Up #5714 remains unmet unless maintainers explicitly accept the narrower safe behavior. Without that acceptance or a user-facing explanation, users may still expect `list` to identify a Deep Agents/Hermes sandbox after registry loss.
  • Recommended action: Either add a safe authoritative agent source for display-only recovered rows, or explicitly document/accept the narrowed behavior that unseeded `list` can show only name and phase while leaving agent/GPU unknown until a follow-up named command reconciles authoritative metadata.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `src/lib/registry-recovery-action.ts` around the read-only `ephemeralSandboxes` construction and `src/lib/inventory/index.ts` around `agent: sandbox.agent || (sandbox.recoveredFromGateway ? "unknown" : null)` plus the render line that prints `agent: ${agent}`.
  • Missing regression test: Existing tests already prove the narrowed behavior: `list displays recovered sandbox Ready phase and authoritative agent when available, otherwise documents unknown fallback`, `shows agent as 'unknown' for a gateway-recovered sandbox ([DGX Station][CLI&UX] nemoclaw list shows "No sandboxes registered" while nemoclaw <name> status reports Phase=Ready and docker ps confirms the container is Up #5714)`, and `renders a gateway-recovered row with the trusted live phase but unknown agent/GPU ([DGX Station][CLI&UX] nemoclaw list shows "No sandboxes registered" while nemoclaw <name> status reports Phase=Ready and docker ps confirms the container is Up #5714)`. If an authoritative agent lookup or user-facing note is added, add `list displays authoritative recovered Deep Agents agent when safe source is available` or `list explains unknown agent for display-only gateway recovery`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `src/lib/registry-recovery-action.ts` around the read-only `ephemeralSandboxes` construction and `src/lib/inventory/index.ts` around `agent: sandbox.agent || (sandbox.recoveredFromGateway ? "unknown" : null)` plus the render line that prints `agent: ${agent}`.
  • Evidence: Issue [DGX Station][CLI&UX] nemoclaw list shows "No sandboxes registered" while nemoclaw <name> status reports Phase=Ready and docker ps confirms the container is Up #5714 Expected Result: "`nemoclaw list` shows `dcode-station` Ready with agent `langchain-deepagents-code`, consistent with both `nemoclaw dcode-station status` (Phase=Ready) and the running Docker container." Current code renders gateway-recovered rows with `agent: unknown` while carrying `livePhase: "Ready"`.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

  • None.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Validate `nemoclaw list` against a live healthy named OpenShell gateway with empty `sandboxes.json` and no onboard session: it prints the live sandbox name with `phase: Ready`, `agent: unknown`, `GPU: unknown`, and leaves `sandboxes.json` empty.. Unit and negative-path coverage is strong and paired with changed source files, but this PR changes sandbox/gateway recovery behavior that depends on real OpenShell lifecycle, process timeouts, stderr/stdout behavior, and live sandbox-list formatting.
  • PRA-T2 Runtime validation — Validate a live `connected_other` or foreign gateway with an empty registry: `nemoclaw list` does not surface that gateway's sandboxes and does not select or start a gateway.. Unit and negative-path coverage is strong and paired with changed source files, but this PR changes sandbox/gateway recovery behavior that depends on real OpenShell lifecycle, process timeouts, stderr/stdout behavior, and live sandbox-list formatting.
  • PRA-T3 Runtime validation — Validate a live probe-failure or timeout path: `nemoclaw list` falls back to the empty-state message without process exit and without parsing stderr/error text as a sandbox name.. Unit and negative-path coverage is strong and paired with changed source files, but this PR changes sandbox/gateway recovery behavior that depends on real OpenShell lifecycle, process timeouts, stderr/stdout behavior, and live sandbox-list formatting.
  • PRA-T4 Runtime validation — Add or identify a parser regression `parseLiveSandboxEntries prefers the actual/trailing PHASE column over earlier phase-like metadata columns` if OpenShell can emit additional columns before PHASE.. Unit and negative-path coverage is strong and paired with changed source files, but this PR changes sandbox/gateway recovery behavior that depends on real OpenShell lifecycle, process timeouts, stderr/stdout behavior, and live sandbox-list formatting.
  • PRA-T5 Acceptance clause — On a DGX Station host (aarch64) running NemoClaw v0.0.67, `nemoclaw list` prints "No sandboxes registered" while `nemoclaw dcode-station status` reports the same sandbox as `Phase: Ready` and `docker ps` shows the sandbox container `Up 17 minutes`. — add test evidence or identify existing coverage. `recoverRegistryEntries()` now attempts recovery when `current.sandboxes.length === 0`, and the read-only path can surface live OpenShell rows with `livePhase`. The patch does not inspect Docker directly.
  • PRA-T6 Acceptance clause — `nemoclaw list` shows `dcode-station` Ready with agent `langchain-deepagents-code`, consistent with both `nemoclaw dcode-station status` (Phase=Ready) and the running Docker container. — add test evidence or identify existing coverage. The `Ready` phase is carried through `parseLiveSandboxEntries()` to `livePhase` and rendered as `phase: Ready`. The literal agent is intentionally not shown; changed tests assert `agent: unknown` for gateway-recovered rows.
  • PRA-T7 Acceptance clause — Actual Result: `$ docker ps` ... `openshell-dcode-station-f9a5aa4b-bd78-4aff-b181-0c64064fc4a3 Up 17 minutes openshell/sandbox-from:1782277000` — add test evidence or identify existing coverage. The PR does not parse Docker container state. It uses `openshell sandbox list` as the trusted source for live name/phase and deliberately avoids deriving agent/GPU/image metadata from Docker names or images.
  • PRA-T8 Acceptance clause — Environment: `Agent: langchain-deepagents-code (LangChain Deep Agents Code)` — add test evidence or identify existing coverage. The changed code avoids misclassifying a Deep Agents sandbox as OpenClaw by rendering `agent: unknown`, but it does not recover or display `langchain-deepagents-code` from an authoritative source.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Recovered list rows still do not show the literal Deep Agents agent requested by #5714

  • Location: src/lib/inventory/index.ts:331
  • Category: acceptance
  • Problem: The linked issue's Expected Result says `nemoclaw list` should show `dcode-station` Ready with agent `langchain-deepagents-code`. This patch correctly avoids inventing an OpenClaw default and now carries the live phase, but gateway-recovered rows still render `agent: unknown` because the live OpenShell sandbox list is not treated as an authoritative agent source.
  • Impact: Users regain discoverability and phase consistency, but the literal agent portion of [DGX Station][CLI&UX] nemoclaw list shows "No sandboxes registered" while nemoclaw <name> status reports Phase=Ready and docker ps confirms the container is Up #5714 remains unmet unless maintainers explicitly accept the narrower safe behavior. Without that acceptance or a user-facing explanation, users may still expect `list` to identify a Deep Agents/Hermes sandbox after registry loss.
  • Recommended action: Either add a safe authoritative agent source for display-only recovered rows, or explicitly document/accept the narrowed behavior that unseeded `list` can show only name and phase while leaving agent/GPU unknown until a follow-up named command reconciles authoritative metadata.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `src/lib/registry-recovery-action.ts` around the read-only `ephemeralSandboxes` construction and `src/lib/inventory/index.ts` around `agent: sandbox.agent || (sandbox.recoveredFromGateway ? "unknown" : null)` plus the render line that prints `agent: ${agent}`.
  • Missing regression test: Existing tests already prove the narrowed behavior: `list displays recovered sandbox Ready phase and authoritative agent when available, otherwise documents unknown fallback`, `shows agent as 'unknown' for a gateway-recovered sandbox ([DGX Station][CLI&UX] nemoclaw list shows "No sandboxes registered" while nemoclaw <name> status reports Phase=Ready and docker ps confirms the container is Up #5714)`, and `renders a gateway-recovered row with the trusted live phase but unknown agent/GPU ([DGX Station][CLI&UX] nemoclaw list shows "No sandboxes registered" while nemoclaw <name> status reports Phase=Ready and docker ps confirms the container is Up #5714)`. If an authoritative agent lookup or user-facing note is added, add `list displays authoritative recovered Deep Agents agent when safe source is available` or `list explains unknown agent for display-only gateway recovery`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `src/lib/registry-recovery-action.ts` around the read-only `ephemeralSandboxes` construction and `src/lib/inventory/index.ts` around `agent: sandbox.agent || (sandbox.recoveredFromGateway ? "unknown" : null)` plus the render line that prints `agent: ${agent}`.
  • Evidence: Issue [DGX Station][CLI&UX] nemoclaw list shows "No sandboxes registered" while nemoclaw <name> status reports Phase=Ready and docker ps confirms the container is Up #5714 Expected Result: "`nemoclaw list` shows `dcode-station` Ready with agent `langchain-deepagents-code`, consistent with both `nemoclaw dcode-station status` (Phase=Ready) and the running Docker container." Current code renders gateway-recovered rows with `agent: unknown` while carrying `livePhase: "Ready"`.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

Address PR review feedback on #5714 registry recovery:

- PRA-2: an incomplete (phantom) onboard session no longer counts as a
  recovery seed. `hasRecoverySeed` now requires a *confirmed* session
  (isSessionSandboxConfirmed && sandboxName), so an empty registry plus a
  phantom session stays in the read-only/display-only path instead of the
  mutating, persisting seeded path.
- Restrict unseeded read-only recovery to `healthy_named` only (drop the
  `connected_other` branch): recover solely from the gateway this process
  resolves/targets, so `nemoclaw list` and a follow-up `nemoclaw <name>
  status` stay consistent and never advertise a sandbox the next command
  would resolve to a different gateway.

Tests: phantom-session stays read-only/non-persisted; connected_other no
longer recovers; assert the includeStderr<->ignoreProbeErrors lockstep in
the probe tests; assert the recoveredFromGateway marker on recovered rows.

Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
@yimoj

yimoj commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator Author

Review feedback addressed (head 3b8a6b42d)

PR Review Advisor

  • PRA-2 (Required) — FIXED. Incomplete sessions no longer count as recovery seeds. hasRecoverySeed in recoverRegistryEntries now requires a confirmed session — isSessionSandboxConfirmed(session) && Boolean(session?.sandboxName) — while keeping existing registry entries and an explicit requestedSandboxName as seeds. An empty registry + incomplete (phantom) session now stays in the read-only/display-only path (no gateway mutation, no persistence). Regression test added: "treats an incomplete (phantom) session as unseeded — stays in read-only/display-only path".

  • PRA-1 (Resolve/justify) — RESOLVED. Source-of-truth analysis for recovery-mode selection:

    • Invalid state: an on-disk onboard session whose steps.sandbox.status !== "complete" (a phantom from an interrupted onboard, [Ubuntu 24.10][Onboard] sandbox name written to onboard-session.json before creation — nemoclaw list shows phantom entry after SIGINT #2753) was being treated as proof a sandbox exists.
    • Source boundary: the authoritative signals that a sandbox is real without a registry entry are (a) a confirmed onboard session, or (b) the live gateway (healthy_named). A phantom session is neither.
    • Source-fix constraint: the true root cause (why DGX Station's onboard didn't persist the registry entry) lives in the onboard write path; this PR is the resilient read side and must not over-trust unconfirmed local state.
    • Regression test: PRA-T1 (added).
    • Removal condition: this read-only display-only recovery can be retired once the onboard write path reliably persists the registry entry on success.
  • PRA-3 (Resolve/justify) — JUSTIFIED (intentional, name-only). nemoclaw list is a registry/inventory view; no list row (recovered or normal) renders live Phase — phase is a status concept surfaced by nemoclaw <name> status, which is the reporter's confirmed working path. The live openshell sandbox list does not expose the agent type, so recovered rows honestly render agent/model/provider/GPU: unknown rather than inventing OpenClaw/CPU defaults (persisting such defaults would permanently misclassify a Deep Agents/Hermes sandbox — see the agent-safety test). Full agent/phase reconciliation comes from the follow-up named command, which is covered by the existing named-status reconciliation path (getReconciledSandboxGatewayState). Covered by tests "…shows agent as 'unknown'…" and "renders a gateway-recovered row with unknown agent/GPU…".

Test follow-ups

  • PRA-T1 — ADDED. Empty registry + incomplete session + healthy gateway → read-only display-only; recoverNamedGatewayRuntime not called; nothing persisted.
  • PRA-T2 — EXISTING COVERAGE. src/lib/runtime-recovery.test.ts already exercises parseLiveSandboxNames against realistic NAME/NAMESPACE/CREATED/PHASE rows, No sandboxes found., and Error: lines (header/status text ignored). The recovery path additionally guards on status === 0 (test "ignores a failed sandbox list probe…").
  • PRA-T3 — COVERED by real-CLI E2E (transcript in the PR description): node ./bin/nemoclaw.js list with an emptied registry + no session against a connected gateway displays the live sandbox names and leaves ~/.nemoclaw/sandboxes.json empty (not persisted).
  • PRA-T4–PRA-T8 / PRA-T7 — JUSTIFIED (no DGX aarch64 runner; hardware-independent fix). The defect is CLI state reconciliation (list trusting an empty registry while status reconciles the live gateway) and is hardware-independent; it was reproduced and fixed via real ./bin/nemoclaw.js E2E against a live OpenShell gateway (registry emptied → pre-fix list blind while status Ready → post-fix list recovers, registry stays empty). The DGX-specific question of why onboard didn't persist the entry is the upstream root cause and is out of scope for this list-side resilience fix. No DGX Station aarch64 CI runner is available to execute the v0.0.67 install → Deep Agents onboard → full command sequence in-repo.

CodeRabbit nitpicks

  • gateway-runtime-action.test.ts (stderr-capture contract) — ADDED. The probe tests now assert the includeStderr ↔ ignoreProbeErrors lockstep: with ignoreProbeErrors: true every capture call has ignoreError === true and includeStderr === true; the default path keeps ignoreError falsy (so stderr is captured anyway).
  • registry-recovery-action.test.ts (recovery marker) — ADDED. The rediscovery test now asserts the recovered dcode-station row carries recoveredFromGateway === true (the transient marker buildSandboxInventoryRow consumes to render unknown agent/GPU).

Also addressed in this round

Per a separate codex finding, unseeded read-only recovery is now restricted to healthy_named (dropped the connected_other branch): list recovers only from the gateway this process resolves/targets, so it never advertises a sandbox a follow-up nemoclaw <name> status would resolve to a different gateway.

Targeted tests: 79 pass (registry-recovery, gateway-runtime, inventory, runtime-recovery); typecheck:cli + Biome clean.

Signed-off-by: Yimo Jiang yimoj@nvidia.com

@yimoj yimoj added the v0.0.69 label Jun 25, 2026
@wscurran wscurran added area: cli Command line interface, flags, terminal UX, or output area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression integration: hermes Hermes integration behavior platform: dgx-station Affects DGX Station hardware or workflows labels Jun 25, 2026
@wscurran

Copy link
Copy Markdown
Contributor

@wscurran wscurran added the integration: dcode LangChain Deep Code integration behavior label Jun 25, 2026
@wscurran
wscurran requested a review from cv June 25, 2026 14:28
yimoj and others added 3 commits June 25, 2026 17:48
Resolve OpenShell runtime adapter overlap: keep both the upstream
includeStreams option and this branch's includeStderr option on
RunnerOptions/captureOpenshell.

Signed-off-by: Yimo Jiang <yimoj@nvidia.com>

# Conflicts:
#	src/lib/adapters/openshell/runtime.ts
…type)

Address PR #5786 PR Review Advisor round 2 for #5714:

- PRA-3 (Required): carry the trusted live PHASE from `openshell sandbox
  list` into recovered list rows so `list` shows e.g. Ready, consistent
  with `nemoclaw <name> status`. Agent stays "unknown" (documented): the
  gateway list is not an authoritative agent source; the real agent is
  reconciled by the follow-up named command. New phase-layout-robust
  parser `parseLiveSandboxEntries` uses a broadened phase vocabulary so
  terminal/transient phases (Failed, CrashLoopBackOff, Creating, …) are
  preserved, not just Ready/Running.
- PRA-4: keep the transient display markers (`recoveredFromGateway`,
  `livePhase`) out of the durable SandboxEntry type — use a narrow
  return-only `RecoveredSandboxEntry` for ephemeral rows and strip both
  keys in serializeSandboxEntryForDisk so a force-passed marker can never
  reach sandboxes.json.
- PRA-5 (and PRA-2): apply isSessionSandboxConfirmed in
  shouldRecoverRegistryEntries too, so an incomplete (phantom) session with
  existing registry entries no longer flips recovery into the mutating
  gateway path. PRA-1 is resolved in code by the PRA-4 narrow type.

Tests: live-phase parsing across column layouts and terminal phases;
recovered row renders phase + unknown agent/GPU; phantom session with
existing registry does not trigger mutating recovery; serialization strips
the transient markers.

Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
@yimoj

yimoj commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator Author

PR Review Advisor round 2 — addressed (head faab1553a)

Required

  • PRA-3 (acceptance) — FIXED in code. The recovered list row now carries the trusted live PHASE from openshell sandbox list (new layout-robust parser parseLiveSandboxEntries, broadened phase vocabulary covering Ready/Failed/CrashLoopBackOff/Creating/… not just Ready/Running), so nemoclaw list shows e.g. phase: Ready — consistent with nemoclaw <name> status. Agent stays unknown by design and is now explicitly documented in code + tests: openshell sandbox list exposes only NAME/CREATED/PHASE and is not an authoritative agent source, so the real agent (langchain-deepagents-code) is reconciled by the follow-up named command rather than guessed in list. Regression test: "list displays recovered sandbox Ready phase and authoritative agent when available, otherwise documents unknown fallback" + the inventory render test asserting phase: Ready with agent/GPU: unknown.

Resolve/justify

  • PRA-4 (correctness) — FIXED in code. recoveredFromGateway/livePhase are removed from the durable SandboxEntry type and live only on a narrow return-only RecoveredSandboxEntry for ephemeral rows. serializeSandboxEntryForDisk now strips both keys, so even a force-passed marker via updateSandbox can never reach sandboxes.json. Regression test: "registry serialization and update strip recoveredFromGateway display marker".
  • PRA-5 (correctness) — FIXED in code. shouldRecoverRegistryEntries now gates hasSessionSandbox on isSessionSandboxConfirmed(session), consistently with the hasRecoverySeed check in recoverRegistryEntries. An incomplete (phantom) session with existing registry entries no longer makes missingSessionSandbox true, so it cannot flip recovery into the mutating gateway path during a plain list. Regression test: "incomplete session with existing registry entries does not trigger mutating gateway recovery solely because the phantom session name is missing".
  • PRA-1 (architecture: display-only marker source-of-truth) — RESOLVED in code by PRA-4.
    • Invalid state: a transient display flag living on the durable persistence type.
    • Source boundary: durable SandboxEntry (sandboxes.json) vs. ephemeral in-memory list rows.
    • Source-fix constraint: the gateway list cannot supply agent/binding, so display must mark them unknown rather than persist defaults.
    • Regression test: the serialization-strip test above.
    • Removal condition: when the onboard write path reliably persists the registry entry on success, gateway-side list recovery (and this marker) become unnecessary.
  • PRA-2 (architecture: incomplete-session seed) — RESOLVED in code by PRA-5.
    • Invalid state: a phantom (incomplete) onboard session treated as proof a sandbox exists.
    • Source boundary: onboard session vs. a confirmed (steps.sandbox.status === "complete") session.
    • Source-fix constraint: the onboard write path is the true fix; the read side must not over-trust unconfirmed local state.
    • Regression test: the existing-registry + phantom-session test above (plus the empty-registry phantom test).
    • Removal condition: same as PRA-1.

Test follow-ups

PRA-T1–PRA-T8 were dispositioned in the prior round (added/justified). The new round-2 tests further strengthen PRA-T2 (realistic multi-layout sandbox list parsing incl. terminal phases) and PRA-T1 (phantom-session read-only path).

Targeted tests: 135 pass (runtime-recovery, registry-recovery, inventory, registry serialization, gateway-runtime); typecheck:cli + Biome clean; codex review run (its API timed out on the final confirmation, so an independent local finder review was run — no findings).

Signed-off-by: Yimo Jiang yimoj@nvidia.com

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
src/lib/runtime-recovery.test.ts (1)

55-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add the protobuf-schema-mismatch assertion here too.

parseLiveSandboxEntries() shares isNonSandboxRow() with the other recovery helpers, but this new suite only checks generic Error: text. A dedicated assertion for the schema-mismatch sentinel would lock in the “don’t invent fake recovered sandboxes from decode errors” behavior.

➕ Suggested test addition
   it("skips headers and error lines when parsing live entries", () => {
     expect(parseLiveSandboxEntries("No sandboxes found.")).toEqual([]);
     expect(parseLiveSandboxEntries("Error: boom")).toEqual([]);
+    expect(
+      parseLiveSandboxEntries(
+        'Error:   × status: Internal, message: "Sandbox.metadata: SandboxResponse.sandbox: invalid wire type value: 6"',
+      ),
+    ).toEqual([]);
   });
🤖 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/runtime-recovery.test.ts` around lines 55 - 58, Add a dedicated
assertion in the parseLiveSandboxEntries test to cover the
protobuf-schema-mismatch sentinel alongside the existing header and Error:
cases. Update the runtime-recovery test around parseLiveSandboxEntries and its
shared isNonSandboxRow path so it explicitly expects an empty result for the
schema-mismatch decode-error text, locking in that recovery helpers do not
fabricate sandbox entries from protobuf decode failures.
src/lib/registry-recovery-action.ts (1)

104-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the recovery-seed rules into one shared helper.

shouldRecoverRegistryEntries() now embeds the same confirmed-session / recovery-seed semantics that recoverRegistryEntries() also recomputes later. Keeping those checks separate makes it easy for the probe gate and the read-only-vs-seeded path to drift apart again on a future edit.

🤖 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/registry-recovery-action.ts` around lines 104 - 128, Extract the
confirmed-session and recovery-seed decision logic into a shared helper so both
shouldRecoverRegistryEntries and recoverRegistryEntries use the exact same
rules. Centralize the checks for isSessionSandboxConfirmed, sessionSandboxName,
requestedSandboxName, hasRecoverySeed, and the empty-registry case in a single
function, then call it from both paths to avoid drift between the probe gate and
the read-only/seeded recovery flow.
🤖 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/registry-recovery-action.ts`:
- Around line 104-128: Extract the confirmed-session and recovery-seed decision
logic into a shared helper so both shouldRecoverRegistryEntries and
recoverRegistryEntries use the exact same rules. Centralize the checks for
isSessionSandboxConfirmed, sessionSandboxName, requestedSandboxName,
hasRecoverySeed, and the empty-registry case in a single function, then call it
from both paths to avoid drift between the probe gate and the read-only/seeded
recovery flow.

In `@src/lib/runtime-recovery.test.ts`:
- Around line 55-58: Add a dedicated assertion in the parseLiveSandboxEntries
test to cover the protobuf-schema-mismatch sentinel alongside the existing
header and Error: cases. Update the runtime-recovery test around
parseLiveSandboxEntries and its shared isNonSandboxRow path so it explicitly
expects an empty result for the schema-mismatch decode-error text, locking in
that recovery helpers do not fabricate sandbox entries from protobuf decode
failures.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: b2afe2e6-b464-4c53-a72b-c4278a17f442

📥 Commits

Reviewing files that changed from the base of the PR and between a0bd164 and faab155.

📒 Files selected for processing (8)
  • src/lib/inventory/index.test.ts
  • src/lib/inventory/index.ts
  • src/lib/registry-recovery-action.test.ts
  • src/lib/registry-recovery-action.ts
  • src/lib/runtime-recovery.test.ts
  • src/lib/runtime-recovery.ts
  • src/lib/state/registry.ts
  • test/registry.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/lib/inventory/index.test.ts
  • src/lib/inventory/index.ts
  • src/lib/registry-recovery-action.test.ts

Restore CodeRabbit docstring coverage above the 80% threshold after the
#5714 round-2 changes brought runtime-recovery.ts into the diff: add TSDoc
to the sandbox-list parsing helpers and serializeSandboxEntryForDisk. No
behavior change.

Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
@yimoj

yimoj commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator Author

Maintainer scope acceptance — recovered-row agent is unknown by design (head 0935ea996)

This addresses the single remaining convergent point from both the PR Review Advisor (PRA-1, warning) and CodeRabbit (Linked Issues ⚠️): an empty-registry recovered list row shows agent: unknown rather than the literal langchain-deepagents-code.

Decision (accepted, documented): unseeded nemoclaw list recovery is intentionally name + trusted-phase only, with agent/GPU left unknown until a follow-up named command reconciles authoritative metadata. Rationale:

  • No safe authoritative agent source exists for this path. openshell sandbox list exposes only NAME / CREATED / PHASE — not the agent. The authoritative source of a sandbox's agent is the local registry, which is precisely what was lost in this scenario.
  • The alternative is worse. Persisting/guessing an agent would default a Deep Agents/Hermes sandbox to OpenClaw across all downstream paths (state dirs, connect, rebuild, doctor) — the exact corruption PRA-3/PRA-4 guard against. A per-sandbox probe during list would add latency/fragility the issue explicitly cautions against, and a registry-lost sandbox has no marker to read safely.
  • The literal acceptance clause is still met via the documented flow. list now restores discoverability AND the trusted live phase: Ready (consistent with nemoclaw <name> status); the full agent identity comes from nemoclaw dcode-station status, which is the reporter's confirmed working path and uses the named-gateway reconciliation (getReconciledSandboxGatewayState).

This is recorded in code (buildSandboxInventoryRow/renderer comments, the RecoveredSandboxEntry type doc) and tests (list displays recovered sandbox Ready phase and authoritative agent when available, otherwise documents unknown fallback). I'm accepting the narrowed display-only behavior as the correct scope for the list-side resilience fix; the durable agent fix is the onboard write path (the upstream root cause noted in the issue), tracked separately.

The PRA-T1–PRA-T8 test follow-ups remain dispositioned per the prior responses (added/justified; round-2 added live-phase multi-layout + terminal-phase parsing and the phantom-session-with-existing-registry regression). Checking the advisor boxes to record this disposition.

Signed-off-by: Yimo Jiang yimoj@nvidia.com

@yimoj

yimoj commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Reviews resumed.

@wscurran wscurran added NV QA Bugs found by the NVIDIA QA Team VDR Linked to VDR finding labels Jun 26, 2026
@cv
cv merged commit b1d3165 into main Jun 26, 2026
49 checks passed
@cv
cv deleted the fix/5714-list-registry-recovery branch June 26, 2026 05:04
@miyoungc miyoungc mentioned this pull request Jun 29, 2026
8 of 21 tasks
cv pushed a commit that referenced this pull request Jun 29, 2026
## Summary
Adds the v0.0.69 release notes to the published release-notes page so
users can see the shipped sandbox recovery, Deep Agents Code, Hermes,
inference, policy, and release-validation changes.
The section is based on the v0.0.69 announcement and links each
user-facing theme to the deeper docs pages that already cover the
behavior.

## Changes
- Added a new `v0.0.69` section to `docs/about/release-notes.mdx`.
- Linked release-note themes to lifecycle, backup, troubleshooting, Deep
Agents Code, commands, workspace, messaging, Hermes, inference,
security, monitoring, and network-policy docs.

Source summary:
- #5455 -> `docs/about/release-notes.mdx`: Summarized persistent
workspace and state cleanup during sandbox destroy.
- #5738 -> `docs/about/release-notes.mdx`: Summarized nonzero exit
status preservation for failed hosted endpoint validation.
- #5786 -> `docs/about/release-notes.mdx`: Summarized live sandbox
rediscovery when local registry state is missing.
- #5881 -> `docs/about/release-notes.mdx`: Summarized the
`nemo-deepagents` alias command surface.
- #5594 -> `docs/about/release-notes.mdx`: Summarized the Hermes Agent
2026.6.19 update.
- #5777 -> `docs/about/release-notes.mdx`: Summarized manifest-derived
messaging channel support.
- #5825 -> `docs/about/release-notes.mdx`: Summarized DeepSeek V4 Flash
managed-vLLM defaults for DGX Station.
- #5877 -> `docs/about/release-notes.mdx`: Summarized provider switch
metadata preservation.
- #5932 -> `docs/about/release-notes.mdx`: Summarized transient
inference smoke retry behavior.
- #5934 -> `docs/about/release-notes.mdx`: Summarized constrained
inference smoke retry boundaries.
- #5681 -> `docs/about/release-notes.mdx`: Summarized Shields
config-hash sealing during auto-restore.
- #5682 -> `docs/about/release-notes.mdx`: Summarized sandbox connect
process-limit enforcement.
- #5683 -> `docs/about/release-notes.mdx`: Summarized JSON agent failure
provenance warnings.
- #5711 -> `docs/about/release-notes.mdx`: Summarized sparse-source log
breadcrumbs.
- #5838 -> `docs/about/release-notes.mdx`: Summarized host-authoritative
Shields status.
- #5880 -> `docs/about/release-notes.mdx`: Summarized policy round-trip
documentation updates.
- #5886 -> `docs/about/release-notes.mdx`: Summarized network request
approval-flow documentation updates.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [x] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: doc-only release-notes
prose; no runtime behavior changed.
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [ ] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run 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)

`npm run docs` passed with 0 errors and the existing Fern light-mode
accent contrast warning.
`fern check --warnings` reported the same accent-color warning.

---
Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Documentation**
* Added release notes for **v0.0.69**, covering improved sandbox
lifecycle recovery (state preservation across
destroy/recreate/rebuild/recovery/validation failures), clearer Deep
Agents Code terminal/CLI behavior, and safer Hermes messaging/provider
switching with manifest-driven channels.
* Improved inference setup validation guidance, including handling of
local/compatible endpoints and redaction of sensitive validation errors.
* Refreshed release-gate documentation with clearer approval examples
and validation behavior for NVIDIA API keys vs hosted inference keys.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
…NVIDIA#5786)

## Summary

`nemoclaw list` printed "No sandboxes registered" while the live
OpenShell gateway and container were healthy and `nemoclaw <name>
status` reported `Phase: Ready` (reported on DGX Station with a Deep
Agents sandbox). The `list` recovery path required a *seed* — an
existing registry entry, an onboard session, or an explicit requested
name — before it would probe the gateway. After a local registry loss
with no seed, it trusted the empty registry and reported nothing, even
though the named-status path could find and reconcile the same sandbox.

## Related Issue

Fixes NVIDIA#5714

## Changes

- **Widen list recovery for an empty registry.**
`recoverRegistryEntries` now attempts recovery whenever the local
registry is empty, even with no session/requested-name seed.
- **Bounded, read-only, non-mutating gateway inspection for the unseeded
case.** Plain `nemoclaw list` never selects or starts a gateway. It
inspects the live sandbox list only when OpenShell is connected to a
**NemoClaw-managed** gateway (`nemoclaw` or a per-port
`nemoclaw-<port>`) — never a foreign OpenShell gateway. Gateway probes
are non-fatal (`ignoreProbeErrors`), so a hung/timed-out gateway falls
back to the empty registry instead of exiting the process, and recovery
is gated on a clean (`status === 0`) `sandbox list` so error text is
never parsed as a sandbox name.
- **Display-only recovery — recovered entries are NOT persisted.** The
global `openshell sandbox list` exposes only NAME/CREATED/PHASE, not the
agent or gateway binding. Persisting a recovered entry would default
`agent` to `openclaw` everywhere downstream (state dirs, connect,
rebuild, doctor) and permanently misclassify a Deep Agents/Hermes
sandbox. Recovered sandboxes are surfaced for the current `list` only;
follow-up named commands reconcile the real agent via the gateway.
- **Honest rendering.** Recovered rows show
`agent`/`model`/`provider`/`GPU` as `unknown` rather than inventing
OpenClaw/CPU defaults.
- `getNamedGatewayLifecycleState` gains an opt-in `ignoreProbeErrors`
(default keeps existing fatal behavior); `captureOpenshell` forwards
`includeStderr` so non-fatal probes still capture the gateway status
text.

## Type of Change

- [x] Code change (feature, bug fix, or refactor)

## Verification

Verified end-to-end through the real worktree CLI (`node
./bin/nemoclaw.js …`) against a live OpenShell gateway. The local
registry was emptied and the onboard session removed to simulate the
reporter's registry loss; the on-disk registry is checked after each
run.

**1. Pre-fix (reporter mismatch) — `list` blind while a Ready sandbox is
live:**

```console
$ node ./bin/nemoclaw.js list

  No sandboxes registered. Run `nemoclaw onboard` to get started.

$ node ./bin/nemoclaw.js sbox-4778 status
  Sandbox: sbox-4778
    Model:    nvidia/nemotron-3-super-120b-a12b
    Provider: nvidia-prod
    ...
  Phase: Ready
# docker ps -a → openshell-sbox-4778-… Up   (container healthy, gateway Connected)
```

**2. Post-fix — `list` rediscovers the live sandboxes (display-only,
registry stays empty):**

```console
$ node ./bin/nemoclaw.js list

  Recovered 5 sandbox entries from the live OpenShell gateway.

  Sandboxes:
    probe3014x
      agent: unknown  model: unknown  provider: unknown  GPU: unknown  policies: none
    e2e-5468-ollama
      agent: unknown  model: unknown  provider: unknown  GPU: unknown  policies: none
    issue4538-fix
      agent: unknown  model: unknown  provider: unknown  GPU: unknown  policies: none
    sbox-4778
      agent: unknown  model: unknown  provider: unknown  GPU: unknown  policies: none
    dcode5744
      agent: unknown  model: unknown  provider: unknown  GPU: unknown  policies: none

$ node -e 'console.log(Object.keys(require(process.env.HOME+"/.nemoclaw/sandboxes.json").sandboxes))'
[]        # recovered for display only — NOT persisted (agent is unknowable from the gateway list)
```

(Recovery was also re-verified while OpenShell was connected to a
NemoClaw per-port gateway `nemoclaw-8092` — `connected_other` to a
NemoClaw-managed name still recovers; a foreign gateway name does not.)

**3. Post-fix graceful degradation — gateway unreachable, empty registry
→ no crash, no bogus names:**

```console
$ node ./bin/nemoclaw.js list      # gateway down: Connection refused on :8080
                                   # `openshell sandbox list` → transport error
  No sandboxes registered. Run `nemoclaw onboard` to get started.
$ echo $?
0
```

Targeted + adjacent unit tests pass (registry-recovery, gateway-runtime,
inventory, openshell adapter, repro-2666, gateway/connect drift; ~290
tests), `npm run typecheck:cli` clean, Biome clean; `nemoclaw-start`
(126) passes with the `nemoclaw/` subproject deps installed.

- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [x] Tests added or updated for new or changed behavior
- [x] No secrets, API keys, or credentials committed

Signed-off-by: Yimo Jiang <yimoj@nvidia.com>

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Improved recovery for empty registries with unseeded sessions via
bounded, read-only ephemeral results.
* Inventory rendering now supports gateway-recovered sandboxes (uses
`unknown` agent/GPU and shows `phase` when available).
  * Added live sandbox phase parsing with `parseLiveSandboxEntries`.
* **Bug Fixes**
* Added stderr retention for lifecycle/probe classification
(`includeStderr`) when probe errors are ignored.
* Prevented transient recovery/display markers (`recoveredFromGateway`,
`livePhase`) from persisting to disk.
* **Tests**
* Added cases for unseeded vs seeded recovery, probe option behavior,
and phase parsing variations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---
Supersedes NVIDIA#5771 (reopened from NVIDIA/NemoClaw branch so trusted
advisor workflows can run).

---------

Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
## Summary
Adds the v0.0.69 release notes to the published release-notes page so
users can see the shipped sandbox recovery, Deep Agents Code, Hermes,
inference, policy, and release-validation changes.
The section is based on the v0.0.69 announcement and links each
user-facing theme to the deeper docs pages that already cover the
behavior.

## Changes
- Added a new `v0.0.69` section to `docs/about/release-notes.mdx`.
- Linked release-note themes to lifecycle, backup, troubleshooting, Deep
Agents Code, commands, workspace, messaging, Hermes, inference,
security, monitoring, and network-policy docs.

Source summary:
- NVIDIA#5455 -> `docs/about/release-notes.mdx`: Summarized persistent
workspace and state cleanup during sandbox destroy.
- NVIDIA#5738 -> `docs/about/release-notes.mdx`: Summarized nonzero exit
status preservation for failed hosted endpoint validation.
- NVIDIA#5786 -> `docs/about/release-notes.mdx`: Summarized live sandbox
rediscovery when local registry state is missing.
- NVIDIA#5881 -> `docs/about/release-notes.mdx`: Summarized the
`nemo-deepagents` alias command surface.
- NVIDIA#5594 -> `docs/about/release-notes.mdx`: Summarized the Hermes Agent
2026.6.19 update.
- NVIDIA#5777 -> `docs/about/release-notes.mdx`: Summarized manifest-derived
messaging channel support.
- NVIDIA#5825 -> `docs/about/release-notes.mdx`: Summarized DeepSeek V4 Flash
managed-vLLM defaults for DGX Station.
- NVIDIA#5877 -> `docs/about/release-notes.mdx`: Summarized provider switch
metadata preservation.
- NVIDIA#5932 -> `docs/about/release-notes.mdx`: Summarized transient
inference smoke retry behavior.
- NVIDIA#5934 -> `docs/about/release-notes.mdx`: Summarized constrained
inference smoke retry boundaries.
- NVIDIA#5681 -> `docs/about/release-notes.mdx`: Summarized Shields
config-hash sealing during auto-restore.
- NVIDIA#5682 -> `docs/about/release-notes.mdx`: Summarized sandbox connect
process-limit enforcement.
- NVIDIA#5683 -> `docs/about/release-notes.mdx`: Summarized JSON agent failure
provenance warnings.
- NVIDIA#5711 -> `docs/about/release-notes.mdx`: Summarized sparse-source log
breadcrumbs.
- NVIDIA#5838 -> `docs/about/release-notes.mdx`: Summarized host-authoritative
Shields status.
- NVIDIA#5880 -> `docs/about/release-notes.mdx`: Summarized policy round-trip
documentation updates.
- NVIDIA#5886 -> `docs/about/release-notes.mdx`: Summarized network request
approval-flow documentation updates.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [x] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: doc-only release-notes
prose; no runtime behavior changed.
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [ ] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run 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)

`npm run docs` passed with 0 errors and the existing Fern light-mode
accent contrast warning.
`fern check --warnings` reported the same accent-color warning.

---
Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Documentation**
* Added release notes for **v0.0.69**, covering improved sandbox
lifecycle recovery (state preservation across
destroy/recreate/rebuild/recovery/validation failures), clearer Deep
Agents Code terminal/CLI behavior, and safer Hermes messaging/provider
switching with manifest-driven channels.
* Improved inference setup validation guidance, including handling of
local/compatible endpoints and redaction of sensitive validation errors.
* Refreshed release-gate documentation with clearer approval examples
and validation behavior for NVIDIA API keys vs hosted inference keys.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: cli Command line interface, flags, terminal UX, or output area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression integration: dcode LangChain Deep Code integration behavior integration: hermes Hermes integration behavior NV QA Bugs found by the NVIDIA QA Team platform: dgx-station Affects DGX Station hardware or workflows VDR Linked to VDR finding

Projects

None yet

3 participants