feat(evals): make surface placement explicit, and own browser disposal - #3387
Merged
Conversation
…t state not route shapes
…come surface, no onboarding needed)
…ics name the interception
…call, not an unverified bridge channel
…ing instead of a hanging server fetch
…ns stop answering
…l when the button is not text
…appear in the route
…shot The skills menu is a scrollable list: /browser-automation was loaded (the DOM assertions passed) while the screenshot showed only the rows above it, so the visual validator honestly reported it absent.
The workspace engine boot blocks the renderer JS thread in bursts. Bare 20s evaluations (plug-menu clicks) died with raw CDP timeouts, and the measure evaluations gave their outer CDP call the same 20s deadline as the in-page poll, so the two raced under load. Fold the menu clicks into a guarded, retried wait and give the measures headroom over their inner deadlines.
…enshot The capability menu is a scrollable popover: all four sections were in the DOM (the code assertion passed) while Agents sat above the fold, so the validator honestly reported it invisible. Scroll the named row into view and let the code assertion carry section completeness.
… see The vision loop caught a real product defect: with a long command list the capability popover extends under the window header, so its first section row (Agents) is covered and unclickable (repo workspace, popover top at y=17 under the title bar). Keep four-section completeness as the DOM assertion, scope the visual claim to the visible sections, and record the defect where the claim is made.
…u with retried waits The scenario opened the plug menu inside its own evaluate with fixed 100/300ms delays right after a reload; under load those clicks land before React handlers attach and the whole block times out. Arm the fetch delay and the connect witness in one small mutation, open the menu with the guarded retried helper, then measure.
…cloud skills The scenario set window.__OPENWORK_GATEWAY__, which flips the app into the hosted gateway runtime and rewires the local openwork-server client to the page origin — so client.listSkills hit the Vite dev server and local skills never rendered, a state no desktop user can reach. The den auth token and active org id it also set are the real cloud-connected desktop arrangement, so keep only those. Drop the unused resetSkillsCloudState.
…t be faithful On desktop, den traffic goes through the main process, so the renderer fetch hook never delayed anything (denRequestCount stayed 0), and the gateway marker it used instead rewired the local server to the page origin. The block therefore only ever measured an impossible state. Cloud latency belongs in a den-boundary fault spec like app-den-tls-fault, where the fault is injected for real.
…pace before sign-in signInInBrowser returned right after clicking Sign in, so the next frame honestly showed 'Working...'. Wait for the signed-in outcome (or the sign-in code) before returning. The signed-in org shell offers no Add workspace entry, and the workspace.create control there reports ok while creating nothing (its error lands in closed-modal state) — probed live twice. Arrange org specs the way a member actually gets there: create the workspace on the proven welcome path first, then sign in; organization onboarding is completed when it appears and the existing workspace is reselected either way.
POST /v1/marketplaces/:id/access rejects a grant without role (viewer|editor|manager). Viewer is what sharing with a colleague means.
…nents The resolved marketplace payload carries plugins with component counts only; skill names live on GET /v1/plugins/:id/resolved as items[].configObject (objectType skill, title). Read them as the member so visibility stays the colleague's, not the admin's.
… managed-recovery defect The picker can paint 'No models' before the engine's catalog lands (observed live: 0 models on first read, 7 shortly after on the same boot), so the primary test polls up to 90s before asserting. The managed test now pins today's truth: after an admin publishes a provider, Retry does not deliver it to the composer even though the API already entitles the member (probed live, 201 + readable as the member, empty notice survives 90s+). The spec asserts the notice stays honest and the composer stays usable; when live recovery ships, the pinned wait fails loudly and the recovery assertions come back from history.
… a model) With no selectable model the composer renders no contenteditable editor, so the pinned block asserts what is really there: the persistent notice with Retry and no crash.
…eads Refreshing mid-flight)
resolveHost() reads one ambient env var, so a whole process got one host:
everything local, or everything in one sandbox. A spec could not say 'app
here, browser there' or 'two desktops in two sandboxes' — the topology was
inherited from the environment instead of declared.
- desktop({ host }) and the new chrome({ host }) take a placement.
- localHost() / daytonaSandbox(id) name one, with REPO_ROOT wiring kept
inside the package. daytonaSandbox() fails loudly from inside a sandbox,
where the CLI indirection has nothing to target.
- resolveHost() default behaviour is unchanged, so existing specs are
untouched.
chrome() also fixes a real leak: attachSurface only closes the CDP socket, so
the hand-rolled spawnChrome + attachSurface in first-run-cloud-share left a
browser process running after every run. Disposing now stops the client and
then disposes the surface, which is what kills it.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
src-opn
self-requested a review
July 31, 2026 13:30
src-opn
previously approved these changes
Jul 31, 2026
benjaminshafii
enabled auto-merge (squash)
July 31, 2026 13:36
…ement # Conflicts: # evals/specs/first-run-cloud-share.slow.test.ts
Member
Author
Conflict resolved + re-verified after merging
|
| Spec | Result | Exercises |
|---|---|---|
app-smoke |
PASS — 52.3s | desktop() default host path |
first-run-cloud-share |
PASS — 41.9s | the new chrome() seam |
Process check after both runs (ps -eo comm=): none. tsc -p evals clean, pnpm --dir evals test 95 pass / 0 fail on the merged tree.
src-opn
self-requested a review
July 31, 2026 13:45
src-opn
approved these changes
Jul 31, 2026
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
2 participants
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.
Why
resolveHost()reads one ambient env var (OPENWORK_EVAL_DAYTONA_SANDBOX), so a whole process got one host: everything local, or everything in one sandbox.desktop()called it internally andDesktopOptionshad no host field, so a spec could not express "app here, browser there", or two desktops in two sandboxes. Topology was inherited from the environment rather than declared — the same shape of problem as env-var-gated skips.What changed
desktop({ host })and a newchrome({ host })accept a placement.localHost()/daytonaSandbox(id)name one, keepingREPO_ROOTwiring inside the package.daytonaSandbox()throws from inside a sandbox, where the CLI indirection has nothing to target.resolveHost()default behaviour is unchanged, so every existing spec is untouched.Bug fixed along the way
attachSurfaceonly closes the CDP socket — it never kills the process.first-run-cloud-sharehand-rolledspawnChrome+attachSurface, sohost.disposeSurfacewas never reached and a Chrome process survived every run.chrome()owns disposal: stop the client, then dispose the surface.Proof (Daytona sandbox, real den)
first-run-cloud-sharechrome()seamapp-smokedesktop()default path, unchangedProcess check before and after all runs, using
ps -eo comm=(self-match-proof, unlikepgrep -f):tsc -p evalsclean;pnpm --dir evals test95 pass / 0 fail.Limits, stated
OPENWORK_EVAL_DAYTONA_SANDBOXmode). Not done here.Host.kindis still a bare string;requires([...])and honest skip reasons are the follow-up this unblocks.fraimz.html: the spec lane writesevals/results/rolls/<stamp>-<name>/index.html. Frames for these runs are validated there. Worth reconciling with AGENTS.md separately.Unrelated finding
org-connection-lifecyclefails on this sandbox: DOM waits pass (NEEDS YOUR SIGN-IN,OAuth requiredpresent inbody.innerText) but the vision check reports the Extensions panel blank. Not touched by this PR and not in #3359's green list either, so it may never have passed. Needs its own investigation — either a reveal/scroll issue before the frame, or a real rendering defect.