test(e2e): add source-sandbox negative control for clone pairing - #7652
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
added
area: sandbox
|
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:
📝 WalkthroughWalkthroughRestored-clone auto-pair approval now handles gated device-list failures by reading clone-local pending state, validates request identity, and limits approvals. Tests expand coverage for state propagation, receipts, environment usage, bounded scopes, stored-auth behavior, primary-state preservation, and CLI module-cache isolation. ChangesRestored-clone approval flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AutoPairApprovalScript
participant OpenClawGateway
participant ClonePendingState
participant DeviceApproval
AutoPairApprovalScript->>OpenClawGateway: request devices list
OpenClawGateway-->>AutoPairApprovalScript: gated list failure
AutoPairApprovalScript->>ClonePendingState: read pending.json
ClonePendingState-->>AutoPairApprovalScript: matching pending request
AutoPairApprovalScript->>DeviceApproval: approve bounded request
DeviceApproval-->>AutoPairApprovalScript: approval receipt and scopes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 9146a55 in the TypeScript / code-coverage/cliThe overall coverage in commit 9146a55 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/lib/actions/sandbox/auto-pair-approval.test.ts (1)
43-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit: keep negative assertions in one style.
Line 43 uses
not.toContainwhile Line 44 usesincludes(...).toBe(false);toContain/not.toContainalso yields a better failure message than a bare boolean.♻️ Suggested tweak
expect(ordinary).not.toContain("local_identity_public_key"); - expect(ordinary.includes("load_clone_local_pending")).toBe(false); + expect(ordinary).not.toContain("load_clone_local_pending");🤖 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 43 - 44, In the negative assertions for the ordinary result, update the load_clone_local_pending check to use the same not.toContain matcher style as the local_identity_public_key check, preserving the existing assertion target and expected absence.src/lib/actions/sandbox/auto-pair-approval.ts (2)
202-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: single source for the clone state directory.
load_clone_local_pendingre-derivesOPENCLAW_STATE_DIR or '/sandbox/.openclaw', duplicating the same resolution inlocalDeviceFilter(Line 324). Emitting one sharedstate_dirhelper in the prelude avoids the two copies drifting apart.🤖 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.ts` around lines 202 - 209, Extract the OPENCLAW_STATE_DIR/default resolution into one shared state-directory helper in the prelude, then update load_clone_local_pending and localDeviceFilter to reuse it. Remove their duplicated fallback expressions while preserving the existing pending-file lookup and state-directory behavior.
172-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLink the retirement issue for this bounded fallback.
The comment states observable exit criteria (pinned OpenClaw release exposing a bootstrap/list API) but no GitHub issue/PR tracks retirement, so the clone-only path can outlive its window silently.
As per path instructions for
src/**: "Retain an old path only for a demonstrated external/persisted-data contract or a bounded confidence/rollback window... link the retirement issue or PR in GitHub, and state observable exit criteria."🤖 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.ts` around lines 172 - 178, Update the SOURCE_OF_TRUTH_REVIEW comment for the restored-clone gated-list fallback to include a link to the GitHub issue or PR tracking its retirement. Preserve the existing observable exit criterion requiring the pinned OpenClaw release to expose a bootstrap/list API.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/helpers/openclaw-real-device-self-approval-proof.ts`:
- Around line 1407-1423: Fix the primary-state isolation check around
primaryDevicesDir and the byte comparison by either wiring ${stateDir}-primary
into approveDevicePairing and the child process’s OPENCLAW_STATE_DIR/runtime
resolution so the comparison observes actual state, or removing this synthetic
directory and comparison. Preserve the meaningful coldPendingAfter.unrelated
assertion, and do not retain setup that can pass without exercising
primary-state isolation.
---
Nitpick comments:
In `@src/lib/actions/sandbox/auto-pair-approval.test.ts`:
- Around line 43-44: In the negative assertions for the ordinary result, update
the load_clone_local_pending check to use the same not.toContain matcher style
as the local_identity_public_key check, preserving the existing assertion target
and expected absence.
In `@src/lib/actions/sandbox/auto-pair-approval.ts`:
- Around line 202-209: Extract the OPENCLAW_STATE_DIR/default resolution into
one shared state-directory helper in the prelude, then update
load_clone_local_pending and localDeviceFilter to reuse it. Remove their
duplicated fallback expressions while preserving the existing pending-file
lookup and state-directory behavior.
- Around line 172-178: Update the SOURCE_OF_TRUTH_REVIEW comment for the
restored-clone gated-list fallback to include a link to the GitHub issue or PR
tracking its retirement. Preserve the existing observable exit criterion
requiring the pinned OpenClaw release to expose a bootstrap/list API.
🪄 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: e4a9c4e6-6705-4adb-a0d7-bdc7c941e858
📒 Files selected for processing (4)
src/lib/actions/sandbox/auto-pair-approval.test.tssrc/lib/actions/sandbox/auto-pair-approval.tstest/helpers/openclaw-real-device-self-approval-proof.tstest/openclaw-device-stored-auth-patch.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
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.
1 additional E2E selection from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 2 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: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary The trusted PR E2E controller evaluates risk-planner code from protected `main`, not from the PR under review. Before #7652 can receive exact `snapshot-commands` coverage, `main` must know that changes to the clone pairing approval runtime require that job. ## Changes - Map the exact `src/lib/actions/sandbox/auto-pair-approval.ts` runtime path to the existing `snapshot-commands` E2E job. - Add a focused deterministic risk-plan regression proving the production path selects that job while the adjacent unit test does not. - Keep authorization, dispatch, exact-SHA binding, workflow permissions, and runtime behavior unchanged. Dependency for #7652. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This internal CI-only change adds one exact source path to existing deterministic E2E coverage. It changes no user or contributor procedure, runtime contract, interface, configuration, default, or schema. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Codex Desktop completed the nine-category security review on exact SHA `5fe83a58e`; all categories passed with no actionable findings. The mapping broadens required coverage, not authorization or privilege. - [ ] 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: `no-docs-needed` - Evidence: No documentation change: this internal CI-only diff adds one source path to existing deterministic `snapshot-commands` coverage and a focused regression; no user or contributor procedure, interface, configuration, runtime contract, or schema changed. - Agent: Codex Desktop <!-- docs-review-head-sha: 5fe83a5 --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## 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 — command/result or justification: `npx vitest run --project integration test/pr-risk-plan.test.ts` passed 67/67; `npm run build:cli` passed; `npm run typecheck:cli` passed; `npm run check:diff` passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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) - [ ] 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) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated end-to-end test selection so changes to restored gateway pairing approval logic correctly focus the `snapshot-commands` job. * Improved risk planning to associate these changes with the appropriate focused E2E test family. * **Tests** * Added coverage verifying the runtime file-to-test job mapping and risk plan behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Maintainer security review refresh — exact head 5b350b1058b74117a1b8115401b1a61fd17d4f26 against base 795de982e26b400009bf1fc0f3826845d79acc4d: PASS. No security findings. Diff fingerprint: aff4010d694bd283db41465be96be6eeed3cdf91d560c469a6694297090a95da.
The verified merge commit preserves the exact reviewed patch (5792cf89c010ff3f32a600be740df6c73ced810a before and after refresh), and current main changed none of the ten PR files. The prior exact-head nine-category review therefore remains applicable.
I also checked the partial low-confidence second-opinion findings against the implementation. They do not change the PASS verdict: the timeout/concurrent-success paths re-read pinned post-approval state and require a rotated token different from previous_approval_token; inherited descriptors are deliberately clone-scoped, opened with O_DIRECTORY/O_NOFOLLOW/O_CLOEXEC, identity-checked, and passed only to the canonical approval child to close pathname races; and the alternate paired-token path is the accepted, one-approval compatibility path with pinned loopback, exact device/request/scope matching, and a documented removal condition.
The controller-selected protected E2E plan is appropriate for this credential and restored-clone boundary: cloud-inference, cloud-onboard, security-posture, onboard-repair, onboard-resume, and snapshot-commands (plan 49d007f6fce3addb3eb8d7cc1162a3667c699a2bfef2255229c361adb83ab301).
|
Exact-head protected evidence is now complete for The failure is in the changed restore contract, not an advisor or ordinary-CI issue: Run: https://github.com/NVIDIA/NemoClaw/actions/runs/30499870513/job/90737087871 This is also the second protected snapshot attempt to fail around the clone's managed-supervisor/config-guard handoff, though the exact symptom differs from the pre-refresh attempt. Please make the newly created/restored clone reach the trusted managed-supervisor availability boundary before beginning the deliberately non-retrying authorization sequence, and add a deterministic test for a clone that reports OpenShell I am not submitting a Changes Requested review and I am not modifying the branch (maintainer edits are disabled). I also reviewed and resolved the test-only CodeQL discussion with a false-positive explanation; the repository-wide alert was not dismissed. |
|
Exact-head babysitting update for |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
VerdictPASS — Commit FindingsNo findings. Detailed Analysis
Files ReviewedChanged file:
Supporting contracts:
|
cv
changed the title
|
Exact-head follow-up for |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed the exact current revision. The live E2E negative control uses a fixed, JSON-quoted source-only marker, creates it with restrictive permissions, checks it synchronously before the pairing probe, and removes it in an asserted finally path. A misrouted probe emits a unique recorded model request and exits, while the expected clone path requires exactly one authenticated fixture request, clone-session presence, and source-session absence. The effective diff is test-only; current main did not change the supporting sandbox, fake-inference, or helper contracts. CI, CodeQL, advisors, platform checks, and the selected E2E gate pass. No blocking correctness, security, compatibility, or regression defect found.
6 participants
Summary
The snapshot-command E2E test for #7608 did not distinguish a restored-clone pairing probe from one run in the source sandbox. The test now installs a source-sandbox negative control and requires one authenticated clone-fixture request, no negative-control request, and no source-sandbox session.
Related PR
Follow-up to #7608.
Changes
finallyblock removes the marker.Type of Change
Quality Gates
9146a55fa3d4fc1aa495ce0c15727705222b82bfagainst base SHA4cd4d64fe67143b57707f874afa0b9d269dfeff2. Each of the nine categories received PASS, with no findings. Security review.Documentation Writer Review
no-docs-needed9146a55fa3d4fc1aa495ce0c15727705222b82bfchanges onlytest/e2e/live/snapshot-commands.test.ts. It adds a source-sandbox negative control for restored-clone pairing verification and changes no command, flag, configuration, output contract, API, protocol, policy schema, or supported behavior. The changed test contract, identifiers, artifact names, and assertion labels were reviewed against the repository writing rules.DGX Station Hardware Evidence
Verification
GitHub CI is authoritative and is still running for commit
9146a55fa3d4fc1aa495ce0c15727705222b82bf. Required checks must pass before merge; no non-success result is waived. No broad local tests were run for this review.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 unavailable9146a55fa3d4fc1aa495ce0c15727705222b82bf.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable to this one-file live E2E test change.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com