feat: isolate E2E test execution in sandboxes - #4358
Conversation
![dyad-assistant[bot]](https://avatars.githubusercontent.com/in/3027874?s=60&v=4){kind=small}
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge This is a well-structured refactor: the sandbox workspace, run-scoped runtime, artifact promotion, and the HTTP-preserving Neon trusted-origin helper are all sensibly separated and reasonably tested, and the recorder path is left genuinely untouched. Two problems in the new orchestration are merge-blocking, and both have mechanical fixes. The most serious one is stage ordering. The refactor moved The second is quit-time process cleanup. Everything else below is informational. Note that the plan document committed with this PR describes several Phase 1/4 items (copy progress telemetry, staged coordination with startup reconciliation, feature-flag rollout) that the implementation does not yet include; the plan's own "Status" section acknowledges partial delivery, so this is scope rather than a defect. The diff was provided in full ( Issues Summary
🟢 Low Priority Notes (8 items)
Generated by Dyadbot persona-based code review |
github-actions
Bot
added
the
needs-human:review-issue
- Evaluate the testingEnabled and runtime-mode guards before the
prepare-e2e-test-workspace stage, so a refused run no longer mutates the
user's real project via ensurePlaywrightBootstrap or takes a multi-gigabyte
snapshot first.
- Restore E2E testing for Docker/cloud runtime: those runtimes fall back to the
pre-sandbox path (bootstrap + normal preview) with the missing runtime
isolation disclosed on the result. Only Neon apps are refused, because
without a sandbox the run would hit the user's real database.
- Tree-kill run-scoped children synchronously on quit via a new process
registry. Aborting alone routed into the async killProcess/tree-kill path,
which will-quit never awaits, leaving the sandbox server holding its port and
its cwd under <userData>/test-sandboxes.
- Scope the startup sandbox sweep: delete run directories individually and skip
any run this process still owns, instead of removing the shared root while a
freshly started run is mid-copy.
- Run custom start commands verbatim instead of appending `-- --port`, and
treat a command as custom only when both installCommand and startCommand are
set, matching getCommand in app_runtime_service. A custom command that never
binds the run-scoped port now gets a {port} hint instead of a bare timeout.
- Select the sandbox package manager with getPackageManagerSignal /
choosePackageManagerFromSignal so a pnpm lockfile falls back to npm when pnpm
is missing or too old, exactly as the normal preview does.
- Thread the stage-1 bootstrap's `installed` flag into runAppTestsCore so the
e2e_tests_run `first_run` telemetry property stops always reporting false.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JhzboMdTdVq829U81ZCiNb
🤖 Claude Code Review SummaryPR Confidence: 4/5All seven review threads were addressed with code changes in 6cad874 and the full unit suite passes (6365 tests), but the sandboxed runtime and the restored Docker/cloud fallback are both exercised only through mocks here — a real E2E run on a non-host runtime would raise this to 5. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsThe following suggestions could improve
🤖 Generated with Claude Code |
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge The main-process architecture here is strong: staged coordinator claims, a run-scoped process registry with a synchronous quit path, reflink-based snapshotting, containment assertions on disposal, and an orphan sweep that respects live runs. The unit-test coverage for the new services is genuinely good, and the Neon The blocking problem is that the renderer was never migrated. The PR's headline promise — the normal preview is never stopped, restarted, or repointed — is contradicted by three user-facing strings that this PR leaves untouched and that are gated on exactly the path it changed. The patch was complete (not truncated), so confidence on the diff itself is high. Where a finding depends on unchanged files ( Issues Summary
🟢 Low Priority Notes (6 items)
Generated by Dyadbot persona-based code review |
- Replace the Tests panel / cancellation-banner copy that still promised Dyad restores the preview and database. Nothing is restored on the host path: the run had its own sandbox and its own server. The Neon disclosure now says the preview keeps its real database, and the cancellationRestoringTestApp string is renamed to cancellationRemovingTestDatabase across all five locales. - Drive the "couldn't finish cleaning up" warning off a new TeardownResult.remoteCleanupCompleted instead of envRestored, which on the sandbox path only reported on a workspace file deleted seconds later. markAndDeleteTempTestBranch now returns the delete verdict. - Return a structured "Test run stopped." result when Stop lands during sandbox setup, instead of rejecting the IPC call and recording an internal product exception for an ordinary cancellation. - Announce cleaning-up before disposing the sandbox for every isolation mode, so removing a cloned node_modules tree is not an unlabelled wait with Run/Record/Delete disabled. - Add workspace copy/dispose telemetry (durations, entry counts, whether a reflink was requested; no absolute paths) and a disableSandboxedE2eTests escape hatch with a Settings toggle. Turning it off routes through the same non-sandboxed path, still failing closed for Neon. - Sweep retained test artifacts: startup reconciliation prunes directories whose app no longer exists, and deleting an app drops its artifacts. - Remove the readiness poll's per-iteration abort listener, which reached Node's MaxListenersExceededWarning on any slow sandbox start. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JhzboMdTdVq829U81ZCiNb
🤖 Claude Code Review SummaryPR Confidence: 4/5The eight new threads are all addressed in 10e0442 with the full unit suite green (6375 tests), and the biggest gaps this round — UI copy that asserted the opposite of what the code does, and a cleanup warning that could never fire — are closed; still 4 rather than 5 because the sandbox path and the restored non-sandboxed path are exercised only through mocks, and the copy telemetry has no field data yet on non-reflink filesystems. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsThe following suggestions could improve
🤖 Generated with Claude Code |
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge The architecture here is careful and well-tested: bootstrap-before-snapshot, staged coordinator claims, a run-scoped process registry killed synchronously on quit, containment checks on every path removal, a startup sweep that skips live runs, and Issues Summary
On the HIGH: Verified as sound (no action needed): app deletion aborts and awaits the in-flight run before the row is deleted, and 🟢 Low Priority Notes (7 items)
Generated by Dyadbot persona-based code review |
- Stop handing the agent an unreadable artifact path. A sandboxed run retains error-context.md under <userData>/test-artifacts, which read_file's safeJoin refuses, so the model's first diagnostic step always failed. The page snapshot is now inlined (bounded, same containment guards as the screenshot reader) and the traversal path is no longer printed at all. - Make the dev-server gate conditional on the non-sandboxed path. A sandboxed run serves its own copy of the app on its own port, so requiring the user's preview blocked the feature's main benefit and contradicted the panel's own "your preview keeps running" disclosure. The panel banner, Run/Retry state and the agent's guardDevServerRunning now share one helper, usesSandboxedE2eTests. - Thread `sandboxed` through the run-state payload so cleanup copy only claims a sandbox when one was taken. The fallback path creates no workspace, and "Cleaning up the test sandbox…" there was the same inaccuracy the previous round removed. Adds a separate cancellationCleaningTestSandbox locale key. - Throw DyadError with DyadErrorKind.Precondition for expected setup failures (dependencies not installed, server never became ready), so ordinary user-fixable problems stop being reported to PostHog as product exceptions. - Add allowCompatibleQueueBypass to the prepare-e2e-test-workspace claim, matching the run stage: work conflicting only on test-files should not queue behind a test run that is itself blocked on something unrelated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JhzboMdTdVq829U81ZCiNb
🤖 Claude Code Review SummaryPR Confidence: 4/5The five new threads are addressed in f6f274a with the full unit suite green (6381 tests), and this round closed two things that would have been felt immediately — the agent's first diagnostic step failing on every sandboxed failure, and the dev-server gate blocking the feature's main benefit; still 4 because both the sandboxed path and the restored fallback are covered by mocks rather than a real end-to-end run, and the copy telemetry has no field data yet on non-reflink filesystems. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsThe following suggestions could improve
🤖 Generated with Claude Code |
| let dependencyEntries = 0; | ||
| const startedAt = Date.now(); | ||
| try { | ||
| await fs.cp(appPath, workspacePath, { |
There was a problem hiding this comment.
🟡 MEDIUM
Sandbox copy failures are reported as internal product exceptions
copyNodeModules deliberately throws a DyadError with DyadErrorKind.Precondition for the "dependencies not installed" case, but the two fs.cp calls themselves let raw Node errors escape. runAppTestsWithIsolation's catch wraps any non-DyadError in new DyadError(message, DyadErrorKind.Internal), so the most likely real-world failures of a per-run app + node_modules snapshot — ENOSPC on a full disk, EPERM/EBUSY on Windows when a file is locked by the dev server or antivirus, EACCES — get counted as unclassified Dyad product exceptions instead of user/environment preconditions. This is the same class of misclassification the comment immediately above the fs.stat guard exists to prevent.
💡 Suggestion: Wrap both fs.cp calls (source tree and node_modules) and translate ENOSPC/EPERM/EBUSY/EACCES into a DyadError with a user-facing kind (e.g. Precondition) and an actionable message, letting only genuinely unexpected errors fall through as Internal.
| { | ||
| appId, | ||
| operation: "prepare-e2e-test-workspace", | ||
| resources: [ |
There was a problem hiding this comment.
🟡 MEDIUM
Snapshot stage does not claim runtime-config, so .env.local can be rewritten mid-copy
The prepare-e2e-test-workspace stage claims app-path (read), repository-ref (read), repository-worktree and test-files, but not runtime-config — the resource that guards .env.local. restoreAppFromTestBranch (neon_test_branch.ts:524) claims app-path (read), provider, runtime and runtime-config, so it does not conflict with the snapshot stage and can rewrite the real .env.local while createE2eTestWorkspace is copying the app directory. The pre-sandbox code held runtime/runtime-config for the entire run precisely to keep startup reconciliation from interleaving, and app_handlers.ts:988 shows the repo's own precedent: copy-app takes readAppResource("runtime-config") because it copies the app directory. A run started shortly after launch (or concurrently with a Run press that triggers ensureAppOffTestBranch) can therefore snapshot a torn or temporary-branch .env.local into the sandbox.
💡 Suggestion: Add readAppResource("runtime-config") to the prepare-e2e-test-workspace resource list so env-file writers cannot interleave with the snapshot, mirroring the copy-app operation.
| }): Promise<E2eTestRuntime> { | ||
| if (signal?.aborted) throw new Error("Test run stopped."); | ||
| const port = await allocateE2eTestPort(); | ||
| const baseUrl = `http://127.0.0.1:${port}`; |
There was a problem hiding this comment.
🟡 MEDIUM
Test server URL is assumed rather than observed, so a mismatch costs a 2-minute dead wait
baseUrl is hardcoded to http://127.0.0.1: and both the readiness probe and Playwright's baseURL use it, but the child is only ever told the port (via --port or PORT) and never the host. If the framework binds somewhere else — a dev script that hardcodes its own --host/--port, Vite falling back to a different port when the allocated one is taken between the probe bind and the spawn, or localhost resolving to ::1 rather than 127.0.0.1 — the probe can never succeed and the run sits for the full SERVER_READY_TIMEOUT_MS before failing. The normal preview avoids this by parsing the URL the dev server actually prints (app_runtime_service.ts:801). The portHint only fires for custom start commands, so a Dyad-managed app gets a bare 2-minute timeout. I have not reproduced this, but the divergence from the normal-preview path is real.
💡 Suggestion: Either pass an explicit host to the managed start commands (e.g. --host 127.0.0.1) and/or parse the dev server's printed URL from the streamed output as the normal runtime does, falling back to the assumed URL only when nothing is printed.
| )} | ||
|
|
||
| {!isRunning && showNeonRestartDisclosure && ( | ||
| {!isRunning && showNeonSandboxDisclosure && ( |
There was a problem hiding this comment.
🟡 MEDIUM
Neon apps that cannot run tests get no disclosure until Run fails
When the sandbox is unavailable (Docker/cloud runtime, or the user's opt-out), a Neon app is now refused outright — a capability removal the PR description calls out. The panel gives no signal: showNeonSandboxDisclosure is false in exactly those configurations, so the teal banner disappears, and the Run button is enabled as soon as the preview is up. The user only learns tests cannot run at all by pressing Run and reading the resulting infra error. A Neon user who toggles the setting off to make runs faster is left with a Run button that always errors.
💡 Suggestion: When app.neonProjectId is set and usesSandboxedE2eTests(settings) is false, show a warning banner naming the reason (docker/cloud runtime or the Settings opt-out) and disable Run, instead of only surfacing the refusal after the run is attempted.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues This is a large, unusually well-documented change: E2E runs now snapshot the app into I found no HIGH issues. Four MEDIUM items are worth a look before or after merge; none of them break the main flow. Two notes on confidence: the context reports Issues Summary
🟢 Low Priority Notes (7 items)
Generated by Dyadbot persona-based code review |
Every finding from the round-1 review of the sandboxed E2E test runtime. Setup failures no longer escape as internal exceptions. `ensurePlaywrightBootstrap` and `createE2eTestWorkspace` both run in the sandbox prepare stage, outside the try/catch that used to cover bootstrap inside `runAppTestsCore`; a registry timeout, a failed browser download or a missing `node_modules` therefore rejected the IPC call, recorded an internal product exception and threw out of the agent's turn instead of counting as a non-attempt infra failure. The stage now reports setup failures as data and the run resolves to an ordinary `infraError`. A custom-command app no longer needs `node_modules` to be sandboxed at all. It need not be a Node project, its install command runs in the sandbox, and the Run button is enabled without a dev server now — so refusing there made the sandbox structurally impossible for those apps. Custom commands run as `install && start`, the same shape `getCommand` builds for the preview. Running the start command alone skipped codegen, builds and non-npm dependency setup the server may need, so an app would start under the preview and fail only under test. Artifact retention is best-effort again. An `fs.cp` failure after the run had already produced results (a trace file still held on Windows, a full disk) discarded the whole run. It now costs at most the screenshots, whose paths are dropped rather than left pointing into a sandbox that is about to be deleted. Test-server ports come from a reserved band (52150..52349) instead of an OS-assigned ephemeral port. The ephemeral range covers almost all of `getAppPort`, `getAppProxyPort` and the proxy fallback band, so a test server could hold another app's deterministic port for a whole run and make that app fail to start later with nothing to point at as the cause. Concurrent allocations are also no longer handed the same port. The Neon branch delete is no longer gated on restoring the sandbox's own `.env.local`. That gate exists because the recorder's swap can leave the real project pointed at the branch; nothing points at a sandbox copy that is deleted seconds later, so the gate only leaked a real branch. The matching "restore your real database settings" warning is now suppressed on that path too. Also: a dev server that survives `killProcess`'s 5s timeout stays registered so `will-quit` can still tree-kill it; a failed `dispose()` no longer replaces the setup error it was cleaning up after; and a server that announces its port was taken and quietly moved (Vite's default `strictPort: false`) triggers the retry instead of a two-minute readiness timeout on a dead port. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FEzQQKdVCw3fuqd8WdAh3d
…iming A custom app's install step no longer spends the server's readiness budget. `install && start` is one spawned command, so `pip install -r requirements.txt`, `bundle install`, `go mod download` or a cold `npm ci` was charged against the 120s server deadline and failed a run whose server was about to come up. Those runs now get a 15-minute budget, and the timeout message names the budget that actually applied instead of always saying "2 minutes". The allocated port is handed back on every exit path. Only the try/catch around the readiness wait released it, so anything that threw earlier — the pnpm version probe, a workspace read, `spawn` itself — permanently burned one of the 200 band ports, and enough failures left the process unable to allocate at all. A port clash is now a distinct error class matched against the allocated port number, not a substring of the failure message. That message embeds the last 8KB of server output, so an app whose dev script also starts a sidecar (Postgres, Redis, a second worker) logging about *its own* taken port was retried three times — up to six more minutes — before the real error reached the user. Retained artifacts are pruned after the new run produces replacements, not when its workspace is created. Pruning up front destroyed the previous run's screenshots for a run that then failed during setup, leaving the panel showing results whose thumbnails silently stopped loading. Run directory names drop the epoch and shorten the UUID to 12 hex characters. `<userData>/test-sandboxes` is already deeper than the app directory, the Windows copy is a real one, and long-path support is off by default — so ~50 characters of pure path depth could push a pnpm tree past MAX_PATH and fail mid-copy with an opaque setup error. The Tests panel no longer flashes "Start the app to run tests." while settings load. `usesSandboxedE2eTests` answers false for absent settings, so a sandbox user with no preview saw a hard refusal on every mount of the tab. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FEzQQKdVCw3fuqd8WdAh3d
…copy Two reviewer passes over this round; both sets of findings are handled here. Artifact pruning no longer deletes a concurrent run's screenshots. A second Run for the same app aborts the first and proceeds without awaiting its teardown, so both cleanups overlap — and whichever retained second deleted the other's artifacts before they reached the panel. The prune now skips run directories `activeWorkspaceNames` still owns, the same check the startup sweep already made for the same reason. It also runs in a `finally`, so a failed copy no longer strands the run it replaced with no owner. Custom commands are spawned as `(install) && (start)`. `&&` binds left-to-right, so an ungrouped `install && A || B` ran `B` when the *install* failed and `install && A; B` ran `B` unconditionally — silently re-associating any start command containing a shell operator. The port band scan consults `isReservedDyadPort` like the fallback loop already did. The band sits above every default reserved range, but Dyad's own E2E shards relocate those: `DYAD_E2E_PORT_BLOCK_INDEX=9` puts a block's proxy sub-range straight through it, so a sandbox server could take a deterministic proxy port. A sandboxed run's Neon branch is marked cleanup-only at creation, inside `createTempTestBranch`, rather than after it returns. Auth provisioning and the cookie secret sit between those two points with their own retries and backoff; a crash there left a raw marker that startup recovery read as the recorder's env swap and "restored" by rewriting the user's real `.env.local` — for a run that never touched it. Retained artifacts win over the app path when both match. `userData` can sit inside the project on a portable or dev install, which made every retained artifact also look like an app path — skipping the app-id check and then comparing the run directory name against "test-results", so legitimate thumbnails silently failed to load. `sandboxed` is now set when the workspace exists, not when the route is chosen, so a run whose setup failed no longer offers to clean up a sandbox it never created. And the cleanup warning names what was actually left behind: the Supabase path leaks a temporary auth user in the user's real project, which is not "the isolated test database" and which no startup sweep picks up. Test fixes: `CancellationBanner`'s i18n mock mapped `cancellationCleaningTestData` to the *sandbox* wording, so the Supabase test asserted the sandbox string while exercising the non-sandbox branch — deleting the component's whole `sandboxed` branch would have left the suite green. Both branches are now covered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FEzQQKdVCw3fuqd8WdAh3d
The Supabase teardown reads `deleteTempTestUser`'s return value, not just the absence of a throw. That delete is best-effort inside — a 5xx from the Auth Admin API, or a service-role key fetch that fails, resolves `false` and deliberately leaves `supabaseTestUserId` on the row for the startup sweep — so the ordinary failure mode reported a clean teardown while a `dyad-test` user sat in the user's real project. The Neon sibling already read its verdict this way. The Supabase setup-failure path carries that verdict forward instead of handing back `NOOP_TEARDOWN`. A Stop pressed just after the test user was created ran teardown inside the catch and then answered "nothing left over", reporting a clean cancellation for a user that had leaked. The Neon path already had `settledTeardown` for exactly this. `ensureNeonAuthTrustedOrigin` backs off on 423/429 like every other Neon call in this flow. Running several specs back to back is the burst that trips the rate limit, and an unretried clash refused the whole run — including specs that never touch sign-in, where the same limit previously only degraded auth. Port exhaustion and an unrecoverable port clash are `DyadErrorKind.Precondition` rather than bare errors. Every other server-start failure here is already classified that way "so it must not be reported as a product exception"; these two were the paths that survived all three retries and landed in telemetry unclassified. Last round's Supabase cleanup copy was itself inaccurate: it told the user to go delete the test user by hand, but `reconcileOrphanTestUsers` sweeps exactly that at startup, keyed on the id the failed delete deliberately leaves behind. It now says Dyad will retry, matching the Neon variant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FEzQQKdVCw3fuqd8WdAh3d
The Tests panel derives both of its settings-dependent banners from one tri-state. `usesSandboxedE2eTests` answers false for absent settings, and the two banners want opposite defaults while settings load — the Run gate must not refuse, the Neon disclosure must not promise — so reading the setting directly in each place made the panel briefly claim sandboxing to a user who had turned it off. `undefined` now means "not loaded", and each banner compares explicitly. `runDirectoryAppId` is the single parser both owner checks go through. The screenshot reader hand-rolled a `startsWith` prefix test while the artifact prune used the helper; they agree today, and sharing the parser is what keeps them from drifting. A child that never got a pid is untracked immediately. There is nothing for `will-quit` to kill and nothing that could later exit to drop it, so it would otherwise sit in the process registry for the life of the process. Also records the invariant the prepare stage's discriminated union rests on: a `setupError` never carries a workspace, because `createE2eTestWorkspace` disposes its own partial tree before throwing. That is what makes the caller's early return safe without a dispose of its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FEzQQKdVCw3fuqd8WdAh3d
| const runName = path.basename(artifactPath); | ||
| const appId = runDirectoryAppId(runName); | ||
| if (appId === null) return; | ||
| await removeRunDirectories( |
There was a problem hiding this comment.
🟡 MEDIUM
Artifact prune deletes screenshots still shown for other specs
retainE2eTestArtifacts always calls pruneSupersededArtifacts, which removes every other retained run directory for the app. But the panel does not clear every spec's results on a single-file re-run: applyTestRunStartedAtom only filters out the targeted files and keeps prev.results for the rest. After a full run leaves failures in specs A and B and the user re-runs only spec A, spec B's result row is still on screen with a screenshotPath that points into the just-deleted directory, so its thumbnail silently fails to load. The comment on pruneSupersededArtifacts assumes 'the results on screen are its own', which only holds for an all-specs run.
💡 Suggestion: Prune only when the run covered every spec (or keep the previous run's directory until its results are actually replaced), and have the panel drop screenshot paths it knows were pruned.
| ? trimmedStart.replaceAll("{port}", String(port)) | ||
| : trimmedStart; | ||
| return { | ||
| command: `(${installCommand!.trim()}) && (${start})`, |
There was a problem hiding this comment.
🟡 MEDIUM
Sandbox re-runs the custom install command on every test run
For an app with both installCommand and startCommand set, buildE2eTestStartCommand emits (install) && (start) inside the sandbox even though createE2eTestWorkspace already cloned node_modules into that workspace. For a Node app whose install command is npm ci this deletes the freshly cloned tree and reinstalls from scratch on every single test run, on top of the full source+dependency copy; that is also why the readiness budget had to be raised to 15 minutes. It contradicts this PR's own plan document, which lists 'Do not install dependencies independently inside every sandbox' as a non-goal and 'prohibit sandbox installs' as a risk mitigation.
💡 Suggestion: Skip the install half when copyNodeModules succeeded (run it only when the dependency tree was absent), or gate the re-install behind an explicit per-app opt-in.
| const sandboxAvailable = settings | ||
| ? usesSandboxedE2eTests(settings) | ||
| : undefined; | ||
| const testRunBlocked = sandboxAvailable === false && !devServerRunning; |
There was a problem hiding this comment.
🟡 MEDIUM
Run stays enabled while settings load for opted-out users
testRunBlocked is only true when sandboxAvailable === false, so while settings are still loading (sandboxAvailable === undefined) the Run/Retry/per-spec buttons stay enabled and the amber 'Start the app to run tests.' gate is suppressed. For a user who turned the sandbox off (or is on docker/cloud runtime) with no dev server running, that first render invites a click that the main process then rejects with 'Start the app before running tests', replacing a local, actionable gate with a run-failure banner. The Neon disclosure has the same undefined hole in the opposite direction.
💡 Suggestion: Treat the loading state as blocked-but-quiet (disable the run affordances until settings resolve, without rendering the amber banner), or render a skeleton/disabled state for the panel actions while settings are pending.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Large, well-structured change (41 files, ~4.7k additions). All 41 changed files are present in the review context with complete, untruncated per-file patches — only the aggregate diff blob is truncated — so confidence in the file-level review is high. The main/renderer boundary looks sound: the new services live entirely in main, the only new IPC surface is an added The issues below are all non-blocking. Issues Summary
🟢 Low Priority Notes (6 items)
Generated by Dyadbot persona-based code review |
🎭 Playwright Test Results❌ Some tests failed
Summary: 290 passed, 3 failed, 1 flaky, 12 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/local_agent_advanced.spec.ts \
e2e-tests/local_agent_auto.spec.ts \
e2e-tests/local_agent_explore_code.spec.ts
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f93ca8f7e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Run each E2E test in a throwaway copy of your app with its own server, | ||
| so tests never touch your preview or your real database. Turn this off |
There was a problem hiding this comment.
For Supabase apps, this guarantee is false: prepareSupabaseTestUserIsolation deliberately uses the real Supabase project under RLS, and when no organization is connected it returns mode: "none" and proceeds against the current data (isolated_test_db.ts:127-130, 418-425). If RLS is absent or that setup is unavailable, destructive tests can therefore modify real data despite this setting explicitly promising otherwise; qualify the text to distinguish filesystem/runtime isolation from Supabase data isolation.
Useful? React with 👍 / 👎.
| await workspace.dispose(); | ||
| } catch (error) { | ||
| logger.error( | ||
| `Failed to remove isolated test workspace for app ${appId}: ${error}`, | ||
| ); |
There was a problem hiding this comment.
When workspace removal fails—for example with EBUSY/EPERM on Windows while a child or antivirus still holds the copied tree—this catch only logs the error and the run still finishes normally. Because the workspace is also removed from activeWorkspaceNames, no retry occurs until a future application startup, while every subsequent run creates another potentially multi-gigabyte node_modules copy; repeated runs in the same session can silently consume disk space, so retain/retry the orphan or report the cleanup failure to the user.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 41 files
Confidence score: 2/5
src/pages/settings.tsxmay overstate sandbox protection: Supabase sandbox runs still use the existing project, andmode: "none"provides no test-user isolation. Clarify the disclosure so filesystem/runtime isolation is not confused with provider-data isolation.src/ipc/services/e2e_test_workspace.tspreserves source symlinks that point outsideappPath, allowing test writes to reach the real filesystem. Validate symlink targets during copying and reject or rewrite links that escape the workspace.src/ipc/handlers/tests_handlers.tslogs a failedworkspace.dispose()but removes the workspace from active reconciliation, so a run can appear successful while cleanup remains incomplete. Keep failed workspaces registered for retry and surface the cleanup failure.src/components/SandboxedE2eTestsSwitch.tsxrenders the switch enabled while settings are loading, even whendisableSandboxedE2eTestsis explicitly true. Avoid showing an enabled state until the setting is known, so the UI does not briefly promise sandboxing incorrectly.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/components/preview_panel/TestsPanel.test.tsx">
<violation number="1" location="src/components/preview_panel/TestsPanel.test.tsx:454">
P3: These three disclosure tests — "tells Neon users their preview keeps its real database", "drops the sandbox disclosure when the sandbox is turned off", and "promises no sandbox while settings are still loading" — live inside `describe("stopping a run")`, but none of them touches stopping, the stop button, or the `cleaning-up` phase. They verify run-gate/disclosure rendering and belong at the top level alongside "runs sandboxed tests without the preview being up" and "still requires the preview...", not buried in the stopping block.</violation>
</file>
<file name="src/ipc/services/e2e_test_workspace.test.ts">
<violation number="1" location="src/ipc/services/e2e_test_workspace.test.ts:302">
P3: This test simulates a failed artifact copy with `chmod 0o000`, which only makes the directory unreadable for a non-root POSIX user. When the suite runs as root (common in the container CI this PR targets) or on Windows, `fs.chmod(0o000)` does not enforce read access, so `fs.cp` in `retainE2eTestArtifacts` succeeds and the `rejects.toThrow()` assertion fails, flaking the suite. The neighboring pnpm-realpath test is correctly gated with `it.runIf(process.platform !== "win32")`, but this one is not. Use a failure mode that is portable (e.g. a read-only artifact destination on ext4 is unreliable as well; prefer a source that is guaranteed unreadable across platforms, or gate the test to POSIX non-root) so the copy-failure branch stays covered without depending on the CI user and platform.</violation>
</file>
<file name="src/components/SandboxedE2eTestsSwitch.tsx">
<violation number="1" location="src/components/SandboxedE2eTestsSwitch.tsx:13">
P2: While the settings query is loading, `settings` is null, so `enabled` evaluates to `!undefined` and the switch renders ON even for users who have explicitly set `disableSandboxedE2eTests: true`. This briefly promises sandboxing that the user turned off (the same inverted-read footgun `TestsPanel.tsx` already guards against), and a toggle during that window writes a value derived from the not-yet-loaded state. Disable the switch until settings have loaded.</violation>
</file>
<file name="src/ipc/services/e2e_test_workspace.ts">
<violation number="1" location="src/ipc/services/e2e_test_workspace.ts:190">
P1: If the app source contains a symlink to a path outside `appPath`, the sandbox preserves that link and test writes can still reach the real filesystem. Validate symlink targets during copy and reject or rewrite links that escape the app root before creating the workspace.</violation>
<violation number="2" location="src/ipc/services/e2e_test_workspace.ts:315">
P2: `runDirectoryAppId` currently treats bare numeric names as valid run directories, even though run names are defined as `<appId>-...`. Require the hyphenated prefix format so non-run directories are not misclassified for cleanup or artifact ownership checks.</violation>
</file>
<file name="plans/sandboxed-e2e-test-runtime.md">
<violation number="1" location="plans/sandboxed-e2e-test-runtime.md:321">
P2: The plan fixes the workspace root at `<userData>/test-sandboxes/<appId>/<runId>` and scopes startup reconciliation ("remove abandoned directories only beneath the recognized `test-sandboxes` root") to that root, but never specifies where `E2eTestWorkspace.artifactPath` lives or how the retained artifact directory is administered. Retained artifacts are promoted out of the sandbox for the result UI, so they need the same bounded root, app-gone/run pruning, and canonical-containment guard the sandbox root gets; otherwise implementers can diverge and stranded artifact directories accumulate. State the artifact root (e.g. `<userData>/test-artifacts/<appId>/<runId>`), include it in startup reconciliation, and reuse the ownership/containment check for it.</violation>
</file>
<file name="src/ipc/handlers/tests_handlers.ts">
<violation number="1" location="src/ipc/handlers/tests_handlers.ts:727">
P2: When `workspace.dispose()` fails, this catch only logs the error, so the run reports success after removing the workspace from active reconciliation. Keep failed workspaces registered for retry and surface the cleanup failure in the terminal state before allowing subsequent runs to accumulate orphaned copies.</violation>
</file>
<file name="src/pages/settings.tsx">
<violation number="1" location="src/pages/settings.tsx:534">
P1: Qualify this disclosure: Supabase sandbox runs still target the existing project, and the `mode: "none"` fallback can run without test-user isolation. Distinguish filesystem/runtime isolation from provider data isolation instead of promising that real Supabase data is never touched.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let dependencyEntries = 0; | ||
| const startedAt = Date.now(); | ||
| try { | ||
| await fs.cp(appPath, workspacePath, { |
There was a problem hiding this comment.
P1: If the app source contains a symlink to a path outside appPath, the sandbox preserves that link and test writes can still reach the real filesystem. Validate symlink targets during copy and reject or rewrite links that escape the app root before creating the workspace.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ipc/services/e2e_test_workspace.ts, line 190:
<comment>If the app source contains a symlink to a path outside `appPath`, the sandbox preserves that link and test writes can still reach the real filesystem. Validate symlink targets during copy and reject or rewrite links that escape the app root before creating the workspace.</comment>
<file context>
@@ -0,0 +1,389 @@
+ let dependencyEntries = 0;
+ const startedAt = Date.now();
+ try {
+ await fs.cp(appPath, workspacePath, {
+ recursive: true,
+ verbatimSymlinks: true,
</file context>
| so tests never touch your preview or your real database. Turn this off | ||
| if copying your dependencies makes runs slow — tests then run against |
There was a problem hiding this comment.
P1: Qualify this disclosure: Supabase sandbox runs still target the existing project, and the mode: "none" fallback can run without test-user isolation. Distinguish filesystem/runtime isolation from provider data isolation instead of promising that real Supabase data is never touched.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/pages/settings.tsx, line 534:
<comment>Qualify this disclosure: Supabase sandbox runs still target the existing project, and the `mode: "none"` fallback can run without test-user isolation. Distinguish filesystem/runtime isolation from provider data isolation instead of promising that real Supabase data is never touched.</comment>
<file context>
@@ -526,6 +527,17 @@ export function WorkflowSettings() {
+ <SandboxedE2eTestsSwitch />
+ <p className={hint}>
+ Run each E2E test in a throwaway copy of your app with its own server,
+ so tests never touch your preview or your real database. Turn this off
+ if copying your dependencies makes runs slow — tests then run against
+ your normal preview, and apps using Neon won't run at all rather than
</file context>
| so tests never touch your preview or your real database. Turn this off | |
| if copying your dependencies makes runs slow — tests then run against | |
| so tests never touch your preview. Neon and no-database runs keep data | |
| isolated from the real environment; Supabase runs use the existing | |
| project and depend on RLS-scoped test-user isolation, so this setting | |
| does not guarantee that real Supabase data is untouched. Turn this off | |
| if copying your dependencies makes runs slow — tests then run against |
| */ | ||
| export function SandboxedE2eTestsSwitch() { | ||
| const { settings, updateSettings } = useSettings(); | ||
| const enabled = !settings?.disableSandboxedE2eTests; |
There was a problem hiding this comment.
P2: While the settings query is loading, settings is null, so enabled evaluates to !undefined and the switch renders ON even for users who have explicitly set disableSandboxedE2eTests: true. This briefly promises sandboxing that the user turned off (the same inverted-read footgun TestsPanel.tsx already guards against), and a toggle during that window writes a value derived from the not-yet-loaded state. Disable the switch until settings have loaded.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/SandboxedE2eTestsSwitch.tsx, line 13:
<comment>While the settings query is loading, `settings` is null, so `enabled` evaluates to `!undefined` and the switch renders ON even for users who have explicitly set `disableSandboxedE2eTests: true`. This briefly promises sandboxing that the user turned off (the same inverted-read footgun `TestsPanel.tsx` already guards against), and a toggle during that window writes a value derived from the not-yet-loaded state. Disable the switch until settings have loaded.</comment>
<file context>
@@ -0,0 +1,29 @@
+ */
+export function SandboxedE2eTestsSwitch() {
+ const { settings, updateSettings } = useSettings();
+ const enabled = !settings?.disableSandboxedE2eTests;
+ return (
+ <div className="flex items-center space-x-2">
</file context>
| export function runDirectoryAppId(name: string): number | null { | ||
| const [prefix] = name.split("-"); | ||
| const appId = Number(prefix); | ||
| return prefix !== "" && Number.isInteger(appId) ? appId : null; |
There was a problem hiding this comment.
P2: runDirectoryAppId currently treats bare numeric names as valid run directories, even though run names are defined as <appId>-.... Require the hyphenated prefix format so non-run directories are not misclassified for cleanup or artifact ownership checks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ipc/services/e2e_test_workspace.ts, line 315:
<comment>`runDirectoryAppId` currently treats bare numeric names as valid run directories, even though run names are defined as `<appId>-...`. Require the hyphenated prefix format so non-run directories are not misclassified for cleanup or artifact ownership checks.</comment>
<file context>
@@ -0,0 +1,389 @@
+export function runDirectoryAppId(name: string): number | null {
+ const [prefix] = name.split("-");
+ const appId = Number(prefix);
+ return prefix !== "" && Number.isInteger(appId) ? appId : null;
+}
+
</file context>
| return prefix !== "" && Number.isInteger(appId) ? appId : null; | |
| return /^\d+-/.test(name) ? Number(prefix) : null; |
|
|
||
| On Dyad startup: | ||
|
|
||
| - remove abandoned directories only beneath the recognized |
There was a problem hiding this comment.
P2: The plan fixes the workspace root at <userData>/test-sandboxes/<appId>/<runId> and scopes startup reconciliation ("remove abandoned directories only beneath the recognized test-sandboxes root") to that root, but never specifies where E2eTestWorkspace.artifactPath lives or how the retained artifact directory is administered. Retained artifacts are promoted out of the sandbox for the result UI, so they need the same bounded root, app-gone/run pruning, and canonical-containment guard the sandbox root gets; otherwise implementers can diverge and stranded artifact directories accumulate. State the artifact root (e.g. <userData>/test-artifacts/<appId>/<runId>), include it in startup reconciliation, and reuse the ownership/containment check for it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plans/sandboxed-e2e-test-runtime.md, line 321:
<comment>The plan fixes the workspace root at `<userData>/test-sandboxes/<appId>/<runId>` and scopes startup reconciliation ("remove abandoned directories only beneath the recognized `test-sandboxes` root") to that root, but never specifies where `E2eTestWorkspace.artifactPath` lives or how the retained artifact directory is administered. Retained artifacts are promoted out of the sandbox for the result UI, so they need the same bounded root, app-gone/run pruning, and canonical-containment guard the sandbox root gets; otherwise implementers can diverge and stranded artifact directories accumulate. State the artifact root (e.g. `<userData>/test-artifacts/<appId>/<runId>`), include it in startup reconciliation, and reuse the ownership/containment check for it.</comment>
<file context>
@@ -0,0 +1,489 @@
+
+On Dyad startup:
+
+- remove abandoned directories only beneath the recognized
+ `test-sandboxes` root;
+- never follow directory links during recursive cleanup;
</file context>
| !(await prepared.teardown()).remoteCleanupCompleted, | ||
| ); | ||
| } catch (error) { | ||
| logger.error( |
There was a problem hiding this comment.
P2: When workspace.dispose() fails, this catch only logs the error, so the run reports success after removing the workspace from active reconciliation. Keep failed workspaces registered for retry and surface the cleanup failure in the terminal state before allowing subsequent runs to accumulate orphaned copies.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ipc/handlers/tests_handlers.ts, line 727:
<comment>When `workspace.dispose()` fails, this catch only logs the error, so the run reports success after removing the workspace from active reconciliation. Keep failed workspaces registered for retry and surface the cleanup failure in the terminal state before allowing subsequent runs to accumulate orphaned copies.</comment>
<file context>
@@ -515,6 +601,155 @@ export async function runAppTestsCore({
+ !(await prepared.teardown()).remoteCleanupCompleted,
+ );
+ } catch (error) {
+ logger.error(
+ `Failed to tear down isolated test environment for app ${appId}: ${error}`,
+ );
</file context>
| }); | ||
|
|
||
| it("names the Neon teardown, which restarts the preview", () => { | ||
| it("tells Neon users their preview keeps its real database", async () => { |
There was a problem hiding this comment.
P3: These three disclosure tests — "tells Neon users their preview keeps its real database", "drops the sandbox disclosure when the sandbox is turned off", and "promises no sandbox while settings are still loading" — live inside describe("stopping a run"), but none of them touches stopping, the stop button, or the cleaning-up phase. They verify run-gate/disclosure rendering and belong at the top level alongside "runs sandboxed tests without the preview being up" and "still requires the preview...", not buried in the stopping block.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/preview_panel/TestsPanel.test.tsx, line 454:
<comment>These three disclosure tests — "tells Neon users their preview keeps its real database", "drops the sandbox disclosure when the sandbox is turned off", and "promises no sandbox while settings are still loading" — live inside `describe("stopping a run")`, but none of them touches stopping, the stop button, or the `cleaning-up` phase. They verify run-gate/disclosure rendering and belong at the top level alongside "runs sandboxed tests without the preview being up" and "still requires the preview...", not buried in the stopping block.</comment>
<file context>
@@ -391,42 +451,92 @@ describe("TestsPanel", () => {
});
- it("names the Neon teardown, which restarts the preview", () => {
+ it("tells Neon users their preview keeps its real database", async () => {
+ // The pre-sandbox copy promised a double preview restart. Nothing
+ // restarts any more, so that disclosure would now be a lie.
</file context>
| }); | ||
| // An unreadable source makes the copy throw the way a real EBUSY/ENOSPC | ||
| // would, without touching the artifact root the prune has to write to. | ||
| await fs.chmod(path.join(workspace.workspacePath, "test-results"), 0o000); |
There was a problem hiding this comment.
P3: This test simulates a failed artifact copy with chmod 0o000, which only makes the directory unreadable for a non-root POSIX user. When the suite runs as root (common in the container CI this PR targets) or on Windows, fs.chmod(0o000) does not enforce read access, so fs.cp in retainE2eTestArtifacts succeeds and the rejects.toThrow() assertion fails, flaking the suite. The neighboring pnpm-realpath test is correctly gated with it.runIf(process.platform !== "win32"), but this one is not. Use a failure mode that is portable (e.g. a read-only artifact destination on ext4 is unreliable as well; prefer a source that is guaranteed unreadable across platforms, or gate the test to POSIX non-root) so the copy-failure branch stays covered without depending on the CI user and platform.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ipc/services/e2e_test_workspace.test.ts, line 302:
<comment>This test simulates a failed artifact copy with `chmod 0o000`, which only makes the directory unreadable for a non-root POSIX user. When the suite runs as root (common in the container CI this PR targets) or on Windows, `fs.chmod(0o000)` does not enforce read access, so `fs.cp` in `retainE2eTestArtifacts` succeeds and the `rejects.toThrow()` assertion fails, flaking the suite. The neighboring pnpm-realpath test is correctly gated with `it.runIf(process.platform !== "win32")`, but this one is not. Use a failure mode that is portable (e.g. a read-only artifact destination on ext4 is unreliable as well; prefer a source that is guaranteed unreadable across platforms, or gate the test to POSIX non-root) so the copy-failure branch stays covered without depending on the CI user and platform.</comment>
<file context>
@@ -0,0 +1,424 @@
+ });
+ // An unreadable source makes the copy throw the way a real EBUSY/ENOSPC
+ // would, without touching the artifact root the prune has to write to.
+ await fs.chmod(path.join(workspace.workspacePath, "test-results"), 0o000);
+
+ try {
</file context>
| // project — calling that second one "the isolated test database" is the | ||
| // same class of wrong-thing copy this work set out to remove. Both are | ||
| // retried by their own startup sweep (`reconcileOrphanTestBranches`, | ||
| // `reconcileOrphanTestUsers`), so both say so. |
There was a problem hiding this comment.
🟡 MEDIUM
Supabase test-user leak is reported as a database cleanup failure
The cleanup warning picks its wording from result.isolation?.mode === "supabase-test-user", but the Supabase setup-failure path in prepareSupabaseTestUserIsolation returns isolation: { mode: "none", reason: "Couldn't set up an isolated Supabase test user." } while the new settledTeardown it now hands back can carry remoteCleanupCompleted: false. In that case the else branch fires and the user is told "Dyad couldn't finish cleaning up the isolated test database ... Dyad will retry remote cleanup on next startup" for an app that has no isolated database at all — what actually leaked is a temporary auth user in their real Supabase project, swept by reconcileOrphanTestUsers. This is the same wrong-thing cleanup copy the PR set out to remove, and the new test that guards it only covers the mode: "supabase-test-user" case.
💡 Suggestion: Key the message off something that survives the failure path — e.g. carry the provider on the isolation object even for the mode: "none" failure returns, or select the wording from app.supabaseProjectId/app.neonProjectId rather than from isolation.mode. Add a case covering a Supabase setup failure whose user delete fails.
| (settings?.runtimeMode2 ?? "host") === "host"; | ||
| // Host runs get the sandbox: a throwaway copy of the app, its own server, and | ||
| // a temporary Neon branch that only that copy points at. Worth saying, since | ||
| // the alternative a user would assume is "my tests hit my real database" — |
There was a problem hiding this comment.
🟡 MEDIUM
No Run gate or disclosure for Neon apps when the sandbox is off
showNeonSandboxDisclosure now requires sandboxAvailable === true, and testRunBlocked only considers the dev server. For a Neon app on Docker/cloud runtime, or with disableSandboxedE2eTests set, the panel therefore shows no disclosure at all and renders Run fully enabled — but the main process refuses the run outright ("Dyad won't run Neon tests against your real database"). That refusal is a deliberate capability removal relative to the previous behaviour, and the only place it is disclosed up front is the Settings hint the user may never open. The panel is where the user presses Run, and it currently promises an action it will reject.
💡 Suggestion: Add a Neon-specific branch: when sandboxAvailable === false and app?.neonProjectId is set, disable Run (fold it into testRunBlocked) and show a banner naming the refusal and the fix (switch to host runtime / re-enable the sandbox setting).
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed all 41 changed files. The aggregate This is a large, unusually well-reasoned change. The main/renderer boundary is preserved (no new broad filesystem or process exposure), the new Two MEDIUM issues below. Neither blocks merge. Issues Summary
🟢 Low Priority Notes (6 items)
Generated by Dyadbot persona-based code review |
1 participant
What this changes
E2E test runs no longer execute against the user's real working tree, real preview, or real database. Each run gets:
<userData>/test-sandboxes— the app's current source (tracked and untracked) plus a copy-on-write clone of itsnode_modules;runningApps, so the normal preview keeps its PID, URL and real branch;The real
.env.localis byte-identical before and after, and the preview is never restarted.Behavior changes worth calling out
Neon apps can no longer run tests in Docker or cloud runtime. Previously
prepareIsolatedTestDatabasereturnedmode: "none"with the disclosure "Isolated test data isn't available in docker runtime yet — tests run against your current data", and the run proceeded against the user's real Neon database. The sandbox is host-only for now, so there is no throwaway branch to point a Docker/cloud app at, and this now fails closed with an explanatory error instead. This is intentional and is the plan's "for Neon specifically, never degrade to the normal preview/real database" applied to the runtime-mode case — but it is a capability removal for Docker/cloud users with a Neon app, not a side effect. Supabase and no-database apps on those runtimes keep working: they run against the normal preview with the missing runtime isolation disclosed on the result.Sandboxing can be turned off.
Settings → Workflow → Run E2E Tests in an Isolated Sandboxis on by default.COPYFILE_FICLONEonly clones on reflink-capable filesystems, so on ext4 and Windows the per-run snapshot is a real copy; turning the setting off routes through the same non-sandboxed path as Docker/cloud (and still refuses Neon apps). Copy/dispose durations and entry counts are reported as telemetry so the cost on non-reflink filesystems is measurable.Sandboxed runs no longer require the preview to be running. The sandbox serves the app itself, so the Tests panel's "Start the app to run tests." gate and the agent's equivalent refusal now apply only to the non-sandboxed path. Recording still requires the preview — it drives the live one.
Test artifacts are now swept. Retained screenshots and traces under
<userData>/test-artifactsare pruned at startup when their app no longer exists, and dropped when an app is deleted. Because they live outside the app directory, the agent'sread_filecannot reach them, so a failing sandboxed run gets Playwright's page snapshot inlined into the tool result instead of a path.