Repository navigation
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 11 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
The Limrun alert legs duplicate the Android tree-alert driver, and that needs to change before merge (reviewed at 4a1862b). The Is there a reason The two threads on Cubic's confirm-on-ambiguous-capture (alert.ts:173) and duplicate-label skipping (alert-detection.ts:152) should be settled once in that shared driver. Please also validate on a live Limrun run that These open threads still apply: pid boundary comment, CHANGELOG app dialogs, plus six lower-priority test and fixture threads. This docs thread does not apply, because the page describes how the backend acts and gives accurate user guidance; please resolve it. The failing Smoke Tests job runs the Apple XCTest-runner simulator smoke. This diff touches only |
…red driver PR #3699 review: the Limrun iOS alert legs duplicated the Android tree-alert driver (poll, confirm, same-alert, response building) and its label vocabulary. Both backends now reach those steps through one backend-neutral driver, runTreeAlertAction in platform-android (exported via the mechanics facet provider-limrun already depends on), and classify button roles through one English vocabulary that lives in contracts/alert-contract with the alert budgets and typed reasons (contracts owns vocabulary only; the driver loop owns the AppError/timer mechanics R18 keeps out of contracts). Each backend supplies only its own seams: Android wraps its dump reader and keeps its platform-id roles, neutral/Back fallbacks, and tap/Back presses; Limrun supplies the blank-root process-exclusive host reader, the label-strict chooser, and client.tap. The confirm-on-ambiguous-capture and duplicate-label threads settle once in the shared code: the three-way alert|none|undecided observation gates confirmation in the driver, and selectUniqueAlertButtonByRole refuses duplicated roles at the shared choice boundary. The deadline order resolves to read-first: over a positive window both predecessor loops already observed before waiting. The alert-not-found and undecided refusals now carry the same typed details.reason and dispatched: 'no' on both backends.
|
Reviewed a27302c. The earlier findings are fixed except the ones below, and one new regression remains in the shared button choice. The 20 checks pass at this commit, but the Android alert suites pass only because no fixture has a multi-grant permission sheet. I know of no conflicts.
The PR body still lists the final-head live evidence as pending, and the device route has changed since the earlier fail-before run: it now goes through the shared driver, read-first deadline order and the uniqueness boundary. Unit fixtures cannot show that the Limrun tap lands, or that the confirm leg sees I could not read the Limrun captures behind the "no hittable key" claim, so I accepted it on consistency. I did not run tests, and I did not confirm on a live Android 11+ device that both grant buttons appear with Simplicity question: could the shared driver and vocabulary stay, with the choice boundary as a declared role-plus-preference table in On the open Cubic threads: these are fixed at this head and can be resolved: blank-root pid subtree, changelog residual, Android dismiss retry, typed refusal reason, driver reason test, one-tap dismiss test, and fixture comment. The label dedupe thread is fixed too, but the fix went too far into role-level uniqueness, as described above. Two threads do not apply: hittable:true because captures omit the key and only an explicit false vetoes, and limrun.md wording because the page describes backend behavior accurately. Before merge, please restore single-press accept on multi-grant permission sheets with regression tests, then post the live Limrun ATT evidence. |
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 13 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
The earlier gaps from a27302c are fixed at 8d5e9d2: the preference table now lives in the shared alert contract, each backend keeps its own fallback, and the regression tests cover the earlier cases. Live proof at this head is still missing, and the cancellation work added in this round goes beyond what #3349 asks for. The device route changed again in this round: ranked choice, window-guarded observer, signal-gated press, and a separate confirm window. The PR body says final-head live verification is still outstanding, and unit fakes cannot show that the Limrun tap lands once or that confirm sees Is the cancellation and window apparatus needed for #3349? It adds a second feature to the shared driver: request signals through every seam, wrapped errors keyed on the window-expired reason, Not blocking, and you can take or leave these: alertSeamInterrupted is declared to return an error but throws in its canceled branch, and the request signal reaches the backends through two channels ( On the open threads from other reviewers, these still apply: the Limrun capture ignoring the seam signal, same-label buttons on Android, unranked accept filtered before the tie check, the docs on alert-window-expired, the changelog claim about bounded budgets and the AbortSignal.timeout overflow. The adb executor options thread does not apply, because every production caller passes only CI is still pending: all 9 non-passing jobs are queued or in progress, with no failure logs yet. The diff overlaps Lint/Typecheck, Coverage and possibly Android smoke, while the Apple smoke lanes do not overlap it. I did not run the tests, and I did not reproduce the "20 ms wait returns alert at 59 ms" case. Before merge, this needs the live Limrun iOS and Android 11+ runs at the final head, and a decision on moving the cancellation work to its own PR. |
…red driver PR #3699 review: the Limrun iOS alert legs duplicated the Android tree-alert driver (poll, confirm, same-alert, response building) and its label vocabulary. Both backends now reach those steps through one backend-neutral driver, runTreeAlertAction in platform-android (exported via the mechanics facet provider-limrun already depends on), and classify button roles through one English vocabulary that lives in contracts/alert-contract with the alert budgets and typed reasons (contracts owns vocabulary only; the driver loop owns the AppError/timer mechanics R18 keeps out of contracts). Each backend supplies only its own seams: Android wraps its dump reader and keeps its platform-id roles, neutral/Back fallbacks, and tap/Back presses; Limrun supplies the blank-root process-exclusive host reader, the label-strict chooser, and client.tap. The confirm-on-ambiguous-capture and duplicate-label threads settle once in the shared code: the three-way alert|none|undecided observation gates confirmation in the driver, and selectUniqueAlertButtonByRole refuses duplicated roles at the shared choice boundary. The deadline order resolves to read-first: over a positive window both predecessor loops already observed before waiting. The alert-not-found and undecided refusals now carry the same typed details.reason and dispatched: 'no' on both backends.
8d5e9d2 to
482cff1
Compare
|
At 482cff1, the earlier gaps are mostly closed, but two defects remain, and the Android smoke check now fails because of one of them. The earlier a27302c review asked for a The Android smoke step Localized Android permission sheets also regress, at https://github.com/callstack/agent-device/blob/482cff1/packages/contracts/src/alert-contract.ts#L118. Could the PR get smaller? Keep the preference table, but rank by backend-declared keys: label rank for Limrun, permission-id rank for Android. Then drop the per-capture window code ( The open inline threads on same-label buttons in The failing Android smoke step looks related to this change. Only the new Before merge, please stop the per-capture window from refusing the first Android capture, resolve the conflicts, and then show two live runs at the final head. First, the Android smoke |
…red driver PR #3699 review: the Limrun iOS alert legs duplicated the Android tree-alert driver (poll, confirm, same-alert, response building) and its label vocabulary. Both backends now reach those steps through one backend-neutral driver, runTreeAlertAction in platform-android (exported via the mechanics facet provider-limrun already depends on), and classify button roles through one English vocabulary that lives in contracts/alert-contract with the alert budgets and typed reasons (contracts owns vocabulary only; the driver loop owns the AppError/timer mechanics R18 keeps out of contracts). Each backend supplies only its own seams: Android wraps its dump reader and keeps its platform-id roles, neutral/Back fallbacks, and tap/Back presses; Limrun supplies the blank-root process-exclusive host reader, the label-strict chooser, and client.tap. The confirm-on-ambiguous-capture and duplicate-label threads settle once in the shared code: the three-way alert|none|undecided observation gates confirmation in the driver, and selectUniqueAlertButtonByRole refuses duplicated roles at the shared choice boundary. The deadline order resolves to read-first: over a positive window both predecessor loops already observed before waiting. The alert-not-found and undecided refusals now carry the same typed details.reason and dispatched: 'no' on both backends.
482cff1 to
5b1e1c1
Compare
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Limrun iOS direct sessions refused all four alert legs with unsupported-provider-mode even though the session's element-tree capture already shows the system alert and its touch seam can press its buttons. Issue #3349. Detect the alert from the capture: a system alert arrives as its own blank-label AXApplication root hosting text and pressable buttons, while the app's own UIAlertController rides the app's named root, so app-owned dialogs stay app UI. Accept and dismiss press exactly the button whose label states the asked-for choice and confirm the alert is gone; an ambiguous choice is reported, never guessed, and dismiss never authorizes tracking. The four legs bind through the same catalog and facts the Android tree-based legs already use.
Review follow-up on the alert legs: a sheet must offer at least two pressable choices, launch-icon buttons never qualify, a capture whose host sheets disagree is refused instead of shadowing one with the other, and an action fires only when exactly one button states its choice. The legs' poll/confirm budgets now spend through an injected sleep seam so their tests drive the clock, a malformed timeout fails as a bounded default instead of an unbound loop, and the grant vocabulary covers the iOS variants (Allow Once / While Using App / All the Time) and Close App. User docs and CHANGELOG updated.
…red driver PR #3699 review: the Limrun iOS alert legs duplicated the Android tree-alert driver (poll, confirm, same-alert, response building) and its label vocabulary. Both backends now reach those steps through one backend-neutral driver, runTreeAlertAction in platform-android (exported via the mechanics facet provider-limrun already depends on), and classify button roles through one English vocabulary that lives in contracts/alert-contract with the alert budgets and typed reasons (contracts owns vocabulary only; the driver loop owns the AppError/timer mechanics R18 keeps out of contracts). Each backend supplies only its own seams: Android wraps its dump reader and keeps its platform-id roles, neutral/Back fallbacks, and tap/Back presses; Limrun supplies the blank-root process-exclusive host reader, the label-strict chooser, and client.tap. The confirm-on-ambiguous-capture and duplicate-label threads settle once in the shared code: the three-way alert|none|undecided observation gates confirmation in the driver, and selectUniqueAlertButtonByRole refuses duplicated roles at the shared choice boundary. The deadline order resolves to read-first: over a positive window both predecessor loops already observed before waiting. The alert-not-found and undecided refusals now carry the same typed details.reason and dispatched: 'no' on both backends.
origin/main migrated the Android alert loops to pollUntilSettled/Deadline and moved timing into fake timers at the test edge. The shared driver adopts the same shape: poll/confirm run on the deadline primitive, so the driver-owned sleep seam is dead weight and goes — Limrun's tests now drive the clock the way the migrated Android tests do.
…y catch-name lint Adds the maintainer's live-validated sequence at the unit seam: accept taps exactly once, and a following get on the same seam reports no alert. Renames catch params to satisfy oxlint's catch-error-name (three findings the affected gate reported).
The unique-role choice boundary refused exactly the sheets the fix is for: Android 11+'s permission_allow_foreground_only_button and permission_allow_one_time_button both classify accept, and iOS's 'Allow Once'/'Allow While Using App' sheet refused the same way. The shared vocabulary is now a per-role preference table: distinct grants resolve by declared rank (a lasting grant before a one-time grant), and the acting legs refuse only when the capture names no single physical button — the same label at two points, or vocabulary-free multiplicity. Android keeps its neutral-button/Back fallback, Limrun keeps none.
… owns Three review findings on the shared tree-alert driver, plus the test that masked a polling loop: - alert get on a tree backend answered undecided exactly like absence (alert: null, message-only difference). The result now carries a typed status (visible/none/undecided) so clients switch on states, not text. - The budgets bounded only the poll sleeps: a capture or press that hung could outrun the leg. Each leg now builds a window (deadline clock plus the joined window/caller signal) that reaches the capture and press seams; the Android legs hand it to the snapshot capture and the adb input sends, which kill their child on abort. A capture that lands once its window closed is refused on a typed alert-window-expired reason and never consumed or pressed; a press cut off mid-flight says dispatched: unknown, one refused before dispatch says no. - Limrun's WebSocket transport exposes no abort on elementTree/tap (checked against the shipped typings), so the consumption rule is what keeps it honest, and the SDK gap is stated rather than papered over. - The awaitAlert first-capture test now asserts the capture count; its same-alert fixture had made it vacuous.
The shared tree-alert driver consumed an answer from a poll that started after its window closed (the loop tests settled before remaining, and an expired-at-start probe rode unguarded), and its first probe paid the Android helper cold start inside the action budget, aborting the very read that leg was about to press on — the executed smoke failure. The first observation is now read-first, unarmed, consumed once, and arms the window behind it; a later probe starting past the deadline never captures at all, and the press rides the armed window. - contracts: a mix of ranked and unranked role candidates refuses instead of letting the one known label win (selectRankedAlertButton- ByRole); MAX_ALERT_WINDOW_MS names the timer ceiling the daemon refuses above (INVALID_ARGS) and the driver defaults above. - android: same-label dedup keys on the physical center, so nested nodes collapse and two distinct OKs reach the shared refusal; the input dispatch option type owns cancellation only, and the tap forward picks fields so allowFailure cannot reach the transport. - docs: alert-window-expired/none/undecided reasons stated exactly per phase, Limrun's non-abortable transport described as post-capture and pre-press guards with the hung-request limit named. Prose at the driver trims to invariants.
…confirmed press The executed Android smoke failure at 5b1e1c1: the session's alert probes ran without the app-backed helper session scope, so each probe cold-started the snapshot helper inside the window that bounds it — the request log shows the post-press confirm capture aborted at 2.3s ("operation was aborted due to timeout") while the canary proves the dialog was in fact dismissed. Alert reads now carry the same helperSessionScope as every other app-backed read. The failure also proved the wording lied: with no confirm capture ever answering, "did not dismiss the visible alert"/"dialog is still visible" accused a dialog that was already gone. Only a re-read naming the same alert earns the still-visible failure; an unconfirmed press now says exactly that, on both the cut-off and the spent-budget exits, with a hint to run alert get before retrying. The same-alert stuck-alert failure keeps its message. Pinned: interactor scoping regression (fail-before against 5b1e1c1) and the three Limrun confirm-phase tests moved to the truthful split; the Android still-visible pin keeps its message via the same-alert path.
Two review findings on the confirm split, one batch: - The spent-budget exits of confirmTreeAlertDismissed omitted details.reason, while commands.md promises every post-press failure branches on alert-window-expired. Both unconfirmed and still-visible failures now carry the typed reason, pinned in the Android and Limrun suites. - The same-alert flag latched on the first matching capture, so a later undecided re-read still reported the dialog as currently visible. Each probe now replaces the verdict with its own observation: the still-visible failure requires the latest capture that answered to name the same alert, and a fresher undecided read exits as an unconfirmed press. A probe whose capture never answered is not a completed observation, so the thrown cutoff keeps saying only that the press went unconfirmed, at any earlier match. Pinned fail-before at the pre-fix alert.ts: same-alert then undecided to budget-end, and same-alert then a hung capture, both expect the unconfirmed message and the typed reason. The helper-scope comment is trimmed to the non-obvious constraint per AGENTS.
2482e24 to
f584e8a
Compare
|
Head Android smoke step. The Live proofs at this head (repo-built CLI / instrumented real client seam, isolated state):
Receipts: direct gate Kept honest, not widened: Limrun SDK 0.64.1 exposes no abort on |
…swered sighting The Coverage run at f584e8a (38924441176) failed the maintainer-owned provider-integration scenario 'Android alert accept fails when the dialog stays visible' — a suite the affected gate's related-test selection does not reach, so the local exact-head receipt never ran it. The driver, not the oracle, was wrong: f584's confirm catch downgraded every thrown cutoff to unconfirmed, discarding the captures that had answered before it. On CI the final confirm capture starts inside the 2s window and lands past it (the thrown path); locally it starts after the deadline (the sentinel path), which is why the same commit passed one and failed the other. The dialog had been re-named by answered reads; the error claimed the check never answered. The latest-completed-observation rule from review thread r4296070770 and from commands.md is: the latest capture that ANSWERED carries the claim. A hung probe is not an observation, so the cutoff now reports still-visible when an answered capture named the alert, and unconfirmed only when none did — the 2482e24 smoke case, with no answer at all. The undecided-overrules-match case is unchanged: undecided is an answered observation. Pin flipped to the CI timeline (fail-before at f584's catch); all 128 alert-family tests and the 197-test provider-integration project pass at this working tree.
The independent review of f584e8a returned merge-ready with three follow-up-scale findings, batched here: - The documented `dispatched: 'unknown'` press-cutoff failure had no test. Pinned in the Android suite with the real transport shape: the adb fake hangs the tap until the 2s action window closes and dies with the signal; the failure must disclose unknown, never 'no'. Mutation-checked: flipping the disclosure to 'no' fails the pin. - The `[timeout]` positional is honored by `alert wait` only — the Apple acting legs pass fixed runner budgets and the tree legs run their own lookup/confirm windows; commands.md now says the argument is wait-only per backend instead of leaving the silence to surprise callers. - `void device;` in limrunAlertOperationFacts follows the `_device` convention; the two byte-identical visible-after-press exits collapse into one helper, keeping the two hints that genuinely differ (latest read hung vs budget ended unresolved). No behavior change for any message, reason, or tap.
|
Coverage run 38924441176 at Fixed at |
|
Receipts for
No further changes planned on my side for the alert family at this head. |
This branch has not been deployed
1 participant
Summary
Closes #3349. Limrun iOS
alert get/wait/accept/dismissride the element-tree capture snapshots already use and the existing tap seam, through one tree-alert driver and one English button-label vocabulary shared with Android. Ambiguous captures refuse untapped on typed reasons;dismissnever authorizes ATT;alert getreturns typedstatus: visible|none|undecided.Validation
Named head
957726b0de7e2049b556a0b58efceca0e4af31a6, rebased on59fdf008b.pnpm check:affected --run --head 957726b0d…passed,exactCommit: true, clean checkout (.tmp/validation/run-Sh88Va/receipt.json). That selection deduplicates provider-integration, so it ran explicitly too: 197/197, plus 131 alert-family tests.f584e8a1failed one real test — thedialog stays visibleoracle — because the confirm catch let a deadline-straddling hung capture erase answered same-alert sightings. Fixed atf4b928974in the driver, oracle untouched: the latest capture that answered carries the claim; unconfirmed only when none did.accept→ "While using the app",granted=true, secondgetnone;dismiss-on-none typedalert-not-found. Live Limrun iOS (instrumented seam): accept-once → none;dismissdeclines once, never authorizes; location preferred grant in one tap; app dialog refused, zero taps. The two newer commits change only the cut-off error path;dispatched: unknownis pinned, mutation-checked.957726b0d); round 2 merge-ready, no new findings, oracle verified unmodified and all four path×evidence cases pinned. All seven threads resolved.Limits: Limrun SDK 0.64.1 cannot abort in-flight
elementTree/tap; native Swift alert choice is #3627.