feat(flow): get a launch past the expo dev-client server chooser - #768
Draft
hubgan wants to merge 47 commits into
Draft
feat(flow): get a launch past the expo dev-client server chooser#768hubgan wants to merge 47 commits into
hubgan wants to merge 47 commits into
Conversation
An expo dev build shows its `DEVELOPMENT SERVERS` chooser whenever it cold-starts and cannot silently reconnect to the server it opened last. Every `launch:` cold-starts by construction (terminate + relaunch), so any run against a dev build can land there. The run does not fail cleanly: every selector resolves against the chooser instead of the app, so the first directive fails on a screen that looks nothing like the flow, and recovering costs an agent several turns of describing and tapping. `launch:` now handles it. After the tree-source gate it waits for the screen to become either the chooser or the app, opens the row for the run's own bundler, and waits for the bundle. The step reports that it did so, so a run never hides that it started somewhere other than the app. Which bundler that is comes from `flow-execute`'s new `metroPort` (default 8081) — the caller passes it because only the caller knows which Metro it started. Nothing is guessed: only rows in the live `DEVELOPMENT SERVERS` section are eligible, never a `RECENTLY OPENED` entry (that list keeps addresses that stopped answering), and a chooser with no row for the port fails the launch naming the port it wanted rather than running the flow against another project's bundle. Two costs are kept off everything else. The wait for the chooser to draw runs only while the screen has drawn no text, which is the only state it can still emerge from; and the whole path is gated on a `dumpsys package` probe for the `exp+` scheme, so an app that is not an expo dev build pays nothing. iOS is not probed — there the remembered server is a stable `localhost`, which is why the chooser is an Android routine and an iOS rarity.
`pickDevServerRow` matched the run's Metro origin against `nodeText(n)` OR `n.subtreeText`. The flow adapters hoist every descendant's text onto its ancestors, so the scroll container wrapping the whole chooser carries the history rows' URLs too - and its top edge sits above the history boundary the picker filters on. A port only a remembered row offered therefore matched that container, and the launch tapped the container's centre: an arbitrary point on the chooser, reported as "opened http://10.0.2.2:8085". Observed before, with 8085 in RECENTLY OPENED and nothing live on it: the run stalled the full 60s exit budget and then errored with `opened http://10.0.2.2:8085 ... but it was still showing 60s later - the bundler at that address did not serve this app`, blaming a bundler it never opened. After: the launch errors in 15s with `lists no reachable server on port 8085`, the message the module was written to give. A live row is unaffected - a run targeting 8081 still opens `http://10.0.2.2:8081` and reaches MainActivity. Own text loses nothing: the hoist is additive, so the leaf that renders a URL still carries it. The test that guarded this invariant was vacuous - it asked for port 8083, which appeared nowhere in the fixture, so it passed because nothing matched at all. The fixture's first remembered row is now 8085, absent from the live rows, and the case asserts the tree really does render it below the boundary before requiring a null pick.
`detectDevLauncher` required both `DEVELOPMENT SERVERS` and the literal `New development server`. The second string is the label of the accordion the launcher draws under its server list, and the launcher only draws that list when it has discovered a running packager (expo-dev-launcher 6.0.21, HomeScreen.kt:100-152). With none discovered it renders an instruction card, an address box and `Fetch development servers` instead - none of which carries the required string. So the one state the flow cannot recover from on its own went unrecognized: the launch reported `pass` with no warning, every later step resolved its selectors against the chooser, and the actionable "lists no reachable server on port N" message was unreachable precisely when no server was there. The same predicate decides when the chooser has GONE, so that state also read as a successful dismissal. The heading is now paired with any ONE of the launcher's own marks, including the header subtitle that both faces carry. Recognizing the no-servers face also means excluding its address box: the adapters give that input an empty label and render its URL as a text leaf inside it, so the leaf matched the run's own origin and the launch pressed the box, opened a keyboard, then spent the 60s exit budget and blamed a bundler it never opened. Candidate rows are now filtered by tap point - anything whose centre lands in a text input is not an offer. Verified on emulator-5556 with the Bluesky dev build and no packager reachable. Before: `launch: pass`, then `await` failing on the chooser with `no element matched selector text="Create account"`, `DevLauncherActivity` still resumed at the end of the run. After: the launch errors in 12s with `the expo dev-client launcher is showing and lists no reachable server on port 8081`. With packagers discovered, a run targeting 8081 still opens `http://10.0.2.2:8081` and reaches MainActivity, and one targeting a history-only port still fails with the same actionable message.
`isExpoDevBuild` matched `Scheme: "exp+`, which the `expo-dev-client` config plugin writes into `android/app/src/main/AndroidManifest.xml` - the manifest every variant merges (confirmed in the project under test: `exp+bluesky` is in `src/main`, and `src/debug` carries no scheme). A RELEASE build of any project with `expo-dev-client` in its dependencies therefore read as a dev build, entered the appear-wait on every launch step, and one whose first screen is wordless paid the full 12s for a chooser it can never show. The probe now looks for expo-dev-launcher itself, which declares its activities in a DEBUG-variant manifest (`expo-dev-launcher/android/src/debug/AndroidManifest.xml`) - so the marker is present exactly when the build can show a chooser. Checked against the real `dumpsys package` text from the emulator: the old regex answers true for both the dev dump and the same dump minus the debug manifest's contribution; the new one answers true only for the dev build. Both dumps are now test fixtures, with the release one keeping its `exp+bluesky` scheme so the distinction is what the case actually asserts. End to end, the dev build still gets the recovery (`dismissed it via http://10.0.2.2:8081`) and a non-expo launch still passes in 8s.
The `gesture-tap` that opens the row had no try/catch, and neither `runLaunch` nor `execLeafStep`'s launch case catches (contrast the directive case, wrapped precisely so a throw still lands in the report). A rejection therefore left `flow-execute` itself: the run returned no report at all, losing every step collected so far, and the failure was booked as a tool failure rather than a step error. There was also no `signal?.aborted` re-check between the detection break and the tap, so a run cancelled during the probe or the tree reads - seconds of work - still tapped the device. The tap now reports as a launch failure naming the URL it could not open, and a cancellation returns unhandled so `runLaunch`'s own abort check reports the step as a skip. `dismissDevLauncher` had no tests at all. It now has the appear-wait (a splash becoming the chooser, and a failed read that does not end the wait), the tap and its point, the no-row branch, the tap-failure branch, the abort exit, and the two paths that leave a launch alone: an app already drawn, and a build with no launcher (which never reads the screen). Both new cases fail without this change: before it, the failing-tap case rejects instead of returning. On device, a normal dismissal still passes with `dismissed it via http://10.0.2.2:8081`.
`parseRunArgs` had no flag for `metroPort` and rejects unknown flags outright, and `buildRunPayload` forwarded only device, platform and updateBaselines - so the parameter the new skill text tells authors to set had no route through the CLI at all. That is the path the QA-flow skill points at for CI, where a dev-client launch could therefore only ever open 8081. (`argent run flow-execute --metroPort` does reach it, but exits 0 on a failed run and has no --output, so it is not a substitute.) The token is forwarded as written: flow-execute's schema coerces and validates it, so a typo fails the call with the schema's own message rather than silently running against the default bundler. Verified: `argent flow run --help` lists the flag; the run path forwards `metroPort: "8085"` to flow-execute (asserted against the mocked tools client, which is where the CLI hands the payload over); and that exact wire form - a string, as the CLI sends it - drives the tool-server on device to `lists no reachable server on port 8085`, so the coercion holds end to end.
`bindDeviceArgs` only rewrites device keys, and `state.metroPort` was read in one place, so a raw `tool: flow-execute` step ran its whole sub-flow on the default port. A `run:` fragment inherits the port by construction - it shares one ExecState - so the two composition forms disagreed about which bundler a dev-client launch opens. Only a non-default port is passed down: the inner run resolves the same default itself, so injecting it would add noise to every nested step's reported args. A port the nested step names for itself still wins. Verified on device with a wrapper flow whose only step is `tool: flow-execute`, run with `metroPort: 8085`. Before: the inner launch reported `dismissed it via http://10.0.2.2:8081` - the outer port ignored. After: the inner launch errors with `lists no reachable server on port 8085`, and the outer step's args carry `metroPort: 8085`. With no outer port the args stay untouched and the inner run dismisses via 8081 as before.
Six statements about the recovery were wrong or missing:
- Nothing said the feature is Android-only. `isExpoDevBuild` returns false for
every other platform, but the `metroPort` description, the `flow-execute`
description and the skill section all stated the recovery unconditionally, so
an iOS expo dev build got no recovery and no explanation.
- "Without it the run stays on the chooser, and every later step reads that
screen instead of the app" was wrong twice: the parameter is optional and
defaults to 8081, so omitting it is the normal case, and a port with no live
row fails the launch outright rather than leaving the run on the chooser.
- "waits for the bundle" overstated the wait. The runner waits for the chooser to
go away and returns immediately, so the next selector can land mid-bundle. Both
places now say so, and tell the author to gate on something the app draws.
- "A recorded row is a coordinate" was not the reason not to hand-write a `when:`
block for the chooser. The recorder derives a text selector from the row's URL.
The real reasons are that the launch dismisses the chooser first, so such a
block meets a different screen, and that the row order changes between launches.
- `warning` was documented as coming only from `await: { idle: true }` in the
runner, the CLI and the MCP content layer; a launch step can now carry one.
`argent-qa-flows` enumerated only the idle warnings, and called a failed
`launch:` a verdict about the app — which the new wrong-Metro-port failure is
not.
- The QA two-pass contract said nothing about the dismissal. The recovery makes
the client remember the server, so pass 1 can carry the warning and pass 2 not
meet the chooser at all; the skill now says that difference does not reset the
streak, and how to keep the two passes alike.
`metroPort` is also introduced where `live-authoring.md` introduces the other
flow-execute parameters, instead of only in the recovery reference.
Verified the published text: `GET /tools` on a server built from this branch
returns the corrected `metroPort` description and flow-execute description. Full
suites green (tool-server 3870, CLI 465, MCP 79).
…ee shape Four gaps the review found, all in the same file: - No test asserted the new `warning` on a passing launch step, the `metroPort` plumb into `ExecState`, or the fail branch. Four run-level cases now drive a real `launch`-only flow on an Android device: the pass carries `dismissed it via http://10.0.2.2:8082`, an unmatched port errors naming 8085, a caller who names no port gets 8081, and an app that starts on its own screen reports no warning at all. - The 60s exit branch was untested. One case holds the chooser on screen through the whole budget (fake timers) and asserts the reason names both the URL that was opened and the wait. - The fixture's tree shape was not production's. Measured on device, the Android flow tree is FLAT: 23 leaves under one synthetic root, depth 1, with the ancestors surviving as leaves that keep their frames and carry the hoisted text. The fixtures stay nested for legibility, and two cases now run the central invariants against the flattened form as well — including the address box, whose exclusion is geometric precisely because flattening drops the link between an input and the text it renders. - One case asserted a branch production cannot reach (the picker with an iOS device) as though it were a supported path. It now says what it is: the helper's total behaviour off Android, which the Android-only gate never reaches. The physical-Android-device case IS reachable and stays as it was. Full tool-server suite green: 3877 passed, 1 skipped.
…xport Exporting `isExpoDevBuild` for its tests took the last slot under knip's `--max-issues` ceiling: an export nothing in `src` imports counts as one parked issue, which put the second pass at 216 against the 215 cap and failed Dead Code. Measured both ways on a tree with no build output, the way knip.jsonc says to count it: 214 with the export, 213 without. The four cases now assert the probe through `dismissDevLauncher`, which is where its answer is observable anyway - a dev dump reaches the tree reads and dismisses, a release dump of the same project reads no tree at all, an unprobeable package leaves the launch alone, and an iOS device is never probed. The real `dumpsys package` excerpts stay exactly as they were. Full tool-server suite green (3877 passed, 1 skipped); on device, a populated chooser still dismisses via http://10.0.2.2:8081 and a history-only port still errors naming 8085.
latekvo
reviewed
Aug 11, 2026
latekvo
left a comment
latekvo
left a comment
Member
There was a problem hiding this comment.
Comprehensive review of the expo dev-client chooser recovery. The implementation is thorough — own-text matching, history-boundary filtering, geometric address-box exclusion, and abort handling are all well-considered. Verified: 35 new unit tests pass (flow-dev-launcher), 94 composition tests pass, 101 CLI tests pass, npm run build clean. Findings below are edge-case observations, none blocking.
latekvo
reviewed
Aug 11, 2026
latekvo
left a comment
latekvo
left a comment
Member
There was a problem hiding this comment.
Comprehensive review of the expo dev-client chooser recovery. The implementation is thorough — own-text matching, history-boundary filtering, geometric address-box exclusion, and abort handling are all well-considered. Verified: 35 new unit tests pass (flow-dev-launcher), 94 composition tests pass, 101 CLI tests pass, npm run build clean. Findings below are edge-case observations, none blocking.
Member
|
Review above completed by GLM-5.2 on xhigh thinking. Personal opinion:When i first ran review with GLM-5.2 on late-stage PR it did AWFULLY, really acting like a 2023 model. It turns out that an overwhelming majority of perceived models intelligence is steered by learned memories and user-created system prompts. After migrating both of these, GLM-5.2 turned from useless to Opus level, which makes me think i was too harsh in the past towards models like gemini, GPT and deepseek, perhaps given the same config as Claude they perform on a similar level. It really is fascinating, I wonder how much further this can be pushed and how much of the potential remains unused. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
`pickDevServerRow` only accepted a row whose text spelled an origin argent built itself — `10.0.2.2`, `localhost` or `127.0.0.1`. Since expo-dev-launcher 56 the Android client discovers servers over mDNS and writes each row's URL from the resolved IPv4 of the advertising machine, so every row carries a LAN address. The two sets never intersect: on every current client the picker returned null, and each dev-client launch errored with "start Metro on that port" while that Metro was running. The host was never argent's to choose. A row is now identified by its port, on whatever host the client wrote into it — the client only lists a server it just reached, over a host it reached it on. The live/history split and the address-box exclusion still decide which rows are eligible. The picker's fixtures all encoded the pre-56 shape, which is why the suite stayed green; a chooser captured from an Expo SDK 57 dev build joins them, carrying the same server in both sections. The failure message now says "no live server", matching what the skills already tell the reader.
An author recording on an Android expo dev build meets the chooser and taps a server row to get past it. The recorder stored that as an ordinary step. It used to be the step that started the app; since the `launch` gained its own recovery it is a second tap, fired into the app the recovery just opened — and green whatever it hits, because a coordinate tap reads no tree. The recorded flow replays differently than it was recorded. The tap still runs live, so the author gets past the chooser; it is only left out of the file, with a message naming the row and the `metroPort` that makes the launch open that same server. Narrow on purpose: only a tap that lands on a LIVE server row, only straight after a recorded `launch:`, and only on Android. A fragment, a raw `restart-app` carrying an activity, and every non-Android platform get no recovery at replay, so there the tap is the step that dismisses the chooser and is kept. `reportFlowUnchanged` gives a declined step the same lock and liveness assertion an append gets, so declining still reports a dead session.
`isExpoDevBuild` is the first thing a launch does after the tree-source
gate, and no AbortSignal reaches adb — so a run cancelled while `dumpsys
package` was out waited the probe's full 10s before the next abort check
saw anything. `settleWithin` is the module the codebase already keeps for
exactly this ("no AbortSignal reaches adb / AXRuntime"): the orphaned call
still finishes on its own, but the launch stops waiting on it.
It subsumes the try/catch too — a read that errors, times out or is
cancelled all answer "not a dev build", which is the behaviour a launch
had before this module existed.
`isExpoDevBuild` ran an `adb shell dumpsys package` on every `launch` step — including the launches inside `when:` blocks and `run:` fragments — for an answer that cannot change while a run is in flight. The run now carries those answers and each (device, app) pair is asked once. Per run, not per process: what is installed does change between runs, and a release build put over a dev one has to be probed again. A probe that errored, timed out or was cancelled is not remembered either — it answers "not a dev build" for that one launch without claiming anything about the app, so caching it would switch the recovery off for the rest of the run.
The exit wait retries a failed tree read because a read can fail transiently while the bundle builds. A read that keeps failing took the same path: the wait spent its full 60s and then reported that "the bundler at that address did not serve this app" — the one subsystem it never observed, while the tree source was the thing that was down. The last read's failure now reaches the timeout, and a wait that could not see the screen says so and quotes the read's own error. A wait that did see the chooser still blames the bundler, which is right there.
`.positive()` accepted 80811. A typo like that got past the schema, then past the dev-build probe and up to 12s of appear wait, and only surfaced as "lists no live server on port 80811" — a message about a chooser, for a number that is not a port. `.min(1).max(65535)` is what every other port argument in the tool-server takes (stop-metro, boot-device).
The `launch` arm of `execLeafStep` was the one step arm with no try/catch: a throw from it left `flow-execute` itself, losing every step collected so far and booking the failure as a tool failure rather than a step error. Nothing under `runLaunch` throws today — each arm catches its own — so the property held by convention across several call sites rather than by structure. Same guard the directive arms already carry. The test proves the escape: with the guard removed the throw comes out of `flow-execute` instead of landing in the report.
All three composition cases used a `tool: flow-execute` step, so deleting `step.name === FLOW_EXECUTE_TOOL_ID` from the injection left the suite green while every `tool:` step — restart-app, gesture-tap, the lot — would have received a `metroPort` argument it never declared. The new case runs two ordinary tool steps and asserts neither is given one; with the guard removed it fails.
Four paths through `dismissDevLauncher` had no case: the appear wait reaching its deadline on a screen that never draws, a cancel arriving mid-appear-wait, a cancel mid-exit-wait, and a cancel that surfaces as the tap's own rejection. The one existing abort case cancels before the first read, so it only ever exercised the pre-tap guard. The exit-wait one is worth the most: it returns SUCCESS on a cancelled run, which is only safe because `runLaunch` re-checks the signal immediately after and turns it into the run's skip.
Only the heading-without-marks direction was tested, so half the pairing that keeps a real app screen from being mistaken for the chooser was unasserted. "Development Build" beside a bundler URL is ordinary wording for an app's own About or debug screen, and taking it for the chooser means tapping at a screen the flow put there deliberately.
`RELEASE_DUMP` was `DEV_DUMP` minus the lines a comment claimed the debug manifest contributes, so the probe's load-bearing distinction — a release build keeps `exp+<slug>` but drops `expo.modules.devlauncher` — was asserted by that comment and by nothing else. Both dumps are now `dumpsys package` read off an emulator, from one Expo SDK 57 project built twice: debug carries the launcher's AuthActivity and its `expo-dev-launcher` scheme, release carries neither and keeps `exp+devclientprobe`. That is the pair the probe has to tell apart.
The skills and the module header both described the chooser as a state a run might land on — shown when the client "cannot silently reconnect to the server it opened last" — and `argent-qa-flows` built on that: pass 1 carries the warning, pass 2 "not show the chooser at all". Observed on an Expo SDK 57 dev build: after a successful open, the very next relaunch is back on the chooser with that server listed in BOTH sections, and two consecutive runs of one flow both carry the warning. A dev client cold-starts onto the launcher by design and `launch:` cold-starts by construction, so it is every launch, not some. Also in the same section, three things it was silent about: the recovery rides on `launch:`, so a `tool: restart-app` carrying an Android activity gets none of it; the "no `when:` block" rule is Android-only, and a `when:` block is exactly right where nothing dismisses the chooser for you; and a chooser tap kept in an old recording has to go, since the launch now performs it. The replay-diagnosis row gets the carve-out the section already made 37 lines above it: a failed `launch:` is a verdict about the app unless its reason names the chooser. The CLI half of the feature is named where `metroPort` is introduced — `--metro-port` in `argent flow run`'s own flag list, and beside every `metroPort` a CI reader meets.
… splicing The chooser paragraph was spliced into the middle of the "Steps run in order:" sentence, so `tool` calls resumed mid-line after it. It moves to the launch's own paragraph, beside the other things a launch does. It also described only the success path — open the server, wait, warn — while the step ERRORS when the chooser lists no live row for the port or the server it opened never serves. Only the `metroPort` description said so, and a reader who never passes `metroPort` never reads that one. The error is a verdict about the bundler, not about the app, which is the part that decides what to do about it.
…" mark The mark list called it "the build's own header subtitle", which reads as the app's own branding — and a review read it exactly that way: text an app could carry into a false positive on a screen that is not the chooser. It is the launcher's own. `AppHeader` renders the literal "Development Build" as a sibling of the app name it interpolates, in the debug-only source set the dev-build probe already gates on, and it renders it above the branch that swaps the chooser's two faces — which is why it is on both. Unchanged across expo-dev-launcher 6.0.21 and 57.0.11. The mark also earns its place rather than only hedging a rewording: while the client is fetching, the offer card reads "Searching for development servers..." in place of the wording the other mark matches, so the subtitle is all that names the launcher on that face.
…rt needs Two of its four sentences were already said in full elsewhere. The tool description says the launch "opens the server listed on `metroPort`, waits for the chooser to go away"; the reliability reference carries the iOS rationale the parenthetical spelled out, which "ANDROID ONLY" has told the caller two sentences earlier anyway. What is left is what decides the value: the port, the default, where it applies, when to pass one, and the launch error a wrong port produces. 731 characters down to 461.
`devServerRowAt` took the tightest node under the tap and, when that node carried no URL of its own, looked inside it for one. The Android flow tree is flat, so "the tightest node under the point" is settled by frames alone, and the chooser is drawn on containers that span the whole screen — a `ComposeView`, the window's content frame — each of them under every tap with a row's URL somewhere inside. Candidates are also cut at the RECENTLY OPENED boundary, so anything below it left the whole-screen container as the tightest one. A tap on the empty margin, on a remembered row, or on the launcher's own tab bar was therefore read as "that opened a server row": the step vanished from the recording, and the message named a server the author never touched. That is the first screen of every dev-build recording. Asked row first instead. Each row's URL leaf is placed in the card it is drawn in — the tightest node it sits inside, skipping any that also wraps the section heading, which is the list rather than a row — and a tap decides by falling inside a card. With no card the leaf answers for itself, which under-claims a press on the rest of the row rather than dropping a step. The recorder fixture becomes the tree an SDK 57 chooser really produces (captured through `fetchFlowTree` on an emulator): flat leaves, three whole-screen containers, the URL on its own leaf inside an unlabelled `Button`. The old one was five tidy siblings whose labels carried the " / " join only the trimmed agent-facing `describe` produces, so `under.length` was always 1 and neither branch this changes ever ran under test.
The per-run cache was documented as safe because "what is installed cannot change under a run". A `tool: reinstall-app` step takes an arbitrary `appPath`, so it can. Release-then-dev is the harmful direction: the remembered `false` makes the next `launch` skip the chooser recovery entirely, on a build that does show the chooser. Emptied at that step — whole rather than by key, since the step names a path, which says nothing about the app id the cache is keyed by. Also covers what nothing tested: that the cache is threaded through the run's ExecState at all, across two `launch` steps and into a `run:` fragment's launch, which inherits the port with it.
The parameter's own description opens "ANDROID ONLY" and the nested run binds this same device, so off Android the inner launch never reads the port — it just turned up in every nested step's reported args.
The CLI forwards the flag's token as written, so `flow-execute`'s schema is the only thing that turns "8085" into a port. Both halves were asserted separately — the CLI forwards the string, the schema accepts numbers — and neither noticed that swapping `z.coerce.number()` for `z.number()` breaks every CLI run that passes the flag. Verified by making that swap: this is the one test that fails.
…extend `reportFlowUnchanged` never re-read the YAML in host mode, and the guard that selects it — "is the step before this one a `launch`?" — was evaluated outside the lock, off `session.flow`. That copy is refreshed only by an append, so after the hand edit this tool's own description invites, both the guard and the reported `stepCount` answered off a flow that no longer matched disk. Folded into `appendStepToFlow` as an optional `decline` predicate instead: it is asked under the append's own lock, against the same re-read flow, and answering true writes nothing and reports the flow as it stands. One place decides, and it cannot disagree with the append it replaces.
`completedMsg` was built from the params alone, so the one path that runs the tool and records nothing still announced "Added <command> step to flow <name>" — contradicting the `message` it accompanies. Read off the result instead.
The guard's comment said nothing at replay dismisses the chooser after a fragment. The fragment's own launch runs through the same execSteps on the same ExecState, so it does get the recovery — for the chooser it met. What makes this tap a real step is that the chooser on screen NOW is a later one, which no recorded launch of this flow opens.
The description promised two outcomes — a recorded step, or an error with nothing recorded. The third case, success with nothing recorded, was invisible to any agent reading the schema.
The ~35-line comment describing `isExpoDevBuild` sat on `PROBE_TIMEOUT_MS`, so the function it describes had no tooltip and the constant had a page of prose about something else.
The loop's per-read local reused `seen`, the name of the (device, app) cache the function takes and its own JSDoc describes.
Both aborted before the probe answered, so `settleWithin` returned aborted inside `isExpoDevBuild` and the run ended there: the pre-tap re-check and the appear loop's own sleep were never entered, and either could have been deleted with both tests still green. They now start from a run whose probe is already answered — filled by running one launch, not by writing the cache key out — so the cancel lands where each test says it does. Verified by deleting each guard in turn: exactly the test that names it fails. Adds the probe TIMEOUT, whose "answer no, remember nothing" was only covered through the error path.
`noServersTree` is evidence about a client that has moved: it wraps the box's URL in a text leaf inside an unlabelled TextField. On expo-dev-launcher 57.0.11 the box's own label is the literal "http://" with no host or port, so no origin sits above the history at all. Both shapes are safe and both are now covered — the geometric exclusion has to survive the one that still spells a port, and the new capture is what a run meets today. It also gives the no-servers face its first recorder cases, including the remembered row a launch refuses to open.
The runner's signal re-check right after `dismissDevLauncher` — a cancelled wait answers "not handled", the same answer an ordinary app gives, so without it the launch reads as a pass that verified nothing. And the declined tap in client mode, where `savedTo` must stay the directive that carries the YAML back: a plain path there would cost the caller its next write. Verified by deleting the re-check: the new case is the one that fails.
latekvo
reviewed
Aug 14, 2026
latekvo
left a comment
latekvo
left a comment
Member
There was a problem hiding this comment.
Diplomat, Deepseek V4 Flash 0731]: Ran the review moves (claims vs code, nearest twin, non-happy paths, inputs, reachability, lifetime) plus the absence pass (sibling, prose, symmetry, mutation) across the new chooser-recovery module, the launch-step/report changes, the nested-composition paths, the recorder-decline mechanism, the CLI plumbing and the skills docs. Sectioned full-suite runs green: flow-dev-launcher (54), flow-composition (98), flow-record-tap (25) at the PR head. Two LOW findings below, each confirmed with a concrete boundary-mocked repro (no Android device on this box, so the trees are fixtures captured from a real chooser).
| } | ||
| if (!root || !launcher) return { handled: false }; | ||
|
|
||
| const target = pickDevServerRow(root, port, launcher.historyY); |
Member
There was a problem hiding this comment.
Diplomat, Deepseek V4 Flash 0731]: The run's Metro row is picked in a single pass the moment the appear-wait breaks on the launcher, with no retry before a missing row becomes a launch error. The chooser has a face it shows while it is still discovering packagers - the no-servers layout made of the heading, the launcher's header subtitle and the address box, all full of visible text - and the appear-loop breaks as soon as detectDevLauncher returns non-null on that face, because it has drawn content. pickDevServerRow then runs once against that face, finds no live row for the port, and the launch errors lists no live server on port N even when Metro is actually up and a live row for it appears on the very next read. Verified against boundary-mocked fixtures: the first tree read returns the discovering face, the second returns the populated chooser, and dismissDevLauncher returns { handled: true, ok: false } rather than ever opening the row. A launch whose first chooser-read lands inside that population window fails spuriously - on exactly the cold starts this module exists to recover - and the message blames the bundler for a state that resolves on its own a read later.
| // remembered `false` would skip the chooser recovery on a build that | ||
| // shows the chooser. Dropped whole rather than by key: the step names a | ||
| // PATH, which says nothing about the app id the probe is keyed by. | ||
| if (step.name === REINSTALL_APP_TOOL_ID) state.devBuilds.clear(); |
Member
There was a problem hiding this comment.
Diplomat, Deepseek V4 Flash 0731]: The dev-build probe cache is evicted only when a reinstall-app step runs as a DIRECT tool step of this run. The same reinstall nested inside a tool: flow-execute step executes against the inner run's own fresh devBuilds map, so the outer run's remembered answer is left untouched. A launch that probed a release build (remembered 'no'), followed by a nested flow-execute whose flow reinstalls a dev build of the same app, then another launch of that app, reuses the stale 'no' and skips the chooser recovery on a build that does show the chooser - the recovery the comment above says the run handles is switched off, and later selectors resolve against the chooser. Reproduced with a boundary-mocked run [launch, tool:flow-execute (inner: reinstall-app), launch]: the probe runs once, not twice, and the second launch carries no recovery warning, whereas the direct-step reinstall case re-probes. run: fragments are unaffected because they share this ExecState; the nested-flow-execute composition is the one form that diverges.
latekvo
reviewed
Aug 14, 2026
| `flow-execute` takes exactly one flow source: `name`, for a flow saved under `.argent/flows/`, or `flow_path`, an absolute path to any flow `.yaml`. `run:` targets and baselines resolve on the tool server's filesystem, beside the YAML it actually reads. `flow_path` therefore requires the agent and the tool server to share a filesystem and is refused when they do not. `name` still runs remotely, but the server receives only that one YAML in a fresh temp directory, so a `run:` target fails as a missing fragment and a `snapshot` fails for a missing baseline. Replay self-contained flows remotely; a composing or snapshotting flow needs one shared filesystem. | ||
|
|
||
| Manual rescue invalidates the pass. An `errored` step was never evaluated: an `idle` wait whose tree source could not be read, a step that threw, an unresolvable `run:` target, or a `launch:` that did not start the app. Read the reason — most name the environment, but a failed `launch:` is a verdict about the app. Unconfirmed focus is not in this class at all: the replay focus poll has no failure return, so a `type:` step whose focus was never confirmed is scored a **pass**, and only the value check after typing catches it. | ||
| Manual rescue invalidates the pass. An `errored` step was never evaluated: an `idle` wait whose tree source could not be read, a step that threw, an unresolvable `run:` target, or a `launch:` that did not start the app. Read the reason — most name the environment, but a failed `launch:` is a verdict about the app. One class of launch reason is the exception: every reason that names the expo dev-client launcher names the bundler, not the app. The chooser listed no live server on the run's `metroPort` (`--metro-port` from the CLI); the tap that opens that server failed; the chooser was still showing 60s later, so the bundler at that address never served this app; or the screen could not be read for 60s afterwards. Repair the bundler or the port and rerun. Unconfirmed focus is not in this class at all: the replay focus poll has no failure return, so a `type:` step whose focus was never confirmed is scored a **pass**, and only the value check after typing catches it. |
Member
There was a problem hiding this comment.
Diplomat, Deepseek V4 Flash 0731]: The claim that every launch-error reason naming the expo dev-client launcher is about the bundler does not hold for two of the four reasons enumerated here. flow-dev-launcher.ts emits a tap-failure reason ("the expo dev-client launcher is showing and the tap that opens failed: ") that names the gesture, and a tree-read reason ("the screen could not be read for 60s afterwards (...) so whether the chooser left is unknown") that the module's own comment says is produced exactly when the reads, not the bundler, are what is down. The blanket "Repair the bundler or the port and rerun" guidance thus points an agent at the wrong subsystem for the unreadable-screen case: a launch whose chooser never left because the tree source was unreachable would be re-run against Metro to no effect. The same overclaim appears in argent-qa-flows/SKILL.md ("every reason that names the expo dev-client launcher... Start that Metro, or correct the port") and in reliability-and-recovery.md ("unless its reason names the expo dev-client chooser, which names the bundler").
Member
There was a problem hiding this comment.
[Diplomat, Deepseek V4 Flash 0731]: The over-claim this thread names in live-authoring.md:241 also survives at head e8d5b96 in two sibling locations: reliability-and-recovery.md:123 ("unless its reason names the expo dev-client chooser, which names the bundler") and argent-qa-flows/SKILL.md:103 ("every reason that names the expo dev-client launcher" enumerated as naming the environment). In both, two of the four enumerated reasons do not name the bundler: the tap-failure reason (flow-dev-launcher.ts:460 names the gesture and its error) and the unreadable-screen reason (flow-dev-launcher.ts:490 names the tree source, not the bundler).
latekvo
reviewed
Aug 17, 2026
latekvo
left a comment
latekvo
left a comment
Member
There was a problem hiding this comment.
[Diplomat, Deepseek V4 Flash 0731]: Full review pass at head e8d5b96. Read the complete diff (16 files, +2735/−84), all 17 existing threads, and confirmed every cited "Fixed in" hash is an ancestor of the head. Dispatched five review agents across the moves — claims vs code, nearest twin + inputs, non-happy paths + reachability + abort handling, the absence pass (sibling / prose / symmetry / mutation) + lifetime, and composition / recorder-decline — working in a throwaway worktree at the head commit. Re-ran the suites at head: flow-dev-launcher 54/54, flow-composition 98/98, flow-record-tap 25/25; a scratch vitest repro additionally confirmed that the no-history chooser face (historyY fallback 1) reports "no live server" and picks a live row when one is present. One new finding below, plus a thread extension on the bundler-taxonomy docs claim. The three open threads (single-pass row pick at flow-dev-launcher.ts:432, nested-reinstall cache eviction at flow-run.ts:2322, bundler-taxonomy docs at live-authoring.md:241) are all confirmed present at head.
| args.metroPort === undefined && | ||
| state.metroPort !== DEFAULT_METRO_PORT | ||
| ) { | ||
| args.metroPort = state.metroPort; |
Member
There was a problem hiding this comment.
[Diplomat, Deepseek V4 Flash 0731]: The comment on this block says a nested flow-execute is "this same run one level down — same device, same app", and forwards metroPort to it on that basis — but the sibling per-run state the launch recovery actually consumes, devBuilds, is not forwarded with it. Every flow-execute invocation initializes a fresh new Map() for it (flow-run.ts:1145), the nested ones included, so an inner run whose flow launches the same dev build the outer run already probed re-runs the dumpsys package probe for the same (device, app) key the outer answer already covers, once per nesting level, each with the full 10s probe budget when the device is slow. Correctness is unaffected — the answer is re-derived identically — which makes this the exact duplication the devBuilds cache exists to prevent, on precisely the composition path this block declares equivalent to the outer run.
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.
Problem
An expo dev build shows its
DEVELOPMENT SERVERSchooser whenever it cold-starts and cannot silently reconnect to the server it opened last. Everylaunch:cold-starts by construction (terminate + relaunch), so any run against a dev build can land there.It reads as intermittent and hits Android hardest, because the remembered address rotates between the emulator's
10.0.2.2host alias,localhost(only reachable throughadb reverse), and a LAN address that goes stale on its own. iOS keeps a stablelocalhost:8081.The run does not fail cleanly. Every selector resolves against the chooser instead of the app, so the first directive fails on a screen that looks nothing like the flow, and recovering costs an agent several turns of describing and tapping.
The chooser, reproduced on an emulator (Bluesky dev build)
Two bundlers live at once, and a history entry carrying the run's own port on a host that stopped answering.
Change
launch:handles it. After the tree-source gate it waits for the screen to become either the chooser or the app, opens the row for the run's own bundler, waits for the bundle, and reports what it did:Which bundler that is comes from
flow-execute's newmetroPort(default 8081). The caller passes it, because only the caller knows which Metro it started.Nothing is guessed:
DEVELOPMENT SERVERSsection are eligible, never aRECENTLY OPENEDentry.http://<host>:<port>origin with a trailing-digit guard, so port808cannot open the row for8081.Two costs are deliberately kept off everything else:
dumpsys packageprobe for theexp+scheme, so an app that is not an expo dev build pays nothing. Measured:com.android.settingslaunches in 5s with no wait and no warning.iOS is not probed (that needs the installed bundle's Info.plist rather than one
dumpsysread), so the chooser is not auto-dismissed there. Given iOS's stablelocalhost, that is the rare case — but it is a gap, not a non-issue.Verification
Run end-to-end through
flow-executeagainst a real emulator, not only unit tests:Corroborated by logcat, which shows the chooser drew at +5.0s and then handed off to the app:
Row selection checked against the live chooser — by port, not position. That rendering listed 8081 first, the reverse of the unit fixture, so both orderings are covered on real data:
Also run: 12 new unit tests over the real captured tree; all 44 flow test files (1055 tests);
npm run build;npm run lint;npx prettier --check .;typecheck:tests+typecheck:scripts; repo-hygiene greps;npm run knipagainst an unbuilt tree per CONTRIBUTING step 5 (exit 0, no new exports flagged).SpiderShield tool-description score is unchanged from
mainat 9.09 / 10 (threshold 9.0);flow-executesits at 6.5 on both sides, so thehas_scenario_triggergap there is pre-existing and left alone.Notes for review
APPEAR_TIMEOUT_MSis 12s. It applies only to expo dev builds whose screen has drawn nothing, and exits the moment the chooser or the app appears. If dev builds routinely sit blank longer than that, this is the number to raise.subtreeText, or the scroll container's repeated text would drag the live/history boundary above the rows it must sit below.