Repository navigation
fix(errors): typed reasons for the session-app and session-or-selector refusals - #3194
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 15 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Thanks for the PR. The code looks right at e9c926f, and all 21 checks pass. Nothing blocks the code, but one piece of evidence is still missing: I did not run the live iOS e2e cleanup harness, so I have not seen a real appless Not blocking, and you can take or leave these: (1) the e2e cleanup test in Would one Once the live run output is in, merge after you triage the notes above. |
…lector refusals Both refusals answer before anything reaches a device, so a driver must branch on details.reason plus dispatched:no rather than matching the message. Declares the two reason values once at the owning kernel type, with the construction helpers and their normalize/wire coverage. Refs #3181
…refusals Both owners already refused an appless `settings permission` and an appless iOS `settings location`; each site now states the shared reason instead of leaving prose as the only signal. Both platforms use one reason because the recovery is the same whichever setting asked. The Apple and Android error assertions gained reason and dispatched so the behavior is keyed on details, not on the message. Refs #3181
…e response The daemon refuses a device-bound command before it resolves a target, which is exactly the case where dispatched:no makes a retry safe. The refusal now carries the published reason through errorResponse, and the request-router test proves the reason survives finalizeDaemonResponse to the caller on the returned-error path rather than only at its construction site. Refs #3181
…its message The mic-permission cleanup skip matched an exact error string, so a reworded refusal would silently resume retrying a command that can never succeed. It now keys on details.reason, and the suite asserts a byte-identical message with no reason still exhausts its retries — the prose alone must never activate the guard. Documents both published reasons with their recovery in the user docs. Refs #3181
…it the details check The affected audit surfaced both test helpers as dead exports and flagged the cyclomatic growth from the new reason/dispatched branches. Removing the sync assertion, which no test imported, and moving the machine-readable checks into their own function keeps each assertion at the complexity it had before. Refs #3181
…al family Every per-app settings refusal must be identifiable by reason, not only the ones first converted: the daemon's clear-app-state pre-check and the Apple, Android, and HarmonyOS owner throws now answer with session_app_required and dispatched:no as well. The clear-app-state doc bullet states the refusal. Docs corrections: session_app_required is raised by the settings handlers after device routing, so the dispatched vocabulary entry describes only the session-or-selector refusal as pre-routing; and the permission bullet no longer claims Android on/off location or macOS system-level permissions need a session app, which they do not. The e2e skip's success case now pairs the reason with different message wording, so a guard that also required the legacy string would fail: only the reason triggers the skip.
…on_app_required A setting that takes an app id, like clear-app-state, recovers by naming the app on the request itself; scripts need not open the app just to satisfy that refusal.
PRE_DISPATCH_REFUSAL_REASONS states the kernel-owned subset whose refusals always disclose dispatched:no, instead of a name that implies it holds every published reason (the WebDriver and other producer reasons live beside their own disclosures). The map's own doc names the versioned docs as the wire vocabulary and the disclosure table as the producer registry. The session-or-selector refusal gains its daemon.route row in contracts/fixtures/dispatch-disclosure.json, driven through the real router with an unopened session. That command declares no recording effect, so the dispatched:no the row asserts can only come from the producer rather than the read-only seam. The e2e cleanup fixture builds its refusal from sessionAppRequiredDetails() instead of re-typing the details shape, and the permission bullet stops implying a request can name an app for settings permission while #3179 is open.
e9c926f to
14dd8b7
Compare
Live run (the missing evidence)Rebased onto $ node bin/agent-device.mjs settings permission reset microphone --platform ios --device "iPhone 18 Pro" --json
{
"success": false,
"error": {
"code": "INVALID_ARGS",
"message": "permission setting requires an active app in session",
"hint": "Check command arguments and run --help for usage examples.",
"diagnosticId": "mutfjxp0-f45edb81",
"details": { "reason": "session_app_required", "dispatched": "no" }
}
}That is the real route: daemon handler → bound Notes triagedFixed on
Not taken:
Your two questionsKept the two named no-arg factories. Can't reuse
|
|
The typed reasons for the session-app and session-or-selector refusals look right at 14dd8b7, and I found no code problems. The five Cubic threads (docs wording, the clear-app-state details on all owner sites, the permission and location scope, and the test fixture wording) are fixed at this head, so you can resolve them: #3194 (comment), #3194 (comment), #3194 (comment), #3194 (comment), #3194 (comment). The evidence gap from the earlier review is now closed. I read the code and did not run the tests, including the new daemon.route driver. The Android and HarmonyOS owner sites, the daemon pre-check and the session-or-selector refusal are covered only by unit and owner tests, which is fine because the refusals come before any device call. I could not confirm the live-run transcript beyond the PR body. Smoke Tests is still running, so no check has failed yet. There are no conflicts. Nothing is left from review. Before merge, Smoke Tests needs to finish and Cubic needs to review this head. |
thymikee
added
the
ready-for-human
1 participant
Summary
Two
INVALID_ARGSrefusals had nodetails.reason, so a client could only recognise them by matching message text. Both now answer with a reason declared once at the owning kernel type, plus thedispatched: 'no'disclosure that makes a retry provably safe. Messages and hints are unchanged.session_app_requiredcovers every appless per-app setting refusal —settings permission, on/offsettings location(iOS), andclear-app-state— at the daemon's pre-check and at the Apple, Android, and HarmonyOS owners.session_or_device_selector_requiredis the daemon's refusal raised before routing.The e2e cleanup harness keys its appless-skip on the reason instead of an exact string. Vocabulary follows
webdriver_route_unsupported: declared once, documented, and registered as adaemon.routerow in the dispatch-disclosure fixture. 22 files, +380/-39; production: kernelerrors.ts, Apple/Android/HarmonyOSsettings, the daemon settings pre-check,session-device-resolution. Relates to #3179.Closes #3181
Validation
pnpm check:affected --runpasses at14dd8b7(baseorigin/main): unit lanes, fallow audits, daemon wire-compat (protocol unchanged), command-docs.typecheckandlintclean. CI on this head: pending.Live run of the changed route (booted iPhone 18 Pro, no session, this checkout's build):
Plus: kernel round-trip through
normalizeError/throwDaemonError; a router test on the returned-error path for the selector refusal (#1391pins the same carry for a thrown error); owner-level assertions at all four settings call sites.