fix(sandbox): verify session-export download wrote a file before recording a bundle (#7367)#7371
Conversation
…rding a bundle (#7367) `openshell sandbox download` has an upstream process-exit race (NVIDIA/OpenShell) that can report success (exit 0) even when the transfer was rejected or failed and no file was written. NemoClaw's session export/backup trusted that exit code alone, so a dropped code could record a rejected or partial download as a valid session bundle. Add assertDownloadedFile(), which re-checks the outcome against the file system after each trusted `sandbox download`: the destination must exist as a regular file (and, for bundles that are never legitimately empty, be non-empty). Wire it into the three export download sites (OpenClaw tar, OpenClaw per-file dir, hermes). The hermes site verifies existence only, since a zero-session hermes export can legitimately be empty. This is a NemoClaw-side defensive mitigation; the root-cause exit-code race remains an upstream OpenShell fix, so this refs rather than closes the issue. Refs #7367 Signed-off-by: Dongni-Yang <dongniy@nvidia.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds filesystem-aware download verification and integrates staged artifact validation into OpenClaw and Hermes session exports. Tests cover missing, directory, empty, non-empty, stale-destination, and non-zero-status outcomes. ChangesSandbox download verification
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 8507e7c in the TypeScript / code-coverage/cliThe overall coverage in commit 8507e7c in the Show a code coverage summary of the most impacted files.
Updated |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/sandbox/sessions/download-verify.ts`:
- Around line 15-20: Update the contract comment for requireNonEmpty to remove
Hermes exports from the examples described as never legitimately empty, while
retaining the guidance for gzip tarballs and individual session files.
- Around line 49-52: Move the fs.statSync(hostPath) call out of the action
helper and into an appropriate adapter under src/lib/adapters. Update the
download verification flow to obtain filesystem metadata through that adapter
while preserving the existing validation and error-handling behavior around
stat.
In `@src/lib/actions/sandbox/sessions/export.test.ts`:
- Around line 523-530: Ensure the mkdirSync spy created in the
exportSandboxSessions test is restored even when the rejection assertion fails.
Wrap the await expect assertion in a try/finally that calls
mkdirSpy.mockRestore(), or add equivalent restoration to shared cleanup while
preserving the existing assertion behavior.
In `@src/lib/actions/sandbox/sessions/export.ts`:
- Around line 218-221: Prevent stale files from being accepted after an
exit-0/no-write download. In src/lib/actions/sandbox/sessions/export.ts lines
218-221, use a fresh per-export staging path for the download, run
assertDownloadedFile against it, then publish it to localPath only after
verification. In src/lib/actions/sandbox/sessions/download-verify.ts lines
37-69, document or enforce that the verified output path must be fresh. In
src/lib/actions/sandbox/sessions/export.test.ts lines 517-531, add a regression
test covering a pre-existing non-empty destination with an exit-0/no-write
download and assert that the export fails.
🪄 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: 78327a0d-49d1-4259-b835-26495fea028e
📒 Files selected for processing (4)
src/lib/actions/sandbox/sessions/download-verify.test.tssrc/lib/actions/sandbox/sessions/download-verify.tssrc/lib/actions/sandbox/sessions/export.test.tssrc/lib/actions/sandbox/sessions/export.ts
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
1 warning · 0 suggestionsWarningsWarnings do not block.
|
…ad-artifact-verify
Address review on #7371: verify each download against a fresh mkdtemp staging path and rename into place only after it passes, so a stale file from an earlier export cannot satisfy the exit-0/no-write check. Covers both the tar bundle and the per-file dir path (the tar path had the same exposure). Document the fresh-path precondition on assertDownloadedFile; correct the requireNonEmpty comment re: hermes; restore leaked fs spies in finally. Refs #7367 Signed-off-by: Dongni-Yang <dongniy@nvidia.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/sandbox-sessions-export-cli.test.ts`:
- Around line 174-180: Update the test around the downloadLines assertions to
verify staging-path targets for both sid-a.jsonl and sid-b.jsonl downloads, not
only downloadLines[0]. Preserve the existing checks that each published file
exists at its final outDir path, ensuring the full export flow validates staging
before publication for every directory download.
🪄 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: 636a4abb-b27d-4c66-b81e-23e3074f1212
📒 Files selected for processing (4)
src/lib/actions/sandbox/sessions/download-verify.tssrc/lib/actions/sandbox/sessions/export.test.tssrc/lib/actions/sandbox/sessions/export.tstest/sandbox-sessions-export-cli.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/actions/sandbox/sessions/download-verify.ts
- src/lib/actions/sandbox/sessions/export.ts
- src/lib/actions/sandbox/sessions/export.test.ts
Fix the build-typecheck and codebase-growth-guardrails failures on the prior commit: type the statSync mock parameter (TS7006) and replace the if-based conditional mocks with a ternary + throwEnoent helper so the changed test files add no if statements. Also assert staging-path targets for both directory downloads (CodeRabbit), not just the first. Refs #7367 Signed-off-by: Dongni-Yang <dongniy@nvidia.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
apurvvkumaria
left a comment
There was a problem hiding this comment.
Sensitive-path review complete at exact head 8507e7c. The fresh 0700 staging directory, artifact verification, chmod, same-filesystem rename, and unconditional cleanup close the exit-0/no-artifact gap without introducing a concrete security or correctness regression. CI, E2E coordination, DCO, signatures, and review threads are clean. Non-blocking test-depth and wording refinements can be handled as fix-forward work.
Resolve to the rebase-equivalent tree after #7371 squash-merged: the branch payload is exactly the user-facing download probe/verification (download.ts, download-verify.ts, and their tests), byte-identical to the reviewed tip, applied on top of main. Refs #7367 Signed-off-by: Dongni-Yang <dongniy@nvidia.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
<!-- markdownlint-disable MD041 --> ## Summary This PR adds the canonical dated release entry for NemoClaw v0.0.94 before the tag is cut. The entry reconciles all 26 commits since v0.0.93 and links each user-visible change to its owning documentation. ## Changes - Add `docs/changelog/2026-07-24.mdx` with the exact `## v0.0.94` heading, parser-safe SPDX comment, release summary, and detailed bullets. - Record sandbox restore and update behavior, onboarding and inference changes, network policy behavior, security evidence, Hermes build performance, DGX Station guidance, and E2E validation changes. - Preserve `docs/` as the source of truth without changing the AI-agent documentation routing skill. - Use [E2E run 30075443016](https://github.com/NVIDIA/NemoClaw/actions/runs/30075443016) for release QA evidence at exact tested SHA `04e6dfa883071dda9df429c66e73168e1a995cba`. ### Source summary - [#7461](#7461) -> `docs/changelog/2026-07-24.mdx`: Record the ownership-preserving Hermes image layer reduction and hosted timing comparison. - [#7460](#7460) -> `docs/changelog/2026-07-24.mdx`: Record removal of candidate Hermes swap setup from E2E validation. - [#7458](#7458) -> `docs/security/fern-5.80.1-dependency-review.md`, `docs/changelog/2026-07-24.mdx`: Record the reviewed Fern CLI update. - [#7457](#7457) -> `docs/changelog/2026-07-24.mdx`: Record periodic runner-pressure telemetry. - [#7455](#7455) -> `docs/changelog/2026-07-24.mdx`: Record non-blocking absent Fern previews. - [#7450](#7450) -> `docs/changelog/2026-07-24.mdx`: Record stable cancellation handling for live-test child processes. - [#7449](#7449) -> `docs/changelog/2026-07-24.mdx`: Record parallel plugin EXDEV coverage. - [#7448](#7448) -> `docs/changelog/2026-07-24.mdx`: Record isolated long-running E2E lanes. - [#7444](#7444) -> `docs/changelog/2026-07-24.mdx`: Record exact-head Hermes swap validation. - [#7437](#7437) -> `docs/manage-sandboxes/backup-restore.mdx`, `docs/changelog/2026-07-24.mdx`: Record gateway pairing and authenticated verification after cross-sandbox restore. - [#7436](#7436) -> `docs/manage-sandboxes/backup-restore.mdx`, `docs/reference/commands.mdx`, `docs/changelog/2026-07-24.mdx`: Record selected stale-state cleanup and Hermes virtual-environment access repair. - [#7385](#7385) -> `docs/network-policy/customize-network-policy.mdx`, `docs/changelog/2026-07-24.mdx`: Record the read-only agent-variant route check. - [#7371](#7371) -> `docs/changelog/2026-07-24.mdx`: Record host-artifact verification for session exports. - [#7359](#7359) -> `docs/changelog/2026-07-24.mdx`: Record platform validation for managed vLLM model overrides. - [#7356](#7356) -> `docs/changelog/2026-07-24.mdx`: Record token-shaped value redaction for `sandbox doctor --json`. - [#7354](#7354) -> `docs/security/advisory-early-warning.md`, `docs/changelog/2026-07-24.mdx`: Record advisory correlation and retained audit provenance. - [#7352](#7352) -> `docs/network-policy/customize-network-policy.mdx`, `docs/network-policy/integration-policy-examples.mdx`, `docs/reference/commands.mdx`, `docs/changelog/2026-07-24.mdx`: Record preset reapplication and bounded `tls: skip` guidance. - [#7345](#7345) -> `docs/security/openclaw-2026.6.10-dependency-review.md`, `docs/security/openclaw-2026.7.1-dependency-review.md`, `docs/changelog/2026-07-24.mdx`: Record reviewed npm audit exception enforcement. - [#7340](#7340) -> `docs/network-policy/customize-network-policy.mdx`, `docs/changelog/2026-07-24.mdx`: Record the repaired CLI-reference route. - [#7334](#7334) -> `docs/get-started/dgx-station-preparation.mdx`, `docs/changelog/2026-07-24.mdx`: Record the qualified OTA metadata fallback and narrowed override wording. - [#7322](#7322) -> `docs/changelog/2026-07-24.mdx`: Reconcile the gateway source tag added to plugin registration banners. - [#7284](#7284) -> `docs/manage-sandboxes/update-sandboxes.mdx`, `docs/changelog/2026-07-24.mdx`: Record read-only `upgrade-sandboxes --check` behavior and recorded-gateway selection. - [#7277](#7277) -> `docs/changelog/2026-07-24.mdx`: Reconcile deterministic gateway TCP refusal coverage. - [#7234](#7234) -> `docs/reference/troubleshooting.mdx`, `docs/changelog/2026-07-24.mdx`: Record preserved DGX Spark managed vLLM Express intent on resume. - [#7185](#7185) -> `docs/reference/troubleshooting.mdx`, `docs/changelog/2026-07-24.mdx`: Record IPv4 fallback DNS selection and exact resolver probing. - [#6820](#6820) -> `docs/reference/commands.mdx`, `docs/changelog/2026-07-24.mdx`: Record the versioned, redacted `--events=jsonl` onboarding stream. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: `npx vitest run test/changelog-docs.test.ts` passed 6/6 tests. - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `docs/changelog/2026-07-24.mdx`; the writing rules, documentation style, exact release range, skip terms, published routes, and product scope were reviewed; the changelog test passed 6/6; `npm run docs` passed with route checking OK, zero errors, and two existing warnings. - Agent: Codex Desktop <!-- docs-review-head-sha: 65368f9 --> <!-- docs-review-agents-blob-sha: 9c9b36d --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable - Station profile/scenario: Not applicable - Result: Not applicable - Supporting evidence: Not applicable. This PR does not change `scripts/prepare-dgx-station-host.sh`. ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run test/changelog-docs.test.ts` passed 6/6 tests. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable to the dated changelog entry. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only). The build passed with zero errors and two existing Fern warnings. - [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). Native dated changelog entries use the required parser-safe MDX SPDX comment and no frontmatter. --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added the v0.0.94 release changelog. * Documented improvements to sandbox snapshot and restore behavior. * Added updates for gateway selection, policy comparisons, onboarding event output, and DGX recovery workflows. * Documented enhanced diagnostics redaction, npm audit provenance, image assembly performance, and validation stability improvements. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Summary
openshell sandbox downloadhas an upstream process-exit race (NVIDIA/OpenShell) that can report success (exit 0) even when the transfer was rejected or failed and no file was written. NemoClaw's session export/backup (sessions/export.ts) trusted that exit code alone, so a dropped exit code could record a rejected or partial download as a valid session bundle. This adds a NemoClaw-side artifact check after each trusted download.Related Issue
Refs #7367. This is a defensive mitigation on the NemoClaw side, not the root-cause fix — the exit-code race lives in OpenShell's
openshell sandbox download(the#[tokio::main]teardown path incrates/openshell-cli), so #7367 is closed on the NemoClaw board as upstream-owned; the root-cause CLI exit-code fix (makingopenshell sandbox downloadexit non-zero on a rejected path) is tracked in NVIDIA/OpenShell. Reproduced against a source build of OpenShell v0.0.85 (the version in #7367): the/sandboxpath-traversal guard correctly rejects the source and prints an error, but the process intermittently exits 0.Changes
src/lib/actions/sandbox/sessions/download-verify.ts—assertDownloadedFile(download, hostPath, { remoteLabel, sandboxName, requireNonEmpty? })re-checks the outcome against the file system after asandbox download: rejects a non-zero exit (preserving the previousFailed to download '<x>' from sandbox '<y>' (exit <n>).wording), and rejects an exit-0 that produced no file, a non-regular file, or (whenrequireNonEmpty) an empty file.sessions/export.ts— the three trusted download sites now callassertDownloadedFileinstead of only checking the exit status:requireNonEmpty: true(a bundle of ≥1 file is never legitimately empty; the zero-session case is already refused earlier).download-verify.test.ts(real tmpdir, one case per branch) plus export-level regressions for the tar (missing + empty), dir (missing), and hermes (missing) sites, each asserting the export aborts and still cleans up the in-sandbox staging file.Out of scope (verified, not gaps): the user-facing
sandbox downloadcommand supports directory downloads and streams to the user;dashboard.ts/selection-drift.tsalready re-check the file system after their downloads. These are documented in the review notes, not left silently.Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailable —npm run check:diffpassed (hooks skipped locally; commitlint + Biome + TypeScript all green)npx vitest run --project cli src/lib/actions/sandbox/sessions/download-verify.test.ts src/lib/actions/sandbox/sessions/export.test.ts→ 39/39;test/sandbox-sessions-export-cli.test.ts→ 13/13Signed-off-by: Dongni-Yang dongniy@nvidia.com
🤖 Generated with Claude Code
Summary by CodeRabbit