fix(snapshot): stabilize pending pairing publication - #8021
Conversation
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
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:
📝 WalkthroughWalkthroughThe restored-clone approval script now retries bounded pending-state publication races, validates file descriptors, and uses shared timeout budgets. Tests cover transient and rotated files, hard links, receipt classification, malformed output, and gateway failure messages. ChangesSandbox approval robustness
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ApprovalScript
participant PendingState
participant OpenClaw
ApprovalScript->>PendingState: poll pending.json within read budget
PendingState-->>ApprovalScript: transient or valid pending state
ApprovalScript->>OpenClaw: approve exact local request
OpenClaw-->>ApprovalScript: sanitized receipt or list failure
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
VerdictPASS. I reviewed PR #8021 at exact head Findings TableNo findings. Detailed Analysis
Files Reviewed
|
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit f02d990 in the TypeScript / code-coverage/cliThe overall coverage in commit f02d990 in the Show a code coverage summary of the most impacted files.
Updated |
|
Cross-issue sweep progress:
Cross-issue scan — PR #8021No adjacent fixes or contradictions found above the medium confidence floor. Suppressed: 20 unrelated, 0 same-issue duplicates. The closest lexical candidate, #7215, has a different root and execution path: transparent-proxy locality for warm-daemon |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/lib/actions/sandbox/auto-pair-approval-script.test.ts (1)
73-74: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReplace source-text checks with descriptor behavior checks.
These assertions only prove that the generated script contains both predicates. They do not prove that a zero-link descriptor retries or that a multiply-linked descriptor fails closed.
Execute the generated script with real hard-link and atomic-replacement fixtures.
As per path instructions, tests must prefer observable outcomes over source-text assertions.
🤖 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/auto-pair-approval-script.test.ts` around lines 73 - 74, Replace the source-text assertions in the restoredClone test with behavioral checks of the generated script: execute it against real hard-link and atomic-replacement fixtures, verify zero-link descriptors retry, and verify multiply-linked descriptors fail closed. Preserve the existing generated-script setup while asserting observable outcomes rather than predicate text.Source: Path instructions
src/lib/actions/sandbox/auto-pair-approval.test.ts (1)
411-425: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an atomic-replacement test for the opened descriptor.
This fixture injects
JSONDecodeErrorbefore the production code can observe a zero link count. It tests malformed JSON retry, but it does not test the rotated-inode path.Add a fixture that calls
os.replaceafter opening the descriptor and before the post-read validation. Assert that the next retry approves the replacement state.As per path instructions, tests must prefer observable outcomes over source-text or implementation assertions.
🤖 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/auto-pair-approval.test.ts` around lines 411 - 425, Extend the transient publication fixture in the test around transientPendingPublicationScript to call os.replace after opening the descriptor and before post-read validation, simulating an atomic inode rotation. Assert the observable outcome that the subsequent retry approves the replacement state, rather than asserting injected script text or implementation details.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 `@src/lib/actions/sandbox/auto-pair-approval-receipt.test.ts`:
- Around line 17-20: Replace the test-body availability check in the test case
with an it.skipIf(...) condition that checks whether python3 is available, so
the test is skipped before execution without adding an executable conditional
inside the body.
In `@src/lib/actions/sandbox/auto-pair-approval.ts`:
- Around line 420-423: Update open_clone_json_descriptor in
src/lib/actions/sandbox/auto-pair-approval.ts at lines 420-423 to repeat
regular-file and link-count validation after json.load, mapping st_nlink == 0 to
CloneStateEntryRotated. In src/lib/actions/sandbox/auto-pair-approval.test.ts at
lines 411-425, add an atomic os.replace-after-open fixture that verifies retry
and approval of the replacement state. In
src/lib/actions/sandbox/auto-pair-approval-script.test.ts at lines 73-74,
replace source-text checks with executable hard-link and rotated-inode cases
that assert observable outcomes.
---
Nitpick comments:
In `@src/lib/actions/sandbox/auto-pair-approval-script.test.ts`:
- Around line 73-74: Replace the source-text assertions in the restoredClone
test with behavioral checks of the generated script: execute it against real
hard-link and atomic-replacement fixtures, verify zero-link descriptors retry,
and verify multiply-linked descriptors fail closed. Preserve the existing
generated-script setup while asserting observable outcomes rather than predicate
text.
In `@src/lib/actions/sandbox/auto-pair-approval.test.ts`:
- Around line 411-425: Extend the transient publication fixture in the test
around transientPendingPublicationScript to call os.replace after opening the
descriptor and before post-read validation, simulating an atomic inode rotation.
Assert the observable outcome that the subsequent retry approves the replacement
state, rather than asserting injected script text or implementation details.
🪄 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: 7a7e5add-7870-4b4f-99e8-148ab1055f2f
📒 Files selected for processing (4)
src/lib/actions/sandbox/auto-pair-approval-receipt.test.tssrc/lib/actions/sandbox/auto-pair-approval-script.test.tssrc/lib/actions/sandbox/auto-pair-approval.test.tssrc/lib/actions/sandbox/auto-pair-approval.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Exact-head security review updatePASS at The only delta from the full nine-category review at All nine verdicts remain PASS: Secrets and Credentials; Input Validation and Data Sanitization; Authentication and Authorization; Dependencies and Third-Party Libraries; Error Handling and Logging; Cryptography and Data Protection; Configuration and Security Headers; Security Testing; and System Security. Full analysis: #8021 (comment) |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Exact-head security reviewPASS at The post-review hardening closes the descriptor TOCTOU window identified by CodeRabbit. Regular-file and link-count validation now runs both before and after JSON parsing. An atomic replacement after open makes the old descriptor unlinked, maps only that state to the bounded retry, and the retry opens and validates the replacement file. Multiple hard links, non-regular files, symlinks, changed directories, other filesystem errors, non-object JSON, and persistent malformed JSON remain fail closed.
Files reviewed: |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
2 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 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. |
Security review — exact head
|
senthilr-nv
added
v0.0.101
Exact-head security review updatePASS at The only head change after the full nine-category PASS review at All nine verdicts remain PASS: Secrets and Credentials; Input Validation and Data Sanitization; Authentication and Authorization; Dependencies and Third-Party Libraries; Error Handling and Logging; Cryptography and Data Protection; Configuration and Security Headers; Security Testing; and System Security. |
Exact-head security review updatePASS. I reviewed PR #8021 at exact head
|
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Exact-head security review —
|
| Category | Result | Evidence |
|---|---|---|
| Secrets and credentials | PASS | No secrets are added or logged. Captured stdout, stderr, error messages, signal values, request IDs, and device IDs remain private. |
| Input validation and sanitization | PASS | Restored-clone selection still requires canonical descriptor-backed JSON, regular single-link files, stable directories, an exact local device/public-key match, and bounded roles/scopes. Retry is limited to missing publication, invalid JSON during publication, or a replaced inode. |
| Authentication and authorization | PASS | The compatibility path remains clone-local and permits only the reviewed cli/operator pairing-to-write transition. Unknown, ambiguous, or mismatched requests fail closed before approval. |
| Dependencies and third-party libraries | PASS | No dependency or action pin changes. |
| Error handling and logging | PASS | Host execution failures now map to fixed non-secret receipts (timeout, spawn, signal, command, or invalid receipt) without forwarding raw command output. The unknown catch path remains the fixed exec-failed fallback. |
| Cryptography and data protection | PASS | No cryptographic primitive or storage-contract change. Rotated credentials are still synchronized through the existing mode-0600 atomic replacement path. |
| Configuration and security headers | PASS | No exposed service, policy, header, or supported configuration change. The bounded outer timeout returns to 25 seconds; the unsupported 45-second hypothesis is removed. |
| Security testing | PASS | Added receipt-classification coverage; changed tests passed 219/219, focused approval tests 15/15, restore/pairing tests 17/17, CLI type-check passed, and normal commit/pre-push hooks passed. |
| System security | PASS | Descriptor pinning, O_NOFOLLOW, directory identity checks, link-count checks, one canonical approval attempt, and bounded subprocess timers remain intact. No shell interpolation or privilege expansion is introduced. |
Files reviewed: all 7 files changed from base 90f73918eeed5dcef9fe5733608e0a40dc3a7411 through exact head bca6e3c7b238480a7fdc2d7a25d0e40bc0c5d983.
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Exact-head security review addendum —
|
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Exact-head sensitive-path security reviewReviewed head: Result: PASS — no findings.
Validation reviewed: normal hooks passed; focused approval/receipt suites passed 29/29; |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Exact-head sensitive-path security review addendumReviewed head: Result: PASS — no findings. The only delta after the full nine-category review at All nine prior verdicts remain PASS: secrets and sensitive output; injection and command execution; authentication and authorization; unsafe configuration and fail-open behavior; path traversal and filesystem races; SSRF and network policy; privilege and sandbox boundary; resource exhaustion and lifecycle; dependencies and supply chain. Exact-head evidence: the focused integration suite passed 9/9, normal hooks passed, and pre-push CLI type-check passed. |
|
Security review addendum for exact head
Result: PASS. This commit is diagnostic-only and does not relax the restored-clone pairing boundary. |
Security review — PASSReviewed exact head No findings table is included because the review found no actionable issue.
Files reviewed: Evidence: approval suites 13/13, restore-pairing/budget 20/20, integration 9/9, E2E support 19/19, |
<!-- markdownlint-disable MD041 --> ## Summary Adds the canonical dated changelog entry for `v0.0.100` so the maintainer release plan can verify the pre-tag documentation prerequisite. The entry summarizes the user-facing changes merged since `v0.0.99` and links to the relevant guides. ## Changes - Add `docs/changelog/2026-07-31.mdx` with the exact `## v0.0.100` heading. - Cover restored OpenClaw pairing, transactional replacement, Deep Agents Code, onboarding recovery, lifecycle cleanup, Hermes builds, host provenance, documentation, and trusted E2E evidence. - Distinguish active Docker and Kubernetes runtime-bundle enforcement from the still-inactive managed shared-state transaction foundation. ## Source Coverage The release entry maps the doc-impacting merged PRs in the `v0.0.99..main` release range to `docs/changelog/2026-07-31.mdx`: #8021, #8024, #7973, #8028, #7947, #7788, #7884, #8023, #7969, #8020, #7989, #8000, #7907, #7942, #7567, #8013, #7955, #8017, #8014, #8015, #7629, #7644, #7821, #7971, and #7991. PR #7974 was reviewed after the final rebase and excluded because it changes internal maintainer-skill attribution policy and tests only; it does not change a user-facing product or documentation surface. ## 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: the changelog contract test validates the dated entry, version heading, SPDX form, and route constraints. - [ ] 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-31.mdx`; exact-head review passed for `6093f44f`; writing rules and documentation style reviewed; `npx vitest run test/changelog-docs.test.ts` passed 6/6; `npm run docs` passed with zero Fern errors and two generic Fern upgrade notices. - Agent: Codex Desktop <!-- docs-review-head-sha: 6093f44 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; no DGX Station host script changed. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## 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 validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run test/changelog-docs.test.ts` passed 6/6 at `6093f44f`. - [ ] 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 a dated prose-only release 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) — validation passed with zero errors; Fern emitted two generic upgrade notices. - [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) — the changelog entry has the required parser-safe MDX SPDX header; dated changelog entries intentionally do not use page frontmatter. --- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.100. * Documented improvements to restore pairing, sandbox replacement, onboarding recovery, lifecycle cleanup, runtime handling, build support, host readiness, and end-to-end validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
3 participants
Summary
Snapshot restore now tolerates the brief interval while the restored gateway publishes its local pending pairing request. The approval pass retries only three publication-in-progress states with up to 10 read attempts separated by 100 ms, allowing at most 900 ms of polling delay: a missing file, invalid JSON during truncate/write, or an inode unlinked by atomic replacement. Unsafe filesystem shapes and persistently malformed state still fail closed.
The restored clone uses path-only descriptors for state-root ancestor traversal. Trusted E2E proved that the restored OpenShell policy permits traversal and metadata access while
os.open("/", O_RDONLY|O_DIRECTORY|O_NOFOLLOW)returnsEACCES, even though the execution identity and/sandbox/.openclawownership and modes are correct. Readable descriptors remain in use for thedevicesandidentitydirectories where file access andfsyncare required.O_DIRECTORY,O_NOFOLLOW, device/inode revalidation, link-count checks, and fail-closed receipts remain unchanged.The restored/connect approval pass uses a 25-second outer execution cap, leaving at least 10 seconds beyond the longer bounded inner path. Host execution failures are categorized with fixed receipts that distinguish timeout, spawn, signal, nonzero-command, and invalid-receipt outcomes without exposing captured output, error text, signal values, or identifiers.
The generated approval program is streamed through stdin to
sh -sinstead of being placed in OpenShell command argv. This avoids the command-argument transport failure that caused the observed 33,016-byte restored-clone program to exit before Python could emit its receipt.Restored-state read failures use fixed boundary receipts for the state path, platform support, state root, devices directory, pending descriptor safety, publication stability, and JSON shape. These diagnostics retain fail-closed behavior and do not expose paths, identifiers, tokens, exception text, or raw state.
Related Issue
Fixes #8019.
Changes
O_PATHonly for restored-clone state-root ancestor traversal so OpenShell's root read restriction does not block safe descriptor-relative access.O_DIRECTORY,O_NOFOLLOW, descriptor/inode revalidation, link-count checks, and fail-closed behavior.pending.json, invalid JSON during truncate/write, or an opened inode unlinked by atomic replacement with up to 10 read attempts separated by 100 ms, allowing at most 900 ms of polling delay.scope-upgrade-pendingandapproval=list-faileddiagnostics so trusted E2E covers both pairing modes.Type of Change
Quality Gates
connectremains accurate.Documentation Writer Review
no-docs-neededDGX 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 unavailablenpm run test:changed216/216; Biome, CLI typecheck, repository checks, test-size checks, and normal hooks passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Required Linux CI and trusted live E2E are pending. The trusted plan must exercise bothissue-4462-scope-upgrade-approvalandsnapshot-commandson exact headf02d990cd49a13469b280baf5379713bd890e6f6.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Signed-off-by: Senthil Ravichandran senthilr@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests