refactor(daemon): give the Maestro fallback and ambiguous-match details real types - #1612
Merged
Merged
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
Member
Author
|
Reviewed exact head Not ready yet:
All other checks are green. |
Member
Author
|
Live evidence at exact head CLI MCP — same build, Byte-identical across both surfaces, udid-first, which is the cross-surface claim. Each run used an isolated Note the run without 🤖 Addressed by Claude Code |
thymikee
marked this pull request as ready for review
August 5, 2026 11:05
…ls real types
Three places smuggled structured data through untyped bags and re-read it
with runtime guards. Each gets an explicit typed boundary.
A. The resolution-suppression rule was encoded twice in
interaction-touch-response.ts — a spread ternary in the runner-payload
branch and an unconditional destructure used conditionally in the runtime
branch, with the ADR 0012 rationale living on only one source variant.
Both branches now read one `suppressesResolutionDisclosure(source)`
predicate through one `applyResolutionDisclosurePolicy` helper, where the
reason is stated once. The union field is renamed
`maestroCoordinateFallbackDispatched` (the dispatch path that ran) and
hoisted into a shared base. handleFillCommand's two-arm interactor.fill
call collapses to one.
B. `Interactor.type` narrows from `Record<string, unknown> | void` to
`TypeTextBackendResult | void`; the Apple runner boundary is the single
place the wire payload becomes that type. `maestroFallbackDetails` returns
a typed `{ used, extra }` instead of a bag both call sites re-read.
C. `details.candidates` meant two incompatible things. The device-domain
resolvers now key their list `devices`, so the shared renderer drops its
shape-disambiguation guards and the device list actually renders.
thymikee
force-pushed
the
claude/type-interaction-boundaries
branch
from
August 5, 2026 11:10
9536876 to
50b7db4
Compare
Member
Author
|
Re-captured at the rebased exact head CLI via 🤖 Addressed by Claude Code |
Member
Author
|
Re-reviewed exact head |
thymikee
added
the
ready-for-human
|
thymikee
added a commit
that referenced
this pull request
Aug 5, 2026
Two review findings, plus a third the gate caught on itself. P1 — the three `DEFAULT_DRAG_*` constants join the existing public-façade suppression, alongside `COORDINATE_GESTURE_KINDS` and `normalizePublicGesture` which the same conversion surfaced. All five are #1567's drag vocabulary, made individually visible to `--production` analysis for the first time because a bare star used to hide them from that exact check. Kept rather than narrowed, for the reason the existing entry already states: the façade's surface stays byte-identical to what the retired pin table asserted, and narrowing is a follow-up with its own review. P2 — the exhaustiveness gate skipped any source carrying a bare `export *`, which dropped that module's DIRECT exports from the check too. `gesture-plan.ts` stars `gesture-plan-types.ts`, so removing `buildDragGesturePlan` from the façade narrowed the public surface and still passed. `readDirectNamedExports` now reads exactly the names a module declares or re-exports BY NAME and ignores the star, so direct exports are checked while the starred set stays covered by the façade's own direct re-export of that module. Red evidence: removing `buildDragGesturePlan` from facades/interaction.ts now fails naming file, source and symbol; 13 pass / 0 fail restored. Third, and the reason the gate is worth having: rebasing onto main after #1612 merged silently dropped `TEXT_ENTRY_ROUTES`, `TextEntryRoute` and `TypeTextBackendResult` from the interaction façade — the same narrowing class as the #1567 one review caught by hand, one merge later. The gate failed on it before CI did. Restored.
thymikee
added a commit
that referenced
this pull request
Aug 5, 2026
…n table (#1614) * refactor(contracts): name façade exports explicitly and retire the pin table Thirteen of the fourteen `@agent-device/contracts` façades were bare `export *` barrels. `facades/snapshot.ts`, added by #1582, was the one exception — explicit named re-exports — and that is now the rule. Everything #1574 built to cope with `export *` goes with them: scripts/layering/facade-symbols.ts -980 (816 pinned names) scripts/layering/facade-exports.ts -192 (readFacadeExports) scripts/layering/facade-exports.test.ts -234 (star semantics) scripts/layering/package-boundaries.test.ts -55 `readFacadeExports` re-implemented ESM `GetExportedNames`/`ResolveExport` — star-chain resolution, ambiguity rejection, diamond binding identity, cycle guards, spec-accurate `default` filtering at the star rather than the source. All of it existed to enumerate what `export *` hides. 523 of the 816 pinned names belonged to contracts, i.e. to those thirteen files. Once a façade names its exports, the façade file IS the pin, and it is visible in the diff of the file that widened rather than in a separate table a reviewer has to cross-check. `readNamedExports` (20 lines) stays and is enough: it already throws on bare `export *` and on `export default`. The pin is replaced by one structural gate — no façade may contain a bare star — which reuses that rejection rather than adding a regex. Surface equivalence verified independently, not asserted: main's own `readFacadeExports` run over the new façades, compared against main's own `FACADE_SYMBOLS` table — 31 subpaths, 0 added, 0 removed. Red evidence for the new gate: planting `export * from '../request-progress.ts'` back into facades/progress.ts fails it with the file named and the reason quoted; 12 pass / 0 fail once reverted. Not included: the `lowerAndroidTouchPlan` tuple-assertion drive-by. It needs `sampleGestureOffsets` to carry a min-arity tuple through `.map()`, which TypeScript will not infer without a typed helper — a real change to the gesture-plan contract rather than a drive-by, so it stays out. * test(layering): assert façades stay exhaustive over their sources Review on #1614 caught this conversion silently narrowing the public surface. The explicit lists were generated against the surface at fork time; #1567 landed 13 exports meanwhile — `DragOptions`, the drag-gesture vocabulary (`COORDINATE_GESTURE_KINDS`, `CoordinateGesturePayload`, the three `DEFAULT_DRAG_*` constants, `DragGestureInput`, `DragGesturePayload`, `GestureCommandInput`, `buildDragGesturePlan`, `dragGesturePayloadFromPositionals`, `normalizeGestureCommandInput`) and `MultiTargetAnnotationV1`. The `export *` barrels had been forwarding all 13 automatically; the rebase dropped every one, and only a human diff caught it. The star-rejection gate could not: it only proves a façade does not WIDEN invisibly. Narrowing is the failure an explicit list newly makes possible, because `export *` could not narrow by construction. So the property the stars gave for free is now asserted directly — every name a re-exported source declares must appear in the façade. Scoped to `packages/*/src/facades/`, the barrels this PR converted. A hand-curated package `index.ts` is a different thing: `ad-replay` deliberately publishes two values out of a much larger `internal/`, and forcing exhaustiveness there would widen a surface its owner narrowed on purpose (#1555). A source that itself carries a bare `export *` is skipped — unknowable from that file alone, and reachable because the façade re-exports the starred module directly too, which IS checked. Red evidence: dropping `MultiTargetAnnotationV1` from facades/replay.ts — one of the 13 the old gate was blind to — fails with the file, the source and the symbol named. 13 pass / 0 fail once restored. * fix(layering): close the exhaustiveness gate's starred-source hole Two review findings, plus a third the gate caught on itself. P1 — the three `DEFAULT_DRAG_*` constants join the existing public-façade suppression, alongside `COORDINATE_GESTURE_KINDS` and `normalizePublicGesture` which the same conversion surfaced. All five are #1567's drag vocabulary, made individually visible to `--production` analysis for the first time because a bare star used to hide them from that exact check. Kept rather than narrowed, for the reason the existing entry already states: the façade's surface stays byte-identical to what the retired pin table asserted, and narrowing is a follow-up with its own review. P2 — the exhaustiveness gate skipped any source carrying a bare `export *`, which dropped that module's DIRECT exports from the check too. `gesture-plan.ts` stars `gesture-plan-types.ts`, so removing `buildDragGesturePlan` from the façade narrowed the public surface and still passed. `readDirectNamedExports` now reads exactly the names a module declares or re-exports BY NAME and ignores the star, so direct exports are checked while the starred set stays covered by the façade's own direct re-export of that module. Red evidence: removing `buildDragGesturePlan` from facades/interaction.ts now fails naming file, source and symbol; 13 pass / 0 fail restored. Third, and the reason the gate is worth having: rebasing onto main after #1612 merged silently dropped `TEXT_ENTRY_ROUTES`, `TextEntryRoute` and `TypeTextBackendResult` from the interaction façade — the same narrowing class as the #1567 one review caught by hand, one merge later. The gate failed on it before CI did. Restored.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three places smuggled structured data through untyped bags and re-read it with runtime guards. Each gets an explicit typed boundary, so the guards and the paragraphs explaining them go away.
No user-visible behavior change except one intended fix, called out in Finding C.
A — one rule, two encodings: the Maestro coordinate-fallback resolution suppression
interaction-touch-response.tscalls itself "the single construction site … composed in exactly one place", but it encoded ONE rule two syntactically unrelated ways:Both mean "the Maestro coordinate path ran, so
resolutionDisclosureis inapplicable (ADR 0012)", and the rationale lived only in a type comment on the other source variant.Now both branches read one predicate through one helper:
The ADR 0012 reason is stated once, at
applyResolutionDisclosurePolicy, including why cell membership inmaestro-non-hittable-fallbackis usage-based (allowed-but-not-taken is still the direct path). The discriminator is renamed after the dispatch path that executed rather than the flag that permitted it —maestroCoordinateFallbackDispatched— and hoisted into a shared base so it is declared once instead of on both union arms.handleFillCommand's two-arm call also collapses:Checked before collapsing, as asked: no
Interactorimplementation distinguishes "options absent" from "options present withfalse".platforms/apple/interactions.tsspreads the runner field only when truthy (falseandundefinedboth omit it);core/interactors/{android,linux,web}.ts,provider-limrun, andprovider-webdriverignore the parameter entirely. Only a test mock recorded the arity, and it is updated to assert the single call shape across all three permission states. No bug to report here.The guarantee matrix was re-read: the
maestro-non-hittable-fallbackresolutionDisclosurecell already documents usage-based membership, and everyviapointer still namesbuildInteractionResponseData. The matrix still tells the truth; no cell changed.src/__tests__/contracts/interaction-guarantees.test.tspasses (8 tests).B — two structured outputs riding an untyped
RecordInteractor.type. #1588 widened it toPromise<Record<string, unknown> | void>purely so atextEntryRoutestring could escape, and the caller re-derived it withtypeof textEntryRoute === 'string'. It is nowPromise<TypeTextBackendResult | void>, withTextEntryRoutea closed union of the four routes the Swift runner assigns. The narrowing happens once, at the Apple runner boundary (readTypeTextBackendResult) — the trust boundary AGENTS.md designates — andhandleTypeCommand's guard is gone. Every non-Apple implementation already returnedvoid.Because the narrowing drops a route it cannot name, a Swift-side addition would otherwise silently vanish from the response. A route-parity test reads the runner sources and pins the union against every
textEntryRouteliteral the Swift side assigns.maestroFallbackDetails. ReturnedRecord<string, unknown>, and both call sites then re-readfallbackDetails.maestroNonHittableCoordinateFallbackUsed === trueout of the bag they had just constructed. It now returns a typed{ used, extra }:usedselects the dispatch path for Finding A's predicate,extrais the typed response-field set. Neither call site re-reads anything.The wide
Record<string, unknown> | voidacross the rest of theInteractorsurface is untouched pre-existing debt.C —
details.candidatesmeant two incompatible thingsformatAmbiguousMatchCandidateLinesneeded three defensive guards and a 14-line comment whose own words were "Both guards below … must hold together, or this renders[object Object]", becausedetails.candidateswasstring[]frombuildAmbiguousMatchErrorand{id, name}[]fromfindBootedAppleSimulatorWithApp.The device domain now owns its own key,
devices. The renderer moves tosrc/utils/error-candidates.ts, which declares both shapes (ElementMatchCandidateDetails,DeviceCandidateDetails) next to the rendering, and both producers construct through those types. What is left is one narrowing per key, at the JSON wire boundary, with no cross-shape reasoning and no[object Object]hazard to explain.This is the one intended visible change. The device shape previously rendered nothing at all on both the CLI and MCP text paths. It now renders, udid-first, matching what its own hint asks for:
The find handler's
AMBIGUOUS_MATCHdetails are byte-identical, key order included (locator, query, matches, candidates). Skew-safe by construction: a daemon predating thedeviceskey sends device objects undercandidates, which the string reader filters to empty — exactly today's "render nothing", never[object Object]. A regression test pins that. MCP declares no schema for error details (command-output-schemas.tsonly schemas the maestro response fields, unchanged), so no schema surface moved.Validation
Behavior preservation for the Finding A unification — proven red first. The suppression rule is a 2×2:
{runner-payload, runtime}×{fallback executed, not executed}. Three cells already existed inmaestro-fallback.contract.test.ts; the fourth (runtime / not executed) was only covered in a distant file, so it is added here and all four now sit together.Against a deliberately wrong predicate, each direction reddens exactly one cell per branch — both branches, both directions:
suppressesResolutionDisclosure → false(never suppress): 2 failed | 3 passed. Red onresponseConstruction: fallback tap response …(runner-payload) andMaestro fill of a non-hittable input …(runtime), both with+ { kind: 'not-observed', source: 'direct-ios' }whereundefinedwas expected.suppressesResolutionDisclosure → true(always suppress): 2 failed | 3 passed. Red onallowed-but-not-taken discloses direct-ios not-observed(runner-payload) andruntime fill the coordinate fallback did not execute keeps its resolution disclosure(runtime).The route-parity guard was likewise proven red: dropping
'xctest-application-fallback'fromTEXT_ENTRY_ROUTESfails with+ "xctest-application-fallback"against the Swift sources.Gates.
pnpm typecheck,pnpm lint,pnpm format:check,pnpm check:layering(1020 files, R2–R11 green after pinning the three newcontracts/interactionfaçade symbols),pnpm check:production-exports(no issues),pnpm test:unit(exit 0 — 611 files, 5390 tests),test/integration/interaction-contract/(8 files, 64 tests),src/__tests__/contracts/(22 tests).pnpm check:affected --runpassed clean on this commit: 403 files, 3801 tests, "all runnable checks passed".Residual, environmental. Later
check:affectedruns on this host flaked onprovider-scenarios/android-lifecycle.test.ts,android-recording.test.ts, anddoctor.test.ts— always a 15sTest timed out, never an assertion, with a different subset each run and all passing in isolation. Reproduced identically on a detached plainorigin/main(same files, same tests, same timeout shape), so it is host load under coverage instrumentation, not this diff. AGENTS.md contention policy. Verify the CI Integration Tests job on this PR head.No device verification: this is a type-level refactor with no device-facing behavior change. No docs or skills updated — no command surface, flag, help text, or output contract moved except Finding C's device block, which is error rendering and not documented anywhere.
19 files, +458/−162. Scope stayed inside the interaction dispatch/response family plus the shared error renderer; no expansion beyond the three findings.