fix(sandbox): bootstrap device identity before restored clone approval - #7968
fix(sandbox): bootstrap device identity before restored clone approval#7968laitingsheng wants to merge 2 commits into
Conversation
A restored clone never carries runtime auth state, so its approval pass held no stored device credential while the approve call already stripped the gateway token. The clone device list now runs the same pairing bootstrap the in-sandbox watcher uses, and a stalled approve reports a timeout receipt instead of a generic failure. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
📝 WalkthroughWalkthroughThe approval flow now creates a forced-device-pairing environment for local device listing and distinguishes approval subprocess timeouts from other failures. Tests cover environment propagation, timeout receipts, and stalled approvals. ChangesPairing and approval flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GeneratedApprovalScript
participant PairingPolicyModule
participant DeviceListSubprocess
participant ApprovalSubprocess
GeneratedApprovalScript->>PairingPolicyModule: call pairing_bootstrap_env
PairingPolicyModule-->>GeneratedApprovalScript: return forced-pairing environment
GeneratedApprovalScript->>DeviceListSubprocess: run device listing with bootstrap environment
GeneratedApprovalScript->>ApprovalSubprocess: request device approval
ApprovalSubprocess-->>GeneratedApprovalScript: timeout or approval result
GeneratedApprovalScript-->>GeneratedApprovalScript: emit approve-timeout or approval receipt
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
src/lib/actions/sandbox/auto-pair-approval.test.ts (1)
44-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace source-text assertions with execution checks.
Lines 44-52 and Line 77 lock the test to generated Python text and policy source layout. Extend the execution fixture to verify that ordinary device listing has no pairing marker and restored-clone listing has the marker. Remove these containment assertions.
As per path instructions, prefer observable public-boundary outcomes and flag source-text assertions.
Also applies to: 77-77
🤖 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 44 - 52, Update the test around the execution fixture in the auto-pair approval flow to validate behavior through public-boundary results: ordinary device listing must have no pairing marker, while restored-clone listing must include one. Remove the generated Python source containment assertions, including the assertion at line 77, and preserve the existing execution setup and other observable checks.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.
Nitpick comments:
In `@src/lib/actions/sandbox/auto-pair-approval.test.ts`:
- Around line 44-52: Update the test around the execution fixture in the
auto-pair approval flow to validate behavior through public-boundary results:
ordinary device listing must have no pairing marker, while restored-clone
listing must include one. Remove the generated Python source containment
assertions, including the assertion at line 77, and preserve the existing
execution setup and other observable checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2208d3b7-9952-48a5-be97-4ba632bb3e64
📒 Files selected for processing (4)
scripts/lib/openclaw_device_approval_policy.pysrc/lib/actions/sandbox/auto-pair-approval.test.tssrc/lib/actions/sandbox/auto-pair-approval.tstest/openclaw-device-approval-policy.test.ts
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 88a9b4d in the TypeScript / code-coverage/cliThe overall coverage in commit 88a9b4d in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported 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
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
laitingsheng
added
NV QA
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Comment — reviewed exact head 88a9b4d. I found no blocking correctness or security regression in the current two-file diff. Non-blocking: after the merge from main, the comparison now only adds pairing_bootstrap_env plus direct unit coverage; no production consumer calls that helper, and the restore-time approval logic described in the PR body is no longer part of this diff. Please consider closing this as superseded by the intervening restored-pairing work, or use a narrowly scoped follow-up PR if a current-main reproduction shows a remaining gap. This does not need to block the current branch on safety grounds.
5 participants
Summary
A cross-sandbox snapshot restore never carries runtime auth state, so the restored clone reaches its pairing approval with no stored operator device credential. The approval pass then called
openclaw devices approvewith the gateway token stripped, leaving the call with nothing to authenticate, and the restore ended withgateway pairing could not be verifiedreportingscope-upgrade-pending; approval=approve-failed. The clone's device list now performs the same pairing bootstrap the in-sandbox watcher already performs, so the credential exists before the stripped approve runs.Related Issue
Fixes #7431
Changes
pairing_bootstrap_envto the shared approval policy module, beside the existinggateway_approval_env. It keeps the gateway environment and sets the device-pairing marker, matching the watcher's unbootstrapped first list call.devices listthrough that environment. The current consumer isapproveRestoredClonePairing, the only caller that passeslocalDeviceOnly; connect and doctor keep a byte-identical script because they run after onboard, when the stored credential already exists.buildAutoPairApprovalScriptscript-shape tests protect that split.approve-timeoutrather thanapprove-failed. Both outcomes previously produced one receipt, which is why the reported QA failure could not be attributed to a rejection or a stall.Type of Change
Quality Gates
Documentation Writer Review
blockedDGX 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 unavailablevitest --project cli auto-pair-approval restore-gateway-pairing18/18;vitest --project integration openclaw-device-approval-policy12/12;vitest --project integration nemoclaw-start114/114;npm run test:changed203/203 across 20 files;npm run typecheck:cliclean.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)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit