Repository navigation
fix(interaction): measure the moving-target gate by adoption, not bounds (#3471) - #3548
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 5 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
db4540a to
40edc78
Compare
Gate evidence at this layer's head
$ pnpm check:affected --run
check:affected — diff origin/main...HEAD
Fail-open: selecting the full check set.
! packages/contracts/package.json [workflow-tooling] — workflow/tooling change can alter any gate
...
Selected 63 check(s):
...
[run] mutation-model: pnpm run mutation:test
check:affected: all runnable checks passed.
$ echo $?
0
|
|
I found no code problems at 40edc78, but the PR still lacks the live and contract evidence the issue asks for. CI is green with 14/14 checks passing, and there are no conflicts. I did not run the tests or the mutations; I judged that the new tests fail on the old code by reading the pre-change code. I also reasoned through the Android family (ADB helper and Appium) without running it. The surface guard does nothing there because surfaceBundleId is iOS-only, and identity and reconciliation are platform-neutral. The ADR 0011 targetStability cells are not in packages/contracts/src/interaction-guarantees.ts at this head, so the Contract requirement of #3471 is not met by this layer alone. Not blocking: measureRefObservation re-derives the adoption rule that reconcileFreshObservation already applies, and the ref look reads snapshot.iosSystemSurfaceBundleId directly while the selector look uses surfaceScopedNodes(snapshot).surfaceBundleId. Take or leave both. The first is the sharper one: if reconciliation gains a rule later, the gate could call a look measured when reconciliation did not adopt it. Could reconcileFreshObservation report what it chose (adopted from the observation, or kept the frame node) on the ref resolution, so the gate reads that value instead of matching identity and rect again? That would also let the gate compare surfaces through resolvePostActionSurfaceChange(first.preAction, second.preAction) === undefined. What would change first is a provenance field on ResolvedRefNode and the ref ResolvedInteractionTarget in packages/contracts/src/interaction.ts. The three cubic-dev-ai threads are fixed at this head, so please resolve them: movedToRect assertions, cross-surface second look, and real @ref refusal assertions. Before merge, decide whether ref adoption should come from reconcileFreshObservation. #3539 must then add the targetStability cells and live Android evidence, with at least 16 trials on both ADB and Appium, or a scoped claim naming the backend it covers. A writer of movingTargetBaseline in #3539 makes the gate reachable, so that is where the live run belongs. |
40edc78 to
19089ee
Compare
Gate evidence at this layer's head
Rebased onto this stack's layer 1 at Reply to the review at
|
|
The PR is ready at 19089ee. The earlier evidence request is now satisfied, and the code looks right to me. Not blocking, and you can take or leave it: in interaction-surface-signature.ts, The three cubic-dev-ai P2 threads are fixed at this head, so you can resolve them: the unmeasured @ref refusal assertion, the surface used for the ref look, and the real @ref refusal assertions. I read the code and the earlier-review changes. I did not run the test suite or the claimed mutations, and I did not check whether focus or gesture responses, which spread Only the Smoke Tests are not passing, and they are queued or in progress, not failed. The diff touches the @ref path in Before the gate is fully proven, #3539 needs to add the ADR 0011 |
thymikee
added
the
ready-for-human
…nds (#3471) The gate's `measured` flag accepted any capture whose rect agreed with the dispatched one. ADR 0014 keeps the authorized frame node when a captured node's identity differs, so a replacement control that took over the ref at the frame's own coordinates produced two agreeing "observations" while the dispatch went to the stale point. A look now counts only when the captured node matches the dispatched node's rect AND its local identity — the ADR 0014 adoption question in the value form that survives the session store's clone. One unmeasured look ends the recheck, so the refusal reports no `movedToRect` rather than a stale one, which is what its published shape now says. Screening for `stationary` compared positions across surfaces: a same-identity node on a newly arrived iOS system sheet could excuse both re-observations. The baseline's `surfaceBundleId` must now match the resolved surface's, the same line `post-action-surface.ts` draws. The hint interpolated a raw selector inside single quotes, so a label carrying an apostrophe taught a `wait` command that cannot run; it goes through the kernel's `shellQuoteIfNeeded` like the daemon's other recovery hints. That also moves the hint beside the refusal that publishes it, since contracts may not reach `device-shell` without growing its own eager closure. The lost-target test asserted only "not the motion refusal" and now names `ref_not_found` with `dispatched: 'no'`.
The re-observations that certify a resting position agreed on the rect alone, while the screening that decides whether to recheck at all already compares surfaces. The same line has to hold in both: an iOS system sheet presented during the sample gap carries the same control at the same rect, and the gate would have read that as rest and dispatched onto the sheet (#2438's transition, guarding the other half of the same decision). A measured look now carries the surface it was measured on, and two looks agree only on one surface. The unmeasured refusals also pin what they withhold — no `movedToRect` when no look measured a position — and the `@ref` branch of the published recovery is now asserted on a real refusal rather than in the contracts suite the hint left.
…3471) `movingTargetRectsStable` and `MOVING_TARGET_GATE_RECT_TOLERANCE_PX` copied the 4-field abs-compare against `RECT_TOLERANCE_PX = 1` that `rectsWithinTolerance` already ran for the post-gesture surface checks, and its own comment named that file as the source of the band. Two copies of one rule: change either band and the gate and the no-effect check disagree about the same pair of rects. `areCapturedRectsAtSamePosition` is that comparison, exported once from `@agent-device/kernel/rect-center` beside `normalizeRect` and `areRectsApproximatelyEqual`. It normalizes first, so a rect that reports no position never reports one position with another, and it owns the 1 px band while `areRectsApproximatelyEqual` keeps its 0.5 px band for "do these two values denote one point" — a different question, with a different reason to be strict. Both call sites now delegate: `rectsWithinTolerance`'s five surface comparisons call the helper directly, and the gate imports it in place of the contracts copy. The contracts module keeps the decision and the window; the band was never its fact to hold, so its constant and loop are gone along with the tests that pinned the constant's value. The band's assertions moved to `rect-center.test.ts` with the rule, including the per-axis 1 px / 2 px boundary and the asymmetry check the old suite made on the moved-to values rather than on the rule. Verified by mutating the band: at 0.5 px `a one-pixel shift is capture rounding, not motion` reddens, and relaxing the comparison to ignore the `width` axis reddens `the declared window is the 500 ms transition budget`'s neighbour, `one axis is enough to move`.
… from (#3471) `measureRefObservation` re-derived the adoption rule that `reconcileFreshObservation` had already applied: it looked the ref up in the capture again and required the dispatched node to match it on rect and local identity. Two implementations of one ADR 0014 step 5 decision, so the day reconciliation gains or drops a rule, the gate can call a look "measured" on an adoption the rule no longer grants — or refuse one it does grant. `reconcileFreshObservation` now states what it chose, on every branch: `adoptedFrom: 'observation' | 'authorized-frame'`. The value rides the ref's `ResolvedInteractionTarget`, which is where a caller reads the node, rect and `preAction` it dispatched, so the provenance travels with the evidence it describes rather than being inferred from it afterwards. The gate reads that field: a look measured the target exactly when the resolution says it read the dispatched node out of the observation, and the look's surface now comes from that same capture through `surfaceScopedNodes` — the way the selector branch reads its resolution's `preAction` — instead of reaching for `snapshot.iosSystemSurfaceBundleId` directly. The two refusal tests are the ones that pay for this, and they are the reason rect agreement is still not enough: a different control that took over the ref at the frame's own coordinates produces two agreeing rects, and only the reported provenance says the dispatch read them from the authorized frame. Verified by mutating the reported value on each branch: reporting 'authorized-frame' where the observation was adopted reddens `the @ref path takes its own observations, because its resolution spends none`; reporting 'observation' on either keep-the-frame branch reddens `a ref the second observation cannot re-measure is refused, never self-agreed` and `a replacement that took the ref at the frame coordinates is refused, not agreed`.
…3471) `stableCaptureSignalsEqual` kept the repository's third copy of the "did this rect move" band: a 4-field abs-compare against a local `GEOMETRY_TOLERANCE_PX = 1` that rounded each field first. `areCapturedRectsAtSamePosition` is now the one place that decides it, so the quiet window, the post-gesture surface check and the moving-target gate cannot drift apart when the band changes. The rounding was load-bearing rather than cosmetic: comparing raw backend geometry against 1 px would restart the quiet window on pairs this loop has always called at rest, because a rect quantized to the pixel can sit up to 2 px apart raw and still be one pixel apart in the unit the band is measured in. So the quantization moves to `stableCaptureNodeSignal`, the one place a signal mints a rect, and the comparison becomes the shared rule over integers. The band is therefore unchanged, stated as a test rather than as a claim: 0.51 against 2.4 is rest, 0.51 against 3.4 is not, and reverting the quantization reddens it. `GEOMETRY_TOLERANCE_PX` and the local loop are deleted.
19089ee to
0d8bf9f
Compare
Gate evidence and replies at this head (
|
|
This PR is ready. Since the earlier review at 19089ee, the moving-target gate now compares bounds through the shared rect normalizer, and I found nothing that needs a change in 0d8bf9f. Not blocking, and you can take or leave it: the settle signal in https://github.com/callstack/agent-device/blob/0d8bf9f/src/commands/interaction/runtime/stable-capture-signal.ts#L48 now goes through normalizeRect, which rejects negative width or height, while the old loop compared those sizes numerically. A signal node with a negative rounded size would then never equal itself and would restart the quiet window on every poll. Android parseBounds clamps sizes to 0 and I know of no producer of negative sizes, so a simple fix is to leave a node's rect out of the signal when it is negative or non-finite, or to note the widening in the commit message. I read the delta and worked out by hand that the new test fails on the old behavior. I did not run the test or your two-way mutation checks. I only confirmed the clamp for Android, so the iOS runner, macOS AX and web producers are unchecked for negative sizes. I did not re-check that adoptedFrom stays out of public output in this delta. CI is still running at 0d8bf9f and no job has failed. Smoke Tests and Lint & Format are queued, and Repo Guards and Compatibility & Provenance are in progress. This delta touches the settle quiet window, which the smoke settle and press routes exercise, so a smoke failure would need a look at that overlap. Before merge, those four checks need to go green. The live Android guarded @ref trials stay owed in #3539. |
1 participant
Summary
Part of #3471 (layer 2 of 3), on top of #3538. Judgement defects in the moving-target gate found in #3538's review, plus the disclosure and hint fixes they imply.
@reflook measured the wrong node.measuredaccepted any capture whose rect agreed with the dispatched one. ADR 0014 keeps the authorized frame node when a captured node's identity differs, so a replacement control that took over the ref at the frame's own coordinates produced two agreeing "observations" while the dispatch went to the stale point. A look now counts only when the reconciliation of the frame model says it read the dispatched node out of the observation (see the provenance bullet below) — the ADR 0014 adoption question, reported rather than re-derived.stationarycompared positions without comparingsurfaceBundleId, so a same-identity node on a newly arrived iOS system sheet could excuse both re-observations. It now requires one surface, the linepost-action-surface.tsalready draws.movingTargetRectsStableandMOVING_TARGET_GATE_RECT_TOLERANCE_PXwere a second copy of the 4-field abs-compare against 1 px thatrectsWithinTolerancealready ran for the post-gesture surface checks.areCapturedRectsAtSamePositionis that comparison once, exported from@agent-device/kernel/rect-centerbesidenormalizeRectandareRectsApproximatelyEqual: it takes a rect pair, normalizes first, and owns the 1 px band.rectsWithinTolerance's five call sites call it directly and the gate calls it too; both local constants and both loops are deleted. A third copy then turned up in review:stableCaptureSignalsEqual, the settle loop's quiet-window comparison, held its ownGEOMETRY_TOLERANCE_PX = 1and its own loop, and it now calls the same helper, so the rule covers the three places that ask the question and no others. That loop rounded each field before comparing, and the rounding was load-bearing: raw geometry compared against 1 px is stricter than what that loop has always run, since a rect quantized to the pixel can sit 1.9 px apart raw while being one pixel apart in the unit the band measures. With no device in this pass to re-tune a timing rule that decides how long every iOS settle waits, the quantization moved tostableCaptureNodeSignal— the one place a signal mints a rect — and the shared comparison runs over integers, which is the old band, kept as a test rather than as a claim.areRectsApproximatelyEqualkeeps its 0.5 px band — it asks whether two values denote one point, which is a different question.reconcileFreshObservationnow states what it chose on every branch —adoptedFrom: 'observation' | 'authorized-frame'— riding the ref'sResolvedInteractionTarget, andmeasureRefObservationreads that value instead of matching node and rect identity a second time. The look's surface also comes from the same capture throughsurfaceScopedNodes, the way the selector branch reads its resolution'spreAction, instead of readingsnapshot.iosSystemSurfaceBundleIddirectly.label="It's done"taught a broken command. It goes through the kernel'sshellQuoteIfNeeded, which also moves the hint beside the refusal that publishes it.One unmeasured look ends the recheck, so a refusal reports no
movedToRectrather than a stale one, and the reason's disclosure says which fields are optional. The screening tier's docblock now states that its claim is an inference from one tree and that the re-observations are the tier carrying evidence. Tests name the outcomes they were accepting-any-rejection about:refNotFoundwithdispatched: 'no', the withheldmovedToRect, and the@refbranch of the hint.13 files, 781 gross lines. Behaviour is still unreachable in production: no writer of
movingTargetBaselineexists until #3539.Where this layer's evidence is
This layer carries the code and its regression tests. The two evidence classes #3471 asks for live on #3539, and cannot live here:
targetStabilitycells (packages/contracts/src/interaction-guarantees.ts) and the contract scenarios that fill them are in fix(daemon): let a finishing mutation record the tree it acted through (#3471) #3539, because a cell must name a guarantee the shipped paths actually provide. Until fix(daemon): let a finishing mutation record the tree it acted through (#3471) #3539 lands there is no writer ofmovingTargetBaseline, so the gate cannot fire and no cell could be filled honestly.--settlebaseline path and a guarded@refpress) is likewise collected on fix(daemon): let a finishing mutation record the tree it acted through (#3471) #3539 — the layer where a press can actually be refused. The measured SHA there isd7a95c683; the open gap noted in fix(daemon): let a finishing mutation record the tree it acted through (#3471) #3539's body is a guarded@refpress and a--settle-backed baseline, both recorded live after that SHA.Validation
Tested at
d1e251aa6.pnpm check:affected --run --base feat/3471-moving-target-judgement— result recorded in a comment on this PR.typecheck,lint,format:check,check:layering— clean.vitest run packages/kernel packages/contracts src/daemon src/commands/interaction test/integration packages/selectors packages/ad-script packages/maestro packages/replay— pass.a one-pixel shift is capture rounding, not motion;adoptedFrom: 'authorized-frame'on the adopt branch reddensthe @ref path takes its own observations, because its resolution spends none;'observation'on either keep-the-frame branch reddensa ref the second observation cannot re-measure is refused, never self-agreedanda replacement that took the ref at the frame coordinates is refused, not agreed;a target newly arrived on a different surface is re-observed, not certified stationary;a surface that arrives between the two looks is not rest;@refrecovery reddensa ref the second observation cannot re-measure is refused, never self-agreed;sub-pixel geometry is quantized before the band, so the band stays in whole pixels.