fix(sandbox): restore startup before readiness - #8146
Conversation
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
senthilr-nv
added
area: docs
|
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:
📝 WalkthroughWalkthroughSandbox startup now accepts ChangesSandbox startup recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant startDockerSandbox
participant recoverDockerDriverSandbox
participant startSandbox
participant restoreLockedStateDirStartupAccess
participant restoreSandboxStartupState
participant gatewayChecks
startDockerSandbox->>recoverDockerDriverSandbox: Request runtime-running readiness
recoverDockerDriverSandbox-->>startDockerSandbox: Confirm running container
startDockerSandbox->>startSandbox: Continue sandbox startup
startSandbox->>restoreLockedStateDirStartupAccess: Restore locked startup access
startSandbox->>restoreSandboxStartupState: Restore managed process state
restoreSandboxStartupState-->>startSandbox: Restoration complete
startSandbox->>gatewayChecks: Verify gateway and host forwards
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit e9735c3 in the TypeScript / code-coverage/cliThe overall coverage in commit e9735c3 in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-8146.docs.buildwithfern.com/nemoclaw |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
4 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. Since last review: 0 prior items resolved · 0 still apply · 0 new items found 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
senthilr-nv
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/actions/sandbox/start.test.ts (1)
48-66: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCover the production restoration wiring.
The harness always injects
restoreStartupStateat Line 48 and passes it tostartSandboxat Line 66. The tests therefore verify only the injected callback contract. They do not execute the default helper insrc/lib/actions/sandbox/start.tsLines 21-24 orrestoreSandboxStartupStateinsrc/lib/actions/sandbox/connect.tsLines 894-897. A broken dynamic import or export wiring can pass this suite. Add one test without the override and stub theconnectmodule, or add a focused test forrestoreSandboxStartupState. (raw.githubusercontent.com)As per path instructions, tests must prove that public entrypoints reach the new path and that the old path is deleted or cannot execute.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/start.test.ts` around lines 48 - 66, Add a test in the startSandbox suite that omits the restoreStartupState dependency override and stubs the connect module to verify the production default reaches restoreSandboxStartupState. Assert the public startSandbox entrypoint invokes the restored startup-state path, and ensure the legacy injected-only path is removed or cannot execute when the override is absent.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/actions/sandbox/start.test.ts`:
- Around line 48-66: Add a test in the startSandbox suite that omits the
restoreStartupState dependency override and stubs the connect module to verify
the production default reaches restoreSandboxStartupState. Assert the public
startSandbox entrypoint invokes the restored startup-state path, and ensure the
legacy injected-only path is removed or cannot execute when the override is
absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 645175c9-0277-4d5c-b65b-aa6a017d0c66
📒 Files selected for processing (4)
docs/manage-sandboxes/run-sandboxes.mdxsrc/lib/actions/sandbox/connect.tssrc/lib/actions/sandbox/start.test.tssrc/lib/actions/sandbox/start.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/manage-sandboxes/run-sandboxes.mdx
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
Maintainer CI exception: The |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed exact head 96991b3. Recovery is correctly placed after Docker runtime availability and before OpenShell readiness verification, and I found no demonstrated blocking defect. Non-blocking fast follow: add the accepted OpenClaw and Hermes by shields-up and shields-down real-startup matrix as a narrowly scoped test PR. Functional CI passes; E2E remains pending.
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.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/e2e/live/hermes-shields-config.test.ts`:
- Around line 110-130: Update the stop, start, and status invocations in the
Hermes shields test to use HostCliClient.nemoclaw() instead of
host.command("nemoclaw", ...), while preserving their existing arguments and
assertions so the configured NEMOCLAW_CLI_BIN is used.
🪄 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: 1978b72d-1eb9-43a9-b8c8-6ff9461905ca
📒 Files selected for processing (2)
test/e2e/live/hermes-shields-config.test.tstest/e2e/live/shields-config.test.ts
senthilr-nv
added
bug-fix
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/state-dir-guard.test.ts (1)
951-958: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the populated startup-traversable recovery state.
Line 951 resets
credentialsDirto0700before addingtoken.json.
The test therefore skips the branch that must reseal a non-emptyroot:sandbox 0710directory.
Keep the directory at0710when adding the token, then verify thatstartupreturns it to0700.Proposed test change
- fs.chmodSync(credentialsDir, 0o700); fs.writeFileSync(path.join(credentialsDir, "token.json"), "secret\n", { mode: 0o600 });As per path instructions, security-boundary tests must cover negative paths that prevent bypasses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/state-dir-guard.test.ts` around lines 951 - 958, Update the test around runGuard("startup", configDir) to keep credentialsDir at 0710 while adding token.json, rather than resetting it to 0700. Preserve the token file’s 0600 mode, then assert startup succeeds and reseals the populated credentialsDir to 0700.Source: Path instructions
🤖 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/nemoclaw-start-sealed-restart.test.ts`:
- Around line 12-33: Remove all if statements from the test file by moving
extractShellFunction’s conditional parsing logic into a non-test helper or
reusing an existing helper. Update the test to call that helper while preserving
shell-function and heredoc extraction behavior.
---
Nitpick comments:
In `@test/state-dir-guard.test.ts`:
- Around line 951-958: Update the test around runGuard("startup", configDir) to
keep credentialsDir at 0710 while adding token.json, rather than resetting it to
0700. Preserve the token file’s 0600 mode, then assert startup succeeds and
reseals the populated credentialsDir to 0700.
🪄 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: 094bd81f-8736-4325-bdfe-537d2224c726
📒 Files selected for processing (13)
docs/deployment/deploy-to-headless-server.mdxdocs/security/credential-storage.mdxscripts/nemoclaw-start.shscripts/state-dir-guard.pysrc/lib/actions/sandbox/start.test.tssrc/lib/actions/sandbox/start.tssrc/lib/shields/index.tssrc/lib/shields/state-dir-lock.test.tssrc/lib/shields/state-dir-lock.tstest/headless-server-docs.test.tstest/nemoclaw-start-sealed-restart.test.tstest/nemoclaw-start.test.tstest/state-dir-guard.test.ts
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
Queue follow-up: the dependency blocker cited in the earlier CI exception is now remediated on Please refresh from current |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Head branch was modified
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
6 participants
Summary
Starting a stopped Shields-protected sandbox could leave the Docker container running while OpenShell remained outside Ready. NemoClaw waited for image-level Docker health before it restored startup state, and non-root OpenClaw startup could not inspect optional credential paths once the state tree was sealed. This change hands a running container to trusted startup recovery, restores the narrow traversal permission needed for an empty credentials directory, and then verifies gateway and host-forward readiness.
Related Issue
Fixes #8112
Changes
runtime-runningrecovery boundary for the explicit Docker lifecyclestartconsumer. Other recovery callers retain the stricter Docker-health boundary.credentialsdirectoryroot:sandbox 0710; keep it non-listable and non-writable, and restoreroot:root 0700when entries exist.Type of Change
Quality Gates
reviewed-npm-auditand its dependentchecks/E2E coordination results; approval: fix(sandbox): restore startup before readiness #8146 (comment); follow-up: [Ubuntu 24.04][Security] npm audit --omit=dev reports 3 HIGH CVEs in production install (brace-expansion, fast-uri, js-yaml) #8116 and fix(security): close managed runtime audit gaps #8156.Documentation Writer Review
docs-updateddocs/manage-sandboxes/run-sandboxes.mdx,docs/security/best-practices.mdx, and generated OpenClaw, Hermes, and Deep Agents guide variants; variant sync, 9 focused documentation tests, and the full docs build passed with 0 errors and 2 existing warnings.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablemain, seven focused files passed 208 tests with one skip; state-guard, documentation, shell syntax, Python compilation, type-check, and repository checks passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Live Brev Linux validation on
nemoclaw-launchable-433-aug03for commitb5db426864a19754db7ee737113a82663df24163:issue-8112-openclaw; startup restored sandbox state before gateway and host-forward verification and exited successfully.root:sandbox 0710; the sandbox user could traverse a known path but could not list or write the directory.root:root 0700, with sandbox metadata access denied.Signed-off-by: Senthil Ravichandran senthilr@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation