Conversation
![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 52 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Reviewed at 471df4a. This needs code changes before merge, mainly to stop the new inspection route from changing existing
Not blocking, and can be taken or left: Is the layering here proportional to what this needs? The change touches the runner, platform-apple, contracts, daemon, and CLI/MCP, at 486 net production lines. A single dedicated runner command that reuses the Swift candidate filter and ordering, with its binding placed next to or inside the element-text runtime binding, would remove the readText overload, the copied bind helper, and the pass-through subpath and re-export file in one move. Positional The one reported check is green, but it does not exercise the Swift runner, so it says nothing about the runner-side behavior change or the new query cost. Two live iOS Simulator runs with the rebuilt runner are still needed: Move point inspection off the |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Reviewed at a55892e. This still has findings, and the delta did not touch the issue raised in the earlier review at 471df4a (#2999 (comment)). The readText case in RunnerTests+CommandExecution.swift:425 still returns ok:true with text nil on a miss, so readRunnerTextAtPoint in packages/platform-apple/src/interactor.ts returns undefined instead of throwing. Before this PR, a miss answered ok:false 'readText did not resolve text' and the call threw. On iOS, In RunnerTests+Interaction.swift:334, readPointAt now tries privateAXPointInspection first on every simulator call, and that route takes the smallest-area element that has any text. That drops the old readTextAt rule where text inputs (prefersExpandedTextRead, including textInputCandidatesAt) win before smaller elements, and it never falls back to XCTest when the capture succeeds. pointInspectionText also casts with In RunnerTests+Interaction.swift:391, privateAXPointInspection ignores response["truncated"] and the deep-extension pending/missed counts that privateAXSnapshotAcquisition uses (AXSnapshotFallback.swift:236-256). A capture capped at 5,000 nodes, or limited in depth, that omits the subtree under the point returns an empty element list, and that gets treated as a successful honest miss that skips the XCTest query. The 8 s deadline also does not bound the initial requestSnapshotFromClient call (RunnerAXSnapshotBridge.m:132), so the 'deadline-bounded' claim in the comment at line 330 does not hold. On a large tree, inspect-point and The comment change at RunnerTests+Interaction.swift:329 touches a device-facing simulator route, but the only evidence is Swift unit tests calling privateAXPointInspection(root:point:) directly with hand-built dictionaries. The one CI check is green, but it does not build or run the Swift runner, so it does not exercise either delta commit. Can this be validated with live iOS Simulator runs from the PR head: Isn't point inspection reusing privateAXSnapshotAcquisition's own completeness checks (truncated, privateAXDepthLimited) and privateAXFields the simpler path here, rather than parsing the raw tree a second time? Splitting point inspection into its own runner command would leave the readText route untouched and remove most of the findings above at once. Get text keeping ok:false on a miss with its text-input-first resolution, followed by the live simulator runs above, is what needs to happen before this is ready to merge. |
There was a problem hiding this comment.
All reported issues were addressed across 30 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
I pushed |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Live follow-up on |
…-point # Conflicts: # test/wire-compat/ledger.json
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Interaction.swift">
<violation number="1" location="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Interaction.swift:327">
P3: Every legacy `get text` call on an iOS Simulator now starts by capturing the full bounded AX tree (`privateAXPointInspection(app:x:y:)` calls `RunnerAXSnapshotBridge.snapshotTree` with maxNodes 5,000 and an 8-second deadline) even when the point resolves nothing. For misses and truncated captures — exactly the cases this guard falls through on — the code then also runs the legacy `app.descendants(matching: .any).allElementsBoundByIndex` query, so those calls acquire the tree twice and add the bridge capture's full latency on top of the path it was meant to replace. Consider only paying for the bridge capture when it can actually avoid the legacy query (e.g., skip the bounded read when the bridge is known-unavailable, or gate it on a cheap first check), or document the doubled cost for the common read-miss path.</violation>
<violation number="2" location="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Interaction.swift:327">
P3: `get text` output for the same coordinate can now differ between iOS Simulator and every other target: on simulators this guard resolves text from the private AX bridge (raw bridge frames and `pointInspectionReadableText`), while macOS/tvOS/physical iOS still use `readableText(for:)` over XCTest elements. The two backends can disagree about which element underlies a point (frame/containment), and the private path caps candidates at the first 24. This is the bounded-read trade-off the PR is deliberately making, but it changes the readText result surface for simulators; a provider-backed parity check asserting the same text on both paths (when both resolve) would confirm the claimed "preserve readText behavior" property rather than leaving it to live runs.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| // text fields, this uses the same text-input-first policy as that fallback. | ||
| // An incomplete capture never answers the request; it falls through to the | ||
| // legacy XCTest path so a capped tree cannot turn a real value into a miss. | ||
| if let inspection = privateAXPointInspection(app: app, x: x, y: y), |
There was a problem hiding this comment.
P3: get text output for the same coordinate can now differ between iOS Simulator and every other target: on simulators this guard resolves text from the private AX bridge (raw bridge frames and pointInspectionReadableText), while macOS/tvOS/physical iOS still use readableText(for:) over XCTest elements. The two backends can disagree about which element underlies a point (frame/containment), and the private path caps candidates at the first 24. This is the bounded-read trade-off the PR is deliberately making, but it changes the readText result surface for simulators; a provider-backed parity check asserting the same text on both paths (when both resolve) would confirm the claimed "preserve readText behavior" property rather than leaving it to live runs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Interaction.swift, line 327:
<comment>`get text` output for the same coordinate can now differ between iOS Simulator and every other target: on simulators this guard resolves text from the private AX bridge (raw bridge frames and `pointInspectionReadableText`), while macOS/tvOS/physical iOS still use `readableText(for:)` over XCTest elements. The two backends can disagree about which element underlies a point (frame/containment), and the private path caps candidates at the first 24. This is the bounded-read trade-off the PR is deliberately making, but it changes the readText result surface for simulators; a provider-backed parity check asserting the same text on both paths (when both resolve) would confirm the claimed "preserve readText behavior" property rather than leaving it to live runs.</comment>
<file context>
@@ -318,6 +318,19 @@ extension RunnerTests {
+ // text fields, this uses the same text-input-first policy as that fallback.
+ // An incomplete capture never answers the request; it falls through to the
+ // legacy XCTest path so a capped tree cannot turn a real value into a miss.
+ if let inspection = privateAXPointInspection(app: app, x: x, y: y),
+ inspection.complete,
+ let text = inspection.text
</file context>
| // text fields, this uses the same text-input-first policy as that fallback. | ||
| // An incomplete capture never answers the request; it falls through to the | ||
| // legacy XCTest path so a capped tree cannot turn a real value into a miss. | ||
| if let inspection = privateAXPointInspection(app: app, x: x, y: y), |
There was a problem hiding this comment.
P3: Every legacy get text call on an iOS Simulator now starts by capturing the full bounded AX tree (privateAXPointInspection(app:x:y:) calls RunnerAXSnapshotBridge.snapshotTree with maxNodes 5,000 and an 8-second deadline) even when the point resolves nothing. For misses and truncated captures — exactly the cases this guard falls through on — the code then also runs the legacy app.descendants(matching: .any).allElementsBoundByIndex query, so those calls acquire the tree twice and add the bridge capture's full latency on top of the path it was meant to replace. Consider only paying for the bridge capture when it can actually avoid the legacy query (e.g., skip the bounded read when the bridge is known-unavailable, or gate it on a cheap first check), or document the doubled cost for the common read-miss path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Interaction.swift, line 327:
<comment>Every legacy `get text` call on an iOS Simulator now starts by capturing the full bounded AX tree (`privateAXPointInspection(app:x:y:)` calls `RunnerAXSnapshotBridge.snapshotTree` with maxNodes 5,000 and an 8-second deadline) even when the point resolves nothing. For misses and truncated captures — exactly the cases this guard falls through on — the code then also runs the legacy `app.descendants(matching: .any).allElementsBoundByIndex` query, so those calls acquire the tree twice and add the bridge capture's full latency on top of the path it was meant to replace. Consider only paying for the bridge capture when it can actually avoid the legacy query (e.g., skip the bounded read when the bridge is known-unavailable, or gate it on a cheap first check), or document the doubled cost for the common read-miss path.</comment>
<file context>
@@ -318,6 +318,19 @@ extension RunnerTests {
+ // text fields, this uses the same text-input-first policy as that fallback.
+ // An incomplete capture never answers the request; it falls through to the
+ // legacy XCTest path so a capped tree cannot turn a real value into a miss.
+ if let inspection = privateAXPointInspection(app: app, x: x, y: y),
+ inspection.complete,
+ let text = inspection.text
</file context>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Interaction.swift">
<violation number="1" location="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Interaction.swift:327">
P2: `readTextAt` now lets a complete bound AX capture answer the legacy `get text` request even when it resolves no text, skipping the live XCTest walk entirely. The comment directly above still promises the opposite — "a capped tree cannot turn a real value into a miss" — and this block now turns a bounded, textless capture into an authoritative miss. `complete` only means the 5_000-node/56-depth caps and the deep-extension bookkeeping were not hit; it says nothing about the snapshot matching the live hierarchy. Elements with missing/empty frames are dropped by the `!frame.isEmpty` guard, and this repo's own docs (`docs/adr/0004-ios-snapshot-backend-strategy.md`, `RunnerTests+AXSnapshotFallback.swift`) treat private AX as a fallback/recovery backend that can return sparse or divergent views. So `get text` can now return `ok:false "readText did not resolve text"` for a point that genuinely holds text — the exact miss-semantics change the earlier reviews asked to avoid. Prefer keeping the fallback when `inspection.text` is nil (only short-circuit with a proven value), or prove the miss with a lightweight live probe before treating it as authoritative.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| if let inspection = privateAXPointInspection(app: app, x: x, y: y), inspection.complete { | ||
| // A complete miss is authoritative too. Falling through would repeat a | ||
| // full XCTest descendant walk after the bounded AX capture already | ||
| // established that no readable element contains the point. | ||
| return inspection.text |
There was a problem hiding this comment.
P2: readTextAt now lets a complete bound AX capture answer the legacy get text request even when it resolves no text, skipping the live XCTest walk entirely. The comment directly above still promises the opposite — "a capped tree cannot turn a real value into a miss" — and this block now turns a bounded, textless capture into an authoritative miss. complete only means the 5_000-node/56-depth caps and the deep-extension bookkeeping were not hit; it says nothing about the snapshot matching the live hierarchy. Elements with missing/empty frames are dropped by the !frame.isEmpty guard, and this repo's own docs (docs/adr/0004-ios-snapshot-backend-strategy.md, RunnerTests+AXSnapshotFallback.swift) treat private AX as a fallback/recovery backend that can return sparse or divergent views. So get text can now return ok:false "readText did not resolve text" for a point that genuinely holds text — the exact miss-semantics change the earlier reviews asked to avoid. Prefer keeping the fallback when inspection.text is nil (only short-circuit with a proven value), or prove the miss with a lightweight live probe before treating it as authoritative.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Interaction.swift, line 327:
<comment>`readTextAt` now lets a complete bound AX capture answer the legacy `get text` request even when it resolves no text, skipping the live XCTest walk entirely. The comment directly above still promises the opposite — "a capped tree cannot turn a real value into a miss" — and this block now turns a bounded, textless capture into an authoritative miss. `complete` only means the 5_000-node/56-depth caps and the deep-extension bookkeeping were not hit; it says nothing about the snapshot matching the live hierarchy. Elements with missing/empty frames are dropped by the `!frame.isEmpty` guard, and this repo's own docs (`docs/adr/0004-ios-snapshot-backend-strategy.md`, `RunnerTests+AXSnapshotFallback.swift`) treat private AX as a fallback/recovery backend that can return sparse or divergent views. So `get text` can now return `ok:false "readText did not resolve text"` for a point that genuinely holds text — the exact miss-semantics change the earlier reviews asked to avoid. Prefer keeping the fallback when `inspection.text` is nil (only short-circuit with a proven value), or prove the miss with a lightweight live probe before treating it as authoritative.</comment>
<file context>
@@ -324,11 +324,11 @@ extension RunnerTests {
- let text = inspection.text
- {
- return text
+ if let inspection = privateAXPointInspection(app: app, x: x, y: y), inspection.complete {
+ // A complete miss is authoritative too. Falling through would repeat a
+ // full XCTest descendant walk after the bounded AX capture already
</file context>
| if let inspection = privateAXPointInspection(app: app, x: x, y: y), inspection.complete { | |
| // A complete miss is authoritative too. Falling through would repeat a | |
| // full XCTest descendant walk after the bounded AX capture already | |
| // established that no readable element contains the point. | |
| return inspection.text | |
| if let inspection = privateAXPointInspection(app: app, x: x, y: y), | |
| inspection.complete, | |
| let text = inspection.text | |
| { | |
| return text | |
| } |
|
Thanks for the update. Commit 612f054 fixes part of the earlier findings, but the runner still has two defects and no live evidence on this head. The single CI check is green, but it does not build the Swift runner: check:packaged-runner-swift only runs
The changed runner routes are device-facing: the Not blocking, take or leave: the new 2 s cap ( Before merge, please move |
This branch has not been deployed
2 participants
Summary
Validation
Live verification boundary
The repository's GitHub-authoritative iOS runner lanes and a live iOS Simulator proof are still required. This change does not claim live Messages, share-sheet, action-extension, or sticker-surface verification from the local run.