Skip to content

DRAFT fix(settle): arm the confirmation on the dismissed surface, not on screen replacement - #3570

Draft
thymikee wants to merge 2 commits into
fix/3470-settle-quiet-per-capture-backendfrom
fix/3470-presented-surface-arming
Draft

thymikee wants to merge 2 commits into
fix/3470-settle-quiet-per-capture-backendfrom
fix/3470-presented-surface-arming

Conversation

@thymikee

@thymikee thymikee commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

Blocked by #3565. Closes #3470. Third layer of the #3470 stack: #3512 (merged) → #3522 → this PR.

Do not merge. This layer holds the narrowing's shape and its device evidence. It will be rewritten to read #3565's producer-stamped presentedSurface fact instead of the node-count rule below, and presentedSurface: 'unknown' will keep this parent's arming — for the trigger itself that is main's rule — so a producer that has not learned to answer degrades to #3512's behaviour and never to "no confirmation".

What this layer does

The layer under it (#3512) decoupled the confirmation from the requested window but left main's trigger in place: it arms whenever the post-action capture carries a complete viewport projection and shares under half its node identities with the pre-action tree. A tab switch or a push is exactly that — both screens project completely, and the rows of one tab are not the rows of the next — so ordinary navigation keeps paying the 1.5 s floor. That is #3470's actual complaint, and it is what this layer narrows.

The trigger becomes the pre-action tree showing a surface presented over app content, which an interactive capture reports as the surface alone. Live ground truth from the iPhone 18 Pro session below: a presented pageSheet projects 4 nodes and a system permission prompt 5 nodes under its own Alert root, while an ordinary screen of the same app projects 24+ nodes including its own nav or tab bar. Whether the surface was actually replaced stays per-capture.

Why it stays a draft

The rule keys on how the baseline was captured, not on what the screen is. Measured on device (#3512 review, iPhone 17, four reps per route, settle.quietMs/waitedMs as the signal):

route baseline it carries waitedMs @ --settle-quiet 150 confirms?
press 'label="Close automation sheet"' --settle interactive projection, 4 nodes 1833 / 1842 / 1917 / 2137 yes, late=0
snapshot then press @ref --settle full tree, 6 nodes 745 / 784 / 793 / 1096 no
back --system --settle over a stored full tree full tree, 6 nodes 732 / 737 / 755 / 763 no

The sheet is pageSheet; the full flavour carries the same surface as the interactive one plus a depth-2 duplicate of each label at the same rect, so 4 → 6 nodes crosses the bound of 5 and the verdict flips on the flavour alone. Route (b) needs nothing exotic — one extra label child in the app's own sheet flips it. Dismissal confirmation, which is the #1831 coverage this rule exists to keep, is therefore lost on 2 of 3 routes.

Re-measured at this head on the iPhone 18 Pro (BDA9B772), same scenario, three reps per route, the only variable being which branch's build was installed:

route parent layer (#3522's head) this head
selector press (interactive baseline, 4 nodes) 1855 / 1890 / 1862 1736 / 1708 / 1714 — still armed
snapshot then @ref press (full baseline, 6 nodes) 1854 / 1991 / 1856 531 / 524 / 522 — lost

Both runs were taken before the stack's restacks; the rule and the tests that pinned those numbers are byte-identical to the head they were taken at (git diff 183801de7 HEAD over stable-capture.ts, stable-capture.fixtures.ts and stable-capture.test.ts is empty), so they describe this head.

The generic-route back case gives the cleanest A/B, because there the loop's own diff says which shape arrived. When the arrival is broad (24 additions / 25 removals / 3 unchanged), the parent arms and this head does not, on the same device and the same build path:

build waitedMs / captures arrival the loop saw
parent 1840 / 5 and 1879 / 5 broad (24+/25-/3=) — confirmed
parent 561 / 2, 562 / 2, 578 / 2, 583 / 2 narrow (4+/3-/25=) — correctly not armed
this head 506 / 2 and 549 / 2 broad (24+/25-/3=) — not armed
this head 537 / 2, 549 / 2 narrow (4+/3-/25=) — correctly not armed

So the same broad arrival that costs the parent the floor's 1.5 s costs this head two captures: the confirmation is gone on a transition that genuinely needs it, not merely on navigation that does not.

Two caveats I want on the record rather than buried:

  • In these reps the dismissal still reached the right verdict (late=0, sheet gone). What the narrowing removes is the guard — the 1.5 s hold that catches a coherent-but-transitional tree — so the exposure is a wrong settled: true on a slower dismissal, not a wrong answer in this run.
  • back --system does not dismiss this pageSheet on the iPhone 18 Pro (the sheet is still up after the command, three independent runs), so the dismissal form of the reviewer's route (c) could not be exercised on this device; its numbers above are the reviewer's iPhone 17 measurement and I am carrying them, not confirming them. The generic-route numbers are mine.

The reviewer also measured what main's viewport-root arm keys on, on the same screens, both flavours: every iOS tree has an Application root at 0,0,402x874, presented or not, interactive or full. So restoring that arm for full-tree baselines re-arms the tab-switch and push cases exactly as at the merge base — it relocates the #3470 cost onto the routes that take a plain snapshot first, rather than fixing it. The chrome heuristic was considered and rejected for the same class of reason: it regresses modals that carry their own nav bar, relative to main.

The durable rule (#3565)

The producer stamps a typed presented-surface fact and this layer reads it:

  • runStableCaptureLoop takes the baseline capture rather than bare nodes;
  • both native producers stamp it (the simulator's interactive trees come from producer simulator-ax-bridge — apple/snapshot-bridge/*.m, which today reports no window or modality at all — and the runner has its own presentation code), plus the wire type and the cross-language golden table under contracts/fixtures/;
  • presentedSurface: 'unknown' keeps this parent's arming (i.e. main's trigger, decoupled from the requested window by fix(settle): arm the iOS broad-transition confirmation at any requested quiet window #3512), so the fact can only ever narrow the guard, never switch it off;
  • showsPresentedSurface, isBlockingSystemAlertRoot and the TINY_STABLE_TREE_NODE_COUNT arming bound all go — the rule becomes if (baseline.presentedSurface === 'absent') return null;, plus passing the baseline SnapshotState instead of bare nodes into runStableCaptureLoop.

Tests

The three navigation cases (#3470's tab switch, push, and push-onto-a-sparse-screen) are written as a table and are mutation-checked against the parent's arming: restoring the viewport-completeness arm fails exactly those three and nothing else in the file. The Alert-root case pins the one surface the projection names outright. Layer 2's window-resolution cases and layer 1's both-windows floor cases pass unchanged on this branch, which is the point: narrowing the trigger must not touch when the confirmation is owed once it is armed.

Two cases pin the finding so the rewrite is graded against it rather than re-argued. generic-settle.test.ts dismisses one pageSheet from an interactive baseline and from a full-tree read of the same screen and asserts both confirm for 1.5 s; it is marked test.fails, so the lane stays green today and vitest reports "expected to fail but passed" the moment #3565's fact makes arming flavour-independent — that red is the signal to drop the marker, and the assertion is the rewrite's acceptance test. It runs on the injected clock, so it costs no wall-clock time.

The provider-scenario dialog is left exactly as its parent has it. An earlier revision of this layer padded it past the arming rule's size bound, which quietly removed the only daemon-route check of the confirmation (the transition stopped being broad — 4 of 6 node identities survive — so waitedMs >= 1_500 had to become captures in 2..3). That padding is reverted: the flagship case asserts the 1.5 s floor again and all four provider-scenario cases pass on this rule, which is why settle-observation.test.ts is no longer part of this PR's diff.

pnpm format, pnpm lint, tsc -b clean.

pnpm check:affected --run --base fix/3470-settle-quiet-per-capture-backend at 06a4c5b86: all runnable checks passed (exit 0) — 296 related test files / 2,050 tests + 1 expected fail, Fallow clean on the changed files — held under the machine-wide lock, started 2026-10-13T13:31:39Z. The run at e43dd7abc was red on one thing only, the test-file-size ratchet, and that finding was correct: the parity case grew generic-settle.test.ts from 801 to 1,035 lines, past the 1,000-line tripwire. It is repaired by moving the three raw trees the case needs into __tests__/support/presented-sheet-trees.ts, which is how the suite stays at 963 lines without dropping an assertion; the case and the helper that drives the route stay where they were.

The rule and the fixtures behind the navigation table are byte-identical to a3dc159c8, the head every device measurement above was taken on (git diff a3dc159c8 HEAD -- stable-capture.ts stable-capture.fixtures.ts stable-capture.test.ts is empty), so those numbers still describe this head; the commit since then only adds test coverage.

Evidence required before this leaves draft

From the review on #3512 (#3512 (comment)). Once #3565's fact exists, run on an iPhone simulator with the RN test app, at least 4 reps each of:

  • (a) a selector press of close,
  • (b) a plain snapshot then @ref press of close,
  • (c) back --settle,
  • (d) a sheet with 6 or more controls — the flavour whose full-tree baseline is exactly what the node-count bound loses today, so it must be measured under the producer fact rather than inferred from (b),

each reporting settle.waitedMs >= 1500 and an empty diff snapshot -i taken 2 s later, plus one selector tab switch that stays near 800 ms — the #3470 cost this layer exists to remove, asserted rather than assumed. Plus the two route regressions the current rule fails: settleAfterInteraction with a full-tree selector-resolution baseline, and settleObservation over a stored full tree, both asserting waitedMs >= 1500.

The parity case in generic-settle.test.ts is the unit-level stand-in for those two route regressions; the device rows are what close them. The numbers in "Why it stays a draft" above are the before state on the count rule and are not this exit criteria met.

@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.18 MB 5.18 MB +79 B
Package (unpacked) 5.18 MB 5.18 MB +79 B
Package (download) 1.56 MB 1.56 MB +50 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.3 ms 26.3 ms -0.0 ms
CLI --help 75.6 ms 77.2 ms +1.6 ms

@thymikee

thymikee commented Oct 13, 2026 •

Copy link
Copy Markdown
Member Author

Gate — fix/3470-presented-surface-arming @ 76bbe2475

Supersedes the earlier gate comment on this PR, taken at 183801de7 before the stack's parents merged and this layer was restacked onto #3522's main-based head.

Local, held under the machine-wide lock, started 2026-10-13T10:59:53Z, against a clean tree.

pnpm install --frozen-lockfile                            exit=0
pnpm build                                                exit=0
pnpm format                                               exit=0  (tree clean afterwards)
pnpm check:affected --run --base fix/3470-settle-quiet-per-capture-backend  exit=0

All runnable checks passed: format:check, lint, typecheck, check:layering, check:fallow --base fix/3470-settle-quiet-per-capture-backend (No issues in 4 changed files), build, and vitest related over the 4 changed files — 296 files / 2,050 tests (unit-core, interaction-contract, provider-integration).

This layer's 4 files are byte-identical to a3dc159c8, the head every measurement in the body was taken on (git diff a3dc159c8 HEAD -- <those 4 files> is empty); the restacks moved only the parent.

Still a draft: the rule it demonstrates keys on how the baseline was captured, and the durable version reads #3565's producer fact. Do not merge.

@thymikee

Copy link
Copy Markdown
Member Author

I found two problems at 183801d, so this PR should not merge as it stands. CI is still pending: Smoke Tests is queued, and since this change alters iOS settle arming, a later failure there could not be ruled unrelated without its log.

The arming rule in showsPresentedSurface (stable-capture.ts#L455) depends on the baseline's node count (5 or fewer) or an Alert root. A baseline from a full-tree capture breaks this. Take a plain snapshot followed by press @ref --settle, or back --settle over a stored full tree. The same pageSheet has 6 nodes there, so broadTransitionBaseline returns null and the 1.5s dismissal confirmation never arms. A sheet with more than 5 controls fails the same way on every route. Your own device runs show it: 531/524/522 ms against the parent's ~1850 ms, and a broad 24+/25- back arrival also goes unarmed. So on 2 of 3 measured iOS routes, and on any sheet with more than 5 controls, --settle can report settled:true on a coherent tree captured mid-dismissal. That removes the #1831 guard on production routes. The rule the code must satisfy is that arming is identical for every baseline flavour (interactive, full, depth-limited, scoped) of one screen. Only a producer fact computed before projection can meet that. Please rebase onto #3565 and arm unless baseline.presentedSurface === 'absent', so 'unknown' keeps #3512's arming. Then delete showsPresentedSurface, isBlockingSystemAlertRoot and the count bound used as an arming rule.

The tests do not catch this. The daemon-route test in settle-observation.test.ts#L21 used a 3-node Continue/Cancel dialog and asserted waitedMs >= 1_500. Now the nodes are padded with SHARED_SCREEN_NODES and the assertion became captures in 2..3, so no provider-scenario test checks the confirmation through the daemon. The new unit tests feed broadTransitionBaselineNodes straight into runStableCaptureLoop, which skips resolveSettleBaseline, the step that picks the interactive or full flavour. Nothing in the suite can see the flavour regression. Please keep one daemon-route dismissal case asserting waitedMs >= 1_500. Please also add a parity case that dismisses the same presented surface from an interactive baseline and from a full snapshot baseline, and asserts both arm. It should fail at this head and pass once #3565's fact is read.

Could this layer skip the node-count classifier entirely? If #3565's typed presentedSurface lands first (computed before projection, carried through resolveSettleBaseline, refFrameSnapshot and planGenericSettleObservation, with golden parity under contracts/fixtures/), this change shrinks to if (baseline.presentedSurface === 'absent') return null; in broadTransitionBaseline, plus passing the baseline SnapshotState instead of bare nodes into runStableCaptureLoop. That would replace about 200 production lines with about 10, and the navigation table tests carry over with presentedSurface:'absent' fixtures. Is there a reason the count-based rule is needed beyond that?

I read the fixtures against the parent's arm but did not run the navigation table or mutation check. The device numbers are yours and I did not reproduce them. They support the finding rather than clear it. Before merge, #3565 must land, then this layer needs the rewrite above and the interactive-versus-full parity test.

@thymikee
thymikee force-pushed the fix/3470-settle-quiet-per-capture-backend branch from b78640a to e8bcff3 Compare October 13, 2026 10:41
@thymikee
thymikee force-pushed the fix/3470-presented-surface-arming branch from 183801d to 3a25bb2 Compare October 13, 2026 10:41
… on screen replacement

DRAFT / BLOCKED BY #3565 — do not merge. The rule below reads a property of the baseline CAPTURE,
and #3565 replaces it with a fact stamped by the capture producer. Kept as the reviewed starting
point and the device evidence, not as a shippable rule.

The layer under this one arms on a complete-viewport post-action projection plus a <50% identity
overlap. A tab switch and a push are exactly that: both screens project completely and the rows of
one tab are not the rows of the next, so ordinary navigation pays the 1.5s floor (#3470's whole
complaint). This narrows the trigger to the pre-action tree showing a surface presented over app
content, which an interactive capture reports as the surface alone: a presented pageSheet projects
4 nodes, a system permission prompt 5 nodes under its own Alert root, while an ordinary screen of
the same app projects 24+ including its nav or tab bar. Whether the surface was actually replaced
stays per-capture.

Why it is still a draft: on device the same automation sheet projects 4 nodes through an interactive
capture and 6 through a full one, so arming survives only on the route whose baseline happens to be
the interactive flavour. A dismissal preceded by a plain `snapshot`, or taken on the generic `back`
route, loses the #1831 coverage this rule exists to keep. The chrome heuristic was considered and
rejected: it regresses modals that carry their own nav bar against `main`. The durable rule is a
typed `presentedSurface` fact from the capture producer (#3565); reading it must keep this parent's
arming when the producer answers 'unknown', and the count bound and the Alert special case both go
with `showsPresentedSurface`.

The three navigation cases are mutation-checked against the parent's arming: restoring the
viewport-completeness arm fails exactly those three and nothing else.
@thymikee
thymikee force-pushed the fix/3470-settle-quiet-per-capture-backend branch from e8bcff3 to 84f170e Compare October 13, 2026 11:02
@thymikee
thymikee force-pushed the fix/3470-presented-surface-arming branch from 3a25bb2 to 76bbe24 Compare October 13, 2026 11:05
@thymikee

Copy link
Copy Markdown
Member Author

The rebase onto main at 76bbe24 leaves the two problems from the earlier review (183801d) in place, so this still needs changes before it can merge. The arming rule is still a node count, which gives different answers for different views of the same screen.

In stable-capture.ts, showsPresentedSurface arms only when the baseline has 5 or fewer nodes or an Alert root. The same pageSheet has 4 nodes in an interactive baseline but 6 in a full one (snapshot then press @ref --settle, or back --settle over a stored full tree). A sheet with 6 or more controls fails on every route. In those cases broadTransitionBaseline returns null and the 1.5 s dismissal confirmation never arms, so iOS --settle can return settled:true on a coherent tree captured mid-dismissal. That removes the #1831 guard on production routes, and the 52x ms timings in the PR table against the parent's ~1850 ms show it. The rule the code must satisfy is that arming is identical for every baseline flavour of one screen (interactive, full, depth-limited, scoped). Only a producer fact computed before projection can guarantee that. Once #3565 lands, could you rebase onto it and arm unless baseline.presentedSurface === 'absent', so that 'unknown' keeps #3512's arming? Then showsPresentedSurface, isBlockingSystemAlertRoot and the count bound can go.

In settle-observation.test.ts, the daemon-route dismissal case no longer asserts waitedMs >= 1500. It now asserts 2..3 captures over padded SHARED_SCREEN_NODES. The new unit tests feed nodes straight into runStableCaptureLoop and skip resolveSettleBaseline, which is the step that picks the baseline flavour, so nothing can see the regression above and it passes CI. Please keep one daemon-route dismissal case that asserts waitedMs >= 1500. Please also add a parity case that dismisses the same presented surface from an interactive baseline and from a full snapshot baseline, and asserts both arm. It should fail at this head and pass once #3565's fact is read.

For live evidence, after the rebase please run at least 4 reps each on an iPhone simulator with the RN test app. The routes are (a) a selector press of close, (b) a plain snapshot then a @ref press of close, (c) back --settle, and (d) a sheet with 6 or more controls. Each must report settle.waitedMs >= 1500, and an empty diff snapshot -i taken 2 s later. Please also include one selector tab switch that stays near 800 ms. The current table shows the state before this change on the count rule.

I read the code only. I did not run the suites or reproduce the device timings. #3565 is not merged yet, so I could not check how the rewrite will consume the fact. All CI checks were still queued or running when I looked, and the Integration Tests, Coverage and iOS Smoke Tests jobs overlap this change, so I can't rule out a failure there yet. Before merge, #3565 must land, then the arming rewrite, the two tests and the device runs above.

@thymikee

Copy link
Copy Markdown
Member Author

Confirming the disposition, taking the two test asks now, and answering your closing question. Head since: e43dd7abc (one commit, on top of the layer you reviewed; the rule and fixtures it demonstrates are byte-identical to what you read — git diff 183801d..HEAD -- stable-capture.ts stable-capture.fixtures.ts stable-capture.test.ts is empty for the rule and its fixtures, so your reading of the mechanism still applies exactly).

Stays a draft, and gets rewritten, not repaired. When #3565 lands, this layer becomes if (baseline.presentedSurface === 'absent') return null; in broadTransitionBaseline, with the baseline SnapshotState threaded through instead of bare nodes, and showsPresentedSurface, isBlockingSystemAlertRoot and the TINY_STABLE_TREE_NODE_COUNT arming bound deleted. presentedSurface: 'unknown' keeps #3512's arming, so a producer that has not learned to answer degrades to today's behaviour and never to no confirmation. The navigation-table cases carry over on presentedSurface: 'absent' fixtures.

The rebase you asked for is not possible yet: #3565 is still an open issue — no PR, no branch on the remote — so there is nothing to rebase onto and no presentedSurface field to read. That is the whole reason for the draft. I have not started a speculative version of the fact; the producer-side shape belongs to #3565.

Answer to "is the count-based rule needed beyond that?" — no. I kept it only because the fact does not exist yet, and the two heuristics that could substitute for it both fail the way the count bound does. Measured on the same screens: main's viewport-root arm is useless for full-tree baselines because every iOS tree has an Application root at 0,0,402x874, presented or not, so restoring it re-arms the tab-switch and push cases (the #3470 cost, relocated to whichever route took a plain snapshot first, not fixed); the chrome-classifier heuristic was rejected because it regresses modals that carry their own nav bar. A size bound and a root-type test are both read-after-projection proxies, which is exactly the property your finding attacks. Nothing argues for keeping the count.

Your line estimate understates how small the change is, which only helps the case: the rule in production code is 19 non-doc lines (broadTransitionBaseline, showsPresentedSurface, isBlockingSystemAlertRoot), not ~200. This layer's production delta is +46/−20 in stable-capture.ts; the ~200 you saw is mostly the new fixtures, which per your own note carry over.

Both test asks are in, and I measured them rather than writing them blind. Reproduced through the generic back --settle route on the injected clock, one pageSheet dismissed from the two flavours: interactive baseline (4 nodes) confirms for 1,502 ms; full-tree baseline of the same sheet (6 nodes) for 27 ms, settled: true either way.

  1. One daemon-route dismissal asserting waitedMs >= 1_500 — restored by deleting the padding rather than adding a parallel case. You are right that SHARED_SCREEN_NODES is what hid this, and the mechanism is worth stating exactly: padding the dialog to 6 nodes made the transition non-broad (4 of 6 node identities survive, a 0.67 ratio against the 0.5 bound; unpadded it is 1 of 3, i.e. 0.33), which is why the assertion had to become captures in 2..3 — the confirmation was not merely unasserted, it genuinely stopped arming. With the padding reverted, all four provider-scenario cases pass and the flagship case asserts the floor again; settle-observation.test.ts now matches its parent exactly, so it is out of this PR's diff entirely. That padding was mine, added to keep the fix(settle): iOS broad-transition confirmation fires on tab switches and pushes and is coupled to the default quiet window #3470 navigation cost story visible in that file, and it silently traded away the fix: derive iOS transition snapshots from visible presentation #1831 check — the review caught something the suite could not.
  2. An interactive-versus-full parity case that fails here and passes once the fact is read — added in generic-settle.test.ts, both flavours in one case, both asserted to confirm.

One deviation to flag, since it inverts your instruction. You asked for a case that fails at this head; as a plain test() it would have made #3570's CI permanently red, and a red lane on a draft blocks nothing except the next person's read of "is this layer green?". I marked it test.fails instead, so the lane is green now (reported as 1 expected fail) and vitest raises "expected to fail but passed" the moment #3565 makes arming flavour-independent — that failure is the signal to drop the marker, and the assertion is the rewrite's acceptance test. If you would rather have the honest red, say so and I will drop the marker in one line; I did not want to spend the shared macOS runner pool deciding that for you. Gate stays exit 0 either way (296 files / 2,050 tests + 1 expected fail, Fallow clean on 4 files, under the machine-wide lock at 11:58:40Z).

Evidence this will carry when it leaves draft — on an iPhone simulator with the RN test app, at least 4 reps each of (a) a selector press of close, (b) a plain snapshot then @ref press of close, (c) back --settle, (d) a sheet with ≥6 controls, each reporting settle.waitedMs >= 1500 and an empty diff snapshot -i taken 2 s later, plus one selector tab switch staying near 800 ms — the #3470 cost this layer exists to remove, asserted rather than assumed — and the two route regressions (settleAfterInteraction with a full-tree selector-resolution baseline, settleObservation over a stored full tree) both asserting waitedMs >= 1500. (d) matters specifically: it is the flavour the count bound loses today, so it must be measured under the producer fact, not inferred from (b). The parity case above is the unit-level stand-in for exactly those two route regressions; the device rows are what close it. Nothing in the current body's table is that evidence — those numbers are the before state on the count rule, and the body now says so.

And noted on your own verification boundary: the device rows here are mine and unreproduced, the navigation table and mutation check were read not run. The parity reproduction above is a unit run on the injected clock, not a device run — it establishes the arming divergence, not the wall-clock behaviour on a real dismissal.

…n, and pin the parity gap

The review on this PR found that padding the provider-scenario dialog past the
arming rule's size bound (SHARED_SCREEN_NODES) silently removed the only test
that checked the 1.5s confirmation through the daemon: the transition stopped
being broad (4 of 6 identities survive), so `waitedMs >= 1_500` had to become
`captures in 2..3`. Revert that padding, which restores the unpadded dialog and
its original assertion; all four provider-scenario cases pass on this rule.

Add the parity case the review asked for: the same presented sheet dismissed
from an interactive baseline (4 nodes, as `snapshot -i` projects it) and from a
full-tree read of the same screen (6 nodes), both asserted to confirm. Measured
on the injected clock, the interactive baseline confirms for 1,502ms and the
full-tree baseline for 27ms with `settled: true` either way — #1831's guard lost
on the route whose baseline happens to be the richer flavour. Marked `fails`, so
the lane stays green and the case turns red ("expected to fail but passed") the
moment #3565's `presentedSurface` replaces the count bound: that red is the
signal to drop the marker, and the assertion is the rewrite's acceptance test.
@thymikee
thymikee force-pushed the fix/3470-presented-surface-arming branch from e43dd7a to 06a4c5b Compare October 13, 2026 13:32
@thymikee

Copy link
Copy Markdown
Member Author

The red Coverage lane at e43dd7abc was mine, and it was not the test.fails marker — recording it so the two are not conflated.

What actually failed was scripts/__tests__/test-file-size-ratchet.test.ts:

src/daemon/__tests__/generic-settle.test.ts is 1035 lines, 801 at the merge-base:
a test file over the tripwire may not grow; split it along the source module it
mirrors (docs/agents/testing.md).

The requested parity case is a good test in the wrong file. That suite was already 801 lines and the 1,000-line tripwire lets an over-tripwire test file hold its size, not grow, so adding 134 lines to it was never allowed. The same run reports 1 expected fail among 14,754 tests and no other failure, which also answers the question the deviation left open: test.fails behaves identically in CI, so the marker is not what reddened the lane.

Fixed the way the finding names rather than by shrinking the assertions: the three raw trees the case needs (the sheet as an interactive capture projects it, the same screen as a full read, and the app screen a dismissal lands on) now live in src/daemon/__tests__/support/presented-sheet-trees.ts, and both the case and the back --settle helper that drives it stay in the suite that owns the route. generic-settle.test.ts is 963 lines, the ratchet is green, and the measured numbers are unchanged — the interactive baseline still confirms for 1,502 ms and the full-tree baseline for 27 ms, settled: true either way. Local: 296 files, 2,050 tests + 1 expected fail, exit 0.

Re-pushed as the layer's single commit 06a4c5b86 (e43dd7abc...06a4c5b86, tests only — the rule and the stable-capture fixtures are byte-identical, so every device number in the description still describes this code). The draft stays draft until #3565, and the test.fails deviation and its offer stand as described above.

@thymikee

Copy link
Copy Markdown
Member Author

The new commit 06a4c5b only adds tests, so the arming defect from the earlier review (76bbe24) is still there, and the required simulator runs have not been taken. CI is pending: both Smoke Tests jobs are queued with no log yet, and since this PR changes iOS --settle arming, a later smoke failure could not be ruled unrelated. I did not run the generic-settle suite, did not reproduce the device timings, and could not check the rewrite against #3565/#3577 because they are not merged. The timings and the 1,502 ms / 27 ms split in the parity case are your numbers. The ratchet's "801 at merge-base" also disagrees with the 902 lines I see at 84f170e, and I did not run the ratchet script. No conflicts.

The main problem is in stable-capture.ts. showsPresentedSurface arms only when the baseline has 5 or fewer nodes or an Alert root. The same pageSheet projects 4 nodes interactive and 6 full, so snapshot then press @ref --settle, back --settle over a stored full tree, and any sheet with 6+ controls never arm the 1.5 s confirmation. In those cases iOS --settle can report settled:true on a coherent tree captured mid-dismissal, which removes the #1831 guard on 2 of the 3 measured routes. Your own table shows about 520 ms against about 1850 ms. The rule should be that arming is identical for every baseline flavour of one screen (interactive, full, depth-limited, scoped), and only a fact computed before projection can meet that. Once #3565/#3577 land, please arm unless baseline.presentedSurface === 'absent' ('unknown' keeps the #3512 arming), pass the baseline SnapshotState into runStableCaptureLoop, and delete showsPresentedSurface, isBlockingSystemAlertRoot and the count-bound arming. You have already agreed to this, so the next step is that #3565's presentedSurface fact lands and the arming is rewritten to read it.

This change also needs device proof on the rewritten rule, because the tables in the body show the state before it. Please run on an iPhone simulator with the RN test app, at least 4 reps each of: (a) selector press of close, (b) plain snapshot then @ref press of close, (c) back --settle with the sheet actually dismissed (use a device or sheet where back dismisses it), and (d) a sheet with 6+ controls. Each run needs settle.waitedMs >= 1500 and an empty diff snapshot -i taken 2 s later. Please add one selector tab switch that stays near 800 ms, so we know the 1.5 s floor does not leak into tab switches.

Not blocking: the test.fails case at generic-settle.test.ts:596 passes on any throw, so a regression of the interactive assertion (checked first) or a setup error would stay green, and it covers only the generic stored-tree route, not the baseline flavour choice in resolveSettleBaseline; you could assert the interactive baseline in a plain test, keep only the full-baseline assertion under test.fails, and drop the marker in the rewrite, but take or leave this as you like.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant