feat: include unchanged interactive refs in settle output#1167
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
thymikee
left a comment
There was a problem hiding this comment.
Adversarial review — feat/settle-actionable-tail
Worked entirely against the branch tip (gh pr diff, git show origin/...:<path>) in a scratch git worktree add --detach /tmp/pr1167-worktree origin/feat/settle-actionable-tail (removed after). No changes made to any checkout.
Verdict: needs-changes (one real blocker, otherwise sound)
The design (ref-blessing safety, generation gating, dedup reasoning) is correct and matches the PR's own claims under adversarial re-derivation. But the branch as pushed does not pass CI: Fallow Code Quality fails for real, file-scoped reasons introduced by this diff. gh pr checks 1167 currently shows:
Coverage— fail, but it's the pre-existingrunner-xctestrun.test.tspackageVersion0.19.0-vs-0.19.1 assertion, reproduced locally on unmodifiedmaintoo. Not caused by this PR (matches the PR description's own callout).Fallow Code Quality— fail, and this one is caused by this PR (reproduced locally withnpx fallow audit --base ff7fc5ba613e7f9c0401d0ebeb0dc097c95af334, scope = the 12 changed files). Not mentioned anywhere in the PR's test plan.
Blocker (must fix before merge)
1. Fallow gate: dead export + complexity, both introduced by this diff
src/commands/interaction/runtime/settle.ts:178—buildSettleTailisexported but has zero external consumers (onlybuildSettleTailEntries, one line below, is imported bysettle.test.ts). Fallow's dead-export check fails the build over this. Fix: drop theexportkeyword —buildSettleDiffAndTail(same file) is the only caller.src/daemon/response-views.ts:154—interactionSettleViewnow trips the complexity gate (11 cyclomatic / 8 cognitive / 37.1 CRAP, over threshold) after thetail/tailTruncateddestructure + conditional slice were folded into the same function as the pre-existingdiff/refsdigest logic. Needs a small extraction (e.g. asliceDigestTail(tail: unknown): unknown[] | undefinedhelper) to get back under threshold.
Both reproduce deterministically outside CI: npx fallow audit --base ff7fc5ba613e7f9c0401d0ebeb0dc097c95af334 inside a clean checkout of this branch.
Non-blocking notes
2. Trigger fires on true no-op settles too, not just modal-dismiss (src/commands/interaction/runtime/settle.ts — buildSettleTail's addedRefs.size > 0 check). The PR description frames this as "the modal-dismiss/toast-only signature," but the literal trigger (!diff.lines.some(l => l.kind==='added' && l.ref)) is vacuously true when the diff has zero lines at all (nothing changed, additions:0/removals:0). That's a broader behavior change than the headline: every "settled, nothing happened" response now also gets up to 20 tail entries attached where previously it was a terse no-op. Probably fine/intended (same fallback-avoidance logic applies), but worth a one-line doc callout since it's a bigger byte-cost surface than "27% of settle actions after modal dismiss" implies.
3. Label quoting in the tail's CLI rendering is unescaped (formatSettleTailLines in src/commands/interaction/output.ts) — a label containing " would render oddly. Not a new defect: it's the same convention formatSnapshotLine/displayLabel (src/snapshot/snapshot-lines.ts) already use for every other snapshot/diff line, so this is consistent with existing behavior, not a regression.
Verified sound (tried to break each, could not)
-
Ref-blessing safety (#1096-class) — Traced
settleAfterInteraction:buildSettleDiffAndTail(which produces bothdiffandtail) is only spread into the observation whenoutcome.settled && storedis true, wherestoredis literally the return value ofstoreSettledSnapshot(runtime, options, outcome.lastCapture)— the same call that writessettledNodesinto the session. Tail candidates come from that identicalsettledNodesarray, so a tail can never exist when the tree wasn't stored (sparse-quality captures returnstored=falseand get no diff/tail at all), and it can never reference a different generation than the diff's own added refs. Ininteraction-touch-response.ts,settleExtraonly attachesrefsGenerationwhensettle?.diffis truthy — and tail is structurally inseparable from diff (both set together, same gating condition), sorefsGenerationis always present whenevertailis. Confirmed via the PR's new daemon test (press --settle on a removals-only diff attaches the unchanged interactive tail at the diff generation, assertssettle.refsGeneration === session.snapshotGeneration) — ran green locally. Could not construct a sequence where a tail ref resolves against a stale/wrong tree. -
Trigger correctness under
capSettleDiffLinestruncation —buildSettleTail(diff, settledNodes)is called with the diff object aftercapSettleDiffLineshas already run (it inspectsdiff.lines, the capped array, not the pre-capchangedarray). So if an added-with-ref line gets truncated out of the visible diff and no other added-ref line survives the cap, the trigger correctly fires (addedRefs.size === 0on the capped view) and the tail — built independently from the full, uncappedsettledNodes— resurfaces that ref. This is the "good" branch per the review brief; adiffwith >80 added lines where at least one ref-bearing line survives correctly still suppresses the tail (that ref is already visible). Zero-line (no-op) diffs also correctly trigger it (see note #2 above on scope). -
Wire/compat —
command-output-schemas.ts'ssettleObservationSchemamatchesSettleTailEntry({ref, role, label?}) exactly. This codebase'sobjectSchemanever emitsadditionalProperties: false(confirmed no.strict()/zod anywhere, andobjectSchema's own doc comment states the non-strict invariant), so additive fields can't break MCP clients. Digest slicing (response-views.ts) uses non-mutatingtail.slice(0, DIGEST_REF_LIMIT), matching the existingrefspattern in the same function. Searched the suite (including the#1114/#1155-adjacent contract tests, e.g.interaction-response-shape.contract.test.ts'sassertExactKeys) for exhaustive-key assertions on a settle response — none exist; nothing exercisessettleshape exhaustively, so nothing breaks. -
CLI text —
formatSettleTailLinesonly emits output whentail.length > 0, and tail is never populated onsettled: false(see #1), so a not-settled response never renders the block. Ran the PR'soutput.test.tsadditions locally (green):unchanged interactive (N):header,= @ref [role] "label"lines,… more interactive elements not shown, use snapshot -itruncation marker. -
Dedup-vs-diff-refs "known gap" self-assessment — Confirmed accurate.
buildSettleTailearly-returns (if (addedRefs.size > 0) return {}) before ever callingbuildSettleTailEntries, so on the real pipelinebuildSettleTailEntriesis only invoked with an emptyexcludeRefsset — the dedup branch is dead in production, exercised only by the direct unit test (buildSettleTailEntriescalled standalone with a synthetic non-empty set). Also confirmed truncation (#2 above) does not make it reachable: even when an added-ref line is truncated out ofdiff.lines,addedRefs(derived fromdiff.lines) is still empty in that scenario, so the exclude set passed tobuildSettleTailEntriesremains empty either way. -
MCP pin merge —
collectRefBodiesreads.refoff each array entry, exactly matchingSettleTailEntry's{ref, role, label?}shape (same contract assettle.refs/diff lines already handled by the same function). Ran the PR's newcommand-tools.test.tstest (tail-only ref pin merge, end-to-end through@e1~s9resolution) — green. -
CI reproduction — Clean scratch worktree at branch tip:
typecheck,lint,check:layering,format:checkall pass. Fullunit-coreproject: 3358 tests, 1 failure — exactly the pre-existingrunner-xctestrun.test.tspackageVersion mismatch (reproduces on unmodifiedmain), no apple process-spawn flakes observed this run. All PR-touched test files plusinteraction-guarantees.test.ts(ADR 0011 gate) plus the provider-scenario integration test pass green in isolation and in the full run.
Summary: logic is sound across every axis in the review brief — this is a well-reasoned, well-tested change. But please fix the export keyword on buildSettleTail and de-complexify interactionSettleView so Fallow Code Quality actually goes green; as pushed, this PR does not pass its own required checks.
|
Addressed the review's two Fallow findings in 85d381a (verified locally with
Also made the trigger scope explicit in the PR description per the review's observation: the tail intentionally fires on true no-op settles (zero diff lines) as well as removals-only diffs — a no-op is exactly the case where the agent has no diff to act on and would otherwise fall back to Re-verified after the fix: |
85d381a to
4cca8a3
Compare
Benchmarks (gpt-5.4-mini + claude-haiku, July 2026) showed 27% of --settle actions were followed by a fallback snapshot -i because a change-only diff omits refs for elements that did not change: after a modal dismiss the diff shows only removals, so the next button to press is invisible. Add an unchanged-interactive tail to SettleObservation, attached only when the diff's added lines carry zero refs (the modal-dismiss/toast-only signature). It lists the settled tree's remaining hittable, uncovered elements so the response stays actionable without an extra round trip. Rides the CLI text, MCP digest view, ref pinning, and output schema the same way the diff's added-line refs already do.
- drop the unused export on buildSettleTail (tests exercise the trigger through the public interaction path and the filter via buildSettleTailEntries) - extract the digest tail capping from interactionSettleView into a module-private helper to stay under the complexity gate
4cca8a3 to
f886b6e
Compare
|
* fix: make settle tail list real actionable targets 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. * fix: classify the whole iOS keyboard window as settle chrome 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.
…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.
Three exported-and-unit-tested-but-unreferenced-in-production incidents this week (#1166 getNearestCommandNames, #1167 buildSettleTail, #1199 clearMetroSessionHints) — the first two were caught by fallow's dead-code check because they had zero importers anywhere; #1199 was missed because a test file imports the export, and fallow's default reachability graph counts a test import as "used". Adds a second, stricter pass reusing fallow's own --production mode (entry.exclude test/story/dev files) via scripts/test-only-exports/check.ts: an export alive in fallow's default graph but dead in its production graph, with no other reference anywhere in its own file, has no production call site — exactly the #1199 shape. Ratchets against a checked-in baseline (scripts/test-only-exports-baseline.json, 77 entries); new findings fail `pnpm check:test-only-exports` (wired into CI's Fallow job and check:tooling). A `// test-seam: <reason>` comment above an export is the escape hatch for intentional test seams. Also extends .fallowrc.json's ignoreExports for seven daemon route handlers (src/daemon/handlers/*.ts) that are genuinely production-reachable through request-handler-chain.ts's `typeof import()` lazy-load pattern, which fallow's static import graph can't trace as a named-export consumer — without this they were false positives in the production-mode pass.
* ci: ratchet against test-only exports Three exported-and-unit-tested-but-unreferenced-in-production incidents this week (#1166 getNearestCommandNames, #1167 buildSettleTail, #1199 clearMetroSessionHints) — the first two were caught by fallow's dead-code check because they had zero importers anywhere; #1199 was missed because a test file imports the export, and fallow's default reachability graph counts a test import as "used". Adds a second, stricter pass reusing fallow's own --production mode (entry.exclude test/story/dev files) via scripts/test-only-exports/check.ts: an export alive in fallow's default graph but dead in its production graph, with no other reference anywhere in its own file, has no production call site — exactly the #1199 shape. Ratchets against a checked-in baseline (scripts/test-only-exports-baseline.json, 77 entries); new findings fail `pnpm check:test-only-exports` (wired into CI's Fallow job and check:tooling). A `// test-seam: <reason>` comment above an export is the escape hatch for intentional test seams. Also extends .fallowrc.json's ignoreExports for seven daemon route handlers (src/daemon/handlers/*.ts) that are genuinely production-reachable through request-handler-chain.ts's `typeof import()` lazy-load pattern, which fallow's static import graph can't trace as a named-export consumer — without this they were false positives in the production-mode pass. * fix: harden test-only-exports ratchet per review Addresses the two should-fixes and all five minors from the independent review of #1202: - Replace the regex own-file occurrence count with an oxc-parser AST walk (typescript@7 ships no JS scanner API, so the review's fallback tool suggestion is the primary): identifiers are counted as AST nodes deduped by source span, so mentions in JSDoc/block comments, strings, and template-literal text no longer masquerade as call sites (review finding 1, both constructed cases re-verified fixed), and a `//` inside a string no longer hides real usages (finding 6). Span dedupe keeps barrel re-exports (`export { x } from`) counting once. The sharper count surfaced one organic false negative on main: `selector` in src/commands/index.ts was previously exempted because the regex matched "selector" inside the './...selector-read.ts' import path string; it is now baselined alongside its sibling `ref` (same re-export line). - Make the baseline shrink-only (finding 2): --update-baseline refuses new findings with the same wire/delete/annotate message, so the `// test-seam:` annotation in the reviewed source diff is the only acceptance path; CONTRIBUTING no longer documents baseline regeneration as an acceptance option and now describes baseline growth as a deliberate manual edit. - Stale baseline entries now emit a `::warning` CI annotation (finding 3). - Commit a re-runnable fixture test (finding 4): check.test.ts mirrors scripts/layering/model.test.ts, builds a synthetic package with a clearMetroSessionHints-shaped export (JSDoc self-mention included), asserts it is flagged, and asserts the annotated twin passes; wired before the check in pnpm check:test-only-exports. - Mark the unreadable/unparseable-file fallbacks CONSERVATIVE: per CONTRIBUTING's convention (finding 5). - Document the dynamic property access (obj[name]) blind spot in the script header and CONTRIBUTING (finding 7). * fix: harden test-only export ratchet * refactor: use native Fallow export gate * chore: refresh production export baseline
Problem
Benchmarks (gpt-5.4-mini + claude-haiku, July 2026) showed 27% of
--settleactions were followed by a fallbacksnapshot -i. Root cause: the settled diff only carries changed lines, andsnapshotNodeToComparableLine(src/snapshot/snapshot-diff.ts) keys ondepth|role|text|enabled|selected|hittablewith no ref for unchanged elements. After a modal dismiss, the diff shows only removals — the next button to press (already on screen, untouched) is invisible, so agents fall back to a full interactive snapshot.Approach
Add an unchanged interactive refs tail to
SettleObservation(src/contracts/interaction.ts):Built in
src/commands/interaction/runtime/settle.tsfromsettledNodes(the settle loop's already-captured interactive-only tree — no extra device call):!diff.lines.some(l => l.kind === 'added' && l.ref)) — the modal-dismiss/toast-only signature. A diff with fresh added refs already hands back the next target, so the tail is pure byte cost there and stays off.snapshot -i; the tail tells it what is still actionable in the same response.hittable === true,interactionBlocked !== 'covered', not already present on the diff's added lines (dedup).MAX_SETTLE_TAIL_ENTRIES = 20;tailTruncated: truewhen candidates exceed the cap.snapshot -ivocabulary (formatRole/displayLabelfromsrc/snapshot/snapshot-lines.ts) — the layering DAG already allows this import fromcommands/interaction/runtime.Wired through every surface the diff's added-line refs already ride:
src/commands/interaction/output.ts): anunchanged interactive (N):block after the diff lines.src/daemon/response-views.ts):tailcapped toDIGEST_REF_LIMIT(12), alongside the existing digest ref list.src/mcp/command-tools.ts):mergeSettleIssuedRefPinsnow also mergessettle.tailrefs, so tail refs pin the same way diff/added refs do — without this the MCP layer would silently drop tail-ref pinning.src/mcp/command-output-schemas.ts):settleObservationSchemadocumentstail/tailTruncated.src/cli/parser/cli-help.ts): one added sentence pointing at the tail before thesnapshot -ifallback.refsGenerationgating (src/daemon/handlers/interaction-touch-response.ts) is untouched: it already attaches only whensettle.diffis present, andtailonly exists alongsidediff, so the tail rides the same generation as the diff's own refs — verified by a new daemon-handler test.No changes were needed in
src/contracts/interaction-guarantees.ts: this is a field addition inside the existingsettleObservationguarantee cell (still implemented bysettleAfterInteraction), not a new guarantee or dispatch path — the ADR 0011 completeness/honesty gate passes unchanged.Output example
A
press <modal-ok-button> --settlethat dismisses a dialog, leavingContinueon screen untouched:(
… more interactive elements not shown, use snapshot -iappended whentailTruncated.)Test plan
pnpm typecheckpnpm lintpnpm check:layeringpnpm buildpnpm format:checknpx fallow audit --base ff7fc5ba6— clean (unusedbuildSettleTailexport dropped; digest tail capping extracted frominteractionSettleViewinto a module-private helper to stay under the complexity gate)src/commands/interaction/runtime/settle.test.ts(13 tests — trigger condition, filtering non-hittable/covered candidates, cap/truncation, dedup via the exportedbuildSettleTailEntries)src/commands/interaction/output.test.ts(CLI text rendering, incl. truncation marker)src/daemon/handlers/__tests__/interaction-settle.test.ts(tail rides the diff'srefsGenerationthrough the daemon response builder)src/mcp/__tests__/command-tools.test.ts(tail refs merge into MCP ref pinning)src/contracts/__tests__/interaction-guarantees.test.ts(ADR 0011 gate unaffected)test/integration/provider-scenarios/settle-observation.test.ts(modal-dismiss produces a tail end-to-end through the daemon; added-lines-with-refs still produces no tail)Full
unit-core+provider-integrationrun: 3433 passed, 43 failed under load — all 43 aresrc/platforms/apple/core/**,src/daemon/__tests__/runtime-hints.test.ts,src/platforms/__tests__/install-source.test.ts, etc. (process-spawn/runner tests), none in a file touched by this PR, and every sampled one passes cleanly re-run in isolation. This matches the known CPU-contention flake (unit-suite-flaky-under-contention), not a regression.src/platforms/apple/core/__tests__/runner-xctestrun.test.ts's package-version assertion (0.19.0vs0.19.1) also fails identically on unmodifiedmainin this worktree — pre-existing, unrelated.Known gaps
buildSettleTailEntriesis exercised directly via a unit test with a syntheticexcludeRefsset; in practice it is currently a no-op given the trigger condition (zero added-line refs implies the excluded-refs set is always empty, since everySnapshotNodecarries aref). Kept for defensiveness/design fidelity per the spec, not because it's reachable through the real pipeline today.tailcap (DIGEST_REF_LIMIT) — covered at the unit level only (response-views.tslogic mirrors the existingrefscapping, untested end-to-end at the digest response level for settle specifically, same as the pre-existingrefsfield).