Feat/improve e2e - #277
Merged
Merged
Conversation
Reconcile the e2e auth/env rework with main's overlapping changes. - Adopt main's helpers/controlplane-url.ts (derives the control-plane origin from ORCABOT_URL) instead of requiring CONTROLPLANE_URL explicitly. - Accept any 2xx from POST /auth/dev/session. It returned 204 historically and now returns 200 with a JSON body, which broke devModeLogin outright. - Use the server-resolved user id for the injected auth state: dev auth is email-keyed and reconciles to an existing user's id. - Probe dev auth via an isolated request context so it can't mint a session in the browser's cookie jar and pre-authenticate devModeLoginViaUI. - Send X-Orcabot-Surface when E2E_SURFACE_TOKEN is set, for targets that gate dev auth behind a surface token. - Support E2E_API_TOKEN (personal access token) for control plane API calls, which is the workable path on instances without dev auth. Demote the Google UI login to a last resort behind storageState and dev auth. - Keep defaults for E2E_USER_EMAIL/NAME; the env gate now only hard-fails on ORCABOT_URL and reports optional tiers instead. - Fix diagnostics: `const performance` shadowed the global inside page.evaluate, and summarizePerformance rejected the null it was passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HX7jmDsoJMfnzbeGyfozcu
All five were only observable at runtime; typechecking and review missed them.
- isAlreadyAuthenticated() returned true while logged out. getByText("New
Dashboard") is a case-insensitive SUBSTRING match, so it matched the splash
page's "Sign in and create a new dashboard - a shared workspace". login()
then silently no-opped and the test failed later on a confusing URL
assertion. Use an exact heading-role match.
- Restore the "Sign In" link selector removed during the merge. It does exist,
in a header component rather than page.tsx.
- assertNoSevereIssues() failed every login test on an expected 401: the login
helpers deliberately probe authenticated endpoints while logged out. Exclude
browser-generated resource-load 401/403 lines from the pass/fail decision,
keep them in the attached diagnostics, and accept a per-call ignore list.
Uncaught exceptions and real console.error calls are still fatal.
- Skip the dev-mode UI login test with an explicit reason. That form was
removed from the splash in "Add new splash page" (#193).
- Replace test.beforeAll() in the shared fixture module with a module-scope
check. Playwright binds such a hook to whichever spec's root suite happens to
be loading and rejects it outright.
Verified against a local frontend + control plane: recipes 01 and 02 pass
(15 passed, 1 skipped across 01/02/05/06/07).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HX7jmDsoJMfnzbeGyfozcu
…m label data-guidance-target was computed as tool.label.toLowerCase(), so the target string was a function of display copy. Google Calendar and Outlook Calendar are both labelled "Calendar", so both buttons rendered data-guidance-target="calendar". UIGuidanceOverlay resolves targets with document.querySelector, which silently takes the first match — in-app guidance pointed at whichever button happened to come first in the DOM, and it would have moved again on any copy change. tool.type isn't usable as the identifier either: every terminal preset shares type "terminal". Add an explicit `id` to BlockTool, unique across all tool arrays, and use it for both data-guidance-target and the React key. Existing target strings are unchanged except the Outlook entry, which becomes "outlook-calendar". Fixes the two e2e tests that failed with a strict-mode violation on [data-guidance-target="calendar"] resolving to 2 elements. Verified locally: recipe 05 now 6/6, recipe 07 5/6 (the remaining one needs a sandbox VM). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HX7jmDsoJMfnzbeGyfozcu
P1 — Google password could reach retained traces. Playwright records action parameters, including the value passed to fill(), and traces were retained on failure, so any failure after the password was typed would persist GOOGLE_TEST_PASSWORD inside trace.zip and carry it wherever CI artifacts go. Force trace and video off whenever GOOGLE_TEST_EMAIL + GOOGLE_TEST_PASSWORD are set, with a warning pointing at E2E_STORAGE_STATE / E2E_API_TOKEN. Config alone isn't enough — `--trace on` overrides it — so googlePopupLogin also checks the effective trace mode and refuses to type the password rather than leak it. Verified: with `--trace on` the run fails closed, and no artifact contains the password. P2 — the UI-login test always skipped. It probed for the dev-mode button while the page was still about:blank, where nothing can be visible, so the skip was unconditional. It also skipped when Google credentials were configured even though loginViaUI supports that path. Navigate first, then skip only when neither strategy is available. devModeLoginViaUI had a related flaw: it chose its strategy from whether dev auth was reachable over the API, which can disagree with what the splash renders. It now branches on the form's actual presence, via a shared devLoginFormVisible() helper so the spec's skip decision and the helper's strategy choice cannot diverge. P2 — dotenv override clobbered explicit configuration. `override: true` on .env.test.local replaced values already in process.env, beating an inline ORCABOT_URL (contradicting the documented invocation) and potentially overwriting injected credentials or CI itself, which drives forbidOnly and retries. Apply the files by hand instead: the real environment always wins, and among files .env.test.local > .env > .env.test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HX7jmDsoJMfnzbeGyfozcu
P1 — the password reached the HTML report, not just the trace. Verified with a
probe: a PASSING test with trace and video disabled still stored
`Fill "<secret>" locator('#p')` in the report's embedded data, because
Playwright puts action arguments in step titles and the HTML reporter
serializes every title. Suppressing trace, then video, then the report is
whack-a-mole — JSON and blob reporters serialize titles too — so remove the
source instead:
- delete googlePopupLogin and every GOOGLE_TEST_PASSWORD reference
- delete the now-pointless trace guard, and restore trace (no password is
typed, so nothing needs suppressing)
- add `npm run auth:capture` (scripts/capture-auth.mjs): log in by hand once
in a headed browser — SSO/2FA included — and only the resulting
storageState is written to the gitignored e2e/.auth/
- drop GOOGLE_TEST_PASSWORD from the google env tier; the rest of that tier
is account DATA for integration assertions, not credentials
Audited every fill() in the suite: only dashboard name and test user name/email
remain.
P2 — stop presenting a PAT as browser auth. E2E_API_TOKEN authenticates
control-plane API calls only and cannot establish a browser session;
E2E_STORAGE_STATE is the sole browser-login mechanism. Corrected in the config,
.env.example, and the "no usable login strategy" error so nobody follows it.
P2 — bump the stale revision markers: auth v5-drop-password-login, config
v4-storage-state-only-ui-auth, env v3-no-password-tier, diagnostics
v2-ignore-4xx-noise.
Two problems this run exposed, both from earlier changes of mine:
Restoring artifacts revealed that assertNoSevereIssues failed three
dashboard-CRUD tests on a 404 for /dashboards/:id/workspace-snapshot. The
endpoint exists and getWorkspaceSnapshot() documents that 404 as expected for a
dashboard with no cached snapshot, so this is the same browser-generated noise
as the 401 — the pattern was just too narrow. Ignore any 4xx; keep 5xx fatal.
`retain-on-failure` records every test and discards passing ones, so it costs on
all of them: 6.4m -> 19.3m for this suite. Every earlier fast run had artifacts
suppressed by the password workaround, hiding it. Keep the trace (DOM snapshots,
network, console) and turn video off.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HX7jmDsoJMfnzbeGyfozcu
The previous comment blamed trace + video for the 3x slowdown. Measured per-test on this suite, the trace alone accounts for nearly all of it: no artifacts ~17s (6.4m / 23 tests) trace + video ~50s (19.3m / 23 tests) trace only ~45s (14.3m / 19 tests) So dropping video recovered little. Keep the trace regardless — failures here are usually slow or racy UI states that a trace explains and a video does not — and document "on-first-retry" + retries as the escape hatch if wall clock becomes the bottleneck. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HX7jmDsoJMfnzbeGyfozcu
The typecheck script I added could never run: neither typescript nor
@types/node was declared, so `npm run typecheck` died with "tsc: command not
found". Declare both and update the lockfile. It now exits 0 and, via the
existing tsconfig include, covers all **/*.ts rather than the file list I had
been passing by hand.
The 4\d\d console filter was too blunt — it hid genuine failures (400, 409,
422, 429, and any 404 outside the one documented case). Replace it with an
enumerated list of expected responses, each carrying the reason it is expected
and scoped by the console message's location URL where the status alone is
ambiguous:
- 401/403 — the login helpers load authenticated routes while logged out on
purpose, so the app correctly refuses them
- 404 on /dashboards/:id/workspace-snapshot ONLY — getWorkspaceSnapshot()
documents that 404 as expected and returns null
Verified two ways. Recipes 01 and 02 pass (6 passed, 1 skipped), which
exercises both rules — recipe 02 creates fresh dashboards and so provokes the
snapshot 404, confirming the location-URL match works against the resource URL.
And a table check over 11 cases confirms the filter discriminates: 400/409/422/
429/500, a 404 on any other endpoint, and app console.error calls are all still
reported.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HX7jmDsoJMfnzbeGyfozcu
The previous numbers attributed a ~2.6x slowdown to tracing. A post-merge run with the SAME trace-only config finished 19 tests in 4.3m (~14s/test) against 14.3m (~45s/test) earlier, so the variable was machine contention from concurrent dev servers, not the trace. Record all four measurements and say plainly which factor dominates, so nobody tunes artifact settings to fix a load problem. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HX7jmDsoJMfnzbeGyfozcu
GET /user/setup answers needsAiSetup = !hasAiKey && !dismissed, and the test user has no AI key. So on every dashboard the chat panel expanded and its setup card sat over the canvas as a fixed centred overlay, swallowing clicks aimed at terminal and block connectors — surfacing as an unhelpful "element intercepts pointer events" in eight tests of a full-suite run. Dismiss through the real POST /user/setup/ai-dismissed rather than stubbing the GET, so the app is in a state a real user can actually be in. Applied on every successful login path, including the already-authenticated early return, since a saved storageState can have the card pending too. Best-effort and idempotent; the flag persists per user. Verified against localhost: the setup-card overlay no longer appears. Five canvas tests still fail, but now on a DIFFERENT overlay — see the next commit message / report for the measured cause. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HX7jmDsoJMfnzbeGyfozcu
…t eat clicks fit() only runs once a terminal connects, so a terminal that fails to get a sandbox session stays at xterm's 80x24 default. In a block narrower than ~454px the .xterm-rows layer then renders wider than its viewport and, being pointer-events:auto with overflow:visible, spills onto the canvas and swallows clicks aimed at whatever sits to its right — a neighbouring block's connector, for instance. One failed terminal made the surrounding canvas unusable. Applied to the terminal's own container, NOT the portal wrapper in TerminalBlock: that wrapper also holds ConnectionMarkers, which are positioned outside the block edges on purpose and must not be clipped. Verified live (measured in-page, not assumed): the container computes overflow:hidden, the revision marker appears in the console, and with a healthy sandbox .xterm-rows is 461px inside a 476px viewport — nothing to clip, so this is a no-op in the normal case. Recipes 03, 04 and 06 pass against it (9 tests), covering terminal connect, command execution and canvas wiring. This is hardening, not the cure. The trigger was sandbox session creation failing, root-caused separately: in shared-sandbox mode nothing ever calls DELETE /sessions/:id, so every session leaks a prewarmed Chromium stack until the VM exhausts its memory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HX7jmDsoJMfnzbeGyfozcu
…leted A sandbox session owns real resources inside the VM — its PTYs and a full Chromium/Xvfb/x11vnc stack, several hundred MB each. Session.Close() is only reachable via DELETE /sessions/:id, and that was called just from the Fly-gated teardown plus a create-race dedup. On any deployment sharing one sandbox (local dev, desktop, self-hosted) deleting a dashboard therefore freed its D1 rows and nothing else, so every dashboard ever created leaked a browser stack until the VM exhausted memory. At that point the Go server was starved badly enough that even GET /health could not answer, and every later session create failed after 33s with "fetch error: The operation was aborted" — 15s fetch timeout, 3s sleep, 15s retry. Read the sandbox row once, release the session unconditionally, then do the Fly-specific machine/volume teardown inside the existing FLY_API_TOKEN guard as before. Best-effort: an unreachable or already-wedged sandbox must not block deleting the dashboard, since that is exactly the state a user needs to clean up. Redundant on Fly, where destroying the machine reclaims everything anyway. Measured against a fresh container, per dashboard created and deleted: before ~140 MB (29 dashboards exhausted 4 GB) after ~8.5 MB (20 dashboards -> 171 MB), 20/20 sessions released Recipes 01, 02, 05 and 09 pass against it (13 tests + the 10x connect loop). Residual, NOT fixed here: 252 zombie processes accumulated over those 20 dashboards. sandbox/internal/browser/browser.go killAll() kills only the four direct children and never Wait()s them, so Chromium's zygote/renderer/crashpad children are orphaned. That leaks PIDs rather than memory and needs a separate sandbox-side fix (process groups + Wait). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HX7jmDsoJMfnzbeGyfozcu
…s-group kill)
Browser sessions leaked PIDs: 500 zombies / 519 PIDs after ~35 dashboards,
which would eventually stop the container forking.
Diagnosis first. Every zombie had PPID 1, and PID 1 was orcabot-server:
zombie count by PPID: 500 -> 1
PID 1 = orcabot-server
sample: chrome_crashpad (Z), chromium (Z)
So these are Chromium's GRANDCHILDREN (zygote, renderers, GPU, crashpad),
orphaned when their parent is killed at teardown and reparented to PID 1, which
has no SIGCHLD reaper. Adding Wait() to killAll() would not have touched them —
that only reaps the four direct children.
Two changes:
1. tini as PID 1 (Dockerfile). Reaps whatever gets reparented to it. A blanket
wait4(-1) reaper inside the Go server was rejected deliberately:
internal/pty/pty.go:359-367 documents that it relies on cmd.Wait() reaping
its own children to recycle pool slots, and a competing reaper would steal
those exit statuses.
2. Process-group kill (internal/browser/browser.go). Each of the four processes
now leads its own group via Setpgid, and teardown signals the group then
Wait()s, via a shared killProcessTree() used by all three teardown paths
(killAll, Stop, and the Status cleanup path — which had drifted apart).
Killing only the direct pid left descendants RUNNING, not merely unreaped:
196 live chromium processes were measured alongside 252 zombies.
Verified on a rebuilt image, same 20-dashboard workload as the measurement that
exposed this:
before 252 zombies, 264 PIDs
after 0 zombies, 14 PIDs
PID 1 confirmed as tini with the server as a child. Recipes 03, 04, 06, 07 pass
(15 tests) plus the 10x connect loop twice, so terminals and browser blocks are
unaffected.
Residual: memory still creeps ~7 MB per dashboard (259 MB over 20). Far from the
~140 MB/dashboard before the control-plane session-release fix, but not zero —
not chased here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HX7jmDsoJMfnzbeGyfozcu
…ak, revision
P1 — dashboard deletion orphaned an R2 object and was slowest when wedged.
deleteDashboard called stopSession() in a sequential loop, one per terminal.
stopSession is the "user closed a terminal, keep the dashboard usable" path: on
the last session it captures a workspace snapshot into R2 for recovery, and
deletes that session's PTY. Both are wrong when the dashboard itself is going
away — the snapshot was written moments before its dashboard ceased to exist and
nothing ever collected it. The loop also cost one sandbox round trip per
terminal, each able to block for the full 15s timeout, making deletion slowest
in exactly the case a user is trying to clean up: a wedged sandbox.
Replaced with a dedicated teardown: one D1 UPDATE marking sessions stopped, an
R2 purge (new exported purgeDashboardStorage, also de-duplicating the dev
clear-workspace endpoint), and the single bounded deleteSession already present.
Destroying the sandbox session tears down every PTY it owns, so the per-PTY
deletes were redundant.
Verified: 5 dashboards WITH terminals created and deleted, workspace snapshot
count in local R2 unchanged at 1 (a pre-existing orphan from before this fix).
P2 — diagnostics suppressed 401/403 from any endpoint, so a genuine
authorization regression anywhere would pass assertNoSevereIssues(). Scoped to
GET /users/me, the only authenticated call made while logged out (the dashboard
page's other queries are gated on `isAuthenticated && isAuthResolved`). A
401/403 from anywhere else now fails, as it should.
P2 — the dev-auth probe leaked a session row per worker per run. It POSTed
/auth/dev/session, which MINTS a 30-day user_sessions row, then discarded the
cookie; expired rows have no cleanup path. Probe GET /users/me instead — same
answer, no write. Measured against local D1:
GET /users/me delta 0 rows
POST /auth/dev/session delta 1 row
P2 — browser.go logged browser-v10 while the file and behaviour were v12,
making the process-group fix unverifiable from logs, contrary to the sandbox's
mandatory revision guidance. Now logs browser-v12-process-group-kill, confirmed
in container output after a rebuild.
Recipes 01, 02, 03, 04 pass (11 tests + 1 skip).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HX7jmDsoJMfnzbeGyfozcu
…ifests
purgeDashboardStorage deleted three manifest keys and nothing else, but each
family stores per-file objects alongside its manifest:
workspace/<id>/snapshot.json
drive/<id>/manifest.json + drive/<id>/files/<fileId>
mirror/<provider>/<id>/manifest.json + mirror/<provider>/<id>/files/<fileId>
So deleting a dashboard removed the index and orphaned every uploaded and
mirrored file indefinitely — unreferenced but still billable. Sweep the prefixes
instead, which also covers any new key shape added under them rather than
leaking quietly until someone notices.
Mirror providers are discovered from the bucket via delimited listing and
unioned with the known list, so a provider added later cannot silently keep
leaking.
The 1000-key list cap needs repeating, and the first version of that loop used a
cursor. Tests caught it: deleting the keys you just listed invalidates a
position-encoded cursor, so with 2500 objects it stranded the remainder. It now
re-lists from the start each round, which is correct under either cursor
semantic and still terminates because every round deletes everything it saw.
Bounded at 1000 rounds with a log rather than spinning.
Added src/sessions/purge-storage.test.ts covering: per-file deletion, other
dashboards untouched, paging past 1000, a provider absent from the hardcoded
list, and one prefix failing without stranding the rest. 86 control-plane tests
pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HX7jmDsoJMfnzbeGyfozcu
Backfill for the leak fixed in the previous commit. Dashboards deleted before
purgeDashboardStorage swept by prefix removed only their manifests, orphaning
every uploaded and mirrored file — unreferenced but still billable. Nothing
reclaims those retroactively, so this finds and removes them.
POST /admin/storage/orphan-sweep, admin-gated and PAT-rejected (a leaked token
must not be able to mass-delete stored content).
DRY RUN unless the body carries { "apply": true }. The input is a whole bucket
and a mistake is unrecoverable, so the safe mode is the one you get by accident;
the dry run reports the orphaned dashboard ids plus object and byte counts.
Optional { "limit": n } processes a batch at a time, with `truncated` in the
response saying whether more remain.
Discovery uses delimited listing (workspace/<id>/, drive/<id>/,
mirror/<provider>/<id>/) so it costs a page per prefix level rather than
enumerating the bucket, then checks the ids against `dashboards` in chunks of
400 to stay under SQLite's bind-parameter cap. Deletion reuses
purgeDashboardStorage, so the sweep and the normal delete path can never
disagree about what belongs to a dashboard.
Tests cover: dry run deletes nothing, apply removes only orphans and leaves live
dashboards intact, limit/truncated, the no-orphans case, and discovery paging
past the 1000-object list cap. 91 control-plane tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HX7jmDsoJMfnzbeGyfozcu
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.