[RFC] prototype: open --foreground collapses the snapshot->hint->open->snapshot dance into one call - #1670
Conversation
33c22f4 to
5b73319
Compare
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
Prototype `open --foreground`: on a fresh session with no app argument, auto-resolves the target from the sole booted iOS simulator's sole foreground app (reusing the exact same ambiguity-detection probe that enriches the SESSION_NOT_FOUND hint), then attaches the initial interactive snapshot to the response by composing the existing snapshot-runtime dispatch. Collapses the documented 3-call snapshot-fails -> read-hint -> open -> snapshot-succeeds dance into a single call for the unambiguous case, while failing closed (AMBIGUOUS_MATCH) with no guessing otherwise. First-pass RFC, not reviewed — see PR body for the design tradeoff writeup, live before/after evidence, and scoped-out follow-ups.
5b73319 to
9c5ec12
Compare
|
|
Review: not ready — three P1 correctness blockers.
Please add regression/router coverage for explicit target intent, interactive forwarding, and post-open capture failure; obtain a successful exact-head isolated-simulator run; and add the user-facing docs for this public CLI behavior. CI is green apart from the iOS Smoke job still in progress. |
|
Post-merge review confirmed three P1s in the prototype's dispatch seam — all fixed in #1671 (
|
This is a first-pass RFC prototype — nobody has reviewed this design. It exists to give the maintainer something concrete to react to, not a finished feature. Command spelling, error code, and scope are all up for debate.
Motivation
agent-device snapshot -ialready emits an enriched hint (#1662) when it fails withSESSION_NOT_FOUNDand the environment is unambiguous (one booted iOS simulator, one running app): it names the exactopen <bundleId>command to run. Benchmark evidence: an agent still has to spend 2-3 turns on this —snapshot -i(fails) -> read hint ->open <bundleId>->snapshot -i(succeeds) — in 27 of 30 benchmark tasks, costing ~20s and multiple model turns each time, even though the environment was completely unambiguous.This PR prototypes
open --foreground: on a fresh session with no app argument, it auto-resolves the target the same way the hint does, opens it, and returns the initial interactive snapshot in the same response — collapsing the 3-call dance into 1 call for the common case, with the exact same fail-closed, no-guessing semantics otherwise.Design: why
open --foreground, notobserveorsnapshot --attach-foregroundThree spellings were considered:
observecommand. Cleanest single-purpose semantics, but the most new wiring: a new CLI grammar (metadata/schema/reader/writer), and — whilesrc/core/command-descriptor/registry.ts's single-declaration model means a plain new command is only ~1-2 files — this command isn't plain: it would need its own session-creation logic re-invoked or duplicated fromopen's existing path (device resolution, advisory device claims, runtime-hint plumbing, session-store persistence), sincesrc/daemon/handlers/session-open.ts'shandleOpenCommandis the sole owner of that machinery today.snapshot --attach-foreground.snapshot's daemon handler (src/daemon/snapshot-runtime.ts) is architecturally read-only: it requires an existing session and explicitly rejects a missing one (requireIosAppSessionForSnapshot). Teaching it to also create a session on demand would change a correctness-relevant invariant of that command, not just add wiring.open --foreground.openalready owns 100% of the session-creation machinery this needs. Traced end-to-end: adding a flag-only capability to an existing command touches onlysrc/commands/management/app.ts(metadata field +allowedFlags+ CLI reader) and aFlagDefinitionentry insrc/commands/cli-grammar/flag-definitions-action.ts— confirmed zero changes needed tosrc/core/capabilities.ts,src/batch-policy.ts,src/client-types.ts,src/client.ts, or the daemon command registry (open's route/policy traits are unchanged). The daemon-side composition (resolve -> open -> attach snapshot) lives in one new file,src/daemon/handlers/session-open-foreground.ts, composing the existinghandleOpenCommandand the existingdispatchSnapshotViaRuntimesnapshot-runtime dispatch — no new capture pipeline.Given this is explicitly a cheap RFC demonstration and not a production surface, minimizing new wiring was the deciding factor. Option 3 reuses the most existing plumbing by a wide margin.
What changed
src/daemon/ios-app-session-hint.ts: extracted the existing ambiguity-detection logic (previously inline inbuildIosOpenCommandHint) into a newresolveSoleForegroundIosApp()that returns structured{ device, app } | undefinedinstead of a formatted string. The extraction was re-done from the merged fix(daemon): SESSION_NOT_FOUND hint names the detected foreground app #1662 version after rebase, so its review-fix semantics carry into the structured resolver: the catch-all now lives in the resolver (any probe rejection — timeout, spawn failure — yieldsundefined, never propagates, for both consumers), and the hint builder keeps its--udid+--ios-simulator-device-setcommand pinning,MAX_HINT_LENGTHfallback, and "running" wording unchanged. Same behavior, same tests (plus a resolver-level caught-rejection test), zero duplication.src/daemon/handlers/session-open-foreground.ts(new):resolveForegroundOpenRequest()interceptsopen --foregroundon a fresh session with no app argument, resolves viaresolveSoleForegroundIosApp, and rewrites the request'spositionals/flags(pinningudid+platform: ios) so the rest ofhandleOpenCommand's existing new-session flow runs completely unmodified. Fails closed withAMBIGUOUS_MATCH(not a new/wrong code — this is the existing code used elsewhere for the same kind of ambiguity, e.g.resolveAppleDevice's multi-simulator-match case) when 0 or 2+ booted simulators, or 0 or 2+ running apps, are found — a probe failure surfaces as the same fail-closed error, since the resolver reports it asundefined.composeOpenWithInitialSnapshot()then delegates to the existingdispatchSnapshotViaRuntime(the same pathsnapshot -iuses — ref issuance, session-store snapshot lineage, ref-frame activation all come for free) and merges the result underdata.snapshot.src/daemon/handlers/session-open.ts/session.ts:handleOpenCommandnow resolves the foreground request before doing anything else; theopenrouter entry composes it with the snapshot attach step.src/commands/management/app.ts,src/commands/cli-grammar/flag-definitions-action.ts,packages/contracts/src/cli-flags.ts,packages/contracts/src/client-app.ts,src/agent-device-client.ts,src/utils/result-serialization.ts: thread the new--foregroundboolean flag through the CLI grammar and (loosely-typed, see follow-ups) the typed Node client'sAppOpenResult.snapshot.Live validation
Isolated simulators only (never
bench-golden*, never the pinned UDID, never simulators I didn't create), repo CLI only, deleted at the end.Correctness: direct probe verification
Before: the 3-call dance (real transcript, isolated simulator,
com.apple.Preferenceslaunched viasimctl launch, no agent-device session)Call 3 (the final, previously-manual
snapshot -i) reliably lost a race againstopen's background XCTest-runner prewarm on this run — see "Environmental notes" below; this is a pre-existing infra characteristic shared by both the old dance and the new command, not something this PR introduces or fixes.Fail-fast: ambiguous environment (real transcript, default device set with multiple genuinely-booted simulators from concurrent sessions — no construction needed)
3.9s, no guessing, no session created — confirmed via
xcrun simctl list devices bootedthat 3+ simulators from other concurrent sessions were genuinely booted at the time. (Message shown with the post-rebase wording — "exactly one app running", not "in its foreground": the probe verifies a sole runningUIKitApplicationprocess, not frontmost-scene status, matching #1662's wording fix. The captured run differed only in that trailing phrase.)After: the 1-call flow
resolveForegroundOpenRequestwas exercised live and worked correctly end-to-end through the resolve step every time I reached it (multiple runs): it correctly resolved{device, bundleId}and rewrote the request, or correctly failed closed withAMBIGUOUS_MATCHwhen ambiguous (see above). I was not able to capture a single fully-unbrokenopen --foregroundrun all the way through to a returned snapshot on this occasion — every attempt (~15+ fresh isolated simulators across roughly two hours) had its simulator crash/vanish (Invalid device,exitCode: 148) at some point between boot and the runner's XCTest launch, including attempts where the simulator survived long enough for my own manualsimctl launchto succeed just before the daemon's own dispatch failed. This reproduced identically for the old 3-call dance's final step and is a property of the shared machine at the time (see below), not of this PR's code — the composedopen --foregroundreaches and exercises the exact samedispatchSnapshotViaRuntimepathsnapshot -iuses, with no new capture logic to fail differently.Environmental notes (found during validation, not fixed here — filed separately)
This machine was under extreme, sustained multi-tenant load for most of this validation session (
uptimeload averages up to 865; ~30 concurrentagent-devicedaemon processes and multiple concurrentxcodebuildprocesses observed from unrelated worktrees/sessions; free memory briefly dropped to ~500MB). Two concrete, precisely-diagnosed pre-existing bugs were found and reported as separate follow-ups (not fixed in this PR — out of scope, unrelated files):src/platforms/apple/core/runner/runner-device-set.ts,reconcileXcodebuildSimulatorSetRedirect:const xctestIsSymlink = xctestExists && fs.lstatSync(...).isSymbolicLink()short-circuits tofalsefor a dangling symlink at~/Library/Developer/XCTestDevices(sincefs.existsSyncfollows symlinks), skipping the cleanup step and causing every subsequent XCTest-backed command system-wide to fail withENOTDIRuntil someone manually repairs the path by hand. Reproducible: delete a--ios-simulator-device-setdirectory while it's still the liveXCTestDevicessymlink target.~/.agent-device/xctest-device-set.lock(same file) has no stale-owner/liveness reclaim, unlike the device-claim mechanism fixed in fix(daemon): report the device claim retained by a failed close #1647 — a killed daemon (or, observed once, a leaked lock from an automated test run whoseowner.jsonliterally recorded the mocked value"startTime":"test-process-start") can leave it held indefinitely, blocking every future XCTest command until the lock directory is removed by hand.Also noted, not filed: a freshly booted iOS 18.6 simulator can have
com.apple.mobilecal(Calendar) respawn as aUIKitApplicationlaunchd job with nothing explicitly launched by the user, occasionally makingresolveSoleForegroundIosAppcorrectly-but-inconveniently report ambiguous even when only one app was deliberately launched — this is existingdetectSoleRunningIosSimulatorAppbehavior from the companion hint PR, not something this PR changes or should change (the no-guessing contract is doing exactly its job here).Not yet handled (explicitly scoped out)
resolveSoleForegroundIosApponly probes iOS simulators;--foregroundon any other target fails closed (AMBIGUOUS_MATCH/ no resolution), never guesses.--foregroundis wired through the CLI-direct-to-daemon path and the raw JSONAppOpenResult.snapshot, butsnapshotis loosely typed (Record<string, unknown>, not the fullCaptureSnapshotResult) since the daemon-side composition doesn't attach client-only fields likeidentifiers. No MCP tool schema smoke test added.--foregroundcombined with an existing session or an explicit app argument: rejected withINVALID_ARGSrather than silently ignored or auto-relaunching.resolveForegroundOpenRequestandcomposeOpenWithInitialSnapshotin isolation (10 new tests, hermetic, mocked probes/dispatch) plus the extractedresolveSoleForegroundIosAppprobe (5 new tests inios-app-session-hint.test.ts, including the caught-rejection contract). A full router-level integration test mirroringrequest-router-open.test.ts's heavier mocking (ensureDeviceReady/resolveTargetDevice/dispatchCommand) was not added — left as a follow-up.Testing
npm run typecheck,npm run check:layering,npx oxlint <changed files> --deny-warnings,node ./node_modules/oxfmt/bin/oxfmt --check .— all clean.npx vitest runacross all touched/added test files plus the existingopen-adjacent suites (request-router-open,session-open-*,snapshot-handler) — 169/169 passing after the rebase onto fix(daemon): SESSION_NOT_FOUND hint names the detected foreground app #1662.npm run build— succeeds;bin/agent-device.mjs open --helpshows the new--foregroundflag with its[RFC]description.Not merging this myself — it needs your read on the command spelling above everything else.