[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 (
|
…ctive snapshot, capture-failure masking Three P1s from the post-merge review of #1670, all at the session-open-foreground dispatch seam: 1. Explicit device selectors were silently overwritten: the resolved-device rewrite pinned --udid/--platform over whatever the caller passed, so `open --foreground --udid B` with sim A sole-booted silently opened A. Now fails fast with INVALID_ARGS (matching the existing app-positional rejection) on --udid/--device, and on --platform other than ios; an explicit --platform ios passes through. 2. The promised interactive snapshot was never requested: the composed snapshot dispatch forwarded the open request's flags untouched, without snapshotInteractiveOnly — so the capture was NOT the `snapshot -i` path the doc comment promised and returned no interactive presentation. The composed request now sets snapshotInteractiveOnly: true (the exact key the CLI maps -i to and the snapshot runtime reads as interactiveOnly). 3. A capture failure masked the successful open: returning the snapshot error discarded openResponse even though the session exists, so a retry of `open --foreground` failed with "close the current session first". Open success + snapshot failure now returns ok with an explicit initialSnapshotError {code, message} detail and a rendered warning that the session IS open and how to capture manually (snapshot -i). Regressions added for all three: explicit-selector rejection (udid/device/both/non-iOS platform + ios pass-through), the composed dispatch carrying snapshotInteractiveOnly, and the snapshot-failure path returning ok + warning + usable session.
…ctive snapshot, capture-failure masking (#1671) * fix(daemon): open --foreground P1 hotfix — selector rejection, interactive snapshot, capture-failure masking Three P1s from the post-merge review of #1670, all at the session-open-foreground dispatch seam: 1. Explicit device selectors were silently overwritten: the resolved-device rewrite pinned --udid/--platform over whatever the caller passed, so `open --foreground --udid B` with sim A sole-booted silently opened A. Now fails fast with INVALID_ARGS (matching the existing app-positional rejection) on --udid/--device, and on --platform other than ios; an explicit --platform ios passes through. 2. The promised interactive snapshot was never requested: the composed snapshot dispatch forwarded the open request's flags untouched, without snapshotInteractiveOnly — so the capture was NOT the `snapshot -i` path the doc comment promised and returned no interactive presentation. The composed request now sets snapshotInteractiveOnly: true (the exact key the CLI maps -i to and the snapshot runtime reads as interactiveOnly). 3. A capture failure masked the successful open: returning the snapshot error discarded openResponse even though the session exists, so a retry of `open --foreground` failed with "close the current session first". Open success + snapshot failure now returns ok with an explicit initialSnapshotError {code, message} detail and a rendered warning that the session IS open and how to capture manually (snapshot -i). Regressions added for all three: explicit-selector rejection (udid/device/both/non-iOS platform + ios pass-through), the composed dispatch carrying snapshotInteractiveOnly, and the snapshot-failure path returning ok + warning + usable session. * fix(cli): render the composed open --foreground snapshot on default stdout and project initialSnapshotError through the public surfaces Post-merge review on #1671 found the daemon fixes never reached the public boundaries: openCliOutput ignored the nested snapshot (the one-call promise held only under --json), and initialSnapshotError was daemon-only — absent from AppOpenResult, Node normalization, and serializeOpenResult, with the normalized shape truncated to code+message. - default open output now renders the composed interactive tree through the same snapshotCliOutput path snapshot -i uses - AppOpenResult carries initialSnapshotError as the FULL daemon error (hint/details/diagnosticId/logPath preserved) through normalization and serialization Worker-authored; committed by the coordinating session after the worker stalled twice mid-push. Tests: output.test.ts + session-open-foreground (26 pass), typecheck, oxfmt. * refactor(client): one daemon-error normalizer + client-route regressions for initialSnapshotError Review follow-ups on #1671: normalizeInitialSnapshotError duplicated the target-shutdown error normalization and pushed the module over the fallow complexity threshold; consolidated into a single internal normalizeDaemonError (table-driven, full shape incl. retriable/supportedOn) used by both result paths, projected through normalizeOpenForegroundComposition. Client-route regressions: createAgentDeviceClient().apps.open now proves the full initialSnapshotError shape (hint/details/diagnosticId/logPath/retriable) survives normalization, and that a malformed one is dropped — deleting the boundary normalization fails these tests. Also rebased onto current main. * fix(daemon): a thrown initial-snapshot capture failure gets the same successful-open contract Review P1 on #1671: dispatchSnapshotViaRuntime rethrows ordinary capture/runner exceptions; the composition only handled a returned { ok: false }, so a thrown failure escaped to the router and failed the whole open after the session was created — retrying then wedged on the existing session. The catch normalizes the rejection (kernel normalizeError, same conversion the router applies) into the shared openWithInitialSnapshotFailure path: ok response, full-shape initialSnapshotError, session-usable warning. Rejecting-mock regression added alongside the returned-failure case.
…workflow card open --foreground (#1670/#1671) and snapshot -i --actions (#1665) shipped with no mention in the compact `help workflow` card, so a planning model never discovers either. Add one terse line each: the foreground fast-path in Bootstrap, and the merged-element custom-action guidance in Validation and evidence. Stays under the 9,000-byte compact-card budget (8493 -> 8908 bytes). Adds two help-conformance bench cases per the repo's changed-guidance rule: foreground-attach-single-sim (correct plan starts with `open --foreground` in an unambiguous single-sim scenario, fail-closed alternative forbidden) and merged-card-actions-not-directly-invokable (a merged Bluesky-style feed card's actions list is evidence, not a selector). Both use a real pinned sample rebuilt through the production snapshot renderer.
…workflow card (#1682) * docs(cli): advertise open --foreground and snapshot --actions in the workflow card open --foreground (#1670/#1671) and snapshot -i --actions (#1665) shipped with no mention in the compact `help workflow` card, so a planning model never discovers either. Add one terse line each: the foreground fast-path in Bootstrap, and the merged-element custom-action guidance in Validation and evidence. Stays under the 9,000-byte compact-card budget (8493 -> 8908 bytes). Adds two help-conformance bench cases per the repo's changed-guidance rule: foreground-attach-single-sim (correct plan starts with `open --foreground` in an unambiguous single-sim scenario, fail-closed alternative forbidden) and merged-card-actions-not-directly-invokable (a merged Bluesky-style feed card's actions list is evidence, not a selector). Both use a real pinned sample rebuilt through the production snapshot renderer. * fix(scripts): accept flag order in the foreground-attach conformance matcher Flag order after `open` isn't semantically meaningful (`open --platform ios --foreground` is exactly as correct as `open --foreground --platform ios`), but startsWithForegroundOpen required --foreground to be the literal next token after `open`. Rescoring the completed repeat=3 bench report shows this docked codex:gpt-5.4-mini on all 3 trials even though its plan was config-order noise, not a real deviation -- the no-positional/no-device guarantee already comes from the forbidden checks. Loosened to require --foreground anywhere on the open line; foreground-attach-single-sim now scores 54/54 across both runners. * fix: close workflow help conformance gaps
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.