Skip to content

fix(interaction): refuse a target with no usable centre - #3842

Open
faresrafat3 wants to merge 2 commits into
callstack:mainfrom
faresrafat3:isnad/0008-agent-device-zero-area
Open

faresrafat3 wants to merge 2 commits into
callstack:mainfrom
faresrafat3:isnad/0008-agent-device-zero-area

Conversation

@faresrafat3

@faresrafat3 faresrafat3 commented Oct 20, 2026 •

Copy link
Copy Markdown

Summary

A selector that resolved to a target with no usable centre was dispatched at
the node's origin, and the command reported success. The touch-point layer now
uses the kernel's isPositiveFiniteRect guard, so a target with no extent — or
one carrying the failed-read CGRectInfinite sentinel — refuses with the
typed reason target_bounds_invalid, and no tap is dispatched.

The layer had no area rule: normalizeRect refuses only a negative
width or height, so a zero-area rect passed through — and so did the sentinel.

9 files: the guard, its unit and command-level tests, the shared fixture, two
contract scenarios with their coverage manifests, and the ADR.
rect-center.ts and the reason vocabulary are untouched; no new reason was
added.

Fixes #3841

Validation

Base 507ef2376621d78aa76cf531bedc5abdf30e7a5a, head
3ea91a37c335c58af975322378758da8f13dafdf.

  • pnpm check:affected --run --head <head> — all runnable checks passed
    (exactCommit=true): format, lint, typecheck, layering, fallow, build, vitest.
  • vitest --project unit-core packages/selectors src/commands/interaction —
    81 files, 695 tests passed.
  • vitest --project interaction-contract — 10 files, 80 tests passed.
  • Base trip (production file restored, new tests kept, isolated module cache):
    the unit cases fail naming point: {x:0,y:0}, both contract scenarios fail
    with Missing expected rejection.

Unresolved risks. The device lane was not run: the change is in the
interaction path, so the repository's gate skipped seven device lanes —
web-smoke, replay-ios, replay-ios-device, replay-macos,
replay-linux, linux-command-evidence, replay-android. The live evidence
is still owed. The full unit-core lane is red on this host for reasons that
reproduce on the base tree.

Copilot AI balanced review requested due to automatic review settings October 20, 2026 08:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

A selector that resolves to a target with no usable centre was dispatched at
the node's origin, and the command reported success. The touch-point layer now
uses the kernel's `isPositiveFiniteRect` guard, so a target with no extent, or
one carrying the failed-read `CGRectInfinite` sentinel, refuses with the
existing typed reason `target_bounds_invalid` and no tap is dispatched.

The layer had no area rule of its own: `normalizeRect` refuses only a negative
width or height, so a zero-area rect passed through it, and so did the
sentinel. The kernel's `isPositiveFiniteRect` refuses both.

The new fixtures publish the capture the issue describes: a zero-area node is
`hittable: false`. ADR 0011 records the guard's rule and names the shapes the
suite keeps, and the runtime-selector and runtime-ref contract suites prove
the refusal in the in-process runtime harness, under the errorTaxonomy cell,
before any dispatch.

Fixes callstack#3841
@thymikee

Copy link
Copy Markdown
Member

The code change in 58b739c looks right, but it is not ready to merge: it needs live device proof. CI shows one green check, and the device lanes that exercise this route were skipped for this run, so green here does not cover the changed press/click path. There are no conflicts.

This change alters which targets the runtime refuses for press, click and fill by selector and @ref on mobile (interaction-touch-point.ts). It comes from a physical iPhone SpringBoard report (#1296/#3841), and the contributor guide asks for a live run of a changed device-facing route. The PR body says the seven device lanes were skipped and live evidence is still owed. Fixtures and the contract harness only prove the fix against hand-written snapshots, so we do not yet know that the real capture yields a 0x0 or placeholder rect for the reported node, or that the real daemon refuses it. Please run this on a real iOS device or a simulator, on SpringBoard page 2 with the page-1 icons still in the tree at 0x0 and hittable:false. Show snapshot -i --json carrying the 0x0 node. Then show press 'label="Clock"' failing with COMMAND_FAILED, details.reason of target_bounds_invalid and dispatched of no, with the runner log showing no tap request. Show press @eN on the same node refusing the same way. Also show a normal selector still tapping in the same session. Please paste the command lines and JSON output in the PR.

I read the diff and the new tests against the old normalizeRect/resolveRectCenter path, but I did not run the tests or the base tree. I also cannot confirm that pnpm check:affected passed or that unit-core is red on base. No other review threads are open on this head, so the live device evidence is the only thing left before merge.

@faresrafat3
faresrafat3 force-pushed the isnad/0008-agent-device-zero-area branch from 58b739c to 3ea91a3 Compare October 20, 2026 09:38
Copilot AI balanced review requested due to automatic review settings October 20, 2026 09:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

@faresrafat3

Copy link
Copy Markdown
Author

Thanks — this is the right ask, and the disclosure in the body is what made
the gap visible instead of invisible.

First, the head moved since your read. The branch is now 3ea91a37. The
change is the same — the guard, the reason, the tap behaviour are byte-identical
— but the words around it changed: the ADR had claimed the touch-point layer
owned a local zero-area predicate that no commit in the repository ever had
(it existed only in an intermediate draft of my branch), and the zero-area
contract scenarios were filed under a guarantee-matrix cell they do not
prove. Both are fixed. If you prefer the older commit for comparison,
58b739c is still in the fork.

Your question, answered from the capture code. You wrote that the fixtures
and the contract harness only prove the change against hand-written snapshots,
"so we do not yet know that the real capture yields a 0x0 or placeholder rect
for the reported node, or that the real daemon refuses it." The first half is
answerable from the code, and the answer is that the guard does fire:

  • The real iOS capture publishes a zero-area rect for a node that is not
    laid out. The runner copies XCTest's reported frame verbatim
    (apple/runner/.../RunnerTests+SnapshotAcquisition.swift:217), and the
    presentation fold rewrites any empty clipped frame into a zero-area rect at
    the reported origin (SnapshotGeometry.swift:32-47). PresentedNode.rect is
    non-optional (SnapshotModels.swift:288-334), so the wire never omits a rect.
  • hittable is false for that node, computed by the Swift twin of the
    kernel's guard (SnapshotGeometry.isPositiveFinite, inside
    SnapshotGeometrySpace.normalized, SnapshotCoordinateSpace.swift:187-193)
    and preserved by clippedHittability (SnapshotVisibilityFold.swift:230-247).
    The runner's own invariant requires that pairing: a degenerate node is
    legal only when it is not hittable (SnapshotPresentationInvariant.swift:54-62).
    So the 0x0 / hittable:false shape is the runner's designed output, not a
    fixture's invention.
  • Therefore isPositiveFiniteRect refuses it (packages/kernel/src/rect.ts:34-41),
    on the selector path and on the @ref path, and the failure is
    COMMAND_FAILED with details.reason target_bounds_invalid and
    details.dispatched no, with no tap dispatched.

Two things the code settles that the diff does not show:

  • Why the old run succeeded. A 0x0 rect at (0,0) has its centre at (0,0),
    which is inside the viewport, so the off-screen stage
    (packages/contracts/src/snapshot-visibility.ts:78-87) never fired and the
    tap landed on the origin. The off-screen guard cannot pre-empt this fix.
  • The CGRectInfinite sentinel is a failed root/viewport read, not a node
    rect
    (5baa18f, fix(ios): make both rect guards refuse the sentinel and let the fold answer clipped carriers #2945). The bridge rewrites null and infinite frames to
    CGRectZero (RunnerAXSnapshotBridge.m:880-882) and the TS reader already
    refuses it (packages/platform-apple/src/snapshot-source/tree.ts:327-345).
    Refusing it in the shared guard is still right, because every component of
    it is finite and its centre is (0,0) — only a value guard can refuse it.

What the code does not settle. Whether XCTest on that particular iPhone
reports the page-1 icon as exactly CGRect.zero (origin 0,0) or as another
degenerate box with a non-zero origin. The published rect is zero-area either
way, so the refusal fires either way — but the literal x/y in a pasted snapshot
line is a device fact, and I will not assert it.

The live lane cannot run here. This host is Linux: no macOS, no Xcode, no
xcrun simctl, no attached iPhone, and no device-farm credentials
(BROWSERSTACK_USERNAME, the AWS device farm, limrun — all absent), so the
repository's replay-ios gate and the physical-device run are both out of
reach. I have not run the lane. The body says so, and it will keep saying so
until someone runs it on a machine that has the device.

What I can do is make the run one command away. device-evidence.sh in the
branch runs exactly the sequence you asked for, using the repository's own
commands, and prints what each output has to show:

  1. agent-device devices --platform ios, then
    agent-device open com.apple.springboard --platform ios --udid <UDID> --json
    — the open JSON carries runnerLogPath
  2. the home screen on page 2, page-1 icons still in the tree
  3. agent-device snapshot -i --platform ios --json — the Clock node must show
    a zero-width and zero-height rect and hittable: false
  4. agent-device press 'label="Clock"' --platform ios --json — non-zero exit,
    error.code COMMAND_FAILED, error.details.reason target_bounds_invalid,
    error.details.dispatched no
  5. the runner log — no command=tap line for that attempt (a
    command=snapshot line for the same attempt is expected: the daemon does
    capture a tree to resolve the selector)
  6. agent-device press @e<N> --platform ios --json — the same refusal
  7. agent-device press 'label="Settings"' --platform ios --json — exit 0 and a
    command=tap line, in the same session, so the guard is shown not to have
    broken the normal path

One question, because it decides what I do next: would the repository's
own nightly replay-ios simulator lane, which you run on macOS, satisfy the
ask — or do you need a physical-device run specifically? If the simulator lane
is enough, I will shape the change so the lane covers this route and the
evidence will be produced by your CI rather than by a human. If it is not
enough, the script above is what a human with the phone runs, and I will keep
the lane marked as owed until the output is pasted here.

@thymikee

Copy link
Copy Markdown
Member

The earlier review of 58b739c asked for live proof of the new refusal. At 3ea91a3 the code is unchanged in intent, but that proof is still missing. The one reported check passes. The seven device lanes were skipped, and they exercise the interaction route this diff touches, so green does not cover the changed press and click path. No other reviewer has looked at this head yet, and I did not run the tests or re-trace the Swift capture files, so the claim that real iOS capture gives the zero-area node is not yet confirmed.

The change in https://github.com/callstack/agent-device/blob/3ea91a3/packages/selectors/src/interaction-touch-point.ts#L37 alters which targets press, click and fill refuse by selector and @ref on mobile. The only proof now is hand-written snapshots in the in-process contract harness. The reply argues from the capture code and asks if the nightly replay-ios lane would count. It counts only if a case reaches a zero-area node. Otherwise, please paste a live run from a simulator or device. A maintainer can do this on macOS if your host cannot. The run needs these five items, on a home screen whose page-1 icons are still in the tree at 0x0 with hittable:false.

  1. snapshot -i --json showing the Clock node at width 0 and height 0.
  2. press 'label="Clock"' --json failing with error.code COMMAND_FAILED, details.reason target_bounds_invalid and details.dispatched 'no'.
  3. The runner log showing a snapshot command but no command=tap for that attempt.
  4. press @eN on the same node refusing the same way.
  5. press on a normal selector exiting 0 with a tap line, in the same session.

Please paste the command lines and JSON in the PR. That live run is what must happen before merge.

@faresrafat3

Copy link
Copy Markdown
Author

Understood — and your answer settles the question I asked: the nightly replay-ios lane counts only if a case reaches a zero-area node. It does not, so the lane is not the proof, and I will not present it as one.

The state that produces it. From the original report in #3841: on SpringBoard home page 2, the page-1 icons stay in the tree with 0×0 rects and hittable: false. press 'label="Clock"' then resolves to (0, 0). So the run needs the home screen on page 2 while page 1 is still in the tree.

The five items, as commands. On a Mac with the branch checked out:

git fetch https://github.com/faresrafat3/agent-device.git isnad/0008-agent-device-zero-area
git checkout FETCH_HEAD && pnpm install && pnpm build

# reach SpringBoard home page 2 (a left swipe by hand is the safest way)
agent-device devices --platform ios
agent-device open com.apple.springboard --platform ios --json

# 1. the node with no usable bounds
agent-device snapshot -i --platform ios --json > snapshot.json
jq '.nodes[] | select(.label=="Clock") | {label, rect, hittable, ref}' snapshot.json

# 2. the selector press refuses and dispatches nothing
agent-device press 'label="Clock"' --platform ios --json; echo "exit=$?"

# 3. the runner log for the same attempt: a snapshot command, no command=tap
#    (agent-device session list --json shows where the runner log is)

# 4. the @ref press refuses the same way
agent-device press @e<N> --platform ios --json; echo "exit=$?"

# 5. a normal target still taps, in the same session
agent-device press 'label="Settings"' --platform ios --json; echo "exit=$?"

Item 2 must show error.code COMMAND_FAILED, error.details.reason target_bounds_invalid, error.details.dispatched "no", and a non-zero exit. Item 5 must show exit 0 and a command=tap line in the runner log. If the icon on your page 1 is not Clock, substitute the label that is there — the guard does not care which icon it is, only that it has no area.

A script that runs all of it and prints what each step must show is in the repository now: device-evidence.sh at the branch head. It is not needed to produce the evidence — it only checks the shape of what you paste.

What I cannot do. This host is Linux with no macOS, no Xcode, no attached iPhone and no device-farm credentials, so I cannot run the lane myself. That is why the body says the device lanes were skipped and the live evidence is still owed.

One thing I can offer, if the simulator will not give us the state. The zero-area node does not have to come from SpringBoard: it only has to exist in the tree. The repository's own test app could carry a view with no area — a zero-size element that stays in the accessibility tree — and a replay or Maestro flow that presses it. That would make the nightly lane reach the refused path on every run, which is a stronger answer than a one-off paste, and it is the answer to "does green cover the changed press and click path". I would rather you did not have to wait for a device to be convinced. Say the word and I will add it as a second commit on the branch, with the same fail-closed shape as the rest of the change: the flow asserts the refusal, and it fails if the node is not refused.

One correction to keep the record honest. The reply above argues from the capture code — the iOS runner publishes a zero-area rect for a node that is not laid out, the runner's own invariant requires that node to be non-hittable, and the guard refuses it on both the selector and the @ref path. That is an argument from the code, not a measurement. It is exactly why the live run is what is missing, and I am not claiming otherwise.

The head is still 3ea91a37.

@thymikee

Copy link
Copy Markdown
Member

Thanks, the page-2 recipe is clear and it matches #3841. At 3ea91a3 the code verdict does not change: the live run is still the open item before merge, and a maintainer will run it on macOS.

One correction: device-evidence.sh is not in this PR at 3ea91a3, so please push it if you want it used, or drop the mention.

A zero-size view in the test app with a flow that asserts the refusal would be useful regression coverage. Should it replace the one-off SpringBoard paste? That is the maintainer's call. It proves the guard on a real capture only if the lane runs on a device or simulator, so please hold it until the maintainer answers.

The review asked for live evidence on a real device, and noted that
`device-evidence.sh` was not in this pull request — so push it or drop the
mention. This pushes it.

The script runs the evidence run and prints what each step must show; it
decides nothing. A human reads the output and pastes it into the pull
request. Every command in it appears in the repository's own CLI help.

The zero-size test-app view and the flow that asserts the refusal are not
here: the review asked for them to be held until the maintainer answers
whether that should replace the one-off SpringBoard paste, so they are held.
Copilot AI balanced review requested due to automatic review settings October 21, 2026 01:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

@faresrafat3

Copy link
Copy Markdown
Author

Pushed, and the head moved again: 314516def. The commit is one file — device-evidence.sh at the repository root — and the change under review is byte-identical to 3ea91a3, so nothing in the diff you were reading moved.

The script runs the evidence run and prints what each step must show; it decides nothing, so a human reads the output and pastes what belongs in the pull request. Its steps are the five items you asked for, in the order you asked for them:

  1. bind the session to SpringBoard on the device (agent-device devices --platform ios, agent-device open com.apple.springboard --platform ios --udid <id> --json, agent-device session list --json),
  2. capture the tree and show the node itself — its rect, its hittable, its ref — with the line that prints the node's bounds and flags that they must be zero,
  3. the selector press, which must refuse with error.code "COMMAND_FAILED", error.details.reason "target_bounds_invalid" and error.details.dispatched "no",
  4. the runner log, which must show no command=tap line for the refused attempt,
  5. the @ref press, which must refuse the same way.

It also presses an icon that is on the current page in the same session, because the proof has two halves: the guard refuses the node with no usable centre, and it does not break the normal path. That last step is the one I would not want skipped — a guard that refuses everything would satisfy steps 2 to 5 alone.

The home screen has to be on page 2 while page 1 is still in the tree, which is the state #3841 reported. The script says so at the top, and it takes UDID, LABEL, GOOD_LABEL, WORK and OUT from the environment so it can be run against a different device or label without editing it.

The zero-size view is held, not forgotten. It is not in this commit and it will not be in the next one either, because you asked for it to wait for your answer on whether it should replace the one-off SpringBoard paste. To be explicit about what I would do in each case: if it should replace the paste, I will add the view and the flow to the test app and make the flow assert the refusal, and the SpringBoard steps come out of the script; if it should sit beside the paste, I will add both and keep the script as it is now. Either way the view only proves the guard on a real capture if the lane runs on a device or a simulator, so it is worth having your answer before I write it.

On the live run itself: agreed, and I am not asking anyone to take my word for it. The lane that would cover it is the changed press/click path, and the seven device lanes were skipped on this run, so green does not cover the change. The script is there so that the run is one command and its output is pasted, rather than a paragraph describing a run.

@thymikee

Copy link
Copy Markdown
Member

Thanks for the update. The refusal change itself is unchanged since 3ea91a3, so the code review carries over, and the one check reported on 314516d is green. The new file does not yet give us the live evidence this PR owes, and it blocks merge.

In device-evidence.sh, expect() (lines 37-43) only prints $ <command>. It never runs it, although its comment says it does. In steps 2, 3 and 6 the > $OUT/*.json redirect applies to that printed text, so the result files hold prose, cat echoes the prose, and jq at line 66 fails silently. Running sh device-evidence.sh gives no device evidence and no tap or no-tap proof. Step 4 also greps the first runner.log found under ~/Library or ~/.agent-device, which may not belong to the session under test. The PR comment says this script runs the evidence run, and it does not. Please delete the file. If you keep a script, it must run the commands ("$@" > file) and fail on a non-zero exit.

Could we drop the script and post the steps as a checklist in a PR comment? 1) Open SpringBoard on the device. 2) Snapshot and show the 0x0 node. 3) Press by selector and by @ref and show target_bounds_invalid with dispatched=no. 4) Show no command=tap in runner.log. 5) Press a normal icon and show a tap in the log. Nothing needs to change first.

Not blocking: the script sits at the repository root, hard-codes the fork URL and the isnad/0008-agent-device-zero-area branch, carries 30 lines of review-history comments, and leaves an untracked device-evidence/ directory on every run. Executable tooling belongs under scripts/ and task procedures under docs/agents/, but removing the file settles all of this, so take or leave these.

Still missing is a live run on a real iOS capture (SpringBoard page 2 or similar) that reaches the refusal route. It must show the press-by-selector and press-by-@ref JSON with error.code COMMAND_FAILED, details.reason target_bounds_invalid and details.dispatched no. It must show no command=tap in that session's runner.log, and a normal-label press in the same session that still taps.

I did not run the script. The no-op result comes from reading expect() and where the redirect sits. I did not re-review the production code or check the command names against agent-device help.

Before merge, remove device-evidence.sh from the tree. Then a maintainer runs the live iOS refusal and normal-tap evidence and the results go into the PR.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

press/click: refuse a zero-area target instead of tapping its origin

3 participants