Skip to content

fix: make settle tail list real actionable targets#1172

Merged
thymikee merged 2 commits into
mainfrom
fix/settle-tail-usefulness
Jul 9, 2026
Merged

fix: make settle tail list real actionable targets#1172
thymikee merged 2 commits into
mainfrom
fix/settle-tail-usefulness

Conversation

@thymikee

@thymikee thymikee commented Jul 9, 2026

Copy link
Copy Markdown
Member

Summary

Post-merge benchmark of #1167 (React Navigation prevent-remove flow, iOS
simulator, claude-haiku/claude-sonnet) found the unchanged-interactive tail
regressing to chrome-only noise in exactly the cases it was built for.

Bug A — tail is chrome-only in the flagship case. After press @e10 --settle dismissing the "Discard changes?" dialog:

settled after 1628ms: +0 -9 (~9 unchanged)
unchanged interactive (2):
= @e1 [application] "React Navigation Example"
= @e2 [window]

The button the agent needed ("Discard and go back") was in the stored
settled capture — the agent's fallback snapshot -i listed it from that same
session snapshot — but buildSettleTailEntries required n.hittable === true && n.interactionBlocked !== 'covered', which is stricter than what
snapshot -i itself displays for the same interactive-only capture. Right
after a dismiss animation, real buttons commonly report hittable: false/undefined while application/window containers still pass. Net effect:
the tail spent its whole budget on useless containers and omitted the actual
next target, so the agent snapshotted anyway.

After the fix, same scenario:

settled after 1628ms: +0 -9 (~9 unchanged)
unchanged interactive (1):
= @e3 [button] "Discard and go back"

Bug B — keyboard chrome defeats the trigger. After fill @e6 "hello" --settle, the diff's added lines included ref-bearing [button] nodes:
shift, Emoji, return, Dictate, Next keyboard — real
XCUIElementTypeButton keyboard chrome, not Key nodes, so the existing
withoutKeyboardKeys stripping missed them. Added-lines-with-refs
suppresses the tail trigger (!diff.lines.some(l => l.kind==='added' && l.ref)), so exactly the post-fill case the tail was built for never fired:

settled after Nms: +6 -0 (~3 unchanged)
+ [keyboard] "keyboard"
+ @e4 [button] "shift"
+ @e5 [button] "Emoji"
+ @e6 [button] "return"
+ @e7 [button] "Dictate"
+ @e8 [button] "Next keyboard"
(no tail — trigger suppressed by chrome refs)

After the fix:

settled after Nms: +1 -0 (~3 unchanged)
+ @e4 [keyboard] "keyboard"
unchanged interactive (3):
= @e1 [text-field] "Search"
= @e2 [button] "Cancel"
= @e3 [button] "Search now"

Fixes

  • AbuildSettleTailEntries (src/commands/interaction/runtime/settle.ts)
    drops the hittable === true requirement (presence in the already
    interactive-only settledNodes IS the interactivity bar, matching
    snapshot -i's own display bar), keeps the interactionBlocked !== 'covered' filter, and additionally excludes structural
    application/window roles — never a next actionable target either way.
  • B — keyboard stripping now detects the whole [keyboard] subtree
    structurally (walking each node's parentIndex chain) instead of matching
    only type === 'Key'. The keyboard container stays as one diff line
    ("keyboard appeared"); its descendants (keys and chrome buttons) collapse
    out of the diff. The trigger decision separately treats the container's own
    added ref as chrome too, so a fill that only summons the keyboard can never
    look like "the diff already handed back a target."

Guard: nobody presses shift via settle diff refs, so collapsing keyboard
chrome does not block a user from explicitly targeting the keyboard itself
(e.g. via a direct selector/ref, bypassing settle's diff/tail entirely).

Test plan

  • src/commands/interaction/runtime/settle.test.ts: extended the
    keyboard-Key fixture with chrome buttons, added a keyboard-only-change
    trigger test, updated the modal-dismiss/non-hittable tests for the
    dropped hittable requirement (with a comment explaining the new
    inclusion bar), added a flagship application/window-chrome regression
    test, and two direct buildSettleTailEntries unit tests (chrome
    exclusion, no-hittable-requirement).
  • test/integration/provider-scenarios/settle-observation.test.ts:
    updated the modal-dismiss fixture to replicate the real-world
    undefined-hittable shape (Application/Window/Continue all lack
    hittable) and assert the tail now contains the actionable button; added
    a new fill-keyboard provider scenario asserting the trigger fires and the
    tail lists the field's screen buttons.
  • pnpm typecheck, pnpm lint, pnpm format:check, pnpm check:layering, pnpm build, npx fallow audit --base origin/main
    all pass.
  • Full vitest run --project unit-core --project android-adb (3498
    tests) passes; a couple of apple process-spawn tests intermittently
    exceed the slow-test-gate budget under CPU contention (documented
    pre-existing flakiness) but pass cleanly in isolation.

Risks

  • The tail's inclusion bar is now intentionally broader (any non-covered,
    non-chrome node in the settled tree, not just hittable ones) — this can
    include plain content nodes (e.g. static text) that survive an
    interactive-only capture, matching what snapshot -i would show. This is a
    deliberate widening to fix the false-negative bug; it is not expected to
    regress token budget meaningfully since the tail is still capped at 20
    entries.
  • Keyboard chrome detection is structural (via parentIndex), not
    label-based, so it should be locale-independent, but it depends on the
    runner consistently emitting parentIndex on keyboard descendants (already
    true in production per the Swift runner and existing daemon-side keyboard
    suppression fixtures).

Post-merge benchmark of #1167 (React Navigation prevent-remove flow, iOS
sim, claude-haiku/sonnet) found the unchanged-interactive tail regressing
to chrome-only noise in exactly the cases it was built for:

- buildSettleTailEntries required `hittable === true`, stricter than what
  `snapshot -i` itself shows for the same interactive-only capture. Right
  after a dismiss animation, real buttons commonly report `hittable:
  false`/undefined while application/window containers pass, so the tail
  surfaced two useless chrome lines and dropped the actionable button.
  Fixed by dropping the hittable requirement and excluding structural
  application/window roles instead.
- withoutKeyboardKeys only stripped `Key` nodes; real keyboard chrome
  (shift/Emoji/return/Dictate/Next keyboard) are XCUIElementTypeButton
  nodes and leaked through as fresh added-line refs, which suppressed the
  tail trigger for exactly the post-fill case it exists for. Fixed by
  detecting the whole keyboard subtree structurally (via parentIndex, not
  a locale-fragile label list): descendants collapse out of the diff, and
  the keyboard container's own ref no longer counts as a "meaningful"
  added ref for the trigger decision.

Nobody presses shift via settle diff refs, so collapsing keyboard chrome
does not block a user from explicitly targeting the keyboard itself.
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.6 MB 1.6 MB +1.6 kB
JS gzip 516.6 kB 517.1 kB +463 B
npm tarball 622.5 kB 623.1 kB +589 B
npm unpacked 2.2 MB 2.2 MB +2.0 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 28.2 ms 28.6 ms +0.4 ms
CLI --help 56.5 ms 58.1 ms +1.6 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/tv-remote.js +1.6 kB +463 B

@thymikee thymikee left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: NEEDS CHANGES

Bug A (structural application/window exclusion, dropped hittable===true bar) verifies cleanly, live, on the flagship scenario. Bug B's fix (structural keyboard-chrome detection via parentIndex) is incomplete on a real iOS keyboard — I reproduced a live counter-example where real chrome buttons (Next keyboard, Dictate) are siblings of the [keyboard] container, not descendants, so they escape collectKeyboardChromeRefs/withoutKeyboardChrome entirely and still defeat the tail trigger — the exact failure mode #1167 shipped with, just narrowed to a different subset of keyboard chrome.

Code review

1. parentIndex chain walk — real pipeline shape doesn't match the fix's assumption.
Captured a raw iOS snapshot (snapshot --raw) mid-keyboard on the real React Navigation playground app (iPhone 17 Pro sim, iOS 18). The XCUIElementTypeButton "Next keyboard" (identifier dictation-adjacent) and XCUIElementTypeButton "Dictate" (identifier dictation) sit under an Other container (index 102) whose parentIndex is 56 — a sibling of the Other→Keyboard branch (index 60→61) that holds the QWERTY keys and shift/Emoji/return. Both branches share the same grandparent (56), but neither is an ancestor of the other:

56 (Other, "Next keyboard")
├─ 60 (Other, "Padding-Left") → 61 (type: Keyboard) → keys, shift, Emoji, return   ← collapsed correctly
└─ 102 (Other, "Next keyboard")  → 103 "Next keyboard" (Button), 105/107 "Dictate" (Button)  ← NOT collapsed

collectKeyboardIndexes only seeds from nodes where type === Keyboard; collectKeyboardDescendantIndexes then walks up from every node looking for a keyboard ancestor. Since 102's ancestor chain is 102→56→55→54→0, it never passes through 61, so Next keyboard/Dictate are never recognized as chrome — on the actual device, not a fixture.

Live evidence (fill @e6 "hello" --settle on the Input screen, org.reactnavigation.playground, rne://stack-prevent-remove):

Filled 5 chars
settled after 862ms: +9 -3 (~5 unchanged)
- @e2 [window]
+ @e2 [window] "Next keyboard"
+ @e3 [other] "Next keyboard"
+ @e4 [other] "Padding-Left"
+ @e5 [keyboard] "Padding-Left"
+ @e40 [button] "Next keyboard"
+ @e41 [button] "Dictate"
- @e5 [scroll-area] "Discard and go back"
- @e6 [text-field]
+ @e44 [other] "Discard and go back"
+ @e45 [scroll-area] "hello"
+ @e46 [text-field] "hello"

No unchanged interactive tail is attached — @e40/@e41 are real added-line refs the trigger doesn't recognize as chrome, so hasMeaningfulAddedRef is true and the tail is suppressed. This is Bug B, reproduced live, on this PR's own branch, just for the language-switcher/dictation chrome instead of shift/Emoji/return (those single-parent cases are fixed — confirmed keys and shift/Emoji/return collapse correctly in the same capture). The PR's own regression test only exercises chrome nodes with parentIndex pointing directly at the keyboard container (single hop) — it never models the sibling-branch shape a real device produces, so it passes despite the fix being incomplete.

Symmetry check: yes, both baseline and settled sides get withoutKeyboardChrome in buildSettleDiff (confirmed by reading the code — both calls wrapped identically), so there's no asymmetric-stripping issue for the part of the fix that does work. But because Next keyboard/Dictate are never recognized as chrome on either side, they also show up later as spurious - removed lines when the keyboard disappears (see the next press's diff — - @e40 [button] "Next keyboard", - @e41 [button] "Dictate" alongside the correctly-collapsed - @e5 [keyboard] line), i.e. exactly the "phantom removed chrome" noise flagged as a risk in the review brief.

Cycle safety: isUnderKeyboardContainer has no visited-set/cycle guard, but this matches the established convention elsewhere in the codebase (snapshot-label-dedup.ts, mobile-snapshot-semantics.ts, snapshot-occlusion.ts, compat/maestro/runtime-targets.ts all walk parentIndex the same unguarded way); only snapshot-processing.ts uses a visited set. Not a regression introduced by this PR, but worth noting the whole family is exposed to a malformed/cyclic capture hanging the daemon on every settle, not just this new code path.

2. Trigger semantics. Verified live: a press that opens a real new screen (the "Discard changes?" dialog, 5 new refs) correctly suppresses the tail — confirmed no tail attached when Don't leave/Discard appeared. A fill that only adds keyboard chrome is intended to still fire the trigger (matches the design comment), but per finding #1 above, it doesn't reliably in practice because not all real chrome is recognized as chrome.

3. Tail breadth/ordering. Live tail from the flagship dialog-dismiss case (7 entries) included a plain non-interactive nav title (@e3 [other] "Input") alongside real buttons — expected per the PR's own documented risk ("can include plain content nodes... not expected to regress token budget meaningfully since capped at 20"). At 7/20 entries here it's not a problem, but the tail is unordered by relevance (raw settled-tree/document order), so on a screen with many real controls plus one relevant target, cheap chrome-adjacent nodes could still crowd the cap ahead of the useful one. Not exercised to failure in this session; flagging as an open design question, not a blocker.

4. Test regression validity — confirmed by simulation, not just inspection. Ran the OLD buildSettleTailEntries logic (hittable-required filter) against the new buildSettleTailEntries drops application/window chrome and does not require hittable fixture: returns [] — the new test would fail under the old code. Ran the OLD withoutKeyboardKeys (type==='Key' only) + OLD trigger (addedRefs.size > 0) against the new keyboard-chrome fixture: shift/return survive the old strip and populate addedRefs, suppressing the tail — reproduces Bug B under old code. Both are genuine regression tests, not tautologies. However, as shown in finding #1, the fixed code's own test suite doesn't cover the sibling-branch shape that the real runner produces, so it passes without actually closing Bug B for Next keyboard/Dictate.

5. Gates — all green in a scratch worktree off origin/fix/settle-tail-usefulness (347ea1f): pnpm typecheck, pnpm lint, pnpm format:check, pnpm check:layering, pnpm build, npx fallow audit --base origin/main (0 issues in 4 changed files), and the touched test files (settle.test.ts 17 tests, settle-observation.test.ts 4 tests) all pass. gh pr checks 1172: all 21 checks green, including iOS Smoke Tests.

Live verification (the part #1167's review lacked)

Ran directly against the iPhone 17 Pro sim (C25DBB5B-9254-4293-A8D5-2785C78DE03A), own session/state-dir, built from this branch:

  • open org.reactnavigation.playground --relaunchopen rne://stack-prevent-remove landed directly on the Input screen (field + Discard-and-go-back + Push Article).
  • fill @e6 "hello" --settle: reproduced Bug B live (see finding #1 above) — Next keyboard/Dictate leak through as added refs, tail does not fire.
  • press <Discard and go back> --settle → dialog appears with real new refs (Don't leave, Discard) — tail correctly absent (real content, not chrome).
  • press <Don't leave> --settlethe flagship Bug A case, verified working:
Tapped @e10 (127, 497)
settled after 812ms: +0 -9 (~9 unchanged)
- @e3 [window] "Discard changes?"
- @e4 [element(7)] "Discard changes?"
- @e5 [scroll-area] "Discard changes?"
- @e6 [text] "Discard changes?"
- @e7 [text] "You have unsaved changes. Discard them and leave the screen?"
- @e8 [scroll-area] "Don't leave"
- @e9 [cell] "Don't leave"
- @e10 [button] "Don't leave"
- @e11 [button] "Discard"
unchanged interactive (7):
= @e3 [other] "Input"
= @e4 [button] "Home, back"
= @e5 [other] "Discard and go back"
= @e6 [scroll-area] "hello"
= @e7 [text-field] "hello"
= @e8 [button] "Discard and go back"
= @e9 [button] "Push Article"

The tail contains the real Discard and go back button (@e8), not just application/window (correctly excluded per STRUCTURAL_TAIL_ROLES).

  • Pressing the tail ref directly works: press @e8 --settle tapped at (201, 202) and the "Discard changes?" dialog reappeared — no refs-generation rejection, no ambiguous-target rejection. Bug A's fix is confirmed working end-to-end on real hardware.

Cleanup performed: closed the review session, killed the review daemon process, pkill -f AgentDeviceRunnerUITests, removed the scratch worktree. Metro (127.0.0.1:8081) left running. No other daemon owned the device when the session started.

Summary

  • Bug A (structural chrome exclusion, dropped hittable bar): verified correct, both in code and live on the flagship dialog-dismiss scenario. Approve-worthy on its own.
  • Bug B (structural keyboard-chrome detection): the parentIndex-ancestor-walk approach is the right idea but doesn't match the real iOS accessibility tree shape for Next keyboard/Dictate (predictive/candidate-bar buttons), which live in a sibling subtree of the [keyboard] container rather than under it. This is a live-reproduced blocker, not a hypothetical: the PR's own target scenario (fill summons keyboard → tail should still fire) fails on the real device for exactly the buttons named in the PR description's own Bug B repro. Needs either broadening the "keyboard chrome" recognition to catch sibling candidate-bar containers (e.g. also treating nodes matching the dictation identifier / Next keyboard/Dictate labels, or better, walking up to any container whose subtree also contains the keyboard, or detecting the whole higher-level "keyboard window" instead of just the Keyboard-typed node) or documenting the narrower scope and re-validating the PR's claimed fix against a real capture before merge.

@thymikee

thymikee commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

Review finding

The size report itself looks fine: +699 B raw / +176 B gzip in dist/src/tv-remote.js, and the startup deltas are sub-millisecond noise. The keyboard-chrome part is also a good direction: structural parentIndex traversal, locale-independent, keeping the keyboard container as a single diff signal while excluding keyboard refs from the tail trigger/candidates.

One quality issue I would fix before calling this ready: buildSettleTailEntries now lets every non-covered, non-chrome node from the settled interactive capture into the tail. Dropping the hittable === true requirement is right for the reported XCTest flake, but the new bar is broader than “actionable”: Apple interactive-only captures can include content-bearing static text, and the updated test explicitly expects Price: $12 in the tail. Since the tail is capped at 20 and preserves tree order, a content-heavy screen can spend the tail budget on labels and omit the real next controls.

I would keep the core change, but tighten candidate quality: controls/action roles first, then content fallback if needed, or exclude pure static text from the tail. The contract wording should also stop promising purely “actionable” entries if content nodes remain allowed.

Live-device review of the first Bug B fix found "Next keyboard" and
"Dictate" still leaking into the settled diff as added refs: on a real
iPhone 17 Pro simulator they live in a SIBLING subtree of the [Keyboard]
container (the candidate bar), not under it, so the container-descendant
walk missed them. A raw hierarchy capture shows the software keyboard in
its own dedicated window hosting both the container and the candidate
bar, so the structural rule is now: every node inside a window that has
a [Keyboard] descendant is keyboard chrome. Conservative guard: a window
also hosting an editable text node outside the container (iOS puts
inputAccessoryView composers in the keyboard window) is never
window-classified — those fall back to the container-descendant walk.

The live run also showed the filled field re-labeling itself with its
new value (ancestor wrappers inherit it), which produced added refs that
suppressed the tail even with chrome fixed. The trigger now also ignores
self-echo refs — added lines whose settled node rect contains the action
point — since they re-describe the acted-on element, not a new target.

The fill-keyboard provider fixture is now a trimmed REAL capture from
the benchmark flow (sibling candidate bar, main app window absent from
the interactive settled capture, self-echo relabels) instead of a
hand-built tree that hid the sibling-branch shape.
@thymikee

thymikee commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

Re: live-device review — sibling-subtree chrome fixed, re-verified live

Thanks for the live repro. Confirmed on the iPhone 17 Pro simulator and fixed in ef98513. Bug A was left exactly as reviewed (re-verified below with the final build anyway).

Structural finding (from a raw hierarchy capture on the sim)

snapshot --raw with the keyboard up shows the software keyboard renders in its own dedicated window (UIRemoteKeyboardWindow), and inside it the [Keyboard] container and the candidate bar are sibling subtrees:

54 [Window] "Next keyboard"            (0,0 402x874)
  55 [Other] "Next keyboard"
    56 [Other] "Next keyboard"         (0,566 402x308)   ← common ancestor
      60 [Other] "Padding-Left"
        61 [Keyboard] "Padding-Left"   ← container: keys + shift/Emoji/return buttons
      102 [Other] "Next keyboard"      ← SIBLING of 60, not under the container
        103 [Button] "Next keyboard"
        105/107 [Button] "Dictate"

The focused TextField lives in the app's main window (a different window), and in the interactive-only settled capture the main app window doesn't even survive — app content re-parents onto the Application node, and the keyboard window is the only [Window] line. So the rule the reality supports is exactly the window rule: every node inside a window that has a [Keyboard] descendant is keyboard chrome. Conservative guard kept: a window that also hosts an editable text node outside the container (iOS hosts inputAccessoryView composers in the keyboard window) is never window-classified — it falls back to the old container-descendant walk, so a messaging composer can't be hidden as chrome.

The live run surfaced one more suppressor the fixtures had hidden: the filled field re-labels itself with its new value (+ [text-field] "hello") and its ancestor wrappers inherit the label — added refs that defeated the tail trigger even with chrome fully stripped. The trigger now also ignores these self-echo refs (added lines whose settled node rect contains the action point): they re-describe the element the caller just acted on, not a new target. They stay visible in the diff.

Live evidence (final pushed build, fill @e6 "hello" --settle on rne://stack-prevent-remove Input)

Before (the reviewed regression):

settled after 856ms: +9 -3 (~5 unchanged)
+ @e2 [window] "Next keyboard"
+ @e3 [other] "Next keyboard"
+ @e4 [other] "Padding-Left"
+ @e5 [keyboard] "Padding-Left"
+ @e40 [button] "Next keyboard"
+ @e41 [button] "Dictate"
...                                    (no tail — trigger suppressed)

After:

Filled 5 chars
settled after 853ms: +4 -3 (~5 unchanged)
- @e2 [window]
+ @e5 [keyboard] "Padding-Left"
- @e5 [scroll-area] "Discard and go back"
- @e6 [text-field]
+ @e44 [other] "Discard and go back"
+ @e45 [scroll-area] "hello"
+ @e46 [text-field] "hello"
unchanged interactive (4):
= @e42 [other] "Input"
= @e43 [button] "Home, back"
= @e47 [button] "Discard and go back"
= @e48 [button] "Push Article"

Keyboard collapses to one line, zero chrome added refs, tail lists the screen's real controls. Bug A re-verified on the same build (press @e10 --settle dismissing "Discard changes?"):

settled after 825ms: +0 -9 (~9 unchanged)
- @e3 [window] "Discard changes?"
... [8 more dialog removals]
unchanged interactive (7):
= @e3 [other] "Input"
= @e4 [button] "Home, back"
= @e5 [other] "Discard and go back"
= @e6 [scroll-area] "hello"
= @e7 [text-field] "hello"
= @e8 [button] "Discard and go back"
= @e9 [button] "Push Article"

Fixtures now come from reality

The fill-keyboard provider fixture is a trimmed real capture (snapshot -i --json before/after the live fill; 31-key block reduced to 2 keys, rects rounded) preserving the three load-bearing facts hand-built fixtures got wrong: sibling candidate-bar buttons, the main app window missing from the interactive settled capture, and the self-echo relabels. Unit fixtures were rebuilt on the same shape, plus new tests for the sibling branch, the self-echo trigger, and the composer-window guard.

Gate: typecheck, lint, format:check, layering, build, fallow audit --base origin/main (one complexity finding surfaced and was refactored away), settle unit + full provider-scenario + interaction-contract suites all green. Sim cleanup done: session closed, /tmp/tailfix-b-state daemon stopped, no stray runner processes.

@thymikee thymikee left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review addendum (commit ef98513): APPROVE-worthy

Scoped re-review of the rework only; the Bug A verdict from my earlier review stands unchanged (and the implementer's posted before/after live outputs on the final build match what I observed on the same sim/flow, including the previously-failing fill --settle now collapsing the keyboard to one line and attaching the 4-entry tail). My earlier blocker — candidate-bar chrome (Next keyboard/Dictate) living in a sibling subtree of the [Keyboard] container — is correctly closed by the window rule. Findings below; none are blockers.

1. Window rule (resolveKeyboardWindowIndexes + collectKeyboardChrome)

  • Cross-platform inertness: confirmed. The rule only activates when a node's normalizeType(type) === 'keyboard' exists; with zero matches collectKeyboardChrome early-returns an empty set before any window resolution. macOS has no soft keyboard nodes (AXWindow → 'window' but no 'keyboard' seed); Android IME views normalize to keyboardview/framelayout/etc., not keyboard, and Android hierarchies carry no 'window'-typed nodes anyway, so even a pathological class basename of exactly Keyboard (e.g. com.foo.Keyboard) would only get the container-descendant walk (arguably correct semantics for a thing that is a keyboard), never whole-window classification.
  • Multi-window: scoped correctly. Window classification climbs from each [Keyboard] container to its nearest window ancestor only — other app windows are untouched. iOS custom keyboard extensions render inside the system keyboard window (UIRemoteKeyboardWindow), so they classify as chrome, which is the desired behavior. The residual theoretical case — an app whose own window legitimately hosts an XCUIElementTypeKeyboard-typed view with no editable field in that window — would have its whole window collapsed from settle diffs; XCTest assigns the Keyboard type to system keyboard implementations, not arbitrary app views, so I consider this acceptable-and-remote. The windowIndexes.delete(windowIndex) during Set iteration only removes the current entry, which is well-defined in JS.
  • Same cycle-guard caveat as before applies to findNearestWindowAncestor/hasAncestorIn (no visited set), consistent with the rest of the codebase's parentIndex walkers — pre-existing convention, not a regression.

2. inputAccessoryView guard

  • Predicate is exact normalized-type membership (textfield, securetextfield, textview, textarea, searchfield, edittext) — consistent with the editable vocabulary elsewhere (prefersValueForReadableText uses a superset via .includes). It checks "editable AND not in the container subtree AND under this window", which is the right shape.
  • Known limitation, accepted-by-design: a search field in the candidate-bar area (e.g. the emoji-search field, or any editable node the keyboard window hosts outside the container) flips window classification OFF for that capture, so candidate-bar chrome (Next keyboard/Dictate) floods back as added refs and suppresses the tail — Bug B's shape returns for that one settle. Container-level stripping (keys/shift/return) still holds, and the code comment states the trade-off explicitly: hiding the composer the user is typing into would be worse than leaking chrome. Agreed with the priority; flagging so it's on record if messaging-app benchmarks regress on the tail trigger.

3. Self-echo heuristic (isSelfEchoNode)

  • Degradation is conservative: missing rect or missing action point → false → the added ref counts as meaningful → tail suppressed, i.e. exactly the pre-heuristic behavior. No NaN/undefined hazards (Rect requires all four fields; zero-sized rects only match the exact point). Ref-target resolutions populate point via resolveNodeCenter, so the heuristic is live in the real pipeline; the rare ref path without a resolved point degrades safely.
  • Applies to all settle-capable interactions (press/click/fill/longpress) — buildSettleDiffAndTail is shared and takes params.resolved.point. For a press that opens a context menu AT the press point: only added nodes whose rect contains the point are ignored by the trigger; menu items offset from the point still count as meaningful and suppress the tail. If every added node did contain the point, the tail attaches alongside the full diff — nothing is hidden (self-echo affects only the trigger, never the diff lines), the cost is a few extra tail lines. Confirmed benign-by-design; worth remembering as a token-cost, not correctness, edge.
  • Conversely, a press navigating to a full screen whose root containers span the action point still suppresses the tail correctly, because leaf additions elsewhere don't contain the point.

4. Tests — regression validity verified by execution, not inspection

Ran the new test files against the OLD settle.ts (347ea1f) in the scratch worktree: 3 unit tests + 1 provider-scenario test fail under old code (candidate-bar siblings never spend the diff budget, keyboard-only changes do not suppress the trigger, self-echo does not suppress the tail, and the fill-keyboard provider scenario) and pass under ef98513 — genuine regression tests. The composer-guard test passes under old code too, which is correct: it guards the new window rule from over-reaching, it isn't a regression test. The provider fixture is a trimmed real capture preserving the three load-bearing facts (window → sibling subtrees with Next keyboard/Dictate NOT under the container; the main app window absent from the interactive settled capture; the filled field's self-echo relabels) — this matches the raw hierarchy I captured live in the first review, so the fixture-vs-reality gap that sank #1167 and the first iteration of this PR is closed.

5. Gates

In a scratch worktree at ef98513: pnpm typecheck, pnpm lint, pnpm format:check, pnpm check:layering, pnpm build, npx fallow audit --base origin/main (0 issues in 4 changed files), and the touched tests (19 unit + 4 provider-scenario) all pass. gh pr checks 1172: all 22 checks green (the last iOS Smoke Tests run completed after a brief pending window).

Verdict

APPROVE-worthy. The two documented residual limitations (composer/emoji-search windows fall back to chrome leakage by conservative design; self-echo can attach a redundant tail next to a context-menu diff) are correctly prioritized trade-offs, visible in code comments, and cheap to revisit if benchmarks surface them.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Jul 9, 2026
@thymikee thymikee merged commit b8ac758 into main Jul 9, 2026
21 checks passed
@thymikee thymikee deleted the fix/settle-tail-usefulness branch July 9, 2026 19:07
@github-actions

github-actions Bot commented Jul 9, 2026

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

thymikee added a commit that referenced this pull request Jul 10, 2026
…z cases to help-conformance bench (#1176)

* feat: fix codex runner, add override-doc grading, port skillgym quiz cases

The 2026-07-09 evaluation of scripts/help-conformance-bench.mjs found it
structurally right but broken for the codex runner (two bugs), thin on
coverage (4 cases), sequential, and unable to grade a draft help rewrite
without a rebuild.

- Fix runCodex: (1) codex exec reads stdin until EOF when not attached to
  a TTY, and execFile never closes the child's stdin, so every codex call
  hung until RUN_TIMEOUT_MS with empty output — close stdin right after
  spawn. (2) `-o outFile` writes the same final JSON that codex also
  prints to stdout, so concatenating both produced two back-to-back JSON
  objects that broke every JSON.parse candidate and silently zeroed
  extractCommands() — prefer the clean -o payload, fall back to stdout
  only when it's empty.
- Add `--override-doc <topicId>=<path>` (repeatable): loads a topic's
  text from a file instead of shelling out to
  `node bin/agent-device.mjs help <topic>`, so a draft help rewrite can
  be A/B graded with zero rebuild.
- Port three cases from
  test/skillgym/suites/agent-device-smoke-suite.ts
  (settle-diff-is-observation, sample-output-settled-diff-next-target,
  sample-output-not-settled-needs-observe) as self-contained
  "next-command quiz" cases, generalizing the scorer to support regex
  matchers/forbidden patterns alongside the existing named expectations.
  Fixture output text matches the CURRENT settle rendering in
  src/commands/interaction/output.ts, including the "unchanged
  interactive (N):" tail added by #1167/#1172.
- Parallelize the runner x case matrix with a concurrency cap
  (HELP_BENCH_CONCURRENCY, default 4); results still print in the
  original matrix order.
- Extend test/skillgym/README.md's existing pointer to this bench with
  the new flags.

Validated with real LLM calls (both runners, all 7 cases, 14 calls,
~$0.25 total): 13/14 pass; the one fail (claude-haiku-4-5 on
dogfood-mode) is a genuine model miss (returned an empty command plan
asking for the app name instead of committing to a generic plan), not a
bench bug. `--override-doc` demonstrated live: stripping the dogfood
doc's evidence-command examples regresses codex:gpt-5.4-mini from 3/3 to
2/3 on the same case, showing the flag both loads and changes grading.

* fix: apply live-doc post-processing to --override-doc, fail fast on bad overrides

Review findings on the initial version (all reproduced):

- HIGH: an override for the --help:first30 doc id skipped the live path's
  firstLines(text, 30) cap, so a 49-line draft leaked lines 31-49 into the
  prompt — grading content a live run never shows, on the doc id every case
  uses. loadDoc now splits source (live shell-out vs override file) from
  post-processing, and the post-processing applies to both, so an override
  differs ONLY in where the text comes from.
- MEDIUM: an --override-doc topic id no selected case uses was silently
  ignored (exit 0, real doc graded). Now fails fast listing the valid doc
  ids for the selection.
- LOW: a missing override file threw a raw ENOENT stack trace; expected
  failures now print one clean Error line. Added --help usage text that
  documents last-wins semantics for repeated same-topic overrides and the
  post-processing parity.

Guard tests (scripts/__tests__/help-conformance-bench.test.ts, wired into
the unit-core vitest project by explicit path): a 49-line fixture whose
prompt must keep line 30 and drop line 31, unknown-topic fail-fast with
valid ids listed, clean no-stack error for a missing file, and last-wins
for repeated overrides. All spawn the script in --dry-run with every
required doc overridden, so they need no LLM calls and no built CLI.

Live re-validation: a 33-line override of --help:first30 whose lines
31-33 instruct the model to emit a sentinel command; neither
claude-haiku-4-5 nor codex:gpt-5.4-mini emitted it (both scored 4/4,
matching the live-doc baseline), proving the cap applies end-to-end.
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.

1 participant