fix(settle): compare post-action trees in one view across every route - #4500

Size Report

Startup median (7 runs, lower is better):

Apex Sentinel · ✅ Approve

Since the last review: 2 new · 0 still open · 1 no longer raised (1 marked fixed)

2 finding(s): 2 minor.

🟡 Minor (2)

Reviewed with this repository's own Sentinel configuration (efe6bb9eece4).

Earlier reviews

CI is green on 6e5ada8 (22 checks, none failing), and there are no conflicts. The fix still needs a live Android run before merge.

The round-4 algorithm lists added lines from a full-tree baseline against the settled interactive view (settle.ts#L316). That is only sound if the Android interactive view adds no node of its own, and no device run shows this. The unchanged-dialog run in the PR body used a79152d, the discarded round-1 algorithm. The head run is a navigating sequence (+5 -0), and the later Back run is a selector press that never carries fullView. If full and interactive nodes differ in any compared attribute (text, flags, rect rounding), an unchanged dialog will still print false +N lines with refs. The headline behavior is therefore unobserved at this SHA. Please run this on the Android emulator at 6e5ada8: open Settings > Device name, run snapshot -i, then snapshot, then press @eN --settle on a ref that does not dismiss the dialog (the status-bar clock, as in the issue). Report the exact settle line, diff.lines and hint. Expected: +0 -0 (or explain any added lines), the full-tree hint, and no wrapper removals. Then press a second @ref right after to show the flag cleared (no hint). That second press must be a @ref press, not a selector. Please record the SHA, device and commands in the PR body.

Not blocking, take or leave: (1) the same full-vs-interactive mismatch has two more baseline producers that are not marked, selector preAction (captured with interactiveOnly false in selector-read-shared.ts:86) and the daemon generic-settle storedTreeBaseline for scroll/back, so both still print wrapper nodes as removed, and test 4 only asserts removals > 0, which holds for false and genuine removals alike; either derive fullView once from snapshot.presentation in surfaceScopedNodes so every producer is covered, or note that selector and scroll/back stay open and file a follow-up. (2) The PR body says commands.md documents the iOS row-rewrite limit, but the paragraph at website/docs/docs/commands.md:497 does not, so please add one sentence that iOS may still list rewritten rows as added, or fix the PR body; issue #4486 lists an iOS 27.2 Simulator and no iOS run is reported. (3) Forcing summary.removals to 0 prints -0 and reads as a fact to JSON/MCP consumers, so a typed removalsOmitted field would be clearer, and baseline.fullView is checked truthy in buildSettleDiff but === true in readSettledOutcome. (4) FULL_VIEW_BASELINE_HINT says later settles diff against the settled interactive view, but the ref frame only advances when a settle issues at least one ref, so a settle with an empty diff leaves the full tree as the baseline; consider softening the wording.

For the open inline threads: the iOS row-rewrite one still applies as a non-blocking note (#4500 (comment)). The other review threads are fixed at this head or benign, so the author can resolve them: #4500 (comment) (production always sets presentation, only fixtures omit it), #4500 (comment), #4500 (comment), #4500 (comment), #4500 (comment), #4500 (comment), #4500 (comment), #4500 (comment), #4500 (comment) (all fixed at head), and #4500 (comment) (truncated is measured after removals are dropped, by design).

This review read the code only. I did not run the unit tests or any device, and the live table in the PR body was not reproduced. The SHA attribution of its rows is taken from the PR text, and the iOS behavior is taken from the code comment and the thread.

Thanks, the live run was the right ask: it disproved round 4.

Live run at 54438311cb (round 4, additions only). Pixel 9a API 37 emulator, Settings › About › Device name dialog. Commands: snapshot -i, snapshot --force-full, press @e3 --settle (status-bar clock). With the keyboard up, the settle reported +43 -0: every IME key read as added. The interactive capture reads the input-method window, and the full capture does not, so additions against a full tree are not sound. Your premise check was correct.

New design at 3b6a9f7d5a. When the ref frame was read as a full tree, the settle loop captures the full tree too. Both diff sides use one view, and the issued refs are full-tree refs, matching what the agent last read. Interactive ref frames and selector presses keep the interactive view. The additions-only filter, the removalsOmitted flag, the formatter change and the full-view hint are all gone, which also settles (3) and (4).

Live at head, same dialog with the keyboard up:

• press @e3 --settle → settled after …: +0 -0 (~17 unchanged), diff.lines: [], no hint, tail of 16 full-tree refs.
• press @e142~s972613 --settle (dialog title, pinned from that response) → +0 -0 (~17 unchanged), diff.lines: [], no hint. The stored settle capture is itself a full tree, so later ref settles stay full against full.

Non-blocking notes:

1. The selector preAction and generic scroll/back baselines are unchanged and noted as open in the PR body. I tried to file a follow-up issue, but the tool permissions in this session blocked it, so please file one or ask me once allowed. Test 4 now asserts that the genuine Device name removal is listed and that the loop stayed interactive.
2. The iOS row-rewrite caveat no longer applies to this route, which compares full against full. The PR body no longer claims it.
3. Moot: no forced removals: 0 remains.
4. Moot: the hint is removed.

Tests: settle-view-baseline.test.ts (both full-frame cases fail on base). The interaction, command-output and capture-kit suites pass (1837 tests), and typecheck and check:quick pass. The PR body has the device, SHA and commands.

I reviewed 3b6a9f7 against the earlier findings. The dropped settle hint, the truncated count after removals, and the additions-only filter with the iOS row-rewrite caveat are all fixed at this head. Three open review threads still apply and should be fixed before merge (below).

Not blocking, and you can take or leave these: the sentence at commands.md:497 says a --settle action after a full snapshot compares one view, but snapshot --depth, --scope and --raw frames take the same route and still diff against a different projection, so either qualify the sentence or carry the frame's presentation into the settle capture. Also, the "Still open" paragraph in the PR body says selector presses resolve on a full capture, but press and click request the interactive tree first and fall back to full only when the node is missing, so please reword it and file the follow-up issue for the selector fallback and the scroll/back baselines.

On the open inline threads: these three should be fixed before merge: the selector test premise, the settle tail comment and depth/scope/raw frames. The two comment-drift threads also still apply. These three no longer apply and can be resolved: row-rewrite caveat (full is now diffed against full), truncated count (removals are no longer dropped), and dropped hint (the hint is spread directly).

I did not run a device or the tests. The Android live table, the 1837-test pass and the typecheck come from the PR body and author comment, and the regression reasoning comes from reading the base code. Nothing covers iOS, where the loop now captures a full tree per poll after a full snapshot, or --settle --verify on a full ref frame. After a full snapshot, a --settle on Android also no longer shows keyboard chrome in the diff.

CI shows 21 of 22 checks passing, and the remaining one is an external review check, still in progress. Before merge, please fix the three threads above and file the follow-up issue.

This PR is ready at 0149a53. The two problems from the earlier review (#4500 (comment)) are fixed: the full-view check now excludes -s and --actions frames, and the tests cover both. All 22 checks pass at this commit. There are no conflicts, and nothing else must happen before merge.

Not blocking, and you can take or leave it: the wholeFullTree predicate at https://github.com/callstack/agent-device/blob/0149a53/packages/command-runtime/src/commands/interaction/runtime/settle.ts#L271 spells out the five presentation option keys by hand, while SNAPSHOT_PRESENTATION_OPTION_KEYS and buildSnapshotPresentationKey in kernel/snapshot.ts already own that list. A sixth presentation key would then wrongly count as a whole full tree. Comparing snapshot.presentationKey with buildSnapshotPresentationKey(undefined) would avoid that, if the fixtures set presentationKey.

Of the earlier inline threads, the five earlier review threads and the two at #4500 (comment) and #4500 (comment) are fixed at this head, so you can resolve them. These two do not apply, so please resolve them too: #4500 (comment) describes an earlier head, and the stored snapshot baseline is now the plain one as on main. #4500 (comment) is a limit the PR body already documents (point taps and scroll/back --settle stay open), not a regression.

I did not re-run the Pixel 9a live runs, and I did not run the new tests locally. The live runs in the PR body are at earlier commits, and this change only narrows which frames get the full view. -d and --raw are not parametrized in the tests and rely on the shared predicate.

I redid this PR as you suggested, so the view rule is consistent across every post-action route instead of only ref presses.

Rule. Compare two trees only when they were read in the same presentation (-i, -d, -s, --raw, --actions).

• isSameSnapshotPresentation (kernel) states it.
• SurfaceScopedNodes.presentation carries each baseline's view.
• postActionBaseline in capture-kit/snapshot-baseline.ts, already the single owner of settle baselines for both routes, picks the baseline in the agent's view, which is the presentation of the ref frame it last read. A candidate read in another view is skipped, and the frame answers.

Routes.

• Touch commands (refs, selectors, coordinates) and the daemon's scroll/back route pick their baseline through postActionBaseline, keeping main's order of preference. The settle loop captures in that baseline's presentation.
• --verify captures in the pre-action tree's view. With --settle --verify, it compares only trees from one view.
• diff snapshot diffs against the stored tree or the ref frame read in the requested view, else it initializes the baseline.

Live, Pixel 9a API 37, at 1f15eb0b4b.

Production diff: 10 files, +170 −60. The earlier ref-only marking, the tail and hint special cases, and the fullView flag are all gone.

The review findings are fixed at 5b2c4cd685:

• Baseline selection loads the presentation comparator on demand, preserving the existing eager closure budget.
• Settle fixtures preserve depth/raw/custom-action options and assert the complete unchanged summary.
• wait stable keeps its requested scope/depth/raw options.
• A ref frame reused by the generic route drops unverified presented-surface facts.
• diff snapshot compares only with the latest stored snapshot; a different view initializes a baseline. The proxy and Android provider scenarios now assert this behavior.
• An absent presentation means the default full tree, including public backends that omit this optional metadata.
• Verification retains known surface transitions even when node presentations differ. The docs distinguish standalone verification from settle plus verification.

Independent review completed; all three findings are fixed. In an isolated checkout of 1f15eb0b4b, the extended tests produce seven intended failures (52 pass). At head they pass; the focused suite including eager closure guards passes 962 tests. Both formerly failing provider scenario files pass (13 tests). The disposable checkout is removed.

The four live sequences in the PR body were repeated on exact base efe6bb9eec and head 5b2c4cd685, using separate source daemons and the T3-bound command. Base reports 10 false removals for both press and scroll, and 19 for the snapshot view switch. Head reports zero, with diff.lines: [] for both settles; the unchanged verification control remains false. Both captures use android-helper 0.21.38-dev. Sessions and daemons are closed, the emulator is shut down, and the comparison checkout is removed.

The previous Android smoke failure was selector_not_found while reading field-email after typing. Its subsequent failure snapshot contains that field. That alone does not establish an infrastructure cause; the new head must pass the Android lane.

Local validation at 5b2c4cd685:

• The first affected receipt (exactCommit: true, checkoutChanged: false) failed in three subprocess timing cases: Apple teardown timeout; helper-exit privacy timeout; missing helper status after a 600 ms executor timeout.
• The same pnpm run check:unit on pristine base efe6bb9eec failed in a different five-second subprocess timeout (platform-runtime-android-adb-host.test.ts:106). This does not prove the cause of the head failures.
• The same three affected test files pass on both commits: 88 tests each.
• One fresh affected attempt passed the full unit suite (2,099 files; 19,726 passed, three skipped) and leak check, then failed smoke-daemon-clean.test.ts:30 when the cleanup subprocess exceeded 5,000 ms. Its receipt remains failed, with exactCommit: true and checkoutChanged: false.
• Standalone pnpm run test:smoke passes on both pristine base and head (76 passed, six skipped each). Cause remains unproven; no timeouts, worker settings, skips, or test selection were changed.
• Related integration passes 88 files / 406 tests at head. CI must validate the complete pushed head before merge readiness; no whole-gate local pass is claimed.