feat(flow): boot a Chromium instance per launch step - #585
Conversation
latekvo
left a comment
There was a problem hiding this comment.
[Diplomat]: Ran a full end-to-end sweep of this branch across the review lenses: the flow runner driven through the real flow-execute tool and through argent flow run against a branch-built tool-server and real Electron apps (one taking a single-instance lock, one logging its quit handlers), the process-lifecycle paths exercised with real detached process groups, and the new branches mutation-checked. Findings are inline, each naming the reproduction it came from and bisected against e7aacdc0^ where the question was whether this change introduced it.
For context on what came back clean: the nested-launch boot / device-rebind / reverse-teardown path, the same-app mid-flow relaunch against a real single-instance lock, the fragment-leading-run: hoist, teardown after a failing run and after cancellation, two concurrent runs (no cross-run kill, no device theft, no shared-state collision), the registry dispose cascade, list-devices and the tracked-ports file during and after a run, and leadingLaunch's depth/cycle parity with execRunStep all behaved as described. tsc --build and prettier --check are clean, and the full tool-server suite is at 3080 passed with only the 6 pre-existing Linux boot-device-hotboot GPU failures.
Two things I chased and dropped for lack of a reproduction: back-to-back runs racing the single-instance lock at the run boundary (passed repeatedly with real Electron), and the same-app relaunch losing that race via killChromiumByPortAndWait resolving early — the mechanism is real with a synthetic process tree, but with real Electron the lock owner is already gone when the wait returns, so only the comment inconsistency is reported.
| } | ||
| // Recorded before the next await so a cancelled run still reclaims it. | ||
| state.owned.push(booted); | ||
| state.device = resolveDevice(booted.deviceId); |
There was a problem hiding this comment.
[Diplomat]: Moving the run onto a second app makes one run capture snapshots of two different apps, and the baseline key has no app or instance component — it is ${name}__${platform}-${w}x${h} (flow-visual.ts:212). Two apps whose windows happen to be the same size therefore share a baseline file. Before this change a second chromium launch was rejected outright ("chromium launches only the top-level flow's app, once per run"), so a chromium run could only ever capture one app and the key was sufficient.
Reproduced against the branch-built tool-server with two real Electron apps, both 600x400, and a flow launch app-plain / snapshot: shot / launch app-b / snapshot: shot:
--- updateBaselines ---
1 snapshot pass | key=shot__chromium-600x372 | baseline written (shot__chromium-600x372.png)
2 launch pass | booted chromium instance chromium-cdp-32931
3 snapshot pass | key=shot__chromium-600x372 | baseline updated (shot__chromium-600x372.png)
$ ls __baselines__/snapmix/ -> shot__chromium-600x372.png (one file, 2779 bytes = app-b)
app-plain's 3403-byte capture is gone
--- plain compare, same flow ---
1 snapshot fail | key=shot__chromium-600x372 | diff 93.64% > 0.5% (shot__chromium-600x372.png)
2 launch skip | 3 snapshot skip
So seeding baselines silently discards the first app's image — reported only as the benign "baseline updated" — and the next run then fails the earlier step against the other app's picture. Giving the two apps different window sizes splits the keys and both pass, which means window size is standing in for app identity. The nested form (run: into a flow that launches the second app) behaves identically, since the baseline directory is keyed on state.topFlowName.
There was a problem hiding this comment.
Fixed in ccfcb50. Kept the key app-free — it names a committed, machine-portable baseline file, and namespacing would invalidate every existing chromium baseline — and made the collision loud instead: flow-run threads the capture's app identity (the owned instance's canonical app path, else attached:) plus a run-scoped key→identity map into runSnapshot, which fails the colliding step before any compare or baseline write, naming the prior app and the shared file and telling the author to give per-app snapshots distinct names. Your repro shape is pinned (updateBaselines proves the first app's baseline bytes survive; compare mode never invokes the differ), and same-app relaunch / same-name-different-cropOn stay legal.
| } catch (err) { | ||
| // The one boot failure the underlying error can't explain. | ||
| const foreign = | ||
| retiring === -1 && !ownedInstance(state) |
There was a problem hiding this comment.
[Diplomat]: This gate keys off whether the run owns state.device, but the single-instance lock is held by whatever instance of the app being launched is alive. Once a previous launch has moved the run onto an instance it owns, an un-owned instance of the app now being launched can no longer be named — so the diagnostic goes missing on the harder-to-diagnose case, not the easier one.
Run against the branch-built tool-server with a real Electron app that calls app.requestSingleInstanceLock(), pinned via device to a foreign instance of that app:
- control,
launch app-lock(attaches) thenlaunch app-lock— hint present:
could not boot the chromium app: Electron boot: child process exited with code 0 before CDP was ready. … An instance this run does not own is running on chromium-cdp-38849; if it is this same app, its single-instance lock would refuse a second copy. - the same flow with one extra
launch app-bin between, soownedInstance(state)is truthy — same root cause (the foreignapp-lockinstance still holds the lock), hint gone:
could not boot the chromium app: Electron boot: child process exited with code 0 before CDP was ready. Inspect [chromium-cdp-37855] stderr above for the cause.
The bare error says "exited with code 0" and points at tool-server stderr, which never reaches the tool result — so for a CI or agent caller the hint is the only in-band signal, and the comment above calls it "the one boot failure the underlying error can't explain". Neither direction is pinned: replacing the condition with true leaves test/boot-electron-kill.test.ts + test/flows/flow-chromium-boot.test.ts at 31/31 (baseline 31/31), so the mirror case — asserting an instance is un-owned when the run owns it — is invisible too.
There was a problem hiding this comment.
Fixed in 4560a5b: the hint no longer keys off ownership of state.device. A run can attach to at most one un-owned instance — the device it starts on; everything it moves onto later is runner-booted, and the runner never kills un-owned instances — so ExecState now tracks that attach and the hint names it for every later boot failure, including after the run moved onto an owned instance. Both directions are pinned: your extra-launch repro asserts the hint names the pinned id, and a hoisted-boot run asserts no hint.
| * tool-server (the flow file lives in a shipped temp dir there). | ||
| * The absolute app path a chromium launch names — relative resolves against the | ||
| * flow file's directory (the anchor `run:` and baselines use), absolute passes | ||
| * through. Normalized either way, so two spellings of one app compare equal. |
There was a problem hiding this comment.
[Diplomat]: path.resolve folds only what is lexical (relative vs absolute, .., trailing slash); it resolves no symlinks and folds no case, and the retire lookup at line 412 compares the result as a raw string. So a second spelling of one app is not recognised as the same app, the retire never runs, and the replacement boots alongside the original.
Reproduced with the real launcher — app-lock-alias is a plain symlink to the app-lock directory:
- launch: { chromium: ../../app-lock } -> 0 launch pass
- launch: { chromium: ../../app-lock-alias } -> 1 launch error
could not boot the chromium app: Electron boot: child process exited with code 0
before CDP was ready. Inspect [chromium-cdp-39209] stderr above for the cause.
Nothing else folds it (grep -rn realpath over the flows tools and boot-electron.ts finds none; bootElectronApp returns the same lexical path.resolve value that went in, so both sides of the compare at line 412 carry it), and because retiring === -1 while ownedInstance(state) is truthy, the hint at line 426 is suppressed as well — the reason gives no clue at all. macOS's default case-insensitive APFS makes a case-differing spelling behave the same way, on the platform this file's dock/launchd comments target. Restoring the pre-change path.isAbsolute(specPath) ? specPath : path.resolve(...) also leaves both suites at 31/31, since the tests use only ./app and /abs/app, both already normal.
There was a problem hiding this comment.
Fixed in fe7eb9f: resolveAppPath now canonicalizes through fs.realpath — symlinks fold, and on macOS the on-disk casing comes back, so the case-differing spelling folds too (verified empirically) — with a lexical fallback for paths not on disk, so the boot still reports a missing app itself. A real-symlink test pins that the alias retires the owned instance (killChromiumByPortAndWait ordered before the second boot) and that bootElectronApp receives the canonical path both times.
|
|
||
| /** | ||
| * Resolve the device a flow runs against. For a Chromium e2e flow with no | ||
| * Resolve the device a flow *starts* on. For a Chromium e2e flow with no |
There was a problem hiding this comment.
[Diplomat]: This docstring and the call-site comment at lines 547-548 ("a Chromium e2e flow boots + owns its own app") both still gate the boot on the flow being an e2e flow — the condition isE2eFlow enforced here before this change. A fragment now boots too, which is the headline behaviour: leadingLaunch follows a leading run:.
Verified against the branch-built tool-server with a fragment that has no launch of its own and no pinned device (- run: e2e-plain, then an assert): it booted chromium-cdp-36405 before step 1 and passed all three steps. A reader of either comment, without the diff, would conclude that flow attaches to an existing instance.
There was a problem hiding this comment.
Fixed in 56147ef — resolveRunDevice's docstring and the call-site comment both now say the boot hoists for any run whose leading launch is chromium, reached directly or through a leading run: chain.
| starts there — following a leading \`run:\`, so a fragment that composes a chromium e2e flow boots too; | ||
| otherwise the first launch attaches to an already-running instance and never kills it. Every later | ||
| launch — a nested e2e flow's own, or a mid-flow relaunch — boots a fresh instance the run moves onto, | ||
| replacing the one it already owns for that app. Instances the runner booted are torn down when the run |
There was a problem hiding this comment.
[Diplomat]: "Instances the runner booted are torn down when the run ends" is this description's only statement of when an instance dies, and it does not hold for the case the sentence before it introduces. bootChromiumForLaunch tears the same-app instance down before booting its replacement, awaiting the exit (lines 412-418, teardownBootedChromium(…, { awaitExit: true })), and splices it out of state.owned so run-end teardown never sees it.
Observed on the live tool-server for launch app-lock / assert / launch app-lock: the first instance's RUNNING → TERMINATING and its electron exited with signal SIGTERM both land mid-run, before the replacement's DevTools listening, and only the replacement is killed at run end. The ordering is pinned by the test at test/flows/flow-chromium-boot.test.ts:305-313.
Two related gaps in the same paragraph now that a run can change device: line 521's Returns a structured report ({ ok, passed, failed, skipped, errored, steps }) omits device (along with flow, executionPrerequisite, aborted), and nothing here says device is where the run started rather than where it ended — summarize is still called with the pre-move value (line 552 → 606), so result.device can name an instance the runner itself killed. That semantic exists only in inline comments at lines 550-551 and 605, which an agent reading this description never sees. It surfaces in the CLI too: live mode prints PASS on chromium-cdp-12345 after having already printed ✓ 3 launch — booted chromium instance chromium-cdp-12346, and the justification for carrying the device on the summary (argent-cli/src/flow.ts:216-218) assumes one device per run.
There was a problem hiding this comment.
Fixed in 27e9958 — the description now separates the mid-run same-app retire (killed with its exit awaited, so the replacement can't lose the lock race) from run-end teardown of instances still owned, lists the report's full shape ({ flow, device, executionPrerequisite, ok, aborted?, passed, failed, skipped, errored, steps }), and states that device is where the run STARTED, with each move onto a runner-booted instance named in that launch step's reason (which, after 0cd132b, includes the hoisted boot).
| * percentage, baseline written/updated). | ||
| * the `when:` guard marker (`condition met (…)`), snapshot passes (diff | ||
| * percentage, baseline written/updated), and a chromium `launch` that booted | ||
| * an instance (naming the device the run moved onto). |
There was a problem hiding this comment.
[Diplomat]: A chromium launch that boots is not distinguishable from one that does not. The run's leading launch also causes a boot — hoisted into resolveRunDevice — and reports a bare pass with no reason, because runChromiumLaunch takes the ownedInstance(state) branch and returns { ok: true }.
From a real run of a parent flow that launches one app and run:s a nested flow launching another, both instances runner-booted:
0 launch pass (no reason) <- booted chromium-cdp-43445, hoisted
3 launch pass reason: booted chromium instance chromium-cdp-37783
So a consumer cannot use the presence of a reason to tell "the runner booted an instance it owns and will kill" from "attached to an instance you started" — which is the distinction this sentence offers, and the one that matters for deciding whether the app under test survives the run.
There was a problem hiding this comment.
Fixed in 0cd132b — the leading launch that settles a hoisted boot now reports the same booted chromium instance <id> reason as later boots (the settle branch is exactly the hoisted-boot case), so a reason's presence reliably distinguishes an instance the runner booted and will kill from one it attached to. The attach path is pinned to report no reason, and the StepReport.reason doc now states the rule.
| } catch { | ||
| /* already gone */ | ||
| } | ||
| if (child.pid !== undefined) signalGroup(child.pid, "SIGTERM"); |
There was a problem hiding this comment.
[Diplomat]: Sending SIGTERM to the group defeats the clean shutdown the comment directly above still promises — "SIGTERM lets Electron flush the renderer's GPU buffers and write a clean exit code" — because the real electron/dist/electron binary and every Chromium helper are members of that group, so they are signalled directly instead of being shut down by the browser process.
A/B against a real Electron app that logs before-quit / will-quit / quit, replicating killChildEscalating exactly for both revisions (including the 2s escalation) and the 5s exit wait, 3 runs each:
BASE (e7aacdc0^, leader handle only)
waited 25ms handlers [ready, before-quit, will-quit, quit] gpuCrashLines 0
waited 28ms handlers [ready, before-quit, will-quit, quit] gpuCrashLines 0
waited 23ms handlers [ready, before-quit, will-quit, quit] gpuCrashLines 0
HEAD (e7aacdc0, + group SIGTERM)
waited 2005ms handlers [ready, before-quit, will-quit, quit] gpuCrashLines 6
waited 12ms handlers [ready] killedBySignal true <- quit handlers never ran
waited 14ms handlers [ready] killedBySignal true <- quit handlers never ran
3/3 base runs exit cleanly in ~25ms; 3/3 head runs diverge — two terminate the app by signal so before-quit/will-quit/quit never run, and the third survives only because the group signal killed its zygote and GPU helpers out from under it, producing six The GPU process has crashed N time(s) lines and pushing teardown onto the 2s SIGKILL escalation.
flow-run.ts:745-746 does this to every runner-booted instance — at each same-app relaunch and at run end — so an app that persists anything on quit (window bounds, session, will-quit cleanup of temp files or child processes) silently loses it. The GPU-crash lines also reach the tool-server's own stderr through the forwarder at lines 403-405, which is never detached, so a normal teardown now reads like a crash.
There was a problem hiding this comment.
Fixed in f1dea11: the synchronous group SIGTERM is gone — the handle SIGTERM alone carries the graceful request (as your A/B shows, the wrapper forwards it and the app quits cleanly, helpers following the browser down). A group SIGTERM is now sent only when the handle is already dead and child.kill reaches nothing, and the 2s group-SIGKILL sweep stays for SIGTERM-trapping / quit-blocking apps. Both directions are mutation-pinned in boot-electron-kill.test.ts.
|
|
||
| beforeEach(() => { | ||
| spawnMock.mockReset(); | ||
| // Teardown sweeps the child's process group. The fake children carry a |
There was a problem hiding this comment.
[Diplomat]: This mock is needed because the group signal added in this change reaches a real pid, and the sibling test/boot-electron-spawn-error.test.ts reaches the same code with no such mock: it has zero process.kill mocks and its fake children carry pid 12345 (line 43), while its boot-failure tests run killChildEscalating via boot-electron.ts:465.
Proven by substituting that file's fake pid with a real, unrelated setsid sleep 600 (its own group leader) and running the file otherwise unmodified:
VICTIM pid=1319780 pgid=1319780
$ npx vitest run test/boot-electron-spawn-error.test.ts --root .
Tests 8 passed (8)
>>> VICTIM KILLED
With e7aacdc0^'s boot-electron.ts swapped in and nothing else changed, the same run leaves the victim alive. Unlike the delayed escalation — which never fires, because vitest tears the per-file worker down before the 2s timer — this SIGTERM is issued synchronously, so it lands under a plain vitest run and in the whole-package run. On a CI runner or a fresh container, where pids start low, pgid 12345 is squarely in the handed-out range.
There was a problem hiding this comment.
Fixed in 9acdf02 — process.kill is mocked at module scope in boot-electron-spawn-error.test.ts for the worker's whole life, deliberately never restored: the unref'd 2s escalation timer can fire after the test that armed it, so a beforeEach/afterEach spy pair would put the real process.kill back in time for the delayed group signal.
| **Two flow types** | ||
|
|
||
| - **e2e** — begins with a `launch:` step, which starts that app from scratch (terminate + relaunch), so the flow controls its own start state. No `executionPrerequisite`. May `run:` other flows, and (on iOS/Android) may itself be a `run:` target — when nested, its `launch` runs inline, restarting the app for that sub-scenario. **Chromium is the exception:** the runner boots one Electron app per run (the top-level flow's), so a nested chromium e2e flow's `launch` can't boot its own instance and fails the run — keep chromium e2e flows top-level. Record one by adding a `restart-app` of the app under test as the **first** step — it is captured as the `launch` step. | ||
| - **e2e** — begins with a `launch:` step, which starts that app from scratch (terminate + relaunch), so the flow controls its own start state. No `executionPrerequisite`. May `run:` other flows, and may itself be a `run:` target — when nested, its `launch` runs inline, restarting the app for that sub-scenario. **On Chromium a launch is a process, not a relaunch:** the "device" is the booted app (its id is the CDP port). The runner needs a device before step 1, so it boots for the launch the run _begins_ with, following a leading `run:` — a fragment whose first step composes a chromium e2e flow boots that flow's app (pass `--platform chromium` when the launch names several platforms, or the target is ambiguous and auto-detection is used instead). That first launch then just settles the instance it was booted for; every _later_ launch — a nested e2e flow's own, or a mid-flow `launch:` of the same app — boots its own instance and the run moves onto it for the remaining steps, replacing the one the runner already owns for that app. Every instance the runner boots is torn down at run end; one you pinned with `--device` is attached to, never killed — so relaunching _that_ app mid-flow fails if it holds a single-instance lock. A launch that names no id for the run's platform is an error — a `chromium:` entry does not make a flow runnable on iOS, and the run never switches platforms mid-flight. Record one by adding a `restart-app` of the app under test as the **first** step — it is captured as the `launch` step. |
There was a problem hiding this comment.
[Diplomat]: The bullet now spends five sentences on Chromium and still closes with "Record one by adding a restart-app of the app under test as the first step — it is captured as the launch step", which cannot be done on Chromium: restart-app declares no chromium support (packages/tool-server/src/tools/restart-app/index.ts:41-45, with its own // No chromium branch at :83).
Measured against the running tool-server:
$ POST /tools/restart-app {"udid":"chromium-cdp-34619","bundleId":"com.acme.app"}
{"error":"Tool 'restart-app' is not supported on chromium app (no chromium support declared)."}
Since recording only captures steps whose tool call succeeded, there is no route to a recorded chromium launch step at all — every recorded chromium flow is a fragment. The same unqualified recording instruction appears at lines 153 and 164, and in flow-add-step.ts:171 / flow-start-recording.ts:29.
There was a problem hiding this comment.
Fixed in 625be54 — the skill bullet, the recording section, and both tool descriptions (flow-add-step, flow-start-recording) now say restart-app has no chromium support and only successful calls are recorded, so a recorded chromium flow is always a fragment: the launch: { chromium: } line is added to the YAML by hand afterward.
| // not fire — it attaches to the pinned instance and passes. Only a *second* | ||
| // launch is rejected. Uses a pinned device: a fragment top-level means the | ||
| // A's launch is the run's FIRST, so it attaches to the pinned instance | ||
| // rather than booting. Uses a pinned device: a fragment top-level means the |
There was a problem hiding this comment.
[Diplomat]: This comment survived the rewrite of the test around it, but its reason no longer holds: a fragment at top level does boot when its leading run: reaches a chromium e2e flow — which is what the new test at line 397 asserts. Nothing boots in this test because a device is pinned, not because the flow is a fragment.
Verified against the branch-built tool-server: the same shape without device (- run: e2e-plain, then an assert) booted chromium-cdp-36405 and passed.
There was a problem hiding this comment.
Fixed in d494902 — the comment (header and assertion-side) now credits the pinned device; without one, this same shape hoist-boots A's app before step 1, as the test at line ~397 asserts.
hubgan
left a comment
There was a problem hiding this comment.
E2E review of the chromium launch rework
Six agents drove this branch against real Electron instances on a dedicated tool-server (macOS, ts-node from source at 625be54). Only findings reproduced first-hand on a running app are reported; code-reading arguments, mocks and unit-test-only observations were discarded, and every claim below was independently re-run by the coordinator before posting.
1 HIGH, 3 MEDIUM, 2 LOW. Two of these (the teardown lock race and the dead-instance false PASS) are pre-existing races this PR does not introduce — flagged because the PR either commits a comment that contradicts them or widens their exposure. The prerequisite bypass and the snapshot false-positive are introduced here.
Verified-and-held, so nobody re-does the work: leadingLaunch's depth/cycle guards accept exactly the chains execRunStep accepts (traced to the MAX_RUN_DEPTH boundary from both sides — no boot-then-refuse window); resolveAppPath folds all 7 path spellings tried, including macOS case and symlinked parents; 45+ mid-flight cancellations stranded zero processes; teardown order is reverse and a wedged instance delays only itself; group signalling reaches reparented (ppid=1) survivors and never targets the tool-server (every Electron child had pgid == pid); un-owned instances were never killed in any scenario. Full unit suite green (53 files / 756 tests).
Note: this review was produced by an automated agent swarm; each comment carries a runnable reproduction so you can re-check it yourself.
| if (resolved.booted) await teardownBootedChromium(registry, resolved.booted); | ||
| // Reverse order: a nested flow's instance goes before the parent's. | ||
| for (let i = state.owned.length - 1; i >= 0; i--) { | ||
| await teardownBootedChromium(registry, state.owned[i]!); |
There was a problem hiding this comment.
E2E review: every finding below was reproduced by driving a real Electron app booted by a tool-server running this branch's source (macOS, ts-node). Nothing here comes from a mock, a stub, or a unit test. Findings that could not be reproduced first-hand were discarded.
HIGH — run-end teardown does not await exit, so two consecutive runs of the same app race its single-instance lock.
The docstring at :774-776 says awaitExit is what "a same-app relaunch needs and run-end teardown does not". Run-end teardown does need it.
Repro — an ordinary Electron app: the stock app.requestSingleInstanceLock() recipe plus an async before-quit that flushes for 1500ms (no signal trapping). Flow is launch: { chromium: <app> } + one assert. Run it back to back:
run #1 ok=True chromium-cdp-58683
run #2 TOOL-REJECT: Electron boot: child process exited with code 0 before CDP was ready
run #3 ok=True chromium-cdp-58720
run #4 TOOL-REJECT: Electron boot: child process exited with code 0 before CDP was ready
Server log — the replacement is denied the lock 765ms before the predecessor finishes exiting:
09:24:28.792 [chromium-cdp-58704] [gq] LOCK DENIED - second instance, quitting
09:24:29.557 [chromium-cdp-58683] [gq] flushed, exiting
Gap sensitivity: gap=0s fails, gap>=1s passes. Note run #2 returns a raw tool rejection with no step report at all. A racier variant instead reports the launch step as pass and then fails the assert, blaming the app.
Control: the in-run relaunch of the same app — the caller you do guard — passes every time.
Fix is one argument: teardownBootedChromium(registry, state.owned[i]!, { awaitExit: true }).
Caveat, stated plainly: the race predates this PR (main's run-end teardown was equally non-blocking). What is new is that this PR introduces the remedy, applies it to the rarer caller, and commits a comment asserting the common one doesn't need it. Graded HIGH on harm — a whole run lost and misattributed, on the core authoring/CI loop — not on novelty.
There was a problem hiding this comment.
Fixed in 6254412: run-end teardown now awaits each owned instance's exit (bounded — killChromiumByPortAndWait's 5s cap), so a back-to-back run of the same app can't race the dying instance's lock. With both callers awaiting, the awaitExit option is gone — teardownBootedChromium always waits, and the docstring you quoted now states the real reason. The run-end teardown tests moved to the awaited kill path and additionally pin that the fire-and-forget kill is never used.
| * there and the baseline-collision guard stays chromium-scoped in effect. | ||
| */ | ||
| function snapshotAppIdentity(state: ExecState): string { | ||
| return ownedInstance(state)?.appPath ?? `attached:${state.device.id}`; |
There was a problem hiding this comment.
E2E review: every finding below was reproduced by driving a real Electron app booted by a tool-server running this branch's source (macOS, ts-node). Nothing here comes from a mock, a stub, or a unit test. Findings that could not be reproduced first-hand were discarded.
MEDIUM — the new cross-app snapshot guard falsely fires on the same app.
One app gets two different identity spellings depending on how the run reached it: attached:chromium-cdp-<port> before the first runner boot, the canonical app path after. A mid-run launch that boots therefore changes the identity without changing the app, and the guard in flow-visual.ts:225 refuses the next same-name snapshot.
Repro — start one instance of app-a by hand, then pin to it. Flow:
steps:
- launch: { chromium: <app-a> }
- assert: { visible: "Argent E2E APP-A" }
- snapshot: { name: home }
- launch: { chromium: <app-a> } # same app
- assert: { visible: "Argent E2E APP-A" }
- snapshot: { name: home }POST /tools/flow-execute {"device":"chromium-cdp-21456","updateBaselines":true} →
2 snapshot pass baseline written (home__chromium-2048x1480.png)
3 launch pass booted chromium instance chromium-cdp-54276
5 snapshot fail snapshot "home" was already captured in this run from a different app
(attached:chromium-cdp-21456) - ... Give per-app snapshots distinct names
Both asserts passed on the same window text, so both captures are provably app-a. Reproduced 2/2; the identical flow with no device (both instances owned) passes cleanly.
The run hard-stops and later steps are skipped. The advice in the message is unactionable — there is only one app. Trigger is this PR's headline feature (mid-flow relaunch) crossed with a --device-pinned run, which the skill docs recommend for CI.
There was a problem hiding this comment.
Fixed in cd4941d: an attaching launch records the app path it declares — canonicalized by the same resolveAppPath the boot uses, so the two spellings are byte-equal — as the capture identity, and folds keys already recorded under the anonymous attached: into it (attaching restarts nothing, so those captures are provably the same app). Your repro shape passes both captures; attach app-a → boot app-b still fires naming the canonical a-path, and a launch-free pinned run keeps the anonymous identity.
| // same — a reason's presence is how a consumer tells an instance the run | ||
| // owns (and will kill) from one it merely attached to. | ||
| if (!(await sleepOrAbort(POST_LAUNCH_SETTLE_MS, signal))) return ABORTED_OUTCOME; | ||
| return { ok: true, reason: `booted chromium instance ${device.id}` }; |
There was a problem hiding this comment.
E2E review: every finding below was reproduced by driving a real Electron app booted by a tool-server running this branch's source (macOS, ts-node). Nothing here comes from a mock, a stub, or a unit test. Findings that could not be reproduced first-hand were discarded.
MEDIUM — a hoisted boot can report launch as PASS, naming an instance that is already dead.
When the app holds requestSingleInstanceLock() and a copy is already running, the second process opens its --remote-debugging-port listener during startup and then quits on the lock. waitForCdpReady polls every 250ms and sometimes wins the race against the child's exit event, so bootElectronApp returns success for a process that is gone.
Repro — boot one instance of a lock-holding app, then run a chromium e2e flow against it in a loop. 1 of 14 iterations (agent saw ~3 in 30):
5: *** FALSE PASS *** launch reason='booted chromium instance chromium-cdp-51577'
next step: could not read the UI tree: ... GET http://127.0.0.1:51577/json/version
could not connect. Is the app running with --remote-debugging-port?
The other 13 failed cleanly with child process exited with code 0 before CDP was ready.
Caveat: the race itself is in boot-electron.ts's Promise.race([waitForCdpReady, earlyExit, spawnError]) and is untouched by this PR — main hoist-booted top-level chromium e2e flows through the same path. Raised here because this line's new reason positively asserts a boot that did not happen, and because the PR widens exposure (fragments with a leading run: now hoist too). MEDIUM, not HIGH: the run still fails overall, so this is a misleading diagnosis rather than a false green.
There was a problem hiding this comment.
Fixed in 781d8ae, in boot-electron where the race lives: a readiness-probe win no longer settles the boot. Success is held for a 300ms confirmation window with the boot listeners still attached, so an exit that already landed — or lands within the window — rejects with the same early-exit error and cleanup path, and a lock-quitting second copy can't be reported as booted. Pinned both ways: exit-after-CDP-ready rejects (port never tracked, no handle retained); a clean window succeeds paying exactly the window.
| // Seeded with the top flow, exactly like `execRunStep`'s runStack — the two | ||
| // must accept the same chains, or a boot could precede a run the executor | ||
| // then refuses for depth. | ||
| const leading = await leadingLaunch(flow, flowDir, [params.name]); |
There was a problem hiding this comment.
E2E review: every finding below was reproduced by driving a real Electron app booted by a tool-server running this branch's source (macOS, ts-node). Nothing here comes from a mock, a stub, or a unit test. Findings that could not be reproduced first-hand were discarded.
MEDIUM — executionPrerequisite is now trivially bypassable by wrapping the launch in a run:, and the boot destroys the state the prerequisite demands.
Dropping the isE2eFlow gate means a fragment whose leading run: reaches a chromium e2e flow boots — but the parse-time rule that a launching flow may not declare a prerequisite still only inspects the flow's own first step.
Repro — two semantically identical flows:
# direct-prereq.yaml
executionPrerequisite: "the counter must already read 'taps: 1'"
steps:
- launch: { chromium: <app-a> }
- assert: { visible: "taps: 1" }# indirect-prereq.yaml (e2e-a.yaml is just the launch)
executionPrerequisite: "the counter must already read 'taps: 1' - tap once before running"
steps:
- run: e2e-a
- assert: { visible: "taps: 1" }Direct — rejected at parse:
A flow that starts with a launch step must not declare executionPrerequisite - it launches
its own app and controls its start state.
Indirect — accepted, boots, echoes the prerequisite back, then fails against the state it just destroyed:
device=chromium-cdp-55412 ok=False
prereq echoed back: "the counter must already read 'taps: 1' - tap once before running"
0 run d0 pass
1 launch d1 pass booted chromium instance chromium-cdp-55412
2 assert d0 fail no element matched selector text="taps: 1"
Without prerequisiteAcknowledged the LLM path still returns the notice, so a caller is told to establish start state the run then throws away; the CLI stamps a false assumes: line on the report.
This one is PR-introduced: on main, chromiumBootSpec required isE2eFlow(flow), so this fragment attached and the prerequisite held. Either the parse rule should follow the leading run: chain, or the hoist should not fire for a flow declaring a prerequisite.
There was a problem hiding this comment.
Fixed in 3a11988 with the run-time analog of the parse rule: flow-execute refuses a top flow declaring executionPrerequisite whose leading run: chain reaches a launch — same FLOW_E2E_HAS_PREREQUISITE validation classification, message naming the launch-carrying flow — checked before the notice handshake and before any hoist boot, on both the acknowledged and unacknowledged paths, regardless of device/platform params. Rejection rather than hoist-skipping on purpose: the nested launch would still boot at step 1 and destroy the state. Plain prereq fragments and broken/unreadable chains behave exactly as before.
| // The one boot failure the underlying error can't explain. | ||
| const foreign = | ||
| retiring === -1 && state.attachedDeviceId !== undefined | ||
| ? ` An instance this run does not own is running on ${state.attachedDeviceId}; if it is this same app, its single-instance lock would refuse a second copy.` |
There was a problem hiding this comment.
E2E review: every finding below was reproduced by driving a real Electron app booted by a tool-server running this branch's source (macOS, ts-node). Nothing here comes from a mock, a stub, or a unit test. Findings that could not be reproduced first-hand were discarded.
LOW — the single-instance-lock hint is unreliable in both directions. Two proven halves, one block.
(a) Absent where it would help. The gate needs attachedDeviceId, which is only set when the run attached at start (:611-613). A run whose leading launch was hoist-booted never has it — so on the self-contained chromium e2e shape this PR is built around, a later launch that fails because a foreign instance holds the lock gets no hint at all. With a foreign lock-holder running:
1 launch error could not boot the chromium app: Electron boot: child process exited with
code 0 before CDP was ready. Inspect [chromium-cdp-57554] stderr above.
The real cause was only in the tool-server log (app-lock: single-instance lock held by another copy). The identical failure on a --device-pinned run does get the hint.
(b) Present where it misleads. foreign is concatenated onto every bootChromiumForFlow rejection, with no check that the failure is lock-shaped and no re-probe of the named instance's liveness. Pinned to an app-a instance, launching a nonexistent path:
could not boot the chromium app: Electron boot: path does not exist:
/.../apps/nope-does-not-exist An instance this run does not own is running on
chromium-cdp-21458; if it is this same app, its single-instance lock would refuse a second copy.
No lock involved, 21458 is a different app — and there is no separator, so the path runs straight into the next sentence (nope-does-not-exist An instance...), which also makes the path itself hard to parse. A separate run showed the hint asserting an instance "is running" after it had provably exited.
LOW: impact is confined to error text. Worth fixing because the trigger — relaunching an app you already have open — is routine, and this string is what an agent reads to decide what to do next.
There was a problem hiding this comment.
Fixed in 1bf117e. (b): the hint fires only on the lock's failure shape — CHROMIUM_ELECTRON_EXITED_BEFORE_READY with exit code 0 — so a missing path, spawn failure, or crash gets the bare error, and it now joins as a separate sentence. The named instance is re-probed (GET /json/version, 800ms bound) before "is running" is claimed; a dead probe degrades to generalized wording. (a): a lock-shaped failure with no attached instance — the hoisted shape — now gets a generalized lock hint, so the diagnosis is in-band there too. Both directions mutation-pinned.
| async function runChromiumLaunch(state: ExecState, app: Launch): Promise<DirectiveOutcome> { | ||
| const { registry, device, signal } = state; | ||
|
|
||
| if (state.chromiumLaunched) return bootChromiumForLaunch(state, app); |
There was a problem hiding this comment.
E2E review: every finding below was reproduced by driving a real Electron app booted by a tool-server running this branch's source (macOS, ts-node). Nothing here comes from a mock, a stub, or a unit test. Findings that could not be reproduced first-hand were discarded.
LOW (design/observability note, not a defect) — composing an e2e flow into another e2e flow now silently reboots the app mid-run, and the report gives no signal that the run moved off the parent's instance.
# checkout.yaml
steps:
- launch: { chromium: <app-a> }
- tap: "Tap me"
- assert: { visible: "taps: 1" }
- run: login # login.yaml is itself an e2e flow (leading launch)
- assert: { visible: "taps: 0" }Observed — the parent's instance is retired mid-run and the counter resets, yet the run reports PASS:
VERDICT ok=True p/f/s/e= 8 0 0 0
1 tap d0 pass "Tap me"
2 assert d0 pass visible "taps: 1"
4 launch d1 pass | booted chromium instance chromium-cdp-53995 <- parent's instance killed
7 assert d0 pass visible "taps: 0" <- reset 1 -> 0, still PASS
I am filing this LOW rather than as a bug because it is documented in the tool description, it is the PR's deliberate intent, and it makes chromium consistent with iOS/Android (where a nested e2e flow's launch also restarts the app). The difference worth noting is that on chromium the device id changes too, and the only signal is a depth-1 booted chromium instance ... line that reads like ordinary fragment narration. On main this shape was a loud error. A distinct marker on the step where the run moves instances would let a green report be trusted.
There was a problem hiding this comment.
Fixed in 7967287: the step where the run moves now says so after the stable prefix — booted chromium instance <new> — retired <prev> (same app relaunched) when the instance the run sat on was killed first, — run moved off <prev> otherwise (an attached or older instance stays alive, so "retired" would be false there). The hoisted settle stays bare, so marker presence ⇔ the run moved — your checkout/login shape now carries the retired marker on the nested launch, and a green report can be trusted by grepping for it. Pinned for the nested, same-app, attach→boot, and cross-app shapes.
hubgan
left a comment
There was a problem hiding this comment.
Wave 2 — recording path, CLI surface, and an adversarial seam sweep
Follow-up to my earlier review. Three more agents drove this branch against real Electron instances, covering what wave 1 did not touch.
1 MEDIUM, 2 LOW below. The MEDIUM is a genuine round-trip break in the recording workflow this PR documents; the two LOWs are accuracy issues in text this PR edited.
The adversarial seam sweep found nothing — worth stating explicitly, since it was looking for exactly the cross-area bugs a per-area review misses. It confirmed: state.device is read per step so injected udids track the current instance across moves (no stale binding); snapshot baselines across a device move are byte-identical to solo-app captures (correct attribution); 8-instance runs leave no stale ports and 20 boot/retire cycles leave the tool-server's fd count unchanged (33 before / 33 after); two concurrent runs both booting and retiring the SAME app never cross-kill; 30 concurrent boots produced zero duplicate ports; 9 cancellation timings all reclaimed every instance; and fs/promises.realpath really does fold macOS casing (unlike realpathSync), so the resolveAppPath doc claim holds.
Also re-confirmed clean: exit codes across 12 scenarios (0 / 1 / 2 as appropriate), --output artifact writing, SIGINT/SIGTERM/SIGHUP/SIGKILL mid-run all reclaiming the app, and the recording round trip itself when no prerequisite is involved (record fragment, add the launch: line, boots fresh and passes 5/5).
Automated agent swarm; every comment carries a runnable reproduction.
| "For a self-contained e2e flow, omit this and record a `restart-app` as the first step instead — " + | ||
| "it is captured as the flow's `launch` step." | ||
| "it is captured as the flow's `launch` step. restart-app has no chromium support, so a chromium " + | ||
| "flow records as a fragment; add the `launch: { chromium: <app path> }` line to the YAML afterward." |
There was a problem hiding this comment.
Wave 2 of the E2E review. Reproduced by driving real Electron apps against a tool-server running this branch's source; re-run independently by the coordinator before posting.
MEDIUM — the chromium recording workaround this PR documents produces a flow that does not parse.
This new text sits on the executionPrerequisite parameter, and SKILL.md:153 separately instructs that for a fragment you "pass an executionPrerequisite describing it". Follow both and the result is rejected by validateFlow (flow-utils.ts:1917). Neither this description, the matching one on flow-add-step.ts:171, nor SKILL.md says to delete the prerequisite line.
Repro — record a chromium flow the documented way (it must be a fragment, and a fragment declares a prerequisite):
POST /tools/flow-start-recording {"name":"recchk","project_root":"<proj>",
"executionPrerequisite":"App-A window open, counter at 0"}
POST /tools/flow-add-step {"command":"gesture-tap","args":"{\"udid\":\"chromium-cdp-24601\",\"x\":0.5,\"y\":0.5}"}
POST /tools/flow-finish-recording {}produces:
steps:
- tap:
id: tap
executionPrerequisite: App-A window open, counter at 0Now do exactly what this description says to do "afterward" — add - launch: { chromium: <app path> } as the first step — and every tool that touches the file fails:
{"error":"[Tool:flow-execute] A flow that starts with a launch step must not declare
executionPrerequisite - it launches its own app and controls its start state.
Drop the leading launch to make it a fragment, or drop executionPrerequisite."}
prerequisiteAcknowledged: true does not help (parse-time rule). flow-read-prerequisite fails identically, and a sibling flow doing - run: recchk errors with could not load fragment.
Controls: the same recording without a prerequisite, given the same launch line, boots a fresh instance and passes 5/5 — so the workaround itself is sound; it is the prerequisite steering that breaks it.
This is the same rule as my comment on flow-run.ts:667, from the opposite side: there it is bypassed by a run: wrapper, here the documented path walks straight into it. Both stem from the rule inspecting only a flow's own first step.
Adjacent, pre-existing, not filed separately: once the YAML is in this state, flow-add-step still executes the action on the device before re-validating (flow-add-step.ts:198 vs :257) — a failed record advanced the app's tap counter 3 → 4 while recording nothing, though the description promises "an error is returned and nothing is recorded".
There was a problem hiding this comment.
Fixed in 78ea4cc — every spot documenting the workaround now says to delete the recorded executionPrerequisite line when adding the launch: flow-start-recording's parameter description, flow-add-step's description, and both SKILL.md spots (the e2e bullet's chromium tail and recording step 1). The adjacent execute-before-revalidate note was left as filed (pre-existing, not part of this thread).
| \`.argent/flows/<name>.yaml\` under the current working directory. A flow that | ||
| begins with a \`launch\` step runs its app from scratch; any other flow (a | ||
| fragment) runs against the device's current state — handy while authoring one. | ||
| Exception: a fragment whose first step \`run:\`s a chromium e2e flow boots that |
There was a problem hiding this comment.
Wave 2 of the E2E review. Reproduced by driving real Electron apps against a tool-server running this branch's source; re-run independently by the coordinator before posting.
LOW — this new sentence is unqualified; the boot only happens when the leading launch is unambiguously chromium.
chromiumBootSpec requires --platform chromium or a lone { chromium: ... } target (flow-run.ts:706-714). The tool description states that qualifier; this help text does not, so a fragment whose leading run: reaches a multi-platform launch silently falls through to device auto-detection instead of booting.
Repro — e2e-multi.yaml has launch: { chromium: <app-a>, ios: com.example.nope }; frag-multi.yaml is just - run: e2e-multi plus an assert.
Without --platform (what this sentence promises will boot):
{"error":"[Tool:flow-execute] 14 booted devices matched - pass --device or --platform
to disambiguate. Available devices: ... (ios, Booted), ...
With --platform chromium (control) it boots as described:
device=chromium-cdp-57626 ok=True
1 launch pass booted chromium instance chromium-cdp-57626
The loud error above only happens because this host has several devices up. On a developer box with one browser open, the same flow attaches to that browser and drives it — the hazard the tool description's qualifier exists to prevent. Multi-platform launch maps are the normal shape for a flow targeting both a mobile app and its Electron build.
Suggest carrying the qualifier across: "...whose first step run:s a chromium e2e flow whose launch is unambiguously chromium (--platform chromium, or a lone { chromium: ... } target)".
There was a problem hiding this comment.
Fixed in 39ecf38 — the help text now carries the qualifier: "boots that flow's app before step 1 — when that launch is unambiguously chromium (a lone { chromium: ... } target, or --platform chromium); a multi-platform launch auto-detects a device instead."
| lines.push(`Flow "${report.flow}" on ${report.device}`); | ||
| // A fragment runs against the device's current state — remind the operator | ||
| // what it assumes was already set up. | ||
| // Remind the operator what the flow assumes was already set up. |
There was a problem hiding this comment.
Wave 2 of the E2E review. Reproduced by driving real Electron apps against a tool-server running this branch's source; re-run independently by the coordinator before posting.
LOW — two rendering claims this PR's semantics falsified. Flagging here because this is the comment the PR revisited.
(a) The verdict line names the device the run STARTED on, even when the failing step ran elsewhere. Now that a run moves onto freshly booted instances, report.device is only the starting device (per the tool description), but :375 and renderSummary at :221 render it as the device of the whole run:
Flow "two-apps"
OK 1 launch - booted chromium instance chromium-cdp-57381
OK 2 assert visible "Argent E2E APP-A-orch"
OK 3 launch - booted chromium instance chromium-cdp-57407
X 4 assert visible "NOPE-this-will-fail" - no element matched selector
FAIL on chromium-cdp-57381 - 3 passed, 1 failed, 0 errored, 0 skipped
The failing step ran on -57407; the verdict blames -57381. CI greps keying off FAIL on <device> point at the wrong instance for any multi-launch chromium flow. Suggest started on <id>, or naming the failing step's device.
(b) assumes: still prints for a run that booted its own app from scratch. A fragment may declare executionPrerequisite and boot (isE2eFlow is false when the first step is run:), so the operator is told the run assumed a state it actually created:
Flow "frag-prereq"
OK 1 run [e2e-a]
OK 2 launch [e2e-a] - booted chromium instance chromium-cdp-63796
OK 4 assert visible "taps: 0"
assumes: the app is already open and the counter shows taps: 3
Step 4 asserting taps: 0 passes, proving the app was fresh and the prerequisite was never met — yet PASS, and the assumes: line prints unconditionally. This is the CLI-visible face of the flow-run.ts:667 prerequisite finding.
There was a problem hiding this comment.
(a) Fixed in b5d0c14: the summary now prints PASS/FAIL (started on <id>) — the report's device is the starting device, and each move is marked on its launch step's reason (7967287), so nothing claims the whole run happened there. (b) is closed by 3a11988: a fragment declaring executionPrerequisite whose leading run: chain reaches a launch is now refused outright, so assumes: can no longer print for a run that booted its own app; where a prerequisite remains legal, the line still prints.
hubgan
left a comment
There was a problem hiding this comment.
Wave 3 — final pass
Two more agents: one re-testing every factual claim in the rewritten prose against live behaviour, one fresh-eyes sweep with no assigned area.
The documentation agent found nothing — ~30 live flows against four purpose-built Electron apps (a lock-holder, a SIGTERM-trapper, one rendering its own argv), and every claim in SKILL.md, the flow-execute description and boot-electron.ts's comments held. Notably confirmed: the same-app retire holds through trailing-slash, /./ and symlink aliases (and each variant failed when a foreign lock holder was alive, so the test had teeth); a --device-pinned instance survived six consecutive runs that each booted and killed their own; and the reparenting claim is exact — after kill -KILL on the leader the survivor's ppid became 1 while its pgid stayed 94586, so the group signal still reached it.
The fresh sweep turned up one LOW finding, below. It also drove hostile launch values (unicode/emoji/space paths, FIFOs, broken symlinks, non-executables, an 800-char path, ../.. escapes, 2000 args, a split --remote-debugging-port 9222 flag, an arg containing a newline) as both the hoisted and the mid-flow launch: all produced clean tool or step errors, with no crash, hang, leak or unhandled rejection. Multi-window apps, 4-level nesting with a launch at each level, when: blocks containing a launch, and cropOn across a same-app relaunch were all clean.
That closes my review: 12 findings over three waves — 1 HIGH, 4 MEDIUM, 7 LOW. Two of them are pre-existing races this PR does not introduce, flagged because the PR either contradicts them in a comment or widens their exposure.
Automated agent swarm; every comment carries a runnable reproduction.
| if (state.chromiumLaunched) return bootChromiumForLaunch(state, app); | ||
| state.chromiumLaunched = true; | ||
|
|
||
| if (ownedInstance(state)) { |
There was a problem hiding this comment.
Wave 3 of the E2E review. Reproduced 3/3 against real Electron apps and re-run independently by the coordinator.
LOW — the first launch settles onto the hoisted instance without checking it is the app the step names, so a flow file rewritten mid-run reports a green PASS having driven the wrong app.
The hoist at :667 reads the nested flow off disk to decide what to boot; execRunStep re-reads that same file when the run: step executes. Every part of the second read is honoured except the launch — this branch sees an owned instance for state.device.id and settles, never comparing the step's app path against BootedChromium.appPath. The run half-applies the newer file: it executes the new steps but reports a boot that never happened.
Repro — frag-lead.yaml is just - run: e2e-target; e2e-target.yaml starts as a launch of app-a and is rewritten to launch app-b (plus an extra tool: describe step) 0.5s into the run:
( sleep 0.5; cat > "$FLOWS/e2e-target.yaml" <<EOF
steps:
- launch: { chromium: <app-b> }
- assert: { visible: "Tap me" }
- tool: describe
EOF
) &
curl -sS -m 240 -X POST http://127.0.0.1:3457/tools/flow-execute \
-H 'Content-Type: application/json' \
-d '{"name":"frag-lead","project_root":"<proj>"}'Observed, 3 of 3 attempts:
ok=True device=chromium-cdp-60864 steps=4
0 run pass
1 launch pass booted chromium instance chromium-cdp-60864
2 assert pass
3 tool pass describe sees: {'APP-A-orch'}
Four steps — so the new file was executed — yet describe on that instance returns app-a's UI. The step reads launch: { chromium: <app-b> } and reports booted chromium instance ..., but app-b was never started.
Control (same post-edit file, no concurrent write) boots app-b and describe returns APP-B, confirming the flow as written is correct.
Hit rate: 3/3 at a 0.5s edit delay, 0/1 at 3.0s. The window is the hoisted boot's duration (~2.5-3s for these apps, bounded above by bootElectronApp's 30s CDP-ready timeout).
Expected — either boot the app the step now names, or fail the launch loudly. Not a green run that drove a different app than the flow declares.
Severity LOW: the trigger is obscure (a flow file rewritten inside a ~3s window, and only on the leading-run:-chain hoist path — the top-level flow is parsed once). Raised because the failure class is the bad one: a silent PASS against the wrong app. A cheap guard would be to compare the settling launch's resolved appPath against the owned instance's and fail on mismatch, which also makes the owned-branch assumption explicit.
There was a problem hiding this comment.
Fixed in f9889cf with the cheap guard: the settling launch resolves the app the step names and compares it against the owned instance's canonical path — a mismatch fails the step naming both paths and the cause (the flow file changed after the run started), and a rewrite to a launch with no chromium entry fails with the standard no-app reason. Your mid-run rewrite mechanics are pinned by a test that rewrites the sibling file between the hoist's read and execRunStep's re-read.
…t the SIGKILL escalation A synchronous group SIGTERM signalled the browser binary and every Chromium helper directly, bypassing Electron's quit sequence (before-quit/will-quit/quit never ran) and making normal teardown read like a GPU crash on stderr. The handle SIGTERM already quits the app cleanly — the npm electron wrapper forwards the signal and exits with its child — so the group SIGTERM now goes out only when the handle is already dead and reaches nothing. The 2s group SIGKILL sweep stays for apps that trap SIGTERM and helpers outliving a wedged browser; signalGroup's docstring now states that actual mechanism.
…ecycled pid The escalation is gated by a group-liveness probe, not the leader's exit status — liveChildren, killChromiumByPort and the kill-test header all still described the pre-sweep guarantee. State the split honestly: the handle's exit-status guard covers the delayed leader SIGKILL only; the group sweep carries the same probe-to-kill window the raw-pid fallback documents.
… path too killChromiumByPidFallback signalled the raw leader pid only and returned early once the leader was gone, so a wrapper that died independently of its child (external kill, OOM) left the whole app running with not even the escalation armed — and killChromiumByPortAndWait's handle-less poll watched the same leader pid, reporting "gone" while the browser still held its single-instance lock. Both now target the group, which every reaching pid leads (all producers spawn detached); the poll-branch comment names its actually reachable cause.
…s off real pids The fake children carry a stand-in pid, and every boot-failure test runs killChildEscalating, whose group sweep goes through process.kill — on a CI runner where low pids are in the handed-out range, a real process group could be signalled. Mock process.kill for the worker's whole life, never restored, since the unref'd 2s escalation timer can fire after the test that armed it.
…p retires it The retire lookup compared lexically resolved paths as raw strings, so a symlink (or, on case-insensitive APFS, case-differing) spelling of the app the run already owns was treated as a different app — the replacement booted alongside the original and died on its single-instance lock, with the hint suppressed because the run owned an instance. resolveAppPath now folds through the OS realpath, falling back to the lexical resolution for paths not on disk so the boot still reports a missing app itself.
…o an owned instance The hint gated on whether the run owns state.device, but the lock is held by whatever instance of the app being launched is alive — the un-owned instance the run started attached to, which the runner never kills. Once a launch moved the run onto an instance it owns, that standing suspect could no longer be named, and the bare boot error points at tool-server stderr, which never reaches the tool result. Track the starting attach on ExecState (a run attaches to at most one un-owned instance) and name it; both directions are now pinned.
The run's leading launch reported a bare pass when its boot was hoisted into resolveRunDevice, while every later boot named its instance — so a consumer could not use the reason's presence to tell an instance the runner booted (and will kill at run end) from one it merely attached to, the distinction the report documents. The settle branch is exactly the hoisted-boot case, so it now reports the same 'booted chromium instance <id>' reason.
…ne run A baseline key is name__platform-WxH — deliberately app-free, since it names a committed, machine-portable file — so once a launch can move the run onto a second app, two same-sized apps sharing a snapshot name silently shared one baseline: updateBaselines overwrote the first app's image reporting only 'baseline updated', and the next compare failed the earlier step against the other app's picture. flow-run now threads the capture's app identity (owned instance's canonical path, else the attached device) and a run-scoped key-to-identity map into runSnapshot, which fails the colliding step before any compare or write, telling the author to name per-app snapshots distinctly.
The run-end teardown loop had no failure-path coverage: guarding it on !state.stopped leaked both mid-flow-booted instances while every suite stayed green, since the failing-run tests asserted statuses and boot counts but never the kills. A two-instance failing run now asserts both go down, newest first.
…oist The guard keeping a leading run: from escaping the flows directory during the hoist had no test — deleting it left every suite green while `run: ../evil` made leadingLaunch read the escaped file and boot the caller-chosen app path it names, before execRunStep's own (post-boot) check could refuse. The new test plants a real ../evil.yaml and asserts nothing boots.
… executor Neither half of the hoist's guard line was pinned: widening the depth bound by 5 let a 20-hop leading chain boot an instance the executor then refused for depth, and deleting the cycle check left the cyclic test green because the depth bound alone also ends the walk — the very re-reading that test's comment says does not happen. Two chain tests now pin the bound from both sides (19 hops boots and passes, 20 does not boot), and a readFile passthrough counter pins that a cycle is abandoned on the first repeat.
resolveRunDevice's docstring and its call-site comment both still said a Chromium e2e flow boots, but a fragment whose leading run: chain reaches one boots identically — a reader of either comment would conclude such a flow attaches to an existing instance.
The description's only teardown statement — instances torn down when the run ends — did not hold for the same-app relaunch it introduces one sentence earlier: that instance is killed mid-run, its exit awaited, and run-end teardown never sees it. The report line also omitted flow/device/ executionPrerequisite/aborted and never said device names where the run STARTED, though a run that moved can end on an instance the runner killed.
bootChromiumForFlow's docstring said boot failures propagate as-is, which now describes only the resolveRunDevice hoist (the tool call rejects with no report); the second caller, bootChromiumForLaunch, catches the same failure and reports a step error inside the run — the docstring names both.
…d of booting The comment credited the flow being a top-level fragment, but a fragment whose leading run: reaches a chromium e2e flow does boot — what suppresses the boot in this test is the pinned device.
…state' claim The fragment bullet, the runner blurb, the CI section, and flow --help all still asserted a fragment runs against whatever is on screen, while a fragment whose first step run:s a chromium e2e flow now boots that flow's app before step 1 — the two places that define a fragment would have an author expecting no process to be spawned. Each spot now carries the chromium leading-run: exception and the --device escape hatch.
…lock-quitting exit
…boot keeps one snapshot identity
…robed live, and on hoisted runs too
…the app the step names
…rded executionPrerequisite
…y chromium leading launch hoists
625be54 to
39ecf38
Compare
hubgan
left a comment
There was a problem hiding this comment.
E2E review — driven against real apps, not read
Every claim below was produced by booting real Electron apps / real iOS simulators / real Android emulators through a tool-server running this branch, and watching the behaviour first-hand. Nothing here is a code-reading hypothesis; anything that could not be reproduced from a clean state at least twice was discarded.
The PR's core mechanism holds up well. Specifically verified working on real processes:
- The single-instance-lock race the PR set out to fix is genuinely fixed — 38/38 green on back-to-back
appLockrelaunches, plus 44 more relaunches through the retire path and 36 boots in another sweep, with zero races.killChromiumByPortAndWaitreally does gate the next boot (instance 1 observed dead before instance 2 appears). - The teardown leak is genuinely fixed. The process-group sweep leaves zero survivors for a normal app, a SIGTERM-trapping app, a
before-quit-blocking app, an orphaned group (leaderkill -9'd), the boot-failure path, and a packaged.appbundle. It never signalled the tool-server or an unrelated group (pgid == wrapper pidon every boot). - Device rebinding after a mid-run launch is complete —
await/assert/tap,snapshot, rawtool:dispatch, and awhen:block's own guard probe all bind the new instance (verified positively and negatively). - Instances the runner does not own are never killed — across ~25 + ~120 runs including failed runs, aborted runs, and runs that later booted their own instances.
- No leaks under cancellation: aborts swept across the hoisted boot, mid-flow boot, kill-wait, long waits, and exactly-at-teardown all left zero processes and no stale port entries. Zero unhandled rejections across thousands of log lines.
- No iOS/Android/Vega regression: relaunch semantics proven by PID on all three, no chromium-only
reasonleaks into native reports, status bar restored on passing/failing/aborted runs, and the new snapshot guard is correctly inert off chromium. - Full static gate is clean:
tsc --build, root eslint, and 3458 tests across the tool-server / CLI / argent suites.
Two reproducible defects are inline below. Both live in the new leading-run: chain logic; everything else came back clean.
Also found, not blocking (LOW)
1. A hoisted first launch gets no single-instance-lock hint. chromiumBootFailureReason() is wired only into bootChromiumForLaunch(), so the most likely way to meet a lock — your app is already open when you start a chromium e2e flow — rejects with the bare Electron boot: child process exited with code 0 before CDP was ready. Inspect [chromium-cdp-NNNNN] stderr above for the cause. The identical failure one step later is fully diagnosed. The referenced stderr is also a dead end: a lock-quit prints nothing, so that log line is empty. The verdict is correct and this is strictly an improvement over main (which had no hint anywhere), hence LOW — but the hoist is the path users will hit most.
2. The lock hint is unactionable when the lock holder is an instance the run itself owns (reachable when two app directories share one Electron name, e.g. a v1/v2 build pair). liveAttachedInstance() only considers state.attachedDeviceId, so the hint falls through to "If a copy of this app is already running, close it and rerun" — but the runner kills that holder at run end, so there is nothing to close and rerunning fails identically.
Pre-existing, not introduced here (MEDIUM)
bootElectronApp overwrites liveChildren[port] with no live-incumbent check. Two boot-device calls with the same caller-pinned electronPort make the second instance fail to bind the port, waitForCdpReady is then satisfied by the first instance's CDP, the confirm window passes (instance 2 is alive, just CDP-less), and boot reports booted:true with instance 2's pid against instance 1's port — evicting instance 1's handle, so killChromiumByPort can never reach it again. Reproduced 2/2; verified identical on origin/main, so it is out of scope for this PR. Flagging it only because this PR's revised doc comment ("Retained so teardown can kill through the handle") now leans harder on that registry.
| // the very state the prerequisite demands. Checked before the notice | ||
| // handshake (and regardless of device/platform params) so a caller is | ||
| // never asked to establish state the run would then throw away. | ||
| if (flow.executionPrerequisite) { |
There was a problem hiding this comment.
[MEDIUM] This guard rejects a pinned chromium run that provably does not restart the app — a regression against main.
The guard runs before resolveRunDevice and ignores params.device. But with an explicit device, resolveRunDevice returns booted: null, so the run's first chromium launch takes the attach branch in runChromiumLaunch — refreshViewport() only, nothing is restarted, and the prerequisite state survives intact. The flow is refused solely because it carries an executionPrerequisite.
That makes the documented escape hatch unreachable for exactly the flow class it was written for:
- PR body: "Behavior change: … pass
--deviceto keep attaching." SKILL.md, the fragment bullet — the one that says a fragment "May declare anexecutionPrerequisite" — ends "(pass--deviceto attach to a running instance instead)".
Repro (real Electron smoke app, hand-booted on a fixed CDP port so the runner does not own it):
# chrome-a.yaml
steps:
- launch: { chromium: <abs>/appA }
- await: { visible: { text: "Tap me" } }
# check-state.yaml
steps:
- assert: { visible: { text: "taps: 3" } }
# pin-noprereq.yaml
steps:
- run: chrome-a
- run: check-state
# pin-prereq.yaml — byte-identical plus one line
executionPrerequisite: "counter must already read taps: 3"
steps:
- run: chrome-a
- run: check-state<electron> <abs>/appA --remote-debugging-port=39501 & # standing, un-owned
# tap it 3 times so the app reads "taps: 3", then:
curl -X POST .../tools/flow-execute -d '{"name":"pin-noprereq","project_root":"…","device":"chromium-cdp-39501"}'
curl -X POST .../tools/flow-execute -d '{"name":"pin-prereq","project_root":"…","device":"chromium-cdp-39501","prerequisiteAcknowledged":true}'
curl -X POST .../tools/flow-execute -d '{"name":"pin-noprereq","project_root":"…","platform":"chromium"}' # control, no --deviceObserved:
(a) pin-noprereq --device -> ok=True device=chromium-cdp-39501
launch: pass (no reason = attached)
assert visible "taps: 3": PASS <- state preserved
(b) pin-prereq --device -> ERROR: A flow whose leading run: chain reaches a launch step must not
declare executionPrerequisite — it launches its own app and controls
its start state. …
(no report at all; CLI exit 1)
(c) pin-noprereq no device -> ok=False device=chromium-cdp-54498
launch: pass "booted chromium instance chromium-cdp-54498"
assert: FAIL 'no element matched selector text="taps: 3"'
The standing instance's pid was unchanged after (a) and it was still alive after all three.
(c) is the positive control: it proves (a)'s pass is not vacuous — without --device the run really does boot a fresh app and destroy the state, so the guard is right in that case and wrong in (a)'s.
Expected: (b) should run exactly like (a). The guard's own comment justifies itself with "so a caller is never asked to establish state the run would then throw away" — with --device the run demonstrably does not throw it away.
Regression: yes. origin/main has no leadingLaunch and no such guard; its execute() goes straight from the handshake to resolveRunDevice, attaches on an explicit device, and runs (b) green. The only workaround on this branch is editing the YAML to delete the prerequisite.
Suggested fix: skip the guard when params.device is set and the resolved run would attach rather than boot.
Reproduced 3/3 by one agent, 2/2 independently by a second from a clean workspace + fresh server, and once more by hand — 6/6 total.
| seen: string[] | ||
| ): Promise<{ app: Launch; flow: string } | null> { | ||
| const first = flow.steps.find((s) => s.kind !== "echo"); | ||
| if (!first) return null; |
There was a problem hiding this comment.
[MEDIUM] A run: hop through a flow with no non-echo steps aborts the whole chain scan, so both the new guard and the new boot hoist silently skip.
flow.steps.find((s) => s.kind !== "echo") returning undefined means "this flow contributes nothing executable" — but returning null here abandons the entire scan instead of resuming at the parent's next step, which is what execRunStep actually does at run time. So [echo, run: <e2e>] and [run: <echo-only frag>, run: <e2e>] are the same run, and only the first is seen.
Two user-visible consequences, both driven for real:
(a) The prerequisite guard is bypassed and the app really is restarted. Reproduced on a real Android emulator and a real iOS simulator, proven by PID:
# echo-only.yaml
steps:
- echo: this fragment does nothing
# and-e2e.yaml
steps:
- launch: { android: com.google.android.deskclock }
- wait: 1500
# guard-inline.yaml — CONTROL (echo inline, not behind a hop)
executionPrerequisite: "Clock must already be open"
steps:
- echo: narration
- run: and-e2e
# guard-hop.yaml — echo behind a run: hop
executionPrerequisite: "Clock must already be open"
steps:
- run: echo-only
- run: and-e2eguard-inline -> ERROR (correctly refused)
guard-hop -> notice handshake, then with prerequisiteAcknowledged:true -> ok=True
adb shell pidof com.google.android.deskclock: 15744 -> 15980
(second round: 15980 -> 16136)
The app was force-stopped and relaunched — exactly the state destruction the guard exists to prevent — after the caller was asked to establish that state and acknowledged it. The report still echoes the prerequisite back. Same result on iOS (93330 -> 94579, 94579 -> 95102), so this is platform-independent. A steps: [] fragment triggers it identically.
(b) On chromium the boot hoist is skipped too, and the launch can go green against the wrong app. resolveRunDevice calls the same helper, so the byte-equivalent flows diverge:
# A-inline.yaml # B-hop.yaml
steps: steps:
- echo: narration - run: echo-only
- run: chrome-a - run: chrome-aA-inline --platform chromium -> ok=True device=chromium-cdp-54712
launch: pass "booted chromium instance chromium-cdp-54712" (2/2)
B-hop --platform chromium -> never boots (3/3). Outcome depends on ambient chromium devices:
* exactly one present -> device=chromium-cdp-9222, launch: **pass**, reason=None
…9222 was a real Google Chrome ("Chrome/150.0.7871.187"), not the
declared app; the next step then failed on the wrong app.
* two or more present -> hard error "2 booted devices matched — pass --device or --platform"
A declared launch: { chromium: <appA> } reporting a green launch step while attached to the user's browser is the failure the pre-PR code explicitly refused ("would run against the already-launched (wrong) app while booting nothing").
Expected: a hop that contributes no non-echo step should be transparent — keep scanning the parent's remaining steps — so guard-hop refuses exactly like guard-inline, and B-hop boots exactly like A-inline.
Scope note: the trigger is narrow (the run's leading step must run: a flow with literally no non-echo steps), and main has no guard at all, so this is an incompleteness in new functionality rather than a regression. I'm calling it MEDIUM rather than LOW because the damage when hit is a wrong verdict / destroyed state rather than something cosmetic, and it reproduces on all three platforms. A reviewer weighting the narrow trigger more heavily could reasonably call it LOW — one of the agents did.
Reproduced 3/3 (Android) + 2/2 (Android + steps: [], second agent, clean state) + 4/4 (iOS) + by hand on chromium — every attempt, on every platform tried.
A
launch:on Chromium now starts its own Electron instance, so nested e2e flows and mid-flow relaunches work as they do on iOS/Android. Previously the runner booted one app per run and rejected any second chromium launch.A chromium device id is its CDP port, so a launch that starts an app from scratch has to boot one and move the run onto it:
state.devicebecame reassignable (steps already read it per call), andstate.ownedtracks what the runner booted, torn down in reverse at run end. An instance the runner did not boot — pinned via--device, or auto-detected — is attached to, never killed. Relaunching the same app kills its instance and waits for the process to actually exit, or the replacement loses a race against the app's single-instance lock.The runner needs a device before step 1, so
resolveRunDevicefollows a leadingrun:chain to find the launch the run begins with: a fragment composing a chromium e2e flow now boots instead of failing with "No booted chromium device found". A launch declaring no app id for the run's platform stays an error — the run's platform is fixed start to finish.Also fixes a pre-existing teardown leak: the spawned handle is the npm
electronnode wrapper, not Electron, so signalling it left the app and its helpers running (reparented to launchd, still in the dock). Teardown now sweeps the child's process group.Behavior change: a fragment leading with
run:into a chromium e2e flow now boots a fresh instance where it previously attached to a running one; pass--deviceto keep attaching.Tests
Cover the boot/attach/settle three-way, teardown ordering and the group sweep, device rebinding after a nested launch, same-app retire ordering, cancellation mid-boot, both "no chromium app declared" guards, and the leading-
run:chain (multi-hop, cyclic, no-launch). The guard, exit-wait and sweep tests are mutation-checked.