feat(cli): open --wait, flag suggestions, and refusals that name what to run - #2665
Conversation
|
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed da2d5f8. Placing the wait before the device lock looks right, but there seems to be a race after the wait ends, so I can't call this ready yet.
Not blocking: the wait tests call Would a smaller wait do? Resolve the target device once, through the same helper One thing I could not confirm from the code: does the Node and MCP All checks pass and there are no conflicts. The next step before merge is closing the race between the end of the wait and the device lock, with a test that shows a second waiter opens, or refuses only after its full budget. |
|
Addressed in ffc2599. The race is closed the way you framed it. With the retry in place the two minimums fell out rather than needing their own wording: the spend is recorded only at that under-lock look, when a holder is visible and the deadline is gone, so "Waited Xms for this device and it stayed busy" is only ever said by an open that really spent the budget; and a refusal whose wait vanished between the look and the lock now reports no spend at all. Tests: The smaller wait, taken. Bounds: MCP yes, Node no — fixed on the daemon.
|
|
Review fixes are now at While proving the new bounds on the live Node client, I found a sequencing hole: an out-of-range Live results and current gate status are recorded in the PR body. The emulator has no sessions left from this validation. |
|
Reviewed f4f4129, following up on da2d5f8 (#2665 (comment)). The race fix itself looks right: the look under the device lock now decides, an open that loses the race releases its locks and re-waits on its remaining budget, cancel still stops the re-wait, and the live runs reported in the PR body cover the handoff, the lost race, budget expiry and the bounds. One thing is left: the new scope-level test does not prove the fix. In the race test (https://github.com/callstack/agent-device/blob/f4f4129/src/daemon/__tests__/request-execution-scope-open-device-wait.test.ts#L76), the second opener's task sets its session without checking whether another session holds the device, so it can never refuse. If Not blocking: All checks pass at f4f4129. The PR now conflicts with main and needs a rebase. Before merge, the race test needs to fail without |
--output and --path are the spellings an agent reaches for before reading the synopsis, and the refusal only said "Unknown flag". Suggestions now come from the live flag registry filtered by what the command in scope accepts, so a guess is never answered with a flag that command would refuse: "trace start --path" keeps the bare refusal, while "screenshot --output" is pointed at --out.
A device another workspace's session is holding refused at once, leaving the caller to poll by hand, and the foreign-workspace refusal named a session its `--session` value could not reach. `open --wait <ms>` spends a budget on that contention before the request takes the device's execution lock, which is the whole design: `close`, `record stop` and every other operation that could free the device need the same lock, so an open that waited while holding it would have blocked its own recovery. Verified on a live emulator — a waiting open now lets a competing `close` return in 0.1s and opens the device itself once the holder is gone; the first attempt at a daemon-side wait inside the lock stalled that close for the full budget. The refusal that ends a spent budget names the owning session address, says the wait happened and does not offer it again, while a refusal from a command that cannot wait never mentions the flag. The budget extends the command's timeout envelope rather than eating into it, so a long wait cannot end in a client-side daemon reset.
"Apple tools are only available on macOS" is true and useless to a caller on Linux that just wants its app driven: it reads as a missing install, and `retriable` was unset so replay could retry it. The refusal now carries `supportedOn`, the observed `hostOs`, `retriable: false`, and a hint naming the Android commands for the same workflow plus the remote-Mac-daemon route.
An `--wait` budget ended as soon as no session held the device, but nothing reserved the device until the request took its execution lock, so two waiters could both see one free device and the loser refused with most of its budget left. The open now looks at the device again under its own locks, hands them back when another session took it in that window, and waits for what remains of its budget. The recorded spend moves to that same look, so only a budget that ran out with a holder visible reports a spent wait, and a refusal stops offering `--wait` to a caller that arrived carrying it. The target device is resolved once, as part of the request's execution-lock plan, and contention is polled in the session store alone. A budget that reaches the daemon anywhere but through the CLI is now checked against the bounds the option declares, which the Node client never enforced.
Reading the budget after the target device was resolved left a hole: an open whose device could not be resolved never reached the bounds check, so a Node client or a raw wire request carrying `waitMs: 5000000` passed the check the CLI parser would have applied and went looking for a device with a budget it could not spend. The budget is now the first thing the wait reads.
The race test's open task bound the device unconditionally, so adding an `open --wait` budget to the current execution locks would allow all old checks to be demonstrated. The task refuses with the same fact the real open does. Out-of-range `waitMs` budgets fail before any device work.
f4f4129 to
8cbf480
Compare
|
Rebased onto current The race scope task now checks the same session-store fact as production and returns a refusal when it finds a current owner. I temporarily bypassed Incremental revalidation for exact current build caused both concurrent waiters to poll holder free. Winner |
|
Update: every listed CI check passes at |
|
Reviewed 8cbf480, a follow-up to f4f4129 (#2665 (comment)). Both open points are fixed. The race test now fails if Not blocking: after the rebase, commit 2 no longer imports I traced the mutation result by reading the code and did not run the tests. The new bounds test covers only the upper bound; the lower bound goes through the same All checks pass at 8cbf480, and there are no conflicts. I have nothing left that blocks this. |
thymikee
added
the
ready-for-human
The request scope now parses the `waitMs` option once and supplies the resulting budget to the wait, instead of validating it before the device lookup and reading it again afterward. The reader fails closed when a caller supplies a budget but its option declaration has gone missing. The refusal helper checks the command directly, so reuse outside the open path cannot offer `--wait` to an interaction.
|
Reviewed 4633472. The new commit only changes where the open wait budget is read. The request scope parses Not blocking: the Coverage now passes and Smoke Tests is still running, with no failures. The commit touches |
|
Applied the three review-triage edits at
The PR body no longer claims fair queueing: concurrent waits re-queue on their remaining budgets, but arrival order is not guaranteed. Full CI is green on the final head; the event-driven drain was deliberately left out of scope. |
1 participant
Summary
Three CLI feedback items, 28 files, across five rebased commits.
Guessed flags now name the real one.
--output/--pathwere refused withUnknown flag. Suggestions are filtered by what the command in scope accepts, so nothing is pushed that the command would reject:open --wait <ms>blocks for a device another session holds, then either opens it or fails withDEVICE_IN_USEnaming the owning session address:Acceptable budgets range from 100 to 120000 ms. Their bounds are declared once and enforced across CLI, MCP, Node, and raw daemon input. Only session contention is waited for; a device claim owned by another workspace’s daemon remains immediately non-retriable and returns its recovery command.
The wait runs before the request takes the device execution lock, because
close,record stopand every other operation that could free the device need that same lock. A first attempt that waited inside the lock stalled the competingclosefor the whole budget — that inversion is the reason for the placement. Once the device appears free, the request queues on its locks and rechecks while holding them. If another open takes the device in that interval, the losing request continues waiting with the remainder of its own budget rather than refusing. Multiple opens can therefore wait for one device until their budgets are spent, though arrival order is not guaranteed. Spent budget is recorded only when a holder is observed and the budget is expired. The budget widens the timeout envelope instead of eating into it.Non-macOS Apple refusal now carries
supportedOn,hostOs,retriable: falseand names the Android commands plus the remote-Mac route, instead of reading like a missing install.Validation
Tested at
46334729f915ae2446857747947c9e5d763d23e9: rebased onto currentmain,pnpm check:affected --runpassed (40 runnable checks), pluspnpm format,pnpm lint,pnpm typecheck,pnpm check:layering,pnpm check:fallow, andpnpm check:mcp-metadata. The focused daemon/session/handler Vitest subset passed 400 files and 2766 tests.The final review round tightened the seams: the budget is parsed once before device resolution and passed to the wait, a missing declaration now fails closed, and
describeOpenWaitForRefusalcheckscommand === 'open'directly rather than inferring eligibility from the presence ofwaitMs.The race task in
request-execution-scope-open-device-wait.test.tsnow refuses through the same session-store fact production uses. IfrunLockedis forced to bypassrunWhenDeviceIsUnheld, it loses because a message is returned; without the production fallback at all, it begins another session and consequently says so. Another scope test proves out-of-range budgets getINVALID_ARGSbeforeresolveTargetDeviceis called.Live on
Pixel 7 CI(emulator-5554) from this exact build, same daemon and app:waiter-aandwaiter-bstarted behind holder. holder let go; both then observed device available.waiter-afirst bound target sessionwaiter-a.waiter-bcontinued to queue, loggingheld by session "waiter-a"at the 1284ms and subsequent checks, not past its budget proxy.waiter-aresultedwaiter-bopened; no session or daemon session left.Waited 1001ms for this device and it stayed busy.with owner and close/reuse guidance, and no repeated--waitinstruction.--wait 50was CLI-rejected; their proxied NodewaitMs: 50andwaitMs: 5000000likewise failed, and their request fetches now fail before Target Device Resolution because their budgets themselves cannot be spent.Branch CI is green at
46334729f915ae2446857747947c9e5d763d23e9, with all required platform/CI checks, Coverage, Repo Guards, Compatibility & Provenance, Typecheck & Package, Integration Tests, command docs, previews, bundle size, and both CodeQL scan lanes passing.