Skip to content

fix: make record app-scoped by default#1163

Merged
thymikee merged 2 commits into
mainfrom
fix/ios-simulator-record-session
Jul 8, 2026
Merged

fix: make record app-scoped by default#1163
thymikee merged 2 commits into
mainfrom
fix/ios-simulator-record-session

Conversation

@thymikee

@thymikee thymikee commented Jul 8, 2026

Copy link
Copy Markdown
Member

Summary

Makes record start app-scoped by default: it now requires an active app session from open <app> instead of silently creating a record-only whole-screen capture when the session is missing.

Adds explicit --scope app|device|system; device/system are the opt-in whole-screen intent values for recordings that intentionally span the full screen, multiple apps, Settings, Home, or app transitions. iOS simulator record-only capture now requires one of those whole-screen scopes.

Persists and returns recordingScope alongside the existing backend, record-only state, active app, and duration metadata, and updates CLI help/docs plus provider scenarios for the new default.

Scope note: this resolves #1162 by making app proof videos the default and whole-screen capture explicit. It does not add a new iOS foreground-bundle probe; app scope is enforced from session state today.

Validation

  • pnpm check:quick
  • ./node_modules/.bin/vitest run --project unit-core src/commands/recording/index.test.ts src/cli/parser/__tests__/args-parse-interaction.test.ts src/cli/parser/__tests__/cli-help-command-usage.test.ts
  • ./node_modules/.bin/vitest run --project unit-core src/commands/recording/index.test.ts src/cli/parser/__tests__/args-parse-interaction.test.ts src/daemon/handlers/__tests__/record-trace.test.ts
  • ./node_modules/.bin/vitest run --project unit-core src/commands/__tests__/command-flags.test.ts src/cli/parser/__tests__/cli-help-command-usage.test.ts
  • ./node_modules/.bin/vitest run --project provider-integration
  • ./node_modules/.bin/vitest run --project unit-core --project android-adb
  • ./node_modules/.bin/vitest run --coverage
  • node --test test/integration/smoke-*.test.ts
  • ./node_modules/.bin/tsdown
  • Direct oxfmt --write over the repo because pnpm format was blocked by pnpm version verification in this environment.
  • Live iPhone 17 simulator: default app scope rejects missing --session with INVALID_ARGS and creates no MP4.
  • Live iPhone 17 simulator: --scope device record-only capture succeeds and produces /private/tmp/agent-device-record-device-scope.mp4 (52K), then cleans up the session.
  • Live iPhone 17 simulator: open settings followed by default record start succeeds with recordingScope: "app" and activeSessionApp.bundleId: "com.apple.Preferences", produces /private/tmp/agent-device-record-app-scope.mp4 (52K), then cleans up the session.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.6 MB 1.6 MB -1.3 kB
JS gzip 510.0 kB 509.5 kB -490 B
npm tarball 615.4 kB 612.6 kB -2.8 kB
npm unpacked 2.2 MB 2.2 MB -9.7 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 25.4 ms 25.4 ms -0.0 ms
CLI --help 52.2 ms 52.5 ms +0.4 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/record-trace-recording.js +2.1 kB +522 B
dist/src/internal/daemon.js -1.5 kB -498 B
dist/src/apps.js -656 B -217 B
dist/src/tv-remote.js -667 B -206 B
dist/src/cli-help.js +363 B +139 B

@thymikee

thymikee commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Review: does this fix #1162 correctly?

TL;DR — yes for the reported repro, and it's well-tested and green, but it's a scoped fix that closes the exact reproduction rather than the general "valid MP4 of the wrong surface" class the issue describes. The gate is built on session state (appBundleId), not on the actual foreground app, so two narrower paths can still produce a wrong-surface recording. Worth merging; worth being explicit that #1162 isn't fully closed by it.

What it fixes, and why it works

The repro hinges on a fact that's easy to miss: open … --session X returns DEVICE_IN_USE before it persists any session (session-open.ts:524-542), so X never exists in the store. record start --session X then falls into the !session branch of handleRecordCommand, which synthesizes a recordOnlySession: true session and lets simctl recordVideo capture the whole simulator — i.e. the runner.

The two new guards both land on this:

  • Handler guard (record-trace-recording.ts, isExplicitSessionRequest) — rejects an explicit --session that resolves to no session. This is the one that actually catches the reported repro. ✅
  • Backend guard (record-trace-recording-backends.ts, iOS-sim start) — requires an appBundleId for non-record-only sessions. Defense-in-depth for a persisted-but-app-less session. ✅

Tests cover both, plus the still-supported record-only path and the metadata. CI is green.

Residual gaps (the part I'd flag)

  1. Stale foreground still slips through. The gate trusts activeSession.appBundleId as a proxy for "the right app is on screen," but record start never queries the actual foreground. Sequence: open Settings succeeds (appBundleId=Settings) → app crashes / backgrounds → runner returns to foreground → record start passes the guard and records the runner. Narrower than the original bug, but it's the same false-proof failure mode.

  2. Record-only mode (omit --session) still captures the runner with no warning. That path is unchanged, so on a busy device where the runner is foregrounded you still get a "valid MP4 of Agent Device Runner." Arguably by-design (opt-in), but issue point 3 explicitly asked to warn/fail when the foreground app is Agent Device Runner — that check doesn't exist for record-only.

  3. The runner-bundle check guards a near-impossible state. isAgentDeviceRunnerBundle(activeSession.appBundleId) only fires when a session's recorded app is the runner, but open sets appBundleId to the target app, never the runner. So it's inert in normal flows — the real protection is the missing-session + missing-app guards, not this. Fine as belt-and-braces, just don't count it as the runner defense.

  4. Issue iOS Simulator record can return a valid MP4 of Agent Device Runner after target app open fails #1162 asks for more metadata than this adds. The PR adds recordingBackend, recordOnlySession, activeSessionApp, durationMs. Still missing from the issue's list: foreground app/bundle at start and at stop (the actual proof-of-surface fields), dimensions, frame count, chunk count/paths. The PR body scopes to a subset, which is reasonable — just calling out that the issue isn't fully satisfied.

Closing (1)–(2) really needs one simctl/AX foreground query at start (and ideally at stop) compared against the expected bundle, warning/failing when it's the runner. That's the piece that turns this from "reject the known-broken path" into "guarantee the surface." Reasonable as a follow-up.

Minor

  • isExplicitSessionRequest (record-trace-recording.ts) is a byte-for-byte duplicate of hasExplicitSessionFlag in session-routing.ts:56-60. Export and reuse the existing one so the "what counts as an explicit session" rule stays single-sourced.
  • The handler guard runs after ensureDeviceReady(device), so a doomed explicit-missing-session still boots/readies the simulator before failing. Cheap to move the check above the readiness call.
  • The handler guard is platform-agnostic, so it also changes android (record start --session missing now rejects instead of whole-screen record-only) and web (already rejected, just earlier). For android that's a consistent improvement; for web it's a no-op in outcome. Broader than the iOS-sim title — intended, but note it.

Net: good, safe, well-tested fix for the filed repro. I'd merge it, but I'd either re-scope #1162 to "explicit-session fallback" (and open a follow-up for real foreground verification) or note in the PR that the foreground-proof guarantee and the remaining JSON fields are deferred.

@thymikee thymikee force-pushed the fix/ios-simulator-record-session branch from 5dfc715 to ecfe909 Compare July 8, 2026 18:53
@thymikee

thymikee commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Review pass on ecfe909: checked the record start/stop route against issue #1162. Explicit missing sessions now fail before record-only capture, existing iOS simulator sessions require active app context, Agent Device Runner is rejected as the active app, and record-only simulator capture remains available without an existing explicit session. Metadata is added to start/stop responses, tests cover the reported path, and the PR includes live iPhone 17 simulator evidence. Checks are 21/21 green. No actionable blockers found.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Jul 8, 2026
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-08 19:48 UTC

@thymikee thymikee changed the title fix: reject recording for failed iOS simulator session fix: make record app-scoped by default Jul 8, 2026
@thymikee

thymikee commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Reconciled the earlier scope concern in 6288138.

record start now defaults to --scope app and requires an active app session from open <app>, so the old missing-session fallback cannot silently produce a whole-simulator MP4. Whole-screen recording is now explicit via --scope device/--scope system; that keeps session names available for multi-agent isolation without using “session was supplied” as the heuristic for whether full-screen capture is intended.

I also verified the revised behavior live on an iPhone 17 simulator:

  • missing session + default app scope fails with INVALID_ARGS
  • missing session + --scope device succeeds as record-only full-screen capture
  • open settings + default record succeeds as app-scoped capture with activeSessionApp.bundleId = com.apple.Preferences

The remaining caveat is intentional for this PR: app scope is enforced from tracked session state, not a new iOS foreground-bundle probe.

@thymikee thymikee force-pushed the fix/ios-simulator-record-session branch from 6288138 to 5364abc Compare July 8, 2026 19:34
@thymikee thymikee merged commit 8889841 into main Jul 8, 2026
22 checks passed
@thymikee thymikee deleted the fix/ios-simulator-record-session branch July 8, 2026 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

iOS Simulator record can return a valid MP4 of Agent Device Runner after target app open fails

1 participant