fix: repair NemoHermes first-run onboarding - #2781
Merged
Merged
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds agent-aware sandbox naming and registry drift checks, standardizes in-sandbox probe execution, prefers user-local OpenShell with env override, restores agent dashboard forwards (Hermes watcher), adds agent binary preflight checks, introduces fuzzy CLI suggestions, and updates many tests and Hermes Dockerfile validation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Installer as Installer Script
participant OpenShell as OpenShell CLI
participant Sandbox as Sandbox (sandbox exec)
participant Registry as Registry API
participant Watcher as Node Watcher
User->>Installer: run installer / upgrade
Installer->>OpenShell: resolve/install openshell (respect NEMOCLAW_OPENSHELL_BIN)
Installer->>Registry: read onboard-session.json / registry state
Installer->>Sandbox: run in-sandbox agent binary preflight (sandbox exec -n ... -- sh -lc "command -v ...")
alt binary missing/invalid
Sandbox->>Installer: return failure
Installer->>Registry: mark agent_setup failed
Installer->>User: abort with logs hint
else binary valid
Installer->>OpenShell: ensure/allocate agent dashboard forward (forward list/stop/start)
OpenShell->>Registry: update sandbox dashboard port & metadata
alt Hermes and watcher enabled
OpenShell->>Watcher: spawn background health watcher
Watcher->>OpenShell: poll /health and stop/start forward on unhealthy
end
end
Installer->>User: export CHAT_UI_URL and complete onboarding
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ericksoa
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/lib/agent-onboard.test.ts (1)
127-135: 🏗️ Heavy liftPrefer behavior assertions over source-string assertions for setup guards
This test validates text, not runtime behavior. It can pass even if
handleAgentSetupstops callingfailAgentSetupunder real execution paths.Use a behavior-level test that invokes
handleAgentSetupwith mockedrunCaptureOpenshell, then assertonboardSession.markStepFailed("agent_setup", ...)and exit behavior directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/agent-onboard.test.ts` around lines 127 - 135, The test currently asserts source text rather than runtime behavior; replace the string-based checks with a behavior test that calls handleAgentSetup (from agent-onboard.ts) and mocks runCaptureOpenshell to simulate missing binary/failed health probe, then assert that onboardSession.markStepFailed("agent_setup", ...) (or failAgentSetup) is invoked and that the function exits/returns the failure path (e.g., process.exit is stubbed or the promise rejects). In short: mock runCaptureOpenshell to produce the failure condition, call handleAgentSetup, and verify onboardSession.markStepFailed("agent_setup", ...) and appropriate exit/rejection instead of checking source strings.test/onboard.test.ts (1)
68-71: ⚡ Quick winStrengthen the internals shape guard for the newly required helper exports.
These helpers are now destructured and called, but
isOnboardTestInternalsdoes not validate them yet. If one export goes missing, failure will happen later with a less clear runtime error.Suggested patch
function isOnboardTestInternals( value: OnboardTestInternalsCandidate, ): value is OnboardTestInternals { return ( value !== null && typeof value.buildProviderArgs === "function" && typeof value.classifySandboxCreateFailure === "function" && typeof value.agentSupportsWebSearch === "function" && typeof value.configureWebSearch === "function" && - typeof value.writeSandboxConfigSyncFile === "function" + typeof value.writeSandboxConfigSyncFile === "function" && + typeof value.getDefaultSandboxNameForAgent === "function" && + typeof value.getSandboxPromptDefault === "function" && + typeof value.getRequestedSandboxAgentName === "function" && + typeof value.normalizeSandboxAgentName === "function" ); }Also applies to: 158-161, 188-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` around lines 68 - 71, The runtime shape guard is too weak: update isOnboardTestInternals to verify the newly required helper exports exist and are functions so destructuring and calls won't fail later; specifically check that getDefaultSandboxNameForAgent, getSandboxPromptDefault, getRequestedSandboxAgentName, and normalizeSandboxAgentName are present and typeof === "function" (and adjust any array/object checks accordingly) wherever isOnboardTestInternals is used so missing exports produce a clear validation error instead of a later crash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/agent-onboard.ts`:
- Around line 130-133: The current preflight in the script array (symbols:
script, executable, binaryPath, agent.binary_path) treats the two checks as
alternatives so a stale agent.binary_path can be ignored if another same-named
executable is in PATH; change the logic so that when binaryPath is provided you
verify BOTH that the provided path is executable and that the PATH-resolved
executable matches that path (resolve command -v for executable and compare it
to the quoted binaryPath), otherwise if no binaryPath is provided keep the
existing command -v check; update the script construction to perform this
equality check (using the same shellQuote helper for safe quoting) so setup
cannot false-pass with the wrong binary.
In `@src/lib/onboard.ts`:
- Around line 3409-3419: getSandboxAgentDrift currently treats a missing
registry entry as the default agent (via normalizeSandboxAgentName), allowing
liveExists to bypass agent-drift checks; change it to "fail closed": if
registry.getSandbox(sandboxName) returns null, set existingAgentName to a
sentinel (e.g. empty string or 'MISSING') and return changed: true so the drift
guard blocks reuse. Update getSandboxAgentDrift (and any callers relying on its
existingAgentName) to detect the null case rather than normalizing to
'openclaw', and ensure the liveExists path no longer skips the agent-drift check
when existingEntry is missing.
- Around line 3388-3391: getSandboxPromptDefault currently reads
NEMOCLAW_SANDBOX_NAME, which pollutes the interactive prompt path; remove the
env lookup so interactive flow always uses getDefaultSandboxNameForAgent(agent)
and let promptOrDefault continue to apply NEMOCLAW_SANDBOX_NAME in
non-interactive mode. Concretely, update getSandboxPromptDefault (the function
named getSandboxPromptDefault) to no longer reference
process.env.NEMOCLAW_SANDBOX_NAME and simply return
getDefaultSandboxNameForAgent(agent).
- Around line 3422-3435: updateReusedSandboxMetadata always writes back
effectiveAgent.expectedVersion (via getSandboxAgentRegistryFields), which
overwrites custom/unknown agentVersion:null from `--from` sandboxes; add a
boolean parameter (e.g., restoreExpectedVersion or preserveUnknownAgentVersion)
to updateReusedSandboxMetadata and use it when calling
getSandboxAgentRegistryFields (or update getSandboxAgentRegistryFields to accept
the flag) so that when the flag is false you do not restore
expectedVersion/effectiveAgent.expectedVersion into the registry. Callers that
are reusing a `--from` sandbox should pass false to preserve the original
unknown/null agentVersion.
In `@src/nemoclaw.ts`:
- Around line 4403-4410: The typo-suggestion early-exit can prevent
stale-registry recovery; before using suggestGlobalCommand and calling
process.exit, invoke recoverRegistryEntries (or the registry recovery function)
and then re-check registry.getSandbox(cmd) so that if recovery restores the
sandbox the suggestion/exit is skipped; update both places that use
suggestGlobalCommand (the block around registry.getSandbox(cmd) and the similar
block later) to call recoverRegistryEntries first, then re-query
registry.getSandbox(cmd), and only call suggestGlobalCommand + process.exit if
the sandbox is still missing.
---
Nitpick comments:
In `@src/lib/agent-onboard.test.ts`:
- Around line 127-135: The test currently asserts source text rather than
runtime behavior; replace the string-based checks with a behavior test that
calls handleAgentSetup (from agent-onboard.ts) and mocks runCaptureOpenshell to
simulate missing binary/failed health probe, then assert that
onboardSession.markStepFailed("agent_setup", ...) (or failAgentSetup) is invoked
and that the function exits/returns the failure path (e.g., process.exit is
stubbed or the promise rejects). In short: mock runCaptureOpenshell to produce
the failure condition, call handleAgentSetup, and verify
onboardSession.markStepFailed("agent_setup", ...) and appropriate exit/rejection
instead of checking source strings.
In `@test/onboard.test.ts`:
- Around line 68-71: The runtime shape guard is too weak: update
isOnboardTestInternals to verify the newly required helper exports exist and are
functions so destructuring and calls won't fail later; specifically check that
getDefaultSandboxNameForAgent, getSandboxPromptDefault,
getRequestedSandboxAgentName, and normalizeSandboxAgentName are present and
typeof === "function" (and adjust any array/object checks accordingly) wherever
isOnboardTestInternals is used so missing exports produce a clear validation
error instead of a later crash.
🪄 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: 13d66cdc-4601-44ee-8b77-05de63334dbc
📒 Files selected for processing (10)
scripts/install.shsrc/lib/agent-onboard.test.tssrc/lib/agent-onboard.tssrc/lib/inventory-commands.test.tssrc/lib/inventory-commands.tssrc/lib/onboard.tssrc/nemoclaw.tstest/cli.test.tstest/install-preflight.test.tstest/onboard.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/nemoclaw.ts (1)
4410-4417:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun stale-registry recovery before typo-suggestion exit.
Line 4410 exits on
suggestGlobalCommand(cmd)before the sandbox-scoped recovery path at Line 4424. That can block valid recovery when a real sandbox name is typo-close to a global command.💡 Suggested ordering fix
- if (!registry.getSandbox(cmd)) { - const suggestion = suggestGlobalCommand(cmd); - if (suggestion) { - console.error(` Unknown command: ${cmd}`); - console.error(` Did you mean: ${CLI_NAME} ${suggestion}?`); - process.exit(1); - } - } - // Sandbox-scoped commands: nemoclaw <name> <action> // If the registry doesn't know this name but the action is a sandbox-scoped // command, attempt recovery — the sandbox may still be live with a stale registry. // Derived from command registry — single source of truth const sandboxActions = sandboxActionTokens(); if (!registry.getSandbox(cmd) && sandboxActions.includes(args[0] || "")) { @@ process.exit(1); } } + + if (!registry.getSandbox(cmd)) { + const suggestion = suggestGlobalCommand(cmd); + if (suggestion) { + console.error(` Unknown command: ${cmd}`); + console.error(` Did you mean: ${CLI_NAME} ${suggestion}?`); + process.exit(1); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 4410 - 4417, The current flow calls suggestGlobalCommand(cmd) and exits before attempting sandbox-scoped stale-registry recovery; change the order so you first attempt the sandbox recovery path (use registry.getSandbox(cmd) and the existing sandbox recovery logic that runs after Line 4424) and only if that recovery path does not apply then call suggestGlobalCommand(cmd) and call process.exit(1). Specifically, move or invoke the stale-registry/sandbox recovery logic before using suggestGlobalCommand(cmd)/CLI_NAME so a typo that matches a sandbox name can be recovered instead of being treated as a global-command suggestion.
🧹 Nitpick comments (1)
test/cli.test.ts (1)
250-270: ⚡ Quick winAdd a typo-close sandbox-name case to protect recovery ordering.
This test uses
hermes connect alpha, buthermesis not close to a global command, so it won’t catch the early typo-suggestion regression. Add a case likeliost connect alpha(withalpharegistered) to ensure recovery still runs before suggestion/exit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.ts` around lines 250 - 270, Add a new test case similar to the existing "explains sandbox connect command order when the sandbox name is last" that uses a typoy global token like "liost connect alpha" (with alpha registered by writeSandboxRegistry) to ensure the recovery path runs before the suggestion/exit; call runWithEnv("liost connect alpha", { HOME: home, PATH: `${localBin}:${process.env.PATH || ""}` }) and assert the exit code is 1 and the output contains the sandbox-not-found message for 'liost', the command-order hint ("Command order is: nemoclaw <sandbox-name> connect"), and the recovered suggestion ("Did you mean: nemoclaw alpha connect?") so the test covers the typo-close sandbox-name case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/nemoclaw.ts`:
- Around line 4410-4417: The current flow calls suggestGlobalCommand(cmd) and
exits before attempting sandbox-scoped stale-registry recovery; change the order
so you first attempt the sandbox recovery path (use registry.getSandbox(cmd) and
the existing sandbox recovery logic that runs after Line 4424) and only if that
recovery path does not apply then call suggestGlobalCommand(cmd) and call
process.exit(1). Specifically, move or invoke the stale-registry/sandbox
recovery logic before using suggestGlobalCommand(cmd)/CLI_NAME so a typo that
matches a sandbox name can be recovered instead of being treated as a
global-command suggestion.
---
Nitpick comments:
In `@test/cli.test.ts`:
- Around line 250-270: Add a new test case similar to the existing "explains
sandbox connect command order when the sandbox name is last" that uses a typoy
global token like "liost connect alpha" (with alpha registered by
writeSandboxRegistry) to ensure the recovery path runs before the
suggestion/exit; call runWithEnv("liost connect alpha", { HOME: home, PATH:
`${localBin}:${process.env.PATH || ""}` }) and assert the exit code is 1 and the
output contains the sandbox-not-found message for 'liost', the command-order
hint ("Command order is: nemoclaw <sandbox-name> connect"), and the recovered
suggestion ("Did you mean: nemoclaw alpha connect?") so the test covers the
typo-close sandbox-name case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: adfecc65-c97b-4ae2-8a9a-de1b3c24b355
📒 Files selected for processing (2)
src/nemoclaw.tstest/cli.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/onboard.ts (1)
3430-3445:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDerive reused-sandbox agent version from the existing sandbox metadata.
agentVersionKnownis still effectively driven by the currentfromDockerfileflag. If I reuse a sandbox that was originally created with--fromduring a normal onboard, this helper will rewrite the existingagentVersion: nullback toeffectiveAgent.expectedVersion, so inventory starts reporting a stock agent version for a custom image again. Preservenullbased on the existing registry entry instead of the current invocation.Suggested direction
function updateReusedSandboxMetadata( sandboxName: string, agent: AgentDefinition | null | undefined, model: string, provider: string, dashboardPort: number, - agentVersionKnown = true, ): void { + const existingEntry = registry.getSandbox(sandboxName); + const agentVersionKnown = existingEntry?.agentVersion !== null; registry.updateSandbox(sandboxName, { model, provider, dashboardPort, ...getSandboxAgentRegistryFields(agent, agentVersionKnown), }); registry.setDefault(sandboxName); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 3430 - 3445, Read the existing sandbox entry from the registry inside updateReusedSandboxMetadata and use its agentVersion to decide whether to treat the agent version as known: fetch existing = registry.getSandbox(sandboxName) and if existing?.agentVersion === null override agentVersionKnown to false so getSandboxAgentRegistryFields(...) preserves a null agentVersion for reused sandboxes created from a custom image; then call registry.updateSandbox(sandboxName, { model, provider, dashboardPort, ...getSandboxAgentRegistryFields(agent, agentVersionKnown) }) and registry.setDefault(sandboxName) as before.
🧹 Nitpick comments (3)
src/lib/agent-onboard.test.ts (1)
128-138: 🏗️ Heavy liftPrefer behavior-level assertions over raw source-string assertions.
On Line 128 and Lines 130-138, this test is tightly coupled to implementation text, so harmless refactors can fail it while real behavior regressions can still slip through. Consider asserting via the onboarding execution path (with mocks) and checking that
agent_setupis marked failed on missing binary/health timeout.src/lib/onboard.ts (1)
3728-3936: Please cover this reuse/recreate branch with the onboarding E2Es.This block now owns the cross-agent reuse guard, default-sandbox selection, and metadata rewrite path. The unit tests listed in the PR are good, but I’d still want the multi-sandbox/rebuild flows exercised before merge.
As per coding guidelines "
src/lib/onboard.ts: This file contains core onboarding logic. Changes here affect the full sandbox creation and configuration flow. E2E test recommendation:cloud-e2e— full onboard + cloud inference,sandbox-operations-e2e— multi-sandbox lifecycle,rebuild-openclaw-e2e— workspace state survives rebuild."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 3728 - 3936, Add end-to-end tests that exercise the reuse/recreate branch around getSandboxReuseState/getSandboxAgentDrift and the subsequent flows (promptYesNoOrDefault, confirmRecreateForSelectionDrift, credentialRotation via detectMessagingCredentialRotation, sandboxState.backupSandboxState and updateReusedSandboxMetadata) so cross-agent reuse guards, default-sandbox selection, provider/model drift, credential rotation backup/restore, and metadata rewrite are validated; specifically add or extend cloud-e2e to run a full onboard + cloud inference path, sandbox-operations-e2e to create/reuse/delete multiple sandboxes and cover agentDrift/recreateForAgentDrift and selectionDrift branches, and rebuild-openclaw-e2e to trigger credential rotation and verify sandboxState.backupSandboxState and pendingStateRestore restore behavior after rebuild.test/onboard.test.ts (1)
2948-2949: ⚡ Quick winAssert the new registry-persistence behavior instead of only stubbing/grepping it.
Line 2948 adds
registry.updateSandbox/registry.setDefaultno-op stubs, but none of these spawned-path tests record whether those calls actually happen. On top of that, Line 6283 now looks for any laterregistry.updateSandbox(sandboxName, {, which can match the new metadata-update sites this PR adds rather than the specific post-create model/provider write this regression test is named after. The new agent-drift check has the same problem: grepping forregistry.setDefault(sandboxName)/updateReusedSandboxMetadata(...)still passes if that code is unreachable. Please turn one create/reuse harness into a behavioral assertion that captures the stub calls and checks order/args.Also applies to: 6250-6270, 6283-6285
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/onboard.ts`:
- Around line 3430-3445: Read the existing sandbox entry from the registry
inside updateReusedSandboxMetadata and use its agentVersion to decide whether to
treat the agent version as known: fetch existing =
registry.getSandbox(sandboxName) and if existing?.agentVersion === null override
agentVersionKnown to false so getSandboxAgentRegistryFields(...) preserves a
null agentVersion for reused sandboxes created from a custom image; then call
registry.updateSandbox(sandboxName, { model, provider, dashboardPort,
...getSandboxAgentRegistryFields(agent, agentVersionKnown) }) and
registry.setDefault(sandboxName) as before.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 3728-3936: Add end-to-end tests that exercise the reuse/recreate
branch around getSandboxReuseState/getSandboxAgentDrift and the subsequent flows
(promptYesNoOrDefault, confirmRecreateForSelectionDrift, credentialRotation via
detectMessagingCredentialRotation, sandboxState.backupSandboxState and
updateReusedSandboxMetadata) so cross-agent reuse guards, default-sandbox
selection, provider/model drift, credential rotation backup/restore, and
metadata rewrite are validated; specifically add or extend cloud-e2e to run a
full onboard + cloud inference path, sandbox-operations-e2e to
create/reuse/delete multiple sandboxes and cover
agentDrift/recreateForAgentDrift and selectionDrift branches, and
rebuild-openclaw-e2e to trigger credential rotation and verify
sandboxState.backupSandboxState and pendingStateRestore restore behavior after
rebuild.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 42eecb48-ac9b-4e47-85b5-b3dd7cc99bb8
📒 Files selected for processing (6)
src/lib/agent-onboard.test.tssrc/lib/agent-onboard.tssrc/lib/onboard.tssrc/nemoclaw.tstest/cli.test.tstest/onboard.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/agent-onboard.ts
- src/nemoclaw.ts
- test/cli.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/lib/onboard.ts (2)
3425-3430:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed when the registry row exists but its
agentfield is blank.The missing-entry case is handled now, but legacy rows can still exist with
agent == null/empty. Normalizing that toopenclawlets a live Hermes sandbox bypass the drift guard when the requested agent is OpenClaw, which reintroduces the silent cross-agent reuse this PR is trying to block. Treat a blank recorded agent the same way as a missing registry entry.Suggested fix
function getSandboxAgentDrift( sandboxName: string, requestedAgentName: string, ): { changed: boolean; existingAgentName: string; requestedAgentName: string } { const existingEntry: SandboxEntry | null = registry.getSandbox(sandboxName); if (!existingEntry) { return { changed: true, existingAgentName: UNKNOWN_SANDBOX_AGENT_NAME, requestedAgentName, }; } - const existingAgentName = normalizeSandboxAgentName(existingEntry?.agent); + const rawExistingAgent = + typeof existingEntry.agent === "string" ? existingEntry.agent.trim() : ""; + if (!rawExistingAgent) { + return { + changed: true, + existingAgentName: UNKNOWN_SANDBOX_AGENT_NAME, + requestedAgentName, + }; + } + const existingAgentName = normalizeSandboxAgentName(rawExistingAgent); return { changed: existingAgentName !== requestedAgentName, existingAgentName, requestedAgentName, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 3425 - 3430, The code currently sets existingAgentName = normalizeSandboxAgentName(existingEntry?.agent) which treats null/empty agent the same as a real agent; instead detect missing/blank recorded agents and treat them like a missing registry entry by only calling normalizeSandboxAgentName when existingEntry?.agent is a non-empty string. Update the assignment for existingAgentName (used with requestedAgentName and the returned changed flag) so that if existingEntry is absent or existingEntry.agent is null/empty you leave existingAgentName undefined (or another sentinel for “missing”), otherwise set it via normalizeSandboxAgentName(existingEntry.agent); keep the returned shape ({ changed: existingAgentName !== requestedAgentName, existingAgentName, requestedAgentName }) unchanged.
3816-3823:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't derive reused
agentVersionfrom the current--fromflag.These reuse paths pass
!fromDockerfile, which describes this onboarding request, not the sandbox being reused. Reusing a stock sandbox during a--fromrun clears its recorded version, while reusing a custom--fromsandbox during a normal run restores the stock expected version. The preserve/clear decision needs to come from the existing registry entry instead.Suggested direction
+const existingEntry = registry.getSandbox(sandboxName); +const preserveKnownAgentVersion = existingEntry?.agentVersion !== null; updateReusedSandboxMetadata( sandboxName, agent, model, provider, reusedPort, - !fromDockerfile, + preserveKnownAgentVersion, );Apply the same change to each reuse return path.
Also applies to: 3852-3859, 3899-3906, 3924-3931
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 3816 - 3823, The call to updateReusedSandboxMetadata(...) is incorrectly computing the preserve/clear flag from the current onboarding's fromDockerfile (using !fromDockerfile) instead of using the existing sandbox's recorded metadata; change the code so that before calling updateReusedSandboxMetadata(sandboxName, agent, model, provider, reusedPort, ...), you look up the existing registry entry for sandboxName (or the reused sandbox record) and derive the boolean preserve flag (agentVersion presence/expected value) from that entry and pass it into updateReusedSandboxMetadata instead of !fromDockerfile; apply the same replacement to the other reuse return paths that call updateReusedSandboxMetadata with !fromDockerfile (the additional reuse call sites in the file that currently pass !fromDockerfile should be updated similarly).
🧹 Nitpick comments (2)
agents/hermes/Dockerfile (1)
14-23: Please run the Hermes E2E jobs for this image-contract change.This check is exactly the sort of change that can pass static/unit coverage but still break real onboard or upgrade flows if the published base image drifts. I’d run the Hermes-specific E2E workflows before merge.
As per coding guidelines
agents/hermes/**:E2E test recommendation: hermes-e2e — Hermes onboard + health probe + live inferenceandrebuild-hermes-e2e — Hermes upgrade path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agents/hermes/Dockerfile` around lines 14 - 23, This change adds a strict image-contract check for the hermes binary path (hermes_path and /usr/local/bin/hermes) in agents/hermes Dockerfile; before merging, run the Hermes-specific E2E workflows (hermes-e2e for onboard + health probe + live inference and rebuild-hermes-e2e for the upgrade path) to validate the image against real onboard/upgrade flows and catch runtime regressions caused by base-image drift.src/lib/agent-onboard.test.ts (1)
127-143: ⚡ Quick winPrefer a behavior-level guard over matching source text.
This locks the test to helper names and exact shell fragments, so a harmless refactor of
verifyAgentBinaryAvailable/failAgentSetupcan fail CI even when onboarding still blocks correctly. If you can, stub the runner and step recorder and assert the observable failure path instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/agent-onboard.test.ts` around lines 127 - 143, The test currently asserts on source text like verifyAgentBinaryAvailable, failAgentSetup, and specific shell fragments; instead stub the execution runner and the onboardSession/step recorder to simulate missing binary or failed health probe and assert the observable failure path (e.g., that onboardSession.markStepFailed("agent_setup", ...) is called and the overall onboarding promise rejects or returns a failure state). Locate the test in agent-onboard.test.ts and replace the string/regex expect(...) checks with mocks/spies on the runner used by the agent onboarding module and on the onboardSession instance to verify markStepFailed is invoked with the correct step id and that the onboarding flow returns/throws the expected failure result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/agent-onboard.ts`:
- Around line 123-145: verifyAgentBinaryAvailable currently returns only a
boolean and masks the real failure when agent.binary_path is set (it could be
missing executable or a PATH mismatch); change verifyAgentBinaryAvailable to
return a structured result (e.g., { available: boolean, reason?: "not_found" |
"path_mismatch" | "not_executable", resolvedPath?: string }) by using
agentExecutableName and runCaptureOpenshell to detect whether command -v finds
nothing, finds a different path than agent.binary_path, or the file is not
executable, and include the resolved path when present; update callers (notably
failAgentSetup) to accept this object and surface the specific reason string in
error messages instead of a generic "missing" message so users see whether it’s
absent, a path mismatch, or not executable.
In `@test/onboard.test.ts`:
- Around line 2962-2963: Replace the no-op stubs for registry.updateSandbox and
registry.setDefault with spies/mocks that capture their call arguments and
assert the expected sandbox name and metadata when createSandbox() runs;
specifically, instrument registry.updateSandbox and registry.setDefault to
record their parameters (e.g., calledWithName, calledWithMeta) and add
assertions that the provided sandbox name and metadata match the expected values
used in the test fixture, mirroring the same tightened pattern in other sibling
fixture tests where these calls are validated rather than stubbed out.
---
Duplicate comments:
In `@src/lib/onboard.ts`:
- Around line 3425-3430: The code currently sets existingAgentName =
normalizeSandboxAgentName(existingEntry?.agent) which treats null/empty agent
the same as a real agent; instead detect missing/blank recorded agents and treat
them like a missing registry entry by only calling normalizeSandboxAgentName
when existingEntry?.agent is a non-empty string. Update the assignment for
existingAgentName (used with requestedAgentName and the returned changed flag)
so that if existingEntry is absent or existingEntry.agent is null/empty you
leave existingAgentName undefined (or another sentinel for “missing”), otherwise
set it via normalizeSandboxAgentName(existingEntry.agent); keep the returned
shape ({ changed: existingAgentName !== requestedAgentName, existingAgentName,
requestedAgentName }) unchanged.
- Around line 3816-3823: The call to updateReusedSandboxMetadata(...) is
incorrectly computing the preserve/clear flag from the current onboarding's
fromDockerfile (using !fromDockerfile) instead of using the existing sandbox's
recorded metadata; change the code so that before calling
updateReusedSandboxMetadata(sandboxName, agent, model, provider, reusedPort,
...), you look up the existing registry entry for sandboxName (or the reused
sandbox record) and derive the boolean preserve flag (agentVersion
presence/expected value) from that entry and pass it into
updateReusedSandboxMetadata instead of !fromDockerfile; apply the same
replacement to the other reuse return paths that call
updateReusedSandboxMetadata with !fromDockerfile (the additional reuse call
sites in the file that currently pass !fromDockerfile should be updated
similarly).
---
Nitpick comments:
In `@agents/hermes/Dockerfile`:
- Around line 14-23: This change adds a strict image-contract check for the
hermes binary path (hermes_path and /usr/local/bin/hermes) in agents/hermes
Dockerfile; before merging, run the Hermes-specific E2E workflows (hermes-e2e
for onboard + health probe + live inference and rebuild-hermes-e2e for the
upgrade path) to validate the image against real onboard/upgrade flows and catch
runtime regressions caused by base-image drift.
In `@src/lib/agent-onboard.test.ts`:
- Around line 127-143: The test currently asserts on source text like
verifyAgentBinaryAvailable, failAgentSetup, and specific shell fragments;
instead stub the execution runner and the onboardSession/step recorder to
simulate missing binary or failed health probe and assert the observable failure
path (e.g., that onboardSession.markStepFailed("agent_setup", ...) is called and
the overall onboarding promise rejects or returns a failure state). Locate the
test in agent-onboard.test.ts and replace the string/regex expect(...) checks
with mocks/spies on the runner used by the agent onboarding module and on the
onboardSession instance to verify markStepFailed is invoked with the correct
step id and that the onboarding flow returns/throws the expected failure result.
🪄 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: 9cc89b34-3819-4c9b-995b-802153de9ac9
📒 Files selected for processing (6)
agents/hermes/Dockerfilesrc/lib/agent-onboard.test.tssrc/lib/agent-onboard.tssrc/lib/onboard.tstest/onboard.test.tstest/sandbox-provisioning.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/onboard.ts (1)
7252-7261:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways derive and resync
CHAT_UI_URLfrom the agent dashboard port.
process.env.CHAT_UI_URLstill wins overagent.forwardPort, so resume/reuse can keep forwarding the previous sandbox's UI port instead of the agent's port. This also only rewritesCHAT_UI_URLwhen allocation changes the port, leaving stale URLs in the common same-port case.Suggested fix
function ensureAgentDashboardForward( sandboxName: string, agent: { forwardPort?: number | null }, ): number { - const agentDashboardPort = agent.forwardPort || CONTROL_UI_PORT; - const agentDashboardUrl = process.env.CHAT_UI_URL || `http://127.0.0.1:${agentDashboardPort}`; - const actualAgentDashboardPort = ensureDashboardForward(sandboxName, agentDashboardUrl); - if (actualAgentDashboardPort !== Number(getDashboardForwardPort(agentDashboardUrl))) { - process.env.CHAT_UI_URL = `http://127.0.0.1:${actualAgentDashboardPort}`; - } + const agentDashboardPort = agent.forwardPort ?? CONTROL_UI_PORT; + const seedUrl = process.env.CHAT_UI_URL || `http://127.0.0.1:${agentDashboardPort}`; + const parsedUrl = new URL(seedUrl.includes("://") ? seedUrl : `http://${seedUrl}`); + parsedUrl.port = String(agentDashboardPort); + const actualAgentDashboardPort = ensureDashboardForward( + sandboxName, + parsedUrl.toString().replace(/\/$/, ""), + ); + parsedUrl.port = String(actualAgentDashboardPort); + process.env.CHAT_UI_URL = parsedUrl.toString().replace(/\/$/, ""); return actualAgentDashboardPort; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 7252 - 7261, ensureAgentDashboardForward currently lets process.env.CHAT_UI_URL override agent.forwardPort which can leave stale URLs; change the logic in ensureAgentDashboardForward so CHAT_UI_URL is always derived from the agent's dashboard port: compute agentDashboardPort = agent.forwardPort || CONTROL_UI_PORT, call ensureDashboardForward(sandboxName, ...) to get actualAgentDashboardPort, then unconditionally set process.env.CHAT_UI_URL = `http://127.0.0.1:${actualAgentDashboardPort}` (you can remove the conditional that compares against getDashboardForwardPort(agentDashboardUrl)). Ensure you update references to agent.forwardPort, CONTROL_UI_PORT, ensureDashboardForward, getDashboardForwardPort and process.env.CHAT_UI_URL accordingly.
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
7659-8332: Consider running the onboarding E2E suite for this path.These changes touch first-run gateway recovery, sandbox reuse/recreate decisions, and agent-specific forwarding in
src/lib/onboard.ts, so the recommended end-to-end jobs would give better coverage than unit tests alone.As per coding guidelines, "
src/lib/onboard.ts: This file contains core onboarding logic. Changes here affect the full sandbox creation and configuration flow." The recommended jobs arecloud-e2e,sandbox-operations-e2e, andrebuild-openclaw-e2e.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 7659 - 8332, The PR changes core onboarding flows in src/lib/onboard.ts (gateway recovery in startGateway, sandbox reuse/recreate logic around getSandboxReuseState/createSandbox/repairRecordedSandbox, and agent forwarding via agentOnboard.handleAgentSetup/ensureAgentDashboardForward) but didn’t run E2E jobs; run the recommended end-to-end suites (cloud-e2e, sandbox-operations-e2e, rebuild-openclaw-e2e) against this branch to validate first-run gateway recovery, sandbox reuse/recreate decisions, and agent dashboard forwarding, capture any failing scenarios, and iterate on fixes (especially around startGateway, getSandboxReuseState, createSandbox, repairRecordedSandbox, agentOnboard.handleAgentSetup, ensureAgentDashboardForward) until all jobs pass; then include the E2E job run results in the PR description.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/onboard.ts`:
- Around line 2830-2839: The recovery path only stops DASHBOARD_PORT before
destroyGateway(), leaving other active forwards running; update the block that
handles image drift (and the duplicated block below) to first list all active
forwards via runOpenshell(["forward","list"]), parse the output to extract each
host port, and call runOpenshell(["forward","stop", String(port)], {
ignoreError: true }) for each one before calling destroyGateway(), then proceed
with registry.clearAll() and setting gatewayReuseState = "missing". Reference
getGatewayClusterImageDrift(), runOpenshell(...), destroyGateway(),
registry.clearAll, and gatewayReuseState when making the change.
---
Duplicate comments:
In `@src/lib/onboard.ts`:
- Around line 7252-7261: ensureAgentDashboardForward currently lets
process.env.CHAT_UI_URL override agent.forwardPort which can leave stale URLs;
change the logic in ensureAgentDashboardForward so CHAT_UI_URL is always derived
from the agent's dashboard port: compute agentDashboardPort = agent.forwardPort
|| CONTROL_UI_PORT, call ensureDashboardForward(sandboxName, ...) to get
actualAgentDashboardPort, then unconditionally set process.env.CHAT_UI_URL =
`http://127.0.0.1:${actualAgentDashboardPort}` (you can remove the conditional
that compares against getDashboardForwardPort(agentDashboardUrl)). Ensure you
update references to agent.forwardPort, CONTROL_UI_PORT, ensureDashboardForward,
getDashboardForwardPort and process.env.CHAT_UI_URL accordingly.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 7659-8332: The PR changes core onboarding flows in
src/lib/onboard.ts (gateway recovery in startGateway, sandbox reuse/recreate
logic around getSandboxReuseState/createSandbox/repairRecordedSandbox, and agent
forwarding via agentOnboard.handleAgentSetup/ensureAgentDashboardForward) but
didn’t run E2E jobs; run the recommended end-to-end suites (cloud-e2e,
sandbox-operations-e2e, rebuild-openclaw-e2e) against this branch to validate
first-run gateway recovery, sandbox reuse/recreate decisions, and agent
dashboard forwarding, capture any failing scenarios, and iterate on fixes
(especially around startGateway, getSandboxReuseState, createSandbox,
repairRecordedSandbox, agentOnboard.handleAgentSetup,
ensureAgentDashboardForward) until all jobs pass; then include the E2E job run
results in the PR description.
🪄 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: 6dd1d36e-0f6d-4257-a7fc-7158d249aa48
📒 Files selected for processing (1)
src/lib/onboard.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/nemoclaw.ts (1)
4415-4416:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImplicit
connectstill skips registry recovery.
requestedSandboxActionis set toargs[0] || "", but an empty string is not insandboxActions. When the user runsnemoclaw <sandbox-name>(no explicit action), the recovery block at line 4427 is skipped becausesandboxActions.includes("")is false. Later at line 4462,actiondefaults to"connect", but by then recovery was never attempted. A live sandbox with a stale registry entry will fall through to the "Unknown command" error instead of recovering.Proposed fix
const sandboxActions = sandboxActionTokens(); -const requestedSandboxAction = args[0] || ""; +const requestedSandboxAction = args[0] || "connect";This makes implicit connect behave consistently with explicit connect for registry recovery purposes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 4415 - 4416, The recovery check skips when requestedSandboxAction is "" because sandboxActions.includes("") is false; change the logic so an implicit invocation (args[0] missing) is treated like "connect" for the recovery step: either set requestedSandboxAction to args[0] ?? "connect" (or map "" to "connect") before the sandboxActions.includes(...) check, or adjust the includes check to treat an empty string as equivalent to "connect"; reference requestedSandboxAction, args, sandboxActions and the recovery block so recovery runs for implicit connect while keeping the later defaulting of action to "connect" intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/nemoclaw.ts`:
- Around line 4415-4416: The recovery check skips when requestedSandboxAction is
"" because sandboxActions.includes("") is false; change the logic so an implicit
invocation (args[0] missing) is treated like "connect" for the recovery step:
either set requestedSandboxAction to args[0] ?? "connect" (or map "" to
"connect") before the sandboxActions.includes(...) check, or adjust the includes
check to treat an empty string as equivalent to "connect"; reference
requestedSandboxAction, args, sandboxActions and the recovery block so recovery
runs for implicit connect while keeping the later defaulting of action to
"connect" intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 09318a3d-adc3-408b-8688-1f1b77551671
📒 Files selected for processing (1)
src/nemoclaw.ts
cv
approved these changes
May 1, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
scripts/install.sh (1)
299-301:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the bare-port stop fallback entirely to avoid cross-sandbox disruption.
Even with the ownership check, a race between
forward listand the bare-port fallback can stop a different binding on shared gateways.Suggested fix
- "$openshell_bin" forward stop "$port" "$sandbox_name" >/dev/null 2>&1 \ - || "$openshell_bin" forward stop "$port" >/dev/null 2>&1 \ - || true + "$openshell_bin" forward stop "$port" "$sandbox_name" >/dev/null 2>&1 || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 299 - 301, Remove the bare-port fallback stop to avoid disrupting other sandboxes: modify the shutdown sequence that currently calls "$openshell_bin" forward stop "$port" "$sandbox_name" || "$openshell_bin" forward stop "$port" || true so that it only attempts the sandbox-scoped stop (i.e., call "$openshell_bin" forward stop "$port" "$sandbox_name" with existing redirection and error handling) and drop the second bare "$openshell_bin" forward stop "$port" invocation and its || true fallback; keep the ownership/check logic as-is and ensure any error suppression still applies to the remaining sandbox-scoped command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/agent-onboard.ts`:
- Around line 154-156: The check for preflight success currently uses
result.includes("ok") which can true-positive on substrings; update the
conditional that inspects the variable result (in the preflight handling in
src/lib/agent-onboard.ts) to require an exact token match instead—e.g.,
normalize whitespace/case with result.trim() (and optionally toLowerCase()) and
compare for equality to "ok" (or use a strict regex for a whole-string match)
before returning { available: true }.
In `@src/lib/onboard.ts`:
- Around line 3476-3490: The helper updateReusedSandboxMetadata is overwriting
model/provider even when the live selection was not read
(selectionDrift.unknown), which can invent state; fix it by only including model
and provider in the registry.updateSandbox payload when the agent indicates the
live selection is verified (i.e., agent?.selectionDrift?.unknown !== true). Keep
dashboardPort and getSandboxAgentRegistryFields(...) as-is, but build the update
object conditionally so model/provider are omitted when selectionDrift.unknown
is true.
- Around line 7188-7196: The branch that decides to stop a forward treats
anything not equal to "running" as stale; change it to use the same live-status
predicate as getRunningForwardPorts (or introduce/shared isForwardLive(status))
so that statuses like "active" are considered live. Specifically, in the block
referencing existingForwards, findForwardEntry, preferredEntry, preferredPort
and runOpenshell, replace the literal check (preferredEntry.status !==
"running") with a call to the shared live-status function (e.g.,
!isForwardLive(preferredEntry.status) or consult getRunningForwardPorts result)
so you only stop forwards that are actually non-live and avoid stealing ports
from other sandboxes.
---
Duplicate comments:
In `@scripts/install.sh`:
- Around line 299-301: Remove the bare-port fallback stop to avoid disrupting
other sandboxes: modify the shutdown sequence that currently calls
"$openshell_bin" forward stop "$port" "$sandbox_name" || "$openshell_bin"
forward stop "$port" || true so that it only attempts the sandbox-scoped stop
(i.e., call "$openshell_bin" forward stop "$port" "$sandbox_name" with existing
redirection and error handling) and drop the second bare "$openshell_bin"
forward stop "$port" invocation and its || true fallback; keep the
ownership/check logic as-is and ensure any error suppression still applies to
the remaining sandbox-scoped command.
🪄 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: 62522f39-32f9-4d57-aa47-52dc9016508d
📒 Files selected for processing (6)
scripts/install.shsrc/lib/agent-onboard.tssrc/lib/onboard.tssrc/nemoclaw.tstest/cli.test.tstest/onboard.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/cli.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nemoclaw.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install.sh`:
- Around line 277-279: The script currently unconditionally kills the PID read
from pid_file (variable pid_file), which can target a reused/stale PID; modify
the logic that reads the PID to first validate that the process exists and is
owned by the current user before calling kill. Specifically, after reading
pid="$(cat "$pid_file" ...)" check that the process ID corresponds to an
existing process (e.g., via /proc/$pid or ps -p $pid) and verify ownership
(compare the process UID from /proc/$pid or ps -o uid= -p $pid to $(id -u));
only call kill on that PID if ownership matches, otherwise remove the stale
pid_file without killing. Ensure you still handle errors gracefully and keep the
existing rm -f "$pid_file" behavior for stale/non-matching PIDs.
🪄 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: dd3c7f84-f145-4707-8e21-588449caddf2
📒 Files selected for processing (1)
scripts/install.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/onboard.test.ts (1)
3083-3089:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert that
updateSandbox()actually ran here.
payload.updateCalls.every(...)is vacuously true on[], so this fixture still passes ifcreateSandbox()stops persisting the post-create metadata entirely. Please add a non-empty assertion before theevery()check, and ideally pin at least one expected field in the update payload.Suggested tightening
+ assert.ok( + payload.updateCalls.length > 0, + "expected registry metadata update for created sandbox", + ); assert.ok( payload.updateCalls.every( (call: { name: string; updates: Record<string, unknown> }) => call.name === "my-assistant" && call.updates, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` around lines 3083 - 3089, The test currently uses payload.updateCalls.every(...) which passes when updateCalls is empty; ensure updateSandbox() actually ran by first asserting payload.updateCalls.length > 0 (or assert.ok(payload.updateCalls.length), referencing payload.updateCalls) and then tighten the existing check to verify at least one update contains a specific expected field/value (e.g., assert that some call in payload.updateCalls has call.name === "my-assistant" and call.updates has a pinned key like metadata.sandboxId or updates.someField === expectedValue), so the test fails if createSandbox() stops persisting post-create metadata (refer to createSandbox() and updateSandbox() flows).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/agent-onboard.ts`:
- Around line 226-230: The code currently treats any response containing the
substring "ok" as success; update the health check that uses
runCaptureOpenshell([... "curl", "-sf", "--max-time", "3", probe.url], ...) to
perform an exact check (e.g., compare result.trim() === "ok") or rely solely on
curl's exit status instead of .includes("ok"); make the same change for the
other probe check later in the file (the second runCaptureOpenshell usage around
the poll/setup logic) so only an exact "ok" body or a successful curl exit is
considered healthy.
---
Duplicate comments:
In `@test/onboard.test.ts`:
- Around line 3083-3089: The test currently uses payload.updateCalls.every(...)
which passes when updateCalls is empty; ensure updateSandbox() actually ran by
first asserting payload.updateCalls.length > 0 (or
assert.ok(payload.updateCalls.length), referencing payload.updateCalls) and then
tighten the existing check to verify at least one update contains a specific
expected field/value (e.g., assert that some call in payload.updateCalls has
call.name === "my-assistant" and call.updates has a pinned key like
metadata.sandboxId or updates.someField === expectedValue), so the test fails if
createSandbox() stops persisting post-create metadata (refer to createSandbox()
and updateSandbox() flows).
🪄 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: d8e2f286-6cf0-4a69-8d7b-c985a4ad1ea7
📒 Files selected for processing (4)
scripts/install.shsrc/lib/agent-onboard.tssrc/lib/onboard.tstest/onboard.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard.ts
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25202672048
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25203253465
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25203658554
|
12 tasks
cv
pushed a commit
that referenced
this pull request
May 2, 2026
## Summary Daily release-prep documentation refresh for merged PRs from the past 24 hours. This updates user-facing docs for Telegram mention-only mode, in-sandbox messaging shutdown, Hermes onboarding/runtime behavior, and compatible-endpoint smoke validation, then bumps the docs metadata to 0.0.33 after tag v0.0.32. ## Related Issue None. ## Changes - #2417 / c7e49ad: Document `TELEGRAM_REQUIRE_MENTION` for Telegram group-chat replies in `docs/manage-sandboxes/messaging-channels.md` and `docs/reference/commands.md`. - #1977 / 69403e0: Update `nemoclaw tunnel stop` and deprecated `nemoclaw stop` docs to explain that NemoClaw also attempts to stop the in-sandbox OpenClaw gateway and messaging polling. - #2781 / b83ffe2, #2859 / 4df8be6, and #2846 / 0968dfd: Refresh the Hermes quickstart for the default `my-hermes` sandbox name, cross-agent same-name guard, agent type visibility in `nemoclaw list`, Brave prompt omission, and supported prebaked Hermes integrations. - #2849 / fd240ff: Document the Telegram plus OpenAI-compatible endpoint `inference.local` smoke check in inference options and troubleshooting. - Bump `docs/versions1.json` and `docs/project.json` from 0.0.32 to 0.0.33 for daily release preparation. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Verification - [ ] `npx prek run --all-files` passes - [ ] `npm test` passes - [ ] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [x] Docs updated for user-facing behavior changes - [x] `make docs` builds without warnings (doc changes only) - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Additional checks run: - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix nemoclaw-user --dry-run` - `git diff --check` - `make docs` passed with the existing local version-switcher read message. - Full `npx prek run --all-files` and `npm test` were skipped for this doc-only automation run. Commit and pre-push hooks otherwise passed docs, lint, secret, and conversion checks until the local `Test (skills YAML)` hook failed because `vitest/config` is not installed in this fresh worktree. --- Signed-off-by: Miyoung Choi <miyoungc@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated Hermes quickstart: default sandbox name is "hermes"; guidance to use distinct sandbox names, note same-name reuse is prevented, Hermes wizard does not request Brave Web Search, and sandbox listings now show agent type. * Clarified provider onboarding: bounded in-sandbox smoke check runs when Telegram messaging is enabled. * Expanded Telegram docs: added TELEGRAM_REQUIRE_MENTION (DMs still governed by TELEGRAM_ALLOWED_IDS), onboarding examples, stop-messaging/tunnel behavior, and troubleshooting. * Promoted docs to version 0.0.33. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Miyoung Choi <miyoungc@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
This was referenced May 2, 2026
ericksoa
added a commit
that referenced
this pull request
May 2, 2026
## Summary - Make Hermes onboarding accept the configured absolute binary path when PATH-based command resolution is unavailable, while keeping a stable Hermes PATH in pre-created rc files. - Restore the OpenClaw mutable config contract in non-root startup with usable group read/write/execute permissions and setgid directories. - Clear setgid during shields-up locking so locked config directories verify as 755, and update shields E2E expectations to 660/2770 mutable mode. ## Validation - npm ci --ignore-scripts - npm run build:cli - npm test -- test/repro-2376.test.ts src/lib/agent-onboard.test.ts test/shields.test.ts test/repro-2681-group-writable.test.ts test/nemoclaw-start.test.ts - bash -n scripts/nemoclaw-start.sh test/e2e/test-shields-config.sh - npm run format:check - npm run lint ## Notes - Follow-up to PR #2781: #2781 - Addresses hermes-e2e and shields-config-e2e regressions from nightly-e2e run 25238729058. - overlayfs-autofix-e2e was attributed to transient Docker build DNS/apt reachability, so this PR does not change that path. - No full nightly-e2e dispatch was launched from this branch. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved agent binary verification behavior and output handling. * Fixed permission and locking behavior for agent config and state directories (explicit group-read/write/execute, setgid handling, and clearer lock/unlock semantics; defaults now 660 for files and 2770 for config dirs). * **Chores** * Sandbox shell init now prepends Hermes runtime paths into PATH for interactive shells. * Normalize mutable config permissions to group-read/write with setgid where appropriate; updated default mutability docs. * **Tests** * Updated and added tests validating permissions, setgid, locking, and PATH behaviors. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
This was referenced Jul 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Supersedes #2780, which was closed because the repository blocks force-pushing the amended DCO-signed commit.
Validation
Summary by CodeRabbit
New Features
Bug Fixes
Tests