Repository navigation
feat(ios): add observe-only snapshots without foreground repair - #3108
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 28 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
At 40d0b19, the Smoke Tests failure is caused by this PR, and the live simulator evidence is still missing. The new The PR body describes a live simulator sequence on the exact route, but it attaches no output (https://github.com/callstack/agent-device/blob/40d0b19/website/docs/docs/snapshots.md#L47). Without it, nobody can check that the refusal left the app in the background, or that the runner session id stayed the same across probe, capture and refusal. Please attach the run's JSON or daemon-log output. It should show: (1) an observe-only success with To fix CI, please capture both observe-only requests (the uptime probe and the snapshot) in the interactor producer test through Not blocking, and you can take or leave these: (a) the success rule should be that the observation proves a non-nil pid at preparation and the same pid after capture, since Could admission become one declared snapshot requirement (for example I did not run the Swift or Vitest suites, and I did not trace adopted runners from other versions. |
|
The earlier gaps are mostly closed at d848493, but one blocker remains. The missing The remaining problem is that the observe-only path has no live evidence. This path adds a Swift runner command field and a TS runner-lifecycle gate. It runs an uptime probe, then a snapshot pinned to the probed I did not run the Swift RunnerTests or the TS runner-requests tests. The fixture conclusion comes from reading the code. After you attach the output, please also wait for Smoke Tests to finish green. |
|
Fixed the macOS request-vocabulary failure in Local before/after proof: the exact macOS workflow XCTest invocation went from 279 tests / 1 missing- All checks on final head One unchanged synthesized-text pacing assertion failed in iOS run 36894206456. The same implementation/test passed on recent main run 36887001093, and the content-identical retry run 36897563973 passed that assertion and all 79 targeted iOS XCTests. No typing implementation or test was changed. The failed-job rerun API required admin rights; |
|
At d848493 this branch now conflicts with main after today's merges. Please rebase onto main. The earlier review findings still apply; I will review the rebased head. |
|
@thymikee no problem |
d848493 to
5a3ea91
Compare
|
The runner-side gap from the earlier review at d848493 is fixed at 5a3ea91. The observe-only context now returns after the SpringBoard and system-surface routing, so that thread no longer applies. One finding remains, and it is about missing live evidence. The live iOS simulator output asked for in the earlier review is still not attached. The TS tests use a fake runnerProvider that supplies runnerSessionId and the observation itself (interactor-target-activation.test.ts:58-85). So nothing shows that the real XCTest runner advertises Of the open bot threads, the P1 on a pid that is nil on both sides (r4157930686) and the P2 on iPad refusal (r4157930727) still apply, as do the P2 on the progress-model label (r4157930719) and two P3 threads. Seven threads do not apply, so please resolve them. r4157930638 and r4157930705 cover admission guards for unsupported cases, which are a different class from unavailable observation and are documented as unsupported. r4157930673 fails closed after one probe. r4157930695 belongs to the existing session pin, and any runner that receives the request still gets I read the code at 5a3ea91 and did not run the Swift RunnerTests or Vitest. I did not trace which provider runtimes bind through All 16 checks pass at 5a3ea91, including the iOS smoke and macOS XCTest lanes that exercise the changed runner requests. Before merge, please attach the simulator output described above. |
Live simulator evidence for
|
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Turn on auto-fix | Re-trigger cubic
|
I pushed four commits on top of ad853d0 to finish this PR:
All review threads are resolved, and CI passes at 1e75ce4. |
|
The code at 1e75ce4 fixes the earlier gaps, but the live evidence for the new pid check is still missing. All 16 checks pass at 1e75ce4, including the iOS smoke and macOS XCTest lanes that build the changed runner. Thanks for the quick follow-ups. The iPadOS wording in all four refusal messages, the nil-pid guard, the shared handheld-device check, the progress-model exclusion, the docs row and the named-command test assertions are now in place. The success rule in RunnerTests+SnapshotExecution.swift changed in ad853d0. An observe-only capture now succeeds only if The six cubic-dev-ai threads are fixed at 1e75ce4, so the author can resolve them: iPadOS refusal messages, nil pid guard, handheld device check, progress model exclusion, observe-only docs row and named command assertions. Not blocking, and fine to take or leave: the new iPadOS admission at snapshot-runtime-binding.ts and in I did not run Vitest or the Swift RunnerTests. The claim that the runner-exchange test fails on old code comes from reading Before merge, the live simulator run of steps (1) and (2) above must show the new pid guard accepts a real foreground app. |
|
I pushed What changed. The capability probe ( Validation at
Not run: physical device. |
There was a problem hiding this comment.
All reported issues were addressed across 14 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
Thanks for the update. The two earlier concerns from 1e75ce4 are addressed in e3a9b87, and CI is green (16 of 16 checks). One live-validation gap remains. The change at runner-lifecycle.ts:284 removes the guard that refused observe-only when the runner was not 'ready'. An observe-only snapshot now goes through The rule to prove is that an observe-only request never changes the foreground app on any route that reaches the runner snapshot: reused runner, cold boot, and preflight-driven restart. The live run at e3a9b87 covered only the warm, reused runner. Could you run the CLI built at e3a9b87 on an iOS simulator with The cubic-dev-ai docs thread still applies: interactor.ts:293-296 still throws COMMAND_FAILED with reason 'observation-unavailable', and snapshots.md lines 55-57 do not name that refusal. See #3108 (comment). I reviewed only the changes from 1e75ce4 to e3a9b87. I did not run Vitest, the Swift RunnerTests, or a simulator, so the live and Swift results come from the maintainer's comment. I also did not check whether a runner built by an older CLI can be reused after an upgrade. If it can, that runner ignores observeOnly and activates the app before the host refusal at interactor.ts:293. Before merge, we need the cold-boot simulator evidence above and the docs sentence from the cubic thread. |
|
Pushed Cold-boot and restart evidence at e3a9b87 (same runner and host code as Route B, preflight-driven restart (terminate, then Response: Route A (terminate, wait 8 s, then observe-only): the daemon had already recovered the session, so the request shows So a runner boot does not move the foreground app, and the ready-runner refusal stays out. The docs sentence "the screen is left exactly as it was" holds on the reused, restarted and freshly started routes. On the older-runner question: Simulator, daemons and state dirs were cleaned up. |
|
This PR is ready at 16e58a0. The earlier findings from e3a9b87 are fixed: the docs at https://github.com/callstack/agent-device/blob/16e58a0/website/docs/docs/snapshots.md#L57-L59 now name the host COMMAND_FAILED refusal with details.reason 'observation-unavailable', thrown at https://github.com/callstack/agent-device/blob/16e58a0/packages/platform-apple/src/interactor.ts#L295-L297. The Swift code has not changed since e3a9b87. Not blocking: The cubic-dev-ai thread on the snapshots.md docs (#3108 (comment)) no longer applies, because 16e58a0 now names the refusal. Please resolve it. The iOS Smoke Tests job failed in testObserveOnlySnapshotRefusesBackgroundWithoutActivationOrDisclosure, a test this PR adds. The failure looks like setup flakiness, not a break in the refusal contract. The The second Smoke Tests job is still queued, and there are no conflicts. Before merge, the iOS Smoke Tests job must pass. Please rerun it, and guard the wait at line 73 ( |
|
This PR is ready at 16e58a0. The two issues from the earlier review (#3108 (comment)) are fixed: the snapshots doc now names COMMAND_FAILED with reason observation-unavailable, and the unsupported-target refusal now says "supported on iOS and iPadOS only". Not blocking: the setup check at https://github.com/callstack/agent-device/blob/16e58a0/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandDispatchTests.swift#L73 uses The two cubic-dev-ai threads no longer apply, so please resolve them: the snapshots.md note (#3108 (comment)) and the unsupported-refusal wording (#3108 (comment)). The iOS Smoke Tests failure is likely tied to this PR. The failing test, testObserveOnlySnapshotRefusesBackgroundWithoutActivationOrDisclosure, is new here. It failed in its setup, not in the refusal path: the Home wait at line 73 timed out with the app still reported as not in the background. The same Swift code passed all checks at e3a9b87, and the sibling Home-press test passed in the same job. This points to simulator state, not the new behavior. No conflicts are known. I did not run Vitest, the Swift RunnerTests, or a simulator. The cold-boot and restart evidence comes from the maintainer's run at e3a9b87, which has the same code as this head. I did not trace every lease-adoption path through verifyLeaseArtifactFingerprint for runners built by an older CLI. Physical devices and iPadOS have not been run live. Before merge, guard the setup wait at line 73 and get a green iOS Smoke Tests run on the new head. |
Squashed from the PR callstack#3108 history through 1e75ce4 and rebased onto main. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
An observe-only snapshot now sends the `observeOnly` field on the snapshot request alone. The runner enforces the no-activation contract and returns `observation`; the host asserts that shape on the response. The separate `uptime` capability probe, `supportsObserveOnlySnapshot`, the ready-runner-only rule, runner session pinning and the readiness-preflight exemption existed to survive a CLI and runner that disagree about the field, which the repo ships together and already guards through the protocol fixture. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Rebased onto main at bf22464 (the branch was 34 commits behind and carried a merge commit). Head is now
The only conflict was Validation at |
16e58a0 to
10bc4f7
Compare
|
The earlier conflict is gone, and the cold-boot, restart and warm evidence from e3a9b87 covers the runner-readiness change. One defect remains at 10bc4f7, and the checks look unrelated to this diff. Observe-only snapshots still reach the SpringBoard system-modal probe in snapshotFast and snapshotRaw (line 289), both called from executeSnapshotPrepared. Suppose a permission prompt is over the session app. boundedBlockingSystemAlertSnapshot then returns SpringBoard's alert nodes instead of the app tree, with no systemSurface provenance. The post-capture check at RunnerTests+SnapshotExecution.swift:114-127 only re-reads app state and pid, which have not changed, so it stamps the payload as an observation of the session app. This contradicts the snapshots.md line saying observe-only does not substitute SpringBoard or a presented system surface. The earlier P1 thread on this path was resolved because routing returns early, but the substitution happens in the capture tier. The rule should be: a successful observe-only response contains only nodes captured from the observed session app, and every capture tier that can return a non-app payload must refuse with OBSERVATION_UNAVAILABLE when observation is set. Today that is the probe both entry points run, so please enforce it once in executeSnapshotPrepared, or pass the observation into both. Please add a Swift unit test that sets systemModalProbeOverrideForTesting to return a payload with the app in the foreground and expects OBSERVATION_UNAVAILABLE. It should fail on the current code. If you would rather return the alert, please drop the docs and PR-body sentence and stamp systemSurface provenance on that payload. Not blocking, and you can take or leave these: the cliDetail text in src/commands/capture/snapshot.ts:79 still says "an already-ready compatible runner", which e3a9b87 removed, and the inputDescription at flag-definitions-workflow.ts:283 says "iOS only" although iPadOS is admitted. On the open threads, the SpringBoard probe thread still applies. The snapshots.md refusal thread (r4211182132) is fixed at this head, because the section now names the COMMAND_FAILED observation-unavailable refusal; please resolve it. The interactor guard thread (r4199054861) is fixed too, because the guard is gone and the remaining refusals name iPadOS; please resolve it. Coverage fails in packages/platform-apple/src/snapshot-source/lifecycle.test.ts, where a bridge-request-deadline timer fired. This PR touches no snapshot-source file, and observe-only bypasses the bridge route at runtime-snapshot.ts:43, so it looks like a timing flake and a re-run should clear it. There are no conflicts. I did not run Vitest, the Swift tests or a simulator for this review. The live evidence is from the run at e3a9b87, and I checked that no upstream commit since then touches the snapshot route. I could not confirm live whether XCUIApplication.state stays runningForeground while a SpringBoard alert is up. If it reports another state, the post-capture check already refuses and this path is safe. Before merge, the capture tier needs to refuse the SpringBoard payload, or the docs need to change, with the Swift regression test in place. |
What changed
Adds
snapshot --observe-onlyfor an existing local iOS app session. Unlike ordinary snapshots, this mode refuses rather than activating the app or repairing foreground state. Default snapshot repair andtargetActivationdisclosure are unchanged.The runner must already be ready. A non-activating capability probe pins the subsequent capture to that exact runner session; missing support, a replaced session or missing/contradictory observation metadata fails closed. There is no runner startup/recycle, activating fallback or system-surface substitution in this mode. Other platforms, provider-owned sessions and diff are rejected.
Successful
data.observationis exactly:{ "mode": "observe-only", "activationPerformed": false, "appState": "runningForeground", "appStateSource": "xcuiapplication-state" }This is sourced XCTest state, not verified foreground or screen ownership. The native path requires the foreground state report before/after capture and an unchanged process identity. Observe-only never emits
targetActivation. Native unavailability isOBSERVATION_UNAVAILABLE; host/capability refusals useCOMMAND_FAILEDwithdetails.reason: "observation-unavailable".Review notes
This is the opt-in prevent complement to #2682's disclose behavior. It does not fix #2694's ordinary interaction disclosure gap or infer ownership from AX PID ordering. #2696 remains the reason not to treat XCTest state as a screen-ownership guarantee. Stronger ownership, observe-only settle, early response history, inline screenshot reuse, private Android comparison and prerelease version codes remain separate follow-ups.
Validation
Original feature validation on
40d0b191c01c8b458f41817552cc9a22d2ebd5c2:corepack pnpm checkpassed: 1,457 Vitest files / 11,973 tests passed, one existing skip, plus the tooling/fallow checks.corepack pnpm check:affected --runpassed its local selected set. Selection included Apple/XCTest source checks; GitHub-authoritative and parked device lanes were explicitly skipped by the selector, not counted as local passes.corepack pnpm build:xcuitest:iosandcorepack pnpm build:xcuitest:macospassed, using isolated derived-data paths. The iOS unit-test-enabled build also passed..xcresultbundle.Protocol fixture follow-up and final CI
The initial macOS host gate found that
Command.observeOnlyhad no production request in the protocol fixture. Added real interactor producer sites for both the non-activating uptime probe and observe-only snapshot. Fixture rows come from the producer helper's captured-request diff; there is no separate generator. The drive also asserts the host-onlyexpectedRunnerSessionId, which is not sent as a Swift request field. The other new Swift fields are responses/context, not missing request vocabulary.Fix commit:
f14ab3589. Final pushed head:d8484932c75a460a5c5a2798b980b86aacb71e15; the latter is a CI-only empty retry with the identical source tree.platform=macOS,arch=arm64, skipping onlyRunnerTests/testCommand): 279 passed locally; the source-derived execution-count reporter also passed. Local ad-hoc signing resolved the original unsigned-bootstrap limitation.corepack pnpm check: 11,973 passed, one existing skip.corepack pnpm check:affected --run: local selected gates passed on the final pushed head.An untouched iOS synthesized-text pacing assertion failed once. The same implementation/test passed on recent main run 36887001093; the identical-source retry run 36897563973 passed that test and all 79 targeted iOS XCTests. No typing behavior/test changes were made. GitHub required repository admin rights for a job rerun, hence the CI-only empty commit.
Limitation: no physical-device run.
Refs #3106