test(sandbox): share the snapshot restore test fixture - #8447
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
📝 WalkthroughWalkthroughSnapshot tests now use shared fixtures and helpers instead of local definitions. The suite covers DCode probes, snapshot restore lifecycle behavior, clone observability, and OTLP policy reconciliation. The baseline-policy resolver and mock now support nullish agents. ChangesSnapshot fixture consolidation
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 8d4c647 in the TypeScript / code-coverage/cliThe overall coverage in commit 8d4c647 in the Show a code coverage summary of the most impacted files.
Updated |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/snapshot-restore-test-fixture.ts`:
- Line 160: Update resolveTestAgentBaselinePolicy and its
resolveAgentBaselinePolicyMock wrapper to match the production resolver
contract: accept string, null, or undefined, and return null when no baseline is
provided while preserving the object result for valid strings.
In `@src/lib/actions/sandbox/snapshot.test.ts`:
- Around line 648-653: Update the lifecycle-order assertions in the snapshot
test to first verify that “harden”, “delete”, and “cleanup-shields” are present
in f.lifecycleMock.events, then compare their indices. Keep the existing
ordering requirements while preventing missing events from satisfying the test
through indexOf returning -1.
🪄 Autofix
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: 2b5eb63e-d269-4772-8b27-156863ab7b85
📒 Files selected for processing (2)
src/lib/actions/sandbox/snapshot-restore-test-fixture.tssrc/lib/actions/sandbox/snapshot.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 1 semantic terminology decisionTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. 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: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@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 `@src/lib/actions/sandbox/snapshot-restore-baseline-exclusions.test.ts`:
- Around line 12-20: Update the test around “preserves the nullable production
baseline resolver contract” to exercise the real resolver through the
snapshot-restore public entrypoint, passing nullish and valid agent values and
asserting the observable results. Do not invoke resolveAgentBaselinePolicyMock
directly; if that boundary cannot expose the contract, rename the test to
explicitly describe fixture-helper behavior instead.
🪄 Autofix
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: 56b61818-d09d-4d71-be2d-a3788605800f
📒 Files selected for processing (2)
src/lib/actions/sandbox/snapshot-restore-baseline-exclusions.test.tstest/support/snapshot-policy-test-fixture.ts
|
CI classification update: every required check is green for the current branch revision. The second-opinion advisor lane ended in receipt validation after producing two suggestions. Both suggestions describe work already present in this revision: the snapshot fixture consolidation and the nullable fallback contract aligned with production. The primary advisor and publisher completed, the actionable warning was addressed with regression coverage, and its review thread is resolved. There is no remaining actionable advisor finding, so I am not rerunning the unchanged second-opinion lane. The focused snapshot suite, type checks, repository gates, DCO, commit verification, documentation review, and security review all pass. The remaining merge gate is independent maintainer approval. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Automated-review follow-up for the current branch revision (8d4c647): the shared snapshot baseline fixture now matches production fallback behavior for absent agent metadata, selecting the OpenClaw baseline for both null and undefined inputs. The regression test is explicitly scoped to the fixture helper and covers null, undefined, and explicit OpenClaw values. This addresses the primary advisor warning and the new CodeRabbit thread; the thread has a direct response and is resolved. Validation passed: affected snapshot suites 47/47, CLI typecheck, test-title policy, source-shape budget, test-size budget, normal commit hooks, and pre-push checks. Independent documentation review: PASS, no docs needed. The full PR and follow-up change only tests and test support; no CLI behavior, configuration, policy schema, default, or docs source changes. Nine-category security review: PASS with no findings. There are no production runtime, secret, input-processing, authorization, dependency, logging, cryptography, or security-configuration changes. The follow-up improves security-test fidelity by removing a fixture fallback that diverged from production. Fresh CI and automated review are running. |
Summary
The snapshot suite carried its own copy of the mock prologue that
snapshot-restore-test-fixture.tsalready owns and that eight sibling suites already share. The suite now imports that fixture instead, so one module owns the shared mock registrations and their reset.Related Issue
Resolves #8349
Changes
snapshot.test.tsdrops the duplicated prologue: two types, three openshell response helpers, twovi.hoistedblocks, 22 mock constants, 16vi.mockregistrations, a 33-linebeforeEachreset body, and theafterEach. It now usesimport * as f from "./snapshot-restore-test-fixture"withf.resetSnapshotRestoreMocks()andf.cleanupSnapshotRestoreMocks(), matching the eight suites that already consume the fixture.f.namespace.dcodeSandboxEntry,mockDcodeProbe,mockDcodeProbeResult,capturedDcodeProbeScript, andrunProbeScriptWithProcesses.snapshot-restore-test-fixture.tsdelegatesresolveAgentBaselinePolicyMocktoresolveTestAgentBaselinePolicy, replacing a byte-identical private copy.test/support/snapshot-policy-test-fixture.tsstays the owner of baseline-policy selection. This matters because a fixturevi.mockregistration runs after the importing suite's hoisted registrations and wins for the same path, so the suite cannot keep a competing../../policymock.snapshot.test.tsgoes from 1476 to 1220 lines. Net 269 fewer lines. No production source changed and no production export added.resolveTestAgentBaselinePolicyintest/support/snapshot-policy-test-fixture.tsnow matches the productionresolveAgentBaselinePolicycontract: it acceptsstring | null | undefined, falls back toopenclawfor a missing agent the same way production does, and declares the nullable return so a suite can model the unreadable-baseline path.harden,delete, andcleanup-shieldswere all recorded before it compares their positions, so a missing event can no longer satisfy the ordering throughindexOfreturning-1.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededVerification
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 unavailablenpx vitest run --project cli src/lib/actions/sandbox/snapshot.test.ts src/lib/actions/sandbox/snapshot-restore-*.test.ts src/lib/actions/sandbox/snapshot-managed-*.test.ts— 10 files, 125 tests passed; after the merge and the review fixes,npx vitest run --project cli src/lib/actions/sandbox— 232 files, 2524 of 2526 tests passed, with the two remaining tests timing out at the 5s default under local parallel load and passing when their files run on their own;npm run typecheck:cliclean;npm run validate:prclean;npm run test:projects:checkreports exact membership;npx biome checkclean on the changed files.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