Repository navigation
fix(daemon): let a finishing mutation record the tree it acted through (#3471) - #3539
Conversation
|
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 28 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Thanks for the fix. The approach looks right, but the key daemon wiring has no test yet (reviewed at b06a9e0). CI shows 6 eager-closure-budget failures in Coverage that look unrelated to this PR, since the new import edge from packages/selectors/src/interaction-error.ts to packages/contracts/src/moving-target-gate.ts is already on the base PR #3538, which this PR stacks on. Both Smoke Tests jobs are still queued, and I know of no conflicts. Two lines carry the whole fix, and no test reaches either of them. interaction-touch-runtime.ts:124 writes the baseline ( Not blocking, and you can take or leave these: the stabilize-variant JSDoc at session-state.ts:590 still points to On the open inline threads, four still apply: metadata.ts:60-62 (name selector/@ref targets), moving-target-gate.ts:124 docs (movedToRect can be absent), commands.md:425 scope (not every click/press), and iOS corroborated baseline (retires the baseline). One lower-priority thread is unverified; I did not trace it. These two do not apply, so please resolve them. r4251043034: #3471 keys the window on the last mutating action, so recording after longpress, fill or hover is intended. r4251043108: both guarded paths dispatch through Before merge, add that press-to-press daemon-route test, and fix the eager-closure growth on #3538 by giving |
b06a9e0 to
1e26155
Compare
There was a problem hiding this comment.
All reported issues were addressed across 29 files
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
View guided diff | Turn on auto-fix | Re-trigger cubic
07c2368 to
017113c
Compare
f0637b5 to
e0aa183
Compare
e0aa183 to
449db3c
Compare
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
75885ff to
abecf2c
Compare
|
The code now looks good on most of what the b06a9e0 review raised, but one test it asked for is still missing. This review covers 0510124. Fixed since the last round: the baseline selection, the iOS corroboration path and the docs wording. The eight earlier inline threads from Cubic that are fixed or intended can be resolved: #3539 (comment) (recording after fill/longpress/hover is intended), #3539 (comment) (claim now scoped to ref or selector targets), #3539 (comment) (movedToRect wording fixed), #3539 (comment) (scope of the check now stated), #3539 (comment) (chooseMovingTargetTree deleted), #3539 (comment) (preAction now forwarded on iOS), #3539 (comment) (says "dispatched"), #3539 (comment) (finishedAt bracketed in the test), and #3539 (comment) (wording now "cannot confirm a stable position"). The press-to-press test the last review asked for is still missing. Not blocking: the daemon infers "settle converged and stored" from whether the response field The one failing check is the iOS runner XCTest |
b0d3f17 to
0849065
Compare
|
The daemon-route press-to-press test is in at It replaced the injected-baseline selector refusal scenario (same Verified by deleting each line in turn:
Your non-blocking On Appium: Gate at |
|
This PR is ready. The earlier findings from 0510124 are fixed at 0849065, and no code issue remains. Not blocking: the contract test in runtime-selector.contract.test.ts (line 276) installs no runtime clock, so on a very loaded host more than 500 ms can pass between press A finishing and press B's gate read, and B would tap and fail with the same "Unexpected provider call: ios.runner.tap" error a wiring regression gives; you can add an injectable clock to the daemon-scenario harness or note the risk in the test, and either is fine. The open inline thread on the recorded window after fill, longpress and hover is resolved as intended behavior, because #3471 keys the window on the last mutating action and a test pins it: #3539 (comment). These threads are fixed at 0849065, so you can resolve them: #3539 (comment), #3539 (comment), #3539 (comment), #3539 (comment), #3539 (comment), #3539 (comment), #3539 (comment), #3539 (comment). No conflicts. Smoke Tests is still running on 0849065 and has not failed. Because this PR changes the press and click dispatch route that smoke exercises, a failure there would need a look before merge. Once Smoke Tests passes, nothing else stands in the way. I did not run the two mutation deletions. I judged the regression tests by reading the strict transcript harness and the baseline read, together with your report. The live evidence is from d7a95c683, and I did not re-check the settle.diff baseline branch or a guarded @ref press live since then. The latest change has no production logic change, so it adds no new live requirement. I also did not trace the Appium-backed Android preAction producer in resolution.ts. |
thymikee
added
the
ready-for-human
0849065 to
b3c7ddb
Compare
Gate evidence at this layer's head
Recorded honestly: the first clean attempt exited 1 on two checks neither of which this stack touches — Rebased onto this stack's layer 2 at Commit map from the head review was written against
Live-device numbers in the body were collected at Reply to the review at
|
|
The code in b3c7ddb looks right, and the one gap is that CI is still pending, so I can't call it ready yet. 12 checks are queued or in progress and none have failed. The change since my earlier review (0849065, #3539 (comment)) has no code edits: it is a restack onto 19089ee plus comment and doc edits, so no job's route is affected by the delta itself. I read the rebase by range-diff only. I did not re-review the base branch's own changes, and I did not run local tests. There are no conflicts. The two open inline threads on the shared finalizer still stand: #3539 (comment) (baseline for longpress/hover/fill) and #3539 (comment) (corroborated-failure route). Both are resolved as intended behavior and the delta does not touch them, so please resolve them. Not blocking, and you can take or leave it: movingTargetRestTree copies the body of surfaceScopedNodes and uses |
thymikee
removed
the
ready-for-human
…gh (#3471) The history the gate reads. The session's deferred surface becomes a two-variant claim — `stabilize` (the capture side still owes a quiet-window loop) or `recheck` (a mutation dispatched through a captured tree whose own effect may still be animating) — and a mutation that resolved a target records that tree at the instant it finished, so the next press can ask whether its own coordinates are still in play. ADR 0014's device side-effect seam retires the record, and a press never erases a stabilization the capture side still owes. The `targetStability` matrix cells, their Layer-3 scenarios on both runtime paths, and the caller-facing help/docs land with it: the refusal is observable from here. Closes #3471.
…3471) `chooseMovingTargetTree` preferred the session snapshot whenever its node array differed from the tree the action resolved against, reasoning that a `--settle` or `--verify` capture must have published something newer. Array identity cannot establish that: a freshness capture and a `@ref` press's authorized frame reach `session.snapshot` the same way, so the marker could record a surface this action never looked at — the exact substitution its own docblock forbids, and a `stationary` verdict made against the wrong tree. The baseline is now the acted-through tree, always. A settled press simply owes its successor one re-observation instead of being answered from a tree of unknown provenance. The corroborated-failure path also dropped the tree. That sibling call reached the same finalizer without `preActionTree`, so a corroborated iOS tap retired the baseline a previous mutation had recorded and left the next press unscreened — a dispatch that touched the device behaving like one that never looked at a tree. It now passes the resolved target's own tree. Help and website docs now scope the guarantee to what the gate actually covers: ref and selector targets on the runtime path, dispatched (not "tapped") at the resting position, `movedToRect` optional, and a raw-coordinate press explicitly outside it. Refusing to narrow the WRITER to press/click: the ADR 0011 cells scope which commands the gate guards, while this marker asks whether a mutation just left an animating surface, and a `fill` that opened a keyboard or a `longpress` that started a drag answers that the same way a press does. The breadth is now pinned by a test rather than left implicit.
…ound it (#3471) The bound was "within the last second" of assertion time, so a CI pause between the mark and the comparison could fail the test while the marker itself stayed correct. Reading the clock before the call and again in the assertion keeps the claim the test actually makes — the stamp is the mutation END, not the action's start — and removes the slack the bound needed.
…oes not cover (#3471) The paragraph claimed the check runs for any @ref or selector target, which held for neither exclusion. `click @ref` on a backend that dispatches to the element itself returns from `maybeTapRefTarget` before the guard is reached, and the Maestro non-hittable coordinate fallback is a fused runner request that never resolves a point at all. Both are now stated as the exceptions they are, and "nothing was tapped" becomes "nothing was dispatched" for the same reason the command metadata changed verb.
…d it (#3471) Removing the session-snapshot preference took the baseline back to the tree the action started from, and that broke a contract the repository already encodes: a press following a converged `--settle` spends a tap and nothing else (test/integration/provider-scenarios/settle-observation. test.ts asserts the runner transcript directly). A settled press is the case the screening exists to answer cheaply — re-observing a surface the settle already proved quiet is the round trip `--settle` was built to remove. The provenance problem the review found stays fixed. Instead of inferring "newer" from a differing node array, the finalizer forwards the action's own settle observation and the marker reads the one field that carries the fact: `diff`, which the runtime attaches only when the loop converged AND stored its settled tree as the session snapshot within one surface. `settled` alone is not that fact — a sparse capture is refused by the store and a cross-surface transition withholds the diff, and in both cases the session's newest tree belongs to some earlier capture. Everything that is not a stored settled tree keeps the acted-through tree: a timed-out loop, a lone `--verify` capture, a freshness capture, a `@ref` frame.
) Both forwards were conditional property spreads, and the complexity ceiling counts each one as a branch on the already-branching dispatch path — `check:fallow` flagged the function at threshold on the previous commit. The finalizer's parameters are optional-with-undefined, so passing the value directly states the same fact with two fewer branches: an absent settle or an unresolved target forwards undefined, which is what the marker already treats as "no such evidence".
…the target moves (#3471) The refusal covers two outcomes and the paragraph described only one. A re-check that never measured a position refuses for the same reason a target that kept moving does, and a reader told the target "was still moving" reads missing evidence as measured motion — then waits for motion that the gate never observed. Same wording the typed reason's own docblock already uses.
…not as measured motion (#3471) A `target_still_moving` refusal covers two outcomes and all three of its surfaces described only one. The gate also refuses when a re-observation never measured a position at all — the `@ref` case where reconciliation kept the authorized frame node — so "the target was still moving" reported an observation the gate had not made, in the message, in the published hint, and in the website paragraph. A caller told the target is moving waits for movement; the actual recovery there is to re-resolve the target. All three now state the confirmation that failed. The reason literal is untouched: `reason` is the branch point, and the prose around it is not allowed to be more specific than the code that raised it. The unmeasured `@ref` refusal asserts both halves of that — the new wording present, the motion claim absent — so the text cannot drift back into a claim this path cannot support.
…ed baseline reasoning (#3471)
… it (#3471) Three places said less than the code does, all found in review: - `MovingTargetBaseline.tree` named only the pre-action tree. #3538 asked for this doc to be widened when the writer landed, because it cannot be read truthfully from that layer: the converged-settle case records the tree that settle published as rest, not the one the action started from. - `markMovingTargetBaseline` listed "a raw coordinate" among the routes that leave the session idle. A point target with `--verify` or `--settle` DOES capture a tree in `resolvePointInteractionTarget`, so the idle list is the routes with no tree of their own, not the point-shaped ones. - `DeferredSurface.stabilize.baselineSignature` still pointed at `markPostGestureStabilization`, which is `markSurfaceStabilization` now. Also shrinks five comments that were narrating control flow rather than stating the invariant: the field-owner claim, the retirement comment, the carrier comment, and two guarantee cells. Line counts matter at this layer's budget, and a claim in prose is worth less than the same claim in a name.
…io trusts (#3471) The press-to-press scenario goes through the real daemon route, and production installs no runtime clock on that route, so the gate's 500 ms window runs on the wall clock: press B has to reach its gate read within 500 ms of press A finishing. A host loaded enough to starve the process past that sends B idle, and B spends the tap — the same `Unexpected provider call: ios.runner.tap` a wiring regression gives, which is exactly the failure this scenario exists to catch. The alternative was threading a `clock` through the router deps so the daemon composition could install one, for a guarantee the scenario does not own; the harness-level `createContractClock` only reaches the in-process runtime, which this scenario deliberately does not use. So the risk is named where it can be read instead of paid for.
b3c7ddb to
a99b315
Compare
Rebased onto main: gate evidence and replies at
|
| reviewed at | now |
|---|---|
dc61e911a |
be334ff09 |
35b4b5d3f |
683f1da43 |
fcb7fab0e |
a1aed7d0e |
ed4be0bd6 |
0369c8ecb |
a383bf4a7 |
bc5c09dfb |
4832560a5 |
9fefab057 |
e1e178ce4 |
7134070b7 |
c3099e23d |
d7517b117 |
7f94134e3 |
c7bacbff5 |
e9ff16137 |
acc0ee1c8 |
2c6a17794 |
220e644b3 |
b3c7ddb85 |
a99b315cb |
git range-diff reports all twelve as identical patches (=); nothing conflicted here — the one conflict in the stack was #3538's, in packages/selectors/src/interaction-error.ts against #3495's covering-node additions. Size is byte-for-byte the same as the head you reviewed: 32 files, 918 insertions, 82 deletions — 1,000 gross, still at the ceiling.
pnpm install --frozen-lockfile, pnpm build and pnpm typecheck are clean on the new base (vitest 5 included), and pnpm check:affected --run --base fix/3471-gate-measurements at a99b315cb — passed (exit 0), pnpm format first, clean worktree at the end.
The take-or-leave item: movingTargetRestTree, and the settle?.diff !== undefined test
Neither of your two shapes is one I can land honestly here, and the reason is a rule this stack already paid for once — not line budget.
Carrying SettleOutcome.settledCapture to the finalizer would replace a proxy with the fact, which is the direction this stack goes in generally. But it changes what "proved rest" means, and that is the thing your earlier review made me tighten. settle.diff is withheld in two distinct cases — the store refuses the settled tree as sparse, and a cross-surface transition (#2438) suppresses the diff — so "diff present" is the conjunction that the settled tree was actually stored and published on the surface the action acted on. That is a narrower claim than convergence, and it is the one a baseline needs: settledCapture exists for every converged run that captured, sparse refusals included, so under your version those runs would start recording a baseline whose tree the session does not hold — a tree that was never the surface anything resolved against. The test pinning today's rule is a converged settle that stored nothing keeps the acted-through tree, and it reddens the moment settle.settled or bare capture presence replaces settle.diff. So your option needs that case re-decided, not merely re-wired, and the state it would allow — a baseline rest was proved for, on a tree that was never published — is the class ADR 0014 refuses.
Moving surfaceScopedNodes into @agent-device/contracts/interaction has a cost the eager-closure gate (ADR 0027) charges without asking first. Every importer of that subpath is type-only today — all ten, import type, which the compiler erases, so the module sits in nobody's evaluated closure. A runtime export turns each into a real module edge, and the gate holds every façade at "no more modules than the merge-base evaluated". That is not hypothetical in this stack: #3538's c4d517f60 exists because importing one new contracts module pulled it and its kernel deps into six closures, packages/maestro/src/daemon-port/daemon-runtime-port.ts and packages/replay-port/src/daemon-port/session-replay-maestro-runtime.ts among them. The fallback would trade an 8-line private helper for a change that has already broken this gate once from exactly that direction, and it would have to be measured against all 842 budget cases rather than assumed.
There is also a direction question underneath the copy: no production file under src/daemon/ imports the command runtime layer today — generic-settle.ts:111 states the rule where it reaches the settle surface ("never by importing commands/") — so a shared call means either a new daemon → commands edge or the contracts change above.
What I accept from the finding is that the duplication should not be silent, and both sides of it are documented at the head you reviewed: movingTargetRestTree's docblock names the shape a baseline keeps and why a stored tree must never smuggle in more, and markMovingTargetBaseline's says why session.snapshot is consulted only under restProved, and that preferring it merely because it differs is the same forbidden substitution seen from the other side. If you want either change regardless, each is cheap as its own PR; the move would also want git diff -M90% evidence under the refactor(move) budget exemption rather than living inside this behaviour change.
The rest
- CI: green at the previous head and re-running at this one; nothing failed on either.
- The two shared-finalizer threads (
4251043034,4251043096) read resolved on this PR, as does everything else — zero unresolved review threads across all three PRs. targetStabilitycells and the Android trials, as fix(interaction): measure the moving-target gate by adoption, not bounds (#3471) #3548's comment now states: the cells and their Layer-3 scenarios are in this layer; 16 trials exist for the ADB helper and the iOS runner route; the missing Appium run and the missing delayed-start-animation row are named in the body as gaps rather than smoothed over.- Live evidence SHA: the table's numbers were collected at
d7a95c683. Nothing since has touched a dispatch path — the later commits are docs, the wall-clock note, and this re-layer.
|
This PR is ready at the code level. The two things I raised in the earlier review at b3c7ddb are now settled, and a99b315 only restacks the same patch. Not blocking: The checks are still running at a99b315 (both Smoke Tests, Repo Guards, Typecheck & Package, Integration Tests), and none has failed. The PR changes the press and click finalizer route, which Smoke and the interaction-contract integration scenarios both exercise, so a failure in either needs a look. CI was green at b3c7ddb. I did not run tests locally, and I only read this restack through a range diff. The guarded @ref press and the Appium-backed Android run are still not validated live, as the PR body says. Before merge, CI must finish green at a99b315, and the base branches must land first, #3548 (fix/3471-gate-measurements), then #3538. There are no conflicts that I know of. On the other threads, the two cubic-dev-ai P2 threads do not apply, so please resolve them. The first is #3539 (comment). Recording a baseline after longpress, hover and fill is intended by #3471, and a test pins it. The second is #3539 (comment). The failure route goes through the catch path and does not pass |
thymikee
added
the
ready-for-human
1 participant
Summary
Closes #3471. Top layer (3 of 3), on #3548 → #3538. This is the history that makes the gate fire, plus the disclosure surfaces.
A
pressresolves a point from one capture, so a press issued right after an action that animated the UI can resolve a point the control has already vacated and reportsuccess: trueon the vacancy.SessionState.deferredSurfacebecomes a two-variant claim instead ofpostGestureStabilization:stabilize(the capture side still owes a quiet-window loop) orrecheck(a mutation dispatched through a captured tree whose own effect may still be animating). A mutation that resolved a target records the tree it acted through, stamped at the instant it finished, andruntime-session.tscarries it to the command layer. ADR 0014's device side-effect seam retires it. One field, one owner (deferred-interaction-outcome.ts). Arechecknever overwrites an owedstabilize: an@refpress spends no capture, so a scroll's quiet-window loop can still be pending when that press finishes.Two judgement fixes from review:
--settleconverged and stored its settled tree — the tree that proved the surface at rest. The signal issettle.diff, which the runtime attaches in exactly that conjunction;settledalone would license recording a tree the store refused as sparse. The old rule preferredsession.snapshotwhenever its node array differed from the resolution's, which could adopt a@refframe tree or a freshness capture: a surface the action never looked at.MovingTargetBaseline.tree's doc now states both routes, which is the widening feat(interaction): add the moving-target gate the tap path consults (#3471) #3538's review asked for once a writer existed.The trigger is deliberately wider than the guard: any command that acted through a tree records a baseline, while only
press/clickare guarded — afillthat opened a keyboard can leave the surface animating for the press that follows, and that is pinned by a test.32 files, 1,000 gross (918 added + 82 deleted). This layer also lands the ADR 0011
targetStabilitycells and their Layer-3 scenarios (the root coverage gate refuses an enforced cell with no scenario), and it is where #3548's review pointed the live evidence: the gate is only reachable once a writer exists, so the cells and the device numbers could not honestly sit below here. Deviation:src/commands/schema/cli-help.tsdoes not name the refusal — the workflow topic is at a hard 9,100-byte cap; the recovery ships in the per-command usage and the website docs, which also scope the guarantee to daemon-resolved targets and name the two routes that hand off before the check.Validation
Tested at
a99b315cb. The live numbers below were collected atd7a95c683, a pre-rebase head of this branch; everything since is comments, docs wording, and the judgement fixes above, each with its own test.Android emulator
ReactNative_API_35, throwawaySettleProbefixture (native slide-in + its own hit counter, so the outcome is read from the app, not the command result);press "Probe slide[ N]"then immediatelypress 'label="Slide target"', no--settle:46dae0f2e(pre-stack, unpatched)waitedMs≈ 600stationary, 243 msnot-recent; 1 capture, none from the gateThe hit counter never moved on a refusal.
Live scope, stated rather than implied. The device runs above cover the ADB-backed Android helper and the iOS runner route. No Appium-backed Android run was made. The claim stays backend-neutral for two reasons that are checkable rather than asserted: the exposure is the daemon's own capture-to-dispatch distance (the gate sits in the shared runtime path, in front of
runtime.backend.tap, with no backend branch anywhere in it), and the baseline writer is pinned on a non-ADB backend by the new daemon-route scenario, which drives the real iOS runner conversation throughdispatchRuntimeInteractionandtoRuntimeSessionRecord. The measurement claim that is backend-specific — "reporting frames in flight" — names ADB in thetargetStabilitycell's own text. Open gaps, unchanged and stated plainly: (a) a guarded@refpress has not been re-verified live since the observation rule changed on #3548 (the probe fixture is gone and the shared emulator is held by another worker) — it is covered by theruntime-reftargetStabilityscenarios; (b) the live table has no delayed-start animation case (a control that has not begun moving when the press resolves, which is the case thestationaryscreening reads exactly like a finished transition). No device was available in this pass, so that row is owed by whoever next holds the emulator; #3538's review flagged it as needing no code change.pnpm check:affected --run --base fix/3471-gate-measurements— result recorded in a comment on this PR.typecheck,lint,format:check,check:layering(R7 field ownership),check:production-exports,check:doc-prose,check:doc-links,check:mcp-metadata,eager-closure-budgets— clean.vitest run src/commands src/daemon src/interactions src/__tests__ test/integration/interaction-contract test/integration/provider-scenarios packages/contracts packages/selectors packages/ad-script— pass, including thetargetStabilityscenarios (refusal, re-aim, stationary on both runtime paths), the gesture→press→capture test that pins the owed-stabilization guard, and the provider-integration contract that a press after a converged settle spends a tap and nothing else.check:fallow --base origin/mainflaggeddispatchRuntimeInteractionat its complexity ceiling once both forwards were conditional spreads; passing the optional values directly clears it (7134070b7).preActionTree: runtimeResult.preActionfromdispatchRuntimeInteraction, or themovingTargetBaselinecarrier fromtoRuntimeSessionRecord, each reddens the daemon-route scenario — press A through the real route, press B screened against what A left, refusing withtarget_still_movinganddispatched: noover four runner captures and one tap. That scenario is the Layer-3targetStabilityrefusal case, now run throughwithIosContractDaemonrather than an injected session record. Deleting the corroborated path'spreActionTreereddensa corroborated tap failure still records the tree it acted through; restoring the array-identity preference reddensthe baseline is the tree the action acted through, whatever the session now holds; readingsettle.settledinstead ofsettle.diffreddensa converged settle that stored nothing keeps the acted-through tree; narrowing the writer to press/click reddensthe trigger is wider than the guard on purpose.Unexpected provider call: ios.runner.tapa wiring regression gives. Pinning the window would mean threading a clock through the router deps for one scenario, so the risk is documented at the test instead.Maestroandnative-refare unguarded by declaration, not omission.