feat(scripts): add upgrade-sandbox.sh and non-interactive inference hydration - #902
feat(scripts): add upgrade-sandbox.sh and non-interactive inference hydration#902HagegeR wants to merge 13 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR introduces a new sandbox upgrade workflow. A new script orchestrates safe upgrades by sequencing workspace backup, optional full-state download, optional onboarding, and restoration. The backup script is refactored to distinguish core and optional workspace items, introducing helper functions for quiet downloads and timestamp selection. Documentation is added describing the upgrade flow. Changes
Sequence DiagramsequenceDiagram
actor User
participant upgrade as upgrade-sandbox.sh
participant backup as backup-workspace.sh
participant openshell as openshell
participant nemoclaw as nemoclaw onboard
participant restore as restore logic
User->>upgrade: Run with options
upgrade->>upgrade: Validate args & deps
upgrade->>backup: Execute backup
backup->>backup: Partition & download files
backup-->>upgrade: Return backup timestamp
alt --full-data enabled
upgrade->>openshell: sandbox download /sandbox/.openclaw-data/
openshell-->>upgrade: Full state captured
end
alt --run-onboard enabled
alt --yes not set
upgrade->>User: Confirm onboarding?
User-->>upgrade: Yes/No
end
upgrade->>nemoclaw: Execute nemoclaw onboard
nemoclaw->>nemoclaw: Rebuild sandbox
nemoclaw-->>upgrade: Onboarding complete
alt --no-restore not set
upgrade->>restore: Auto-restore from backup
restore-->>upgrade: Workspace restored
else
upgrade-->>User: Manual restore instructions
end
else
upgrade-->>User: Instructions to run onboarding later
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/lib/onboard.js (1)
1686-1705:⚠️ Potential issue | 🟠 MajorReuse saved credentials for all non-interactive remote providers.
This branch now falls back to
getCredential("NVIDIA_API_KEY"), but the non-interactive path foropenai,anthropic,gemini, and the compatible endpoints still aborts unless the key is already exported in the environment. Users who previously onboarded interactively will usually have those keys only in~/.nemoclaw/credentials.json, soscripts/upgrade-sandbox.sh --run-onboardstill fails for those providers.💡 Suggested fix
} else { if (isNonInteractive()) { - if (!process.env[credentialEnv]) { + const key = getCredential(credentialEnv); + if (!key) { console.error(` ${credentialEnv} is required for ${remoteConfig.label} in non-interactive mode.`); process.exit(1); } + process.env[credentialEnv] = key; } else { await ensureNamedCredential(credentialEnv, remoteConfig.label + " API key", remoteConfig.helpUrl); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1686 - 1705, The non-interactive branch for other remote providers currently aborts if process.env[credentialEnv] is missing instead of reusing a saved key from getCredential("..."); update the non-interactive check in the else branch (the block using isNonInteractive(), credentialEnv, and remoteConfig.label) to first call getCredential(credentialEnv) and if that returns a key set process.env[credentialEnv] = key and continue; only call process.exit(1) if neither the environment nor getCredential provides a key; keep the interactive path using ensureNamedCredential unchanged.
🧹 Nitpick comments (2)
docs/workspace/backup-restore.md (2)
101-101: Split this into one sentence per source line.Line 101 packs two sentences onto one line.
As per coding guidelines, "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/workspace/backup-restore.md` at line 101, Split the combined sentence into two separate source lines so each sentence is on its own line: put "Before you change the sandbox image (for example after editing `Dockerfile`) or run `nemoclaw onboard`, back up." on its own line and place "Optional files such as `MEMORY.md` or `memory/` may not exist yet; the backup script skips them with a warning." on the following line to satisfy the one-sentence-per-line guideline.
112-112: Drop the routine bolding and break the sentences apart.
**automatically**is not a UI label or warning, and this line currently contains multiple sentences. LLM pattern detected.As per coding guidelines, "Bold is reserved for UI labels, parameter names, and genuine warnings." and "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/workspace/backup-restore.md` at line 112, Remove the bold formatting around "automatically" and split the existing single-line paragraph into separate sentences each on its own line: describe that `--run-onboard` runs `nemoclaw onboard` from the repo root (or `NEMOCLAW_REPO_ROOT`), state that after a successful onboard the script runs `backup-workspace.sh restore` for the backup timestamp created at the start (remove bolding around "automatically"), and add a separate sentence that `--no-restore` skips that restore so you get a fresh workspace after the rebuild; ensure each sentence is on its own line.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 2246-2247: The registry entry for new sandboxes only stores {
model, provider } so hydrateNonInteractiveInferenceFromRegistry() cannot restore
NEMOCLAW_ENDPOINT_URL required by setupNim() for
compatible-endpoint/compatible-anthropic-endpoint; update the logic after
createSandbox in onboard.js (where createSandbox is called and
registry.updateSandbox is invoked) to also persist the sandbox's endpoint URL
(e.g., a field like nemoclawEndpoint or endpointUrl) and then ensure
hydrateNonInteractiveInferenceFromRegistry() sets
process.env.NEMOCLAW_ENDPOINT_URL from that stored field so setupNim() can run
non-interactively with compatible endpoints.
In `@scripts/backup-workspace.sh`:
- Around line 74-95: The loop currently treats any non-zero exit from
sandbox_download as a benign skip; change logic so core items (FILES_CORE loop
using sandbox_download ... 0) treat any non-zero as fatal: log the
sandbox_download stderr (use warn or processLogger equivalent) and exit non-zero
immediately (do not increment count) so partial backups fail; for optional items
(FILES_OPTIONAL and DIRS_OPTIONAL using sandbox_download ... 1) call
sandbox_download and inspect its exit code/output: if it explicitly indicates
"not found" keep the current quiet skip message and do not increment count, but
if it fails for any other reason surface a clear error (echo/warn with stderr),
mark the backup as failed (set a failure flag or exit non-zero at end) and do
not increment count; update references to sandbox_download, FILES_CORE,
FILES_OPTIONAL, DIRS_OPTIONAL, count, warn, and the optional message branches
accordingly so only true "not found" remains quiet and all other errors are
propagated.
In `@scripts/upgrade-sandbox.sh`:
- Around line 94-100: The current TS lookup is a TOCTOU race (using ls on
BACKUP_BASE) — instead have the backup command itself emit the created
timestamp/path and consume that output instead of rescanning; modify the code
that runs backup-workspace.sh backup to capture its stdout (or have
backup-workspace.sh write the created backup path to a known file) and assign
that value to TS, then validate [ -n "$TS" ] || fail and set
DEST="${BACKUP_BASE}/${TS}" as before; reference the TS variable and the
backup-workspace.sh invocation so you replace the ls-based discovery with the
backup command’s explicit output.
---
Outside diff comments:
In `@bin/lib/onboard.js`:
- Around line 1686-1705: The non-interactive branch for other remote providers
currently aborts if process.env[credentialEnv] is missing instead of reusing a
saved key from getCredential("..."); update the non-interactive check in the
else branch (the block using isNonInteractive(), credentialEnv, and
remoteConfig.label) to first call getCredential(credentialEnv) and if that
returns a key set process.env[credentialEnv] = key and continue; only call
process.exit(1) if neither the environment nor getCredential provides a key;
keep the interactive path using ensureNamedCredential unchanged.
---
Nitpick comments:
In `@docs/workspace/backup-restore.md`:
- Line 101: Split the combined sentence into two separate source lines so each
sentence is on its own line: put "Before you change the sandbox image (for
example after editing `Dockerfile`) or run `nemoclaw onboard`, back up." on its
own line and place "Optional files such as `MEMORY.md` or `memory/` may not
exist yet; the backup script skips them with a warning." on the following line
to satisfy the one-sentence-per-line guideline.
- Line 112: Remove the bold formatting around "automatically" and split the
existing single-line paragraph into separate sentences each on its own line:
describe that `--run-onboard` runs `nemoclaw onboard` from the repo root (or
`NEMOCLAW_REPO_ROOT`), state that after a successful onboard the script runs
`backup-workspace.sh restore` for the backup timestamp created at the start
(remove bolding around "automatically"), and add a separate sentence that
`--no-restore` skips that restore so you get a fresh workspace after the
rebuild; ensure each sentence is on its own line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4025feaf-c8d0-4a4b-9040-562d2f2e53a3
📒 Files selected for processing (4)
bin/lib/onboard.jsdocs/workspace/backup-restore.mdscripts/backup-workspace.shscripts/upgrade-sandbox.sh
| const sandboxName = await createSandbox(gpu, model, provider, preferredInferenceApi); | ||
| registry.updateSandbox(sandboxName, { model, provider }); |
There was a problem hiding this comment.
Persist compatible endpoint URLs with the sandbox metadata.
hydrateNonInteractiveInferenceFromRegistry() can only reconstruct what gets stored here. Right now this writes { model, provider }, but setupNim() still requires NEMOCLAW_ENDPOINT_URL for compatible-endpoint and compatible-anthropic-endpoint, so non-interactive upgrades of those sandboxes still exit even though provider/model hydration succeeds.
💡 Suggested fix
- registry.updateSandbox(sandboxName, { model, provider });
+ registry.updateSandbox(sandboxName, {
+ model,
+ provider,
+ endpointUrl:
+ provider === "compatible-endpoint" || provider === "compatible-anthropic-endpoint"
+ ? endpointUrl
+ : undefined,
+ });You’ll also need to hydrate process.env.NEMOCLAW_ENDPOINT_URL from that stored field.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 2246 - 2247, The registry entry for new
sandboxes only stores { model, provider } so
hydrateNonInteractiveInferenceFromRegistry() cannot restore
NEMOCLAW_ENDPOINT_URL required by setupNim() for
compatible-endpoint/compatible-anthropic-endpoint; update the logic after
createSandbox in onboard.js (where createSandbox is called and
registry.updateSandbox is invoked) to also persist the sandbox's endpoint URL
(e.g., a field like nemoclawEndpoint or endpointUrl) and then ensure
hydrateNonInteractiveInferenceFromRegistry() sets
process.env.NEMOCLAW_ENDPOINT_URL from that stored field so setupNim() can run
non-interactively with compatible endpoints.
| for f in "${FILES_CORE[@]}"; do | ||
| if sandbox_download "$sandbox" "${WORKSPACE_PATH}/${f}" "${dest}/" 0; then | ||
| count=$((count + 1)) | ||
| else | ||
| warn "Skipped ${f} (not found or download failed)" | ||
| fi | ||
| done | ||
|
|
||
| for d in "${DIRS[@]}"; do | ||
| if openshell sandbox download "$sandbox" "${WORKSPACE_PATH}/${d}/" "${dest}/${d}/"; then | ||
| for f in "${FILES_OPTIONAL[@]}"; do | ||
| if sandbox_download "$sandbox" "${WORKSPACE_PATH}/${f}" "${dest}/" 1; then | ||
| count=$((count + 1)) | ||
| else | ||
| echo -e "${DIM}[backup]${NC} Optional ${f} not in sandbox — skipped (normal until created)." | ||
| fi | ||
| done | ||
|
|
||
| for d in "${DIRS_OPTIONAL[@]}"; do | ||
| if sandbox_download "$sandbox" "${WORKSPACE_PATH}/${d}/" "${dest}/${d}/" 1; then | ||
| count=$((count + 1)) | ||
| else | ||
| warn "Skipped ${d}/ (not found or download failed)" | ||
| echo -e "${DIM}[backup]${NC} Optional ${d}/ not in sandbox — skipped (normal until created)." | ||
| fi |
There was a problem hiding this comment.
Don't report a partial backup as success.
Every non-zero openshell sandbox download is currently treated as a skip. That means a transient gateway/download failure can leave you with a partial backup that still looks successful, which is risky before destroy or upgrade-sandbox.sh. Core artifacts should fail the run, and optional artifacts should only be downgraded when the remote path is actually absent.
💡 Minimum hardening for the core files
for f in "${FILES_CORE[@]}"; do
if sandbox_download "$sandbox" "${WORKSPACE_PATH}/${f}" "${dest}/" 0; then
count=$((count + 1))
else
- warn "Skipped ${f} (not found or download failed)"
+ fail "Required workspace file ${f} was not backed up."
fi
doneFor the optional paths, keep the quiet UX only for the specific "not found yet" case and surface other download errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/backup-workspace.sh` around lines 74 - 95, The loop currently treats
any non-zero exit from sandbox_download as a benign skip; change logic so core
items (FILES_CORE loop using sandbox_download ... 0) treat any non-zero as
fatal: log the sandbox_download stderr (use warn or processLogger equivalent)
and exit non-zero immediately (do not increment count) so partial backups fail;
for optional items (FILES_OPTIONAL and DIRS_OPTIONAL using sandbox_download ...
1) call sandbox_download and inspect its exit code/output: if it explicitly
indicates "not found" keep the current quiet skip message and do not increment
count, but if it fails for any other reason surface a clear error (echo/warn
with stderr), mark the backup as failed (set a failure flag or exit non-zero at
end) and do not increment count; update references to sandbox_download,
FILES_CORE, FILES_OPTIONAL, DIRS_OPTIONAL, count, warn, and the optional message
branches accordingly so only true "not found" remains quiet and all other errors
are propagated.
| TS="" | ||
| if [ -d "$BACKUP_BASE" ]; then | ||
| # shellcheck disable=SC2012 # backup dirs are YYYYMMDD-HHMMSS — no special chars | ||
| TS="$(ls -1t "$BACKUP_BASE" 2>/dev/null | head -n1 || true)" | ||
| fi | ||
| [ -n "$TS" ] || fail "No backup directory found under ${BACKUP_BASE}/" | ||
| DEST="${BACKUP_BASE}/${TS}" |
There was a problem hiding this comment.
Avoid the TOCTOU lookup for the backup timestamp.
This rescans ~/.nemoclaw/backups after the backup command finishes. If another backup lands between Line 92 and Line 97, TS can point at the wrong directory and the post-onboard restore uploads a different sandbox's workspace into this one. Please capture the timestamp/path from backup-workspace.sh backup itself instead of asking "what is latest now?".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/upgrade-sandbox.sh` around lines 94 - 100, The current TS lookup is a
TOCTOU race (using ls on BACKUP_BASE) — instead have the backup command itself
emit the created timestamp/path and consume that output instead of rescanning;
modify the code that runs backup-workspace.sh backup to capture its stdout (or
have backup-workspace.sh write the created backup path to a known file) and
assign that value to TS, then validate [ -n "$TS" ] || fail and set
DEST="${BACKUP_BASE}/${TS}" as before; reference the TS variable and the
backup-workspace.sh invocation so you replace the ls-based discovery with the
backup command’s explicit output.
11f856d to
79dd891
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
bin/lib/onboard.js (1)
1229-1239:⚠️ Potential issue | 🟠 MajorEndpoint URL not hydrated for compatible-endpoint providers.
Even after fixing the model/provider persistence issue,
hydrateNonInteractiveInferenceFromRegistry()doesn't restoreNEMOCLAW_ENDPOINT_URL. Non-interactive upgrades of sandboxes usingcompatible-endpointorcompatible-anthropic-endpointwill still fail at lines 1667-1681 where the endpoint URL is required.The registry entry would need to persist
endpointUrlalongside model/provider, and hydration would need to setprocess.env.NEMOCLAW_ENDPOINT_URLfrom that stored field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1229 - 1239, hydrateNonInteractiveInferenceFromRegistry currently infers and sets NEMOCLAW_PROVIDER from the registry entry (using inferRemoteProviderKeyFromStoredName and entry.provider) but does not restore endpoint information for compatible-endpoint providers; update the registry handling to read the stored endpoint field (e.g., entry.endpointUrl) and, when a compatible-endpoint or compatible-anthropic-endpoint provider is inferred, set process.env.NEMOCLAW_ENDPOINT_URL to that value and log it similarly to the provider note; also ensure the registry persistence logic saves endpointUrl alongside model/provider so hydrateNonInteractiveInferenceFromRegistry can read it.scripts/upgrade-sandbox.sh (1)
94-101:⚠️ Potential issue | 🟡 MinorTOCTOU race when discovering backup timestamp.
This rescans
~/.nemoclaw/backupsafterbackup-workspace.sh backupfinishes. If another backup completes between line 92 and line 97,TScould point to the wrong directory.Consider having
backup-workspace.sh backupemit the created timestamp to stdout, then capture it:-bash "$BACKUP_WORKSPACE" backup "$SANDBOX" - -TS="" -if [ -d "$BACKUP_BASE" ]; then - # shellcheck disable=SC2012 # backup dirs are YYYYMMDD-HHMMSS — no special chars - TS="$(ls -1t "$BACKUP_BASE" 2>/dev/null | head -n1 || true)" -fi +TS="$(bash "$BACKUP_WORKSPACE" backup "$SANDBOX" | tail -n1)" +# Assuming backup-workspace.sh outputs the timestamp on the last lineAlternatively, source
backup-workspace.shand calllatest_backup_timestamp()to avoid duplicating the logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgrade-sandbox.sh` around lines 94 - 101, The code currently discovers the latest backup by re-listing BACKUP_BASE into TS which creates a TOCTOU race; instead have the producer script emit the exact timestamp and consume that instead (e.g. run backup-workspace.sh backup and capture its timestamp stdout into TS) or source backup-workspace.sh and call its latest_backup_timestamp() function to get a deterministic timestamp; update the block that sets TS and DEST to use the captured/returned timestamp (referencing TS, BACKUP_BASE, DEST and the backup-workspace.sh backup invocation or latest_backup_timestamp()) and remove the ls-based discovery.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 1217-1245: hydrateNonInteractiveInferenceFromRegistry fails
because registerSandbox never persisted model/provider and
setupInference/createSandbox never call registry.updateSandbox, so
entry.model/provider remain null; fix by persisting the inference config after
sandbox creation or inside setupInference: call
registry.updateSandbox(sandboxName, { model: model /*or modelName var*/,
provider: providerKey /*or provider var*/ }) (and include nimContainer if
applicable) so hydrateNonInteractiveInferenceFromRegistry can read entry.model
and entry.provider; update the call sites (createSandbox/ setupInference) to
pass the correct sandboxName and values.
---
Duplicate comments:
In `@bin/lib/onboard.js`:
- Around line 1229-1239: hydrateNonInteractiveInferenceFromRegistry currently
infers and sets NEMOCLAW_PROVIDER from the registry entry (using
inferRemoteProviderKeyFromStoredName and entry.provider) but does not restore
endpoint information for compatible-endpoint providers; update the registry
handling to read the stored endpoint field (e.g., entry.endpointUrl) and, when a
compatible-endpoint or compatible-anthropic-endpoint provider is inferred, set
process.env.NEMOCLAW_ENDPOINT_URL to that value and log it similarly to the
provider note; also ensure the registry persistence logic saves endpointUrl
alongside model/provider so hydrateNonInteractiveInferenceFromRegistry can read
it.
In `@scripts/upgrade-sandbox.sh`:
- Around line 94-101: The code currently discovers the latest backup by
re-listing BACKUP_BASE into TS which creates a TOCTOU race; instead have the
producer script emit the exact timestamp and consume that instead (e.g. run
backup-workspace.sh backup and capture its timestamp stdout into TS) or source
backup-workspace.sh and call its latest_backup_timestamp() function to get a
deterministic timestamp; update the block that sets TS and DEST to use the
captured/returned timestamp (referencing TS, BACKUP_BASE, DEST and the
backup-workspace.sh backup invocation or latest_backup_timestamp()) and remove
the ls-based discovery.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 74526e07-c1c6-48fb-ac7e-ebd521ecbb1f
📒 Files selected for processing (4)
bin/lib/onboard.jsdocs/workspace/backup-restore.mdscripts/backup-workspace.shscripts/upgrade-sandbox.sh
✅ Files skipped from review due to trivial changes (1)
- docs/workspace/backup-restore.md
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/backup-workspace.sh
79dd891 to
f6c060d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
scripts/backup-workspace.sh (1)
45-55:⚠️ Potential issue | 🟠 MajorDon't downgrade real backup failures to "skipped".
Because
sandbox_download()only returns success/failure, Lines 75-95 still treat every non-zeroopenshell sandbox downloadas a benign skip. That means a transient failure on a core file can still yield a "successful" backup, while optional paths mislabel real gateway/download errors as "not in sandbox."FILES_COREshould fail the run, and the quiet path should only apply when the remote path is definitively missing.Also applies to: 74-95
scripts/upgrade-sandbox.sh (1)
91-100:⚠️ Potential issue | 🟠 MajorCapture the created backup timestamp instead of rescanning
~/.nemoclaw/backups.Line 92 completes the backup, then Lines 95-99 ask “what is latest now?”. If another backup lands in between,
TScan point at the wrong directory and the later restore uploads a different workspace into this sandbox. Havebackup-workspace.sh backupemit the timestamp/path it just created and consume that directly here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgrade-sandbox.sh` around lines 91 - 100, The current flow runs bash "$BACKUP_WORKSPACE" backup "$SANDBOX" then rescans BACKUP_BASE to set TS/DEST which can pick up a race; modify the script so the backup command (BACKUP_WORKSPACE) emits the exact timestamp or path it created (e.g., print the created directory on stdout), capture that output into TS (replace the ls/rescan logic) and validate it before setting DEST, and keep using TS/DEST downstream; update variables and error handling around the call to BACKUP_WORKSPACE, TS, and DEST to rely on the captured value instead of rescanning.bin/lib/onboard.js (2)
1239-1247:⚠️ Potential issue | 🟠 MajorPersist
modelandproviderbefore relying on registry hydration.Line 1244 assumes the sandbox entry already carries inference metadata, but sandboxes created by the current flow are still registered with only
{ name, gpuEnabled }, andbin/lib/registry.js:41-56initializesmodel/providertonull. For those entries, this helper falls through and non-interactive upgrades revert to the default build/model instead of the previous selection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1239 - 1247, hydrateNonInteractiveInferenceFromRegistry fails when registry entries are created without inference metadata because it assumes entry.model/provider are already set; update the sandbox registration flow to persist chosen inference metadata (model and provider) into the registry entry at creation time (the code path that calls registry.getSandbox / registry.add or equivalent), and ensure hydrateNonInteractiveInferenceFromRegistry reads and uses entry.model and entry.provider when present (or sets them on the registry entry) instead of falling back to defaults; reference the hydrateNonInteractiveInferenceFromRegistry helper and the registry.getSandbox/registry.add (or the sandbox registration function) to locate the changes.
1251-1265:⚠️ Potential issue | 🟠 MajorHydrate compatible endpoint URLs too.
This restores
NEMOCLAW_PROVIDERandNEMOCLAW_MODEL, butsetupNim()still requiresNEMOCLAW_ENDPOINT_URLforcompatible-endpointandcompatible-anthropic-endpoint. Non-interactive upgrades of those sandboxes will still fail here unless the endpoint URL is persisted and rehydrated alongside the provider metadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1251 - 1265, The current non-interactive rehydration sets NEMOCLAW_PROVIDER and NEMOCLAW_MODEL from entry but misses restoring endpoint URLs, causing setupNim() to fail for providers like "compatible-endpoint" and "compatible-anthropic-endpoint"; update the hydration logic (near inferRemoteProviderKeyFromStoredName and the block that sets NEMOCLAW_MODEL) to also check entry for persisted endpoint fields (e.g., entry.endpointUrl, entry.endpoint_url, or whichever key your registry uses) and, when present, set process.env.NEMOCLAW_ENDPOINT_URL (and any provider-specific variants) and emit a note() similar to the other rehydrated vars so setupNim() can run non-interactively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 1761-1767: The non-interactive onboarding path only falls back to
~/.nemoclaw/credentials.json for NVIDIA via getCredential("NVIDIA_API_KEY");
mirror that behavior for other remote providers by attempting to read
getCredential(credentialEnv) before exiting when !process.env[credentialEnv]; if
a key is returned, set process.env[credentialEnv] = key so the later checks
succeed. Update the code paths that currently do if
(!process.env[credentialEnv]) { ... process.exit(1) } to first call
getCredential(credentialEnv) and assign it into process.env when present,
referencing the getCredential function and the credentialEnv variable used in
the surrounding logic.
In `@scripts/backup-workspace.sh`:
- Around line 132-139: The loop handling FILES_CORE currently logs a warning on
an openshell sandbox upload failure and continues; change this to fail fast by
exiting with a non-zero status so the restore aborts on any core file upload
error. Specifically, inside the for loop that iterates FILES_CORE and calls
openshell sandbox upload "$sandbox" "${src}/${f}" "${WORKSPACE_PATH}/", replace
the warn "Failed to restore ${f}" behavior with an immediate exit (e.g., call
exit 1 or a dedicated fatal/error function) so count isn't incremented and
upgrade-sandbox.sh cannot complete with partial core restores.
In `@scripts/upgrade-sandbox.sh`:
- Around line 103-109: When FULL_DATA is requested, the script must not proceed
if the openshell sandbox download of /sandbox/.openclaw-data/ into
"${DEST}/openclaw-data/" fails; change the logic around the openshell sandbox
download (the block using FULL_DATA, mkdir -p "${DEST}/openclaw-data", and the
openshell command) so you only create the destination directory after a
successful download or save to a temporary location and move it on success, and
on download failure exit non‑zero (do not just warn) to stop subsequent steps
like the "nemoclaw onboard" sequence; also ensure the later restore hint
printing (around the existing restore-hint lines) is suppressed when the
download failed so you don't show misleading restore instructions for an empty
or absent snapshot.
---
Duplicate comments:
In `@bin/lib/onboard.js`:
- Around line 1239-1247: hydrateNonInteractiveInferenceFromRegistry fails when
registry entries are created without inference metadata because it assumes
entry.model/provider are already set; update the sandbox registration flow to
persist chosen inference metadata (model and provider) into the registry entry
at creation time (the code path that calls registry.getSandbox / registry.add or
equivalent), and ensure hydrateNonInteractiveInferenceFromRegistry reads and
uses entry.model and entry.provider when present (or sets them on the registry
entry) instead of falling back to defaults; reference the
hydrateNonInteractiveInferenceFromRegistry helper and the
registry.getSandbox/registry.add (or the sandbox registration function) to
locate the changes.
- Around line 1251-1265: The current non-interactive rehydration sets
NEMOCLAW_PROVIDER and NEMOCLAW_MODEL from entry but misses restoring endpoint
URLs, causing setupNim() to fail for providers like "compatible-endpoint" and
"compatible-anthropic-endpoint"; update the hydration logic (near
inferRemoteProviderKeyFromStoredName and the block that sets NEMOCLAW_MODEL) to
also check entry for persisted endpoint fields (e.g., entry.endpointUrl,
entry.endpoint_url, or whichever key your registry uses) and, when present, set
process.env.NEMOCLAW_ENDPOINT_URL (and any provider-specific variants) and emit
a note() similar to the other rehydrated vars so setupNim() can run
non-interactively.
In `@scripts/upgrade-sandbox.sh`:
- Around line 91-100: The current flow runs bash "$BACKUP_WORKSPACE" backup
"$SANDBOX" then rescans BACKUP_BASE to set TS/DEST which can pick up a race;
modify the script so the backup command (BACKUP_WORKSPACE) emits the exact
timestamp or path it created (e.g., print the created directory on stdout),
capture that output into TS (replace the ls/rescan logic) and validate it before
setting DEST, and keep using TS/DEST downstream; update variables and error
handling around the call to BACKUP_WORKSPACE, TS, and DEST to rely on the
captured value instead of rescanning.
🪄 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: Pro
Run ID: c11f4466-0a2f-45ae-bc93-2cbe616e4ce6
📒 Files selected for processing (4)
bin/lib/onboard.jsdocs/workspace/backup-restore.mdscripts/backup-workspace.shscripts/upgrade-sandbox.sh
✅ Files skipped from review due to trivial changes (1)
- docs/workspace/backup-restore.md
| const key = getCredential("NVIDIA_API_KEY"); | ||
| if (!key) { | ||
| console.error(" NVIDIA_API_KEY is required for NVIDIA Endpoints in non-interactive mode."); | ||
| console.error(" Set it in the environment or ~/.nemoclaw/credentials.json, or run interactive onboard."); | ||
| process.exit(1); | ||
| } | ||
| process.env.NVIDIA_API_KEY = key; |
There was a problem hiding this comment.
Mirror the credential fallback for the other remote providers.
This fixes NVIDIA_API_KEY, but the adjacent non-build path still exits on !process.env[credentialEnv]. Saved OPENAI_API_KEY/ANTHROPIC_API_KEY/GEMINI_API_KEY/compatible-endpoint credentials in ~/.nemoclaw/credentials.json are still ignored in non-interactive mode, so those upgrades keep failing even when the credential already exists.
Minimal follow-up
- if (isNonInteractive()) {
- if (!process.env[credentialEnv]) {
+ if (isNonInteractive()) {
+ const key = getCredential(credentialEnv);
+ if (!key) {
console.error(` ${credentialEnv} is required for ${remoteConfig.label} in non-interactive mode.`);
process.exit(1);
}
+ process.env[credentialEnv] = key;
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 1761 - 1767, The non-interactive onboarding
path only falls back to ~/.nemoclaw/credentials.json for NVIDIA via
getCredential("NVIDIA_API_KEY"); mirror that behavior for other remote providers
by attempting to read getCredential(credentialEnv) before exiting when
!process.env[credentialEnv]; if a key is returned, set
process.env[credentialEnv] = key so the later checks succeed. Update the code
paths that currently do if (!process.env[credentialEnv]) { ... process.exit(1) }
to first call getCredential(credentialEnv) and assign it into process.env when
present, referencing the getCredential function and the credentialEnv variable
used in the surrounding logic.
| for f in "${FILES_CORE[@]}"; do | ||
| if [ -f "${src}/${f}" ]; then | ||
| if openshell sandbox upload "$sandbox" "${src}/${f}" "${WORKSPACE_PATH}/"; then | ||
| count=$((count + 1)) | ||
| else | ||
| warn "Failed to restore ${f}" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Fail fast on core restore errors.
Line 137 only warns and continues, so upgrade-sandbox.sh can finish with an incomplete persona if one core upload flakes. For FILES_CORE, this should abort the restore instead of counting a partial result as success.
Minimal hardening
for f in "${FILES_CORE[@]}"; do
if [ -f "${src}/${f}" ]; then
if openshell sandbox upload "$sandbox" "${src}/${f}" "${WORKSPACE_PATH}/"; then
count=$((count + 1))
else
- warn "Failed to restore ${f}"
+ fail "Failed to restore required workspace file ${f}"
fi
fi
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for f in "${FILES_CORE[@]}"; do | |
| if [ -f "${src}/${f}" ]; then | |
| if openshell sandbox upload "$sandbox" "${src}/${f}" "${WORKSPACE_PATH}/"; then | |
| count=$((count + 1)) | |
| else | |
| warn "Failed to restore ${f}" | |
| fi | |
| fi | |
| for f in "${FILES_CORE[@]}"; do | |
| if [ -f "${src}/${f}" ]; then | |
| if openshell sandbox upload "$sandbox" "${src}/${f}" "${WORKSPACE_PATH}/"; then | |
| count=$((count + 1)) | |
| else | |
| fail "Failed to restore required workspace file ${f}" | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/backup-workspace.sh` around lines 132 - 139, The loop handling
FILES_CORE currently logs a warning on an openshell sandbox upload failure and
continues; change this to fail fast by exiting with a non-zero status so the
restore aborts on any core file upload error. Specifically, inside the for loop
that iterates FILES_CORE and calls openshell sandbox upload "$sandbox"
"${src}/${f}" "${WORKSPACE_PATH}/", replace the warn "Failed to restore ${f}"
behavior with an immediate exit (e.g., call exit 1 or a dedicated fatal/error
function) so count isn't incremented and upgrade-sandbox.sh cannot complete with
partial core restores.
| if [ "$FULL_DATA" -eq 1 ]; then | ||
| info "Step 2/3: Full OpenClaw snapshot (/sandbox/.openclaw-data/)..." | ||
| mkdir -p "${DEST}/openclaw-data" | ||
| if openshell sandbox download "$SANDBOX" /sandbox/.openclaw-data/ "${DEST}/openclaw-data/"; then | ||
| info "Saved openclaw-data under ${DEST}/openclaw-data/" | ||
| else | ||
| warn "openclaw-data download failed or was empty; workspace files in ${DEST}/ are still valid." |
There was a problem hiding this comment.
Don't continue a --full-data upgrade after the snapshot fails.
If the user explicitly asked for --full-data, Lines 106-109 should not just warn and keep going into nemoclaw onboard. A transient failure here can destroy agents/sessions/plugins during the recreate step, and Lines 151-152 can still print a restore hint because the empty destination directory was already created.
Also applies to: 151-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/upgrade-sandbox.sh` around lines 103 - 109, When FULL_DATA is
requested, the script must not proceed if the openshell sandbox download of
/sandbox/.openclaw-data/ into "${DEST}/openclaw-data/" fails; change the logic
around the openshell sandbox download (the block using FULL_DATA, mkdir -p
"${DEST}/openclaw-data", and the openshell command) so you only create the
destination directory after a successful download or save to a temporary
location and move it on success, and on download failure exit non‑zero (do not
just warn) to stop subsequent steps like the "nemoclaw onboard" sequence; also
ensure the later restore hint printing (around the existing restore-hint lines)
is suppressed when the download failed so you don't show misleading restore
instructions for an empty or absent snapshot.
|
✨ Thanks for submitting this PR with a detailed summary, it proposes a new feature to improve the upgrade experience and inference setup. |
|
I attempted to port this branch across the JS→TS migration and merge the latest Please start with: git fetch origin
git merge origin/main
npx tsx scripts/ts-migration-assist.ts --base origin/main --write
npm run build:cli
npm run typecheck:cli
npm run lint
npm test |
Thanks for not forgetting, I'll update the PR ASAP |
…dpoint URL support
f6c060d to
12e623e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemoclaw/scripts/backup-workspace.sh`:
- Around line 57-61: The current do_backup function creates global
timestamp-only backup dirs (dest="${BACKUP_BASE}/${ts}") so restores can
mistakenly apply sandbox A backups to sandbox B; modify do_backup (and
corresponding restore logic) to either namespace backups per sandbox (e.g.,
include the sandbox name in the path under BACKUP_BASE like
"${BACKUP_BASE}/${sandbox}/${ts}") or write a small manifest file inside each
timestamped backup (containing the originating sandbox name) and update the
restore routine to read that manifest and reject restores when the requested
target sandbox does not match the manifest's sandbox value (ensure you update
functions/variables: do_backup, BACKUP_BASE, dest, and the restore function
handling lines ~118-125).
In `@nemoclaw/scripts/upgrade-sandbox.sh`:
- Around line 135-141: Wrap the "nemoclaw onboard" invocation so its non-zero
exit doesn't abort before printing restore instructions: run the onboard command
((cd "$ROOT" && nemoclaw onboard)) and if it fails capture its exit code, then
reuse the existing info messages that reference $NO_RESTORE, $SANDBOX, $TS and
$BACKUP_WORKSPACE to print either the skip message or the manual restore
command, and finally exit with the original onboard exit code; update the block
that currently contains "(cd "$ROOT" && exec nemoclaw onboard)" to perform this
guarded call and conditional printing.
In `@src/lib/onboard.ts`:
- Around line 4631-4643: The resumeUpdate block currently calls
registry.updateSandbox(sandboxName, ...) while sandboxName may be unset; change
the flow so updates persist under a real key: when sandboxName is falsy, write
the resumeUpdate to the temporary GATEWAY_NAME key (instead of sandboxName) and
then after createSandbox() (the function that returns the real sandbox name in
Step 6) completes, call registry.updateSandbox(realSandboxName, resumeUpdate)
again to copy/merge settings. Update the logic around resumeUpdate,
registry.updateSandbox, sandboxName and createSandbox() so the
provider/model/endpointUrl are guaranteed to be stored under GATEWAY_NAME
initially and reconciled to the real sandbox name after creation.
- Around line 1796-1800: Add the missing endpointUrl field to the SandboxEntry
interface by declaring endpointUrl?: string | null; inside the SandboxEntry
interface in src/lib/registry.ts so usages like entry.endpointUrl in onboard.ts
are properly typed; also remove or revisit the `@ts-nocheck` at the top of
src/lib/onboard.ts so TypeScript will catch similar mismatches during build/CI.
🪄 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: Pro
Run ID: fd16b383-0a3c-4744-b0aa-1aece4cb028f
📒 Files selected for processing (4)
nemoclaw/docs/workspace/backup-restore.mdnemoclaw/scripts/backup-workspace.shnemoclaw/scripts/upgrade-sandbox.shsrc/lib/onboard.ts
✅ Files skipped from review due to trivial changes (1)
- nemoclaw/docs/workspace/backup-restore.md
| const resumeUpdate = {}; | ||
| if (nimContainer) resumeUpdate.nimContainer = nimContainer; | ||
| if (model) resumeUpdate.model = model; | ||
| if (provider) resumeUpdate.provider = provider; | ||
| if ( | ||
| endpointUrl && | ||
| (provider === "compatible-endpoint" || provider === "compatible-anthropic-endpoint") | ||
| ) { | ||
| resumeUpdate.endpointUrl = endpointUrl; | ||
| } | ||
| if (Object.keys(resumeUpdate).length > 0) { | ||
| registry.updateSandbox(sandboxName, resumeUpdate); | ||
| } |
There was a problem hiding this comment.
Persist inference settings under a real registry key.
At this point the sandbox name is still unset on a fresh onboard, so these registry.updateSandbox(sandboxName, ...) calls can run with null/empty and never attach provider/model/endpointUrl to the actual sandbox entry. That breaks the next non-interactive upgrade, because the new hydration path has nothing reliable to read back. Write to GATEWAY_NAME until Step 6 produces a real sandbox name, or repeat the update after createSandbox() returns it.
Also applies to: 4666-4676
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/onboard.ts` around lines 4631 - 4643, The resumeUpdate block
currently calls registry.updateSandbox(sandboxName, ...) while sandboxName may
be unset; change the flow so updates persist under a real key: when sandboxName
is falsy, write the resumeUpdate to the temporary GATEWAY_NAME key (instead of
sandboxName) and then after createSandbox() (the function that returns the real
sandbox name in Step 6) completes, call registry.updateSandbox(realSandboxName,
resumeUpdate) again to copy/merge settings. Update the logic around
resumeUpdate, registry.updateSandbox, sandboxName and createSandbox() so the
provider/model/endpointUrl are guaranteed to be stored under GATEWAY_NAME
initially and reconciled to the real sandbox name after creation.
12e623e to
5e09de3
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (5)
scripts/backup-workspace.sh (2)
132-138:⚠️ Potential issue | 🟠 MajorAbort the restore when a core file upload fails.
Line 137 only warns and continues, so the upgrade flow can finish with an incomplete persona after a transient upload error.
Minimal hardening
for f in "${FILES_CORE[@]}"; do if [ -f "${src}/${f}" ]; then if openshell sandbox upload "$sandbox" "${src}/${f}" "${WORKSPACE_PATH}/"; then count=$((count + 1)) else - warn "Failed to restore ${f}" + fail "Failed to restore required workspace file ${f}" fi fi done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/backup-workspace.sh` around lines 132 - 138, The loop over FILES_CORE uses openshell sandbox upload but only calls warn on failure, which allows an incomplete restore; update the failure branch in the for loop that iterates over "${FILES_CORE[@]}" (the block using openshell sandbox upload "$sandbox" "${src}/${f}" "${WORKSPACE_PATH}/") to abort the script on upload failure—either call exit 1 immediately or set a failure flag and break then exit non‑zero after the loop; ensure any cleanup/logging (e.g., via warn) still runs before exiting so the restore is not allowed to continue with missing core files.
74-95:⚠️ Potential issue | 🟠 MajorDon't treat failed downloads as benign skips.
Lines 74-95 still allow a backup to look successful after required files fail, and optional paths still collapse every non-zero
openshell sandbox downloadinto a normal "not in sandbox" skip. That can leaveupgrade-sandbox.shoperating on a partial backup without surfacing the real failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/backup-workspace.sh` around lines 74 - 95, The backup currently treats any non-zero exit from sandbox_download as a benign skip; change the handling so true failures surface: update sandbox_download to return distinct codes (e.g., 2 = not found, 1 = fatal error) and then modify the loops that iterate over FILES_CORE, FILES_OPTIONAL and DIRS_OPTIONAL so that sandbox_download success still increments count, a return code indicating "not found" continues to print the existing optional skip message, but any fatal error return causes an error log and immediate exit (non-zero) for core files and for optional paths when the error code == 1; reference sandbox_download, FILES_CORE, FILES_OPTIONAL, DIRS_OPTIONAL and count when making these changes.scripts/upgrade-sandbox.sh (3)
92-100:⚠️ Potential issue | 🟠 MajorCapture the backup timestamp from the backup command itself.
Lines 92-100 ask "what is latest now?" after the backup finishes. If another backup lands in
${BACKUP_BASE}in that window, this script can restore the wrong workspace into the sandbox.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgrade-sandbox.sh` around lines 92 - 100, The current logic computes TS by listing ${BACKUP_BASE} after running bash "$BACKUP_WORKSPACE" backup "$SANDBOX", which can race with other backups; instead capture the backup identifier/timestamp directly from the backup command invocation (e.g., assign TS from the stdout of bash "$BACKUP_WORKSPACE" backup "$SANDBOX"), validate it's non-empty (same check that currently uses [ -n "$TS" ] || fail), and then set DEST="${BACKUP_BASE}/${TS}"; update references to TS/DEST and the fail path accordingly so the script restores the exact backup the backup command created rather than re-scanning ${BACKUP_BASE}.
103-110:⚠️ Potential issue | 🟠 Major
--full-datashould fail the upgrade if the snapshot fails.When the user explicitly requests
--full-data, Lines 103-110 should not warn and continue into the rebuild path. Also, because Line 105 pre-creates${DEST}/openclaw-data, Lines 151-152 can still print a restore hint for an empty snapshot.Also applies to: 151-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgrade-sandbox.sh` around lines 103 - 110, The script currently pre-creates "${DEST}/openclaw-data" and merely warns if openshell sandbox download fails, which allows --full-data (FULL_DATA) to continue incorrectly; change the logic so the directory is only created after a successful download and, if FULL_DATA is 1 and the openshell sandbox download command fails or is empty, exit with a non-zero status (use error/exit) instead of warn so the upgrade aborts; update the else branch of the openshell sandbox download check to call error+exit when FULL_DATA is set and move the mkdir -p "${DEST}/openclaw-data" to only run on successful download to avoid leaving an empty directory that later prints a misleading restore hint.
135-141:⚠️ Potential issue | 🟠 MajorPrint the manual restore command if
nemoclaw onboardfails.With
set -e, a non-zero exit at Line 135 aborts the script before either automatic restore or the fallback restore instructions are shown.Guard the onboard call and preserve the restore hint
- (cd "$ROOT" && exec nemoclaw onboard) + if ! (cd "$ROOT" && nemoclaw onboard); then + rc=$? + warn "nemoclaw onboard failed." + info "Restore manually with: ./scripts/backup-workspace.sh restore ${SANDBOX} ${TS}" + exit "$rc" + fi if [ "$NO_RESTORE" -eq 1 ]; then info "Skipping automatic workspace restore (--no-restore)." info "To restore manually: ./scripts/backup-workspace.sh restore ${SANDBOX} ${TS}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgrade-sandbox.sh` around lines 135 - 141, The call to "nemoclaw onboard" can abort the script under set -e, preventing the restore hints from being printed; wrap or guard that call so failure is caught and a helpful message plus the manual restore hint are still displayed. Specifically, run the "nemoclaw onboard" invocation in a guarded way (capture its exit status or temporarily disable set -e) and on non-zero return call info with the failure context and still continue to the existing restore hint and restore logic that references BACKUP_WORKSPACE, SANDBOX, TS, and NO_RESTORE so the user always sees the fallback restore instructions even when "nemoclaw onboard" fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/backup-workspace.sh`:
- Around line 132-138: The loop over FILES_CORE uses openshell sandbox upload
but only calls warn on failure, which allows an incomplete restore; update the
failure branch in the for loop that iterates over "${FILES_CORE[@]}" (the block
using openshell sandbox upload "$sandbox" "${src}/${f}" "${WORKSPACE_PATH}/") to
abort the script on upload failure—either call exit 1 immediately or set a
failure flag and break then exit non‑zero after the loop; ensure any
cleanup/logging (e.g., via warn) still runs before exiting so the restore is not
allowed to continue with missing core files.
- Around line 74-95: The backup currently treats any non-zero exit from
sandbox_download as a benign skip; change the handling so true failures surface:
update sandbox_download to return distinct codes (e.g., 2 = not found, 1 = fatal
error) and then modify the loops that iterate over FILES_CORE, FILES_OPTIONAL
and DIRS_OPTIONAL so that sandbox_download success still increments count, a
return code indicating "not found" continues to print the existing optional skip
message, but any fatal error return causes an error log and immediate exit
(non-zero) for core files and for optional paths when the error code == 1;
reference sandbox_download, FILES_CORE, FILES_OPTIONAL, DIRS_OPTIONAL and count
when making these changes.
In `@scripts/upgrade-sandbox.sh`:
- Around line 92-100: The current logic computes TS by listing ${BACKUP_BASE}
after running bash "$BACKUP_WORKSPACE" backup "$SANDBOX", which can race with
other backups; instead capture the backup identifier/timestamp directly from the
backup command invocation (e.g., assign TS from the stdout of bash
"$BACKUP_WORKSPACE" backup "$SANDBOX"), validate it's non-empty (same check that
currently uses [ -n "$TS" ] || fail), and then set DEST="${BACKUP_BASE}/${TS}";
update references to TS/DEST and the fail path accordingly so the script
restores the exact backup the backup command created rather than re-scanning
${BACKUP_BASE}.
- Around line 103-110: The script currently pre-creates "${DEST}/openclaw-data"
and merely warns if openshell sandbox download fails, which allows --full-data
(FULL_DATA) to continue incorrectly; change the logic so the directory is only
created after a successful download and, if FULL_DATA is 1 and the openshell
sandbox download command fails or is empty, exit with a non-zero status (use
error/exit) instead of warn so the upgrade aborts; update the else branch of the
openshell sandbox download check to call error+exit when FULL_DATA is set and
move the mkdir -p "${DEST}/openclaw-data" to only run on successful download to
avoid leaving an empty directory that later prints a misleading restore hint.
- Around line 135-141: The call to "nemoclaw onboard" can abort the script under
set -e, preventing the restore hints from being printed; wrap or guard that call
so failure is caught and a helpful message plus the manual restore hint are
still displayed. Specifically, run the "nemoclaw onboard" invocation in a
guarded way (capture its exit status or temporarily disable set -e) and on
non-zero return call info with the failure context and still continue to the
existing restore hint and restore logic that references BACKUP_WORKSPACE,
SANDBOX, TS, and NO_RESTORE so the user always sees the fallback restore
instructions even when "nemoclaw onboard" fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f708bc5b-aad2-4493-aef5-c4a8373eba1d
📒 Files selected for processing (3)
docs/workspace/backup-restore.mdscripts/backup-workspace.shscripts/upgrade-sandbox.sh
✅ Files skipped from review due to trivial changes (1)
- docs/workspace/backup-restore.md
5e09de3 to
4dc4c1f
Compare
|
Thanks for the upgrade script and inference hydration work. The codebase has changed significantly since March 25 — including a full TypeScript migration — so this will need a rebase on |
I did rebase a couple of days ago, but @ericksoa said he wants to do a more holistic approach anyways |
wscurran
added
feature
3 participants
Summary
Port of the original PR to the TypeScript-migrated codebase (
src/lib/onboard.ts), with CodeRabbit feedback addressed.Add a sandbox upgrade workflow that backs up workspace files, optionally snapshots full OpenClaw state, and re-runs
nemoclaw onboard— restoring the workspace automatically afterward. Also enables non-interactive onboard to reuse provider/model/endpointUrl settings from the sandbox registry, so upgrades skip inference prompts.Changes
src/lib/onboard.ts: addinferRemoteProviderKeyFromStoredName()andhydrateNonInteractiveInferenceFromRegistry()to fill provider/model/endpointUrl env vars from registry during non-interactive onboard; usegetCredential()for NVIDIA_API_KEY and all remote provider credentials in non-interactive mode; move registry persistence fromsetupInference()toonboard()with endpointUrl support for compatible endpointsscripts/upgrade-sandbox.sh(new): orchestrates backup → optional--full-datasnapshot → optional--run-onboard→ automatic restorescripts/backup-workspace.sh: split file lists into core/optional (quieter output for missing optional files), addsandbox_download()helper, add portablelatest_backup_timestamp()docs/workspace/backup-restore.md: document the upgrade workflowChanges from original submission
src/lib/onboard.tsinstead of deletedbin/lib/onboard.js(TypeScript migration landed in refactor(ts-migration): move onboard and cli to typescript #1673/refactor(cli): remove legacy bin lib shims #1713)endpointUrlfor compatible endpoints in registry, hydrateNEMOCLAW_ENDPOINT_URLfrom registrygetCredential(credentialEnv)for all remote providers (OpenAI, Anthropic, Gemini, compatible) in non-interactive mode, not just NVIDIAsetupInference()toonboard()for complete metadata (model, provider, nimContainer, endpointUrl)Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check)../scripts/upgrade-sandbox.sh my-assistantbacks up and prints instructions./scripts/upgrade-sandbox.sh --run-onboard --yes my-assistantruns full cyclenpm testpasses. (blocked by pre-existinglegacy-path-guardtest failure on upstream/main)Checklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Summary by CodeRabbit
Documentation
New Features
Improvements
Signed-off-by: Ruben Hagege rhagege@nvidia.com