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 25 files
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
|
Reviewed 7c3657d. The change has a defect: Apple
This also changes what snapshot, wait, get, is, record and close answer on a device, but the PR body only cites fixture tests and a historical revert result at explicit-session-resolution.ts:50. No live run shows the issue's one-command repro, so nothing proves the real host listing is skipped. With two iOS simulators booted and Not blocking, take or leave: the guard at explicit-session-resolution.ts:59 exempts any Could the refusal live in one place in the router instead, after the locked binding resolves The cubic-dev-ai thread on the router test still applies: request-router-named-session.test.ts:112-126 asserts the code and reason but not The Coverage failure comes from this diff, not from a flake. I read the log and the head code and did not run the tests locally. I also did not check which clients send |
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Thanks for the update. The two simple-case fixes look right, but the Apple carve-out in appstate still misses a route, so the change is not ready at 81b22ab. The check at packages/daemon/src/sessions/handlers/session-appstate.ts:68 only reads the --platform flag. Take The live repro in the PR body is still pending, and this change affects snapshot, wait, get, is, record, close and appstate on real hosts (explicit-session-resolution.ts:47). Please boot two iOS simulators and set The earlier question is still open and has no reply: could the router refuse once, after the binding resolves, when there is no held session, On the open threads: the P3 thread at #3701 (comment) is addressed, because request-router-named-session.test.ts:130 now asserts The code does not pass yet because of the appstate route. Smoke Tests were still running when I checked, so I cannot attribute any CI result yet. Their route overlaps this diff (session resolution in snapshot-session.ts and selector-runtime-backend.ts), so a smoke failure there should be treated as real. Before merge, make Apple appstate answer |
|
Both requirements are addressed at Apple appstate typed reason. Restored in Live repro (repo-built
Take-or-leave items. The single-predicate/bare-platform gap and the router-central guard with a |
|
I reviewed c6be0ce. The 81b22ab finding on the selector-only route is still open, because the new commit changed only a test. Checks are pending: both Smoke Tests jobs are queued or running. They cover the session resolution this diff changes, so treat a failure there as related until shown otherwise. There are no conflicts. The earlier review is here: #3701 (comment)
The live run does not cover that route yet. The author's proof and the pinned test both pass The Cubic thread on the Before merge, |
c6be0ce to
e18e053
Compare
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Round-c6 review consumed; both findings addressed in bffd99a. F1 (P2, record stop manifest recovery): accepted. The guard now runs after F2 (P3, reasonless SESSION_NOT_FOUND family): accepted as doc-only. The Rejected-lead adjudication from your report matches the owner's dispositions; nothing reopened. |
…evice (#3674) A command that names a session this daemon does not hold used to fall through to resolveTargetDevice({}) — the session-attached command carries no device selectors, so the device was picked from the host inventory. With two devices booted the caller met AMBIGUOUS_MATCH telling it to select a device; with one it met an app-session error. Either way the answer depended on how many devices happened to be up and sent the caller after a device selector when what it lost was its session. Refuse at the seam instead: requireNamedSessionHoldsDevice throws SESSION_NOT_FOUND with typed reason named_session_not_found and dispatched=no, wired into every resolution that would otherwise choose a device (snapshot, selector runtime, recording, sessionless close). A request that never named a session stays sessionless by design, and one that names a device selector as well keeps its stated target. Provenance is traced, not inferred: namesSessionExplicitly reads meta.sessionExplicit or a non-blank flags.session, replacing the two copies of hasExplicitSessionFlag. It lives in @agent-device/kernel/ contracts, beside the meta field it reads, so every consumer already evaluates it; the guard module reaches the hot seams through a function-scoped import, keeping the eager-closure gate at parity (ADR-0019 loading shape). The refusal is pinned by a router regression test, unit tests, and a new daemon.route.named-session-refusal dispatch-disclosure row.
#3674) The batch runner stamped flags.session on every step so nested invokes route to the parent's resolved address. The wire `session` field already does that; the flag is a provenance carrier, and the new named-session refusal reads it. An unconditional stamp therefore made a sessionless batch look caller-addressed and met SESSION_NOT_FOUND at the daemon — an error no caller of the batch surface asked for, with advice ("omit --session") the surface does not even offer. The stamp now follows the same traced provenance as every other carrier: meta.sessionExplicit or a caller-set step flag. Also from the same review: the named-session disclosure row's driver now pins the producer's own code and typed reason, so a reverted guard cannot satisfy it with the device-selection error it replaced; commands.md states which pre-dispatch refusals answer session_or_device_selector_required even for a named-but-unheld session.
…every seam Round-2 review found #3674's refusal reaching some callers as an untyped event. `get`/`is` answered a missing session with the generic "no active session" error before the provenance guard could run; `record start` let its app-scope INVALID_ARGS refusal win over a session that was gone altogether; `appstate` carried no typed details and refused requests that had also stated a concrete device. The refusal details now have one producer, `namedSessionNotFoundDetails` beside its sibling builders, that the guard's AppError and `appstate`'s platform-aware response both build from; each seam orders the guard before its own missing-session refusal; `appstate` preserves a request naming an identity selector, matching the resolution guard. The guard module's function-scoped import is confined to the missing-session branch it can fire on: that keeps the eager- closure gate at parity and removes a first-time transform cost from the held-session wait path, which under coverage instrumentation alone overran `wait-runtime.test.ts`'s fake-timer budget at the unconditional form (3/3 deterministic failures restored, 3/3 passes at this form). `namesSessionExplicitly` now accepts the generic flag-record shape the batch runner carries, fixing the TS2345 the batch call site raised.
…selector states appstate's identity-selector exception over-preserved: a named missing session with an explicit Apple device fell through to the session-required refusal, a route with no sessionless answer. Keep the typed named-session refusal wherever the command cannot serve the stated target without a session, and let only non-Apple identity selectors keep their target. Also pins the appstate refusal's typed reason/session/dispatched details, adds the record seam's no-inventory assertion, and trims seam comments to their invariants.
The appstate refusal reported dispatched: 'no', a value it writes itself, so the test would still pass if the handler resolved a device before answering it. The new assertion pins the behavior instead: resolving the target device never runs on the named-session path. Reordering the guard to resolve-then-refuse with the same code, message, reason and dispatched fields passes every other pin and fails only this one.
…on addresses (#3674) The named-session guard covered every record action, so after a daemon restart — the exact state #3674 serves — `record stop --session qa` was refused before the on-disk `screen-recording.resource.json` was read, killing the manifest recovery that `help record` and commands.md promise. That recovery is only reachable through a named session: the manifest path derives from the session address, and the lane proves the manifest's session and device before serving. Start is different: it can only be answered on a chosen device, so it keeps the pre-selection refusal. Order the guard on the resolved plan (`plan.kind === 'start'`, still before the app-scope refusal) and pin both directions: the replayed completed export survives store loss, and a start after the same loss still refuses on the session. Reverting the guard fails both start pins; keeping it stop-wide fails the replay pin. Also complete the refusal taxonomy in commands.md: the session-required family answers the same event as a reasonless SESSION_NOT_FOUND, so document that branch.
bffd99a to
309a3be
Compare
|
Round-bffd99ad9 review consumed: verdict READY (F1/F2 accepted-fixed; no new triggering findings; rejects recorded). Full report: /tmp/3674-review-bffd99ad9-final-r1.md. The branch was then rebased onto origin/main 87fe3ea (content byte-identical, 44/44 hunks); fresh final review at 309a3be is running now. The two new review threads at this head are answered on-thread: the Current head 309a3be: exact-head check:affected exit 0 with exactCommit true (receipt run-qDhNlT); focused suites 54/54; live repro re-run at this build (typed refusal 388 ms no resolve phase; apps --udid A succeeds; sessionless control still AMBIGUOUS_MATCH). CI at this head is still in flight — no green claim. |
|
Thanks for the update. At 309a3be the appstate and test-assertion threads are partly fixed, but record stop after a daemon restart still resolves a device with no constraint, so this is not ready to merge yet. Fixed since c6be0ce: the listing-count assertion in request-router-named-session.test.ts and the "not called" assertion in session-appstate-input.test.ts (the second covers only the Still open, first: in record-runtime.ts:158 the named-session guard runs only when Still open, second (unchanged from c6be0ce): Could the f1 guard live inside Not blocking, take or leave: the reasonless SESSION_NOT_FOUND from Open review threads: the record stop device resolution thread and the reasonless refusal docs thread still apply. These two no longer apply, so please resolve them: listing count assertion is fixed at head, and appstate not-called assertion is fixed at head for the I ran no tests, and my notes on which tests would fail without each fix come from reading the code. I did not check whether any CLI or client path fills in No conflicts are known. CI is pending: Smoke Tests is still running. Smoke runs CLI flows through daemon session resolution, which this diff changes, so please treat a failure there as related until shown otherwise. Before merge, record stop for a named session the daemon no longer holds must take its device from the manifest, or refuse with |
…tch (#3674) The named-session row sits under the retry legend's dispatched: no umbrella, but the session-required family answers with no details at all. Reading the cause equivalence under that umbrella would promise a non-dispatch those responses never make, inviting a blind retry. The row now says the branch is on the missing-session cause only, and the missing dispatched keeps its usual no-guarantee reading.
|
Two defects from the earlier review (309a3be) are still open at 16f9460. The docs rewrite fixes the cubic-dev-ai threads on the commands.md wording and the two test assertions. The code is unchanged. The
Still open: the record-runtime resolveTargetDevice thread applies and is the first defect above. The other three inline threads are fixed at this head and you can resolve them: the commands.md:488 scoping, the listing-count assertion, and the resolver-not-called assertion. The last one covers I ran no tests, and I did not verify the exact-head CI is pending. Both Smoke Tests jobs are queued with no log yet. Smoke runs CLI flows through daemon session resolution, which commits 1-6 change, so treat a failure there as related until shown otherwise. Before merge, record stop for a named session the daemon does not hold must take its device from the manifest, or refuse with named_session_not_found, before any unconstrained |
…a host sweep (#3674) A named session the daemon no longer holds was still answered on an unconstrained resolveTargetDevice for record stop, so a multi-boot host replied AMBIGUOUS_MATCH before the durable manifest — the one thing that can answer that stop — was read. The named-session rule now lives at the handler's only device selection: a request addressed to a session takes its device from a decodable manifest envelope, and a start, or a stop with nothing decodable to recover from, refuses with the typed named-session error. Selector and sessionless lanes keep their targets, and the manifest's own session and device proofs still fail closed. Co-Authored-Worked-On-By: Claude
…ce route (#3674) `appstate --session lost --udid <ios>` without `--platform` passed the admission guard, listed the host, resolved onto an Apple device, and answered the reasonless session-required refusal — contradicting the commands.md promise and paying for a listing the typed answer makes unnecessary. The device-runtime lane now answers a named-missing session with the same typed refusal as every other seam; a request that named no session keeps the plain session-required advice, and the non-Apple identity-selector lane is untouched. The refusal builder moved next to its sibling Apple refusals so both routes share one wire shape. Co-Authored-Worked-On-By: Claude
…el (#3674) The r4 sentence claimed the family answers with no details at all and voided the dispatched guarantee; the disclosure layer stamps click/fill/screenshot-style reads dispatched: no and type/swipe-style mutations dispatched: unknown, so the claim was false for every member it named and wrong in both reading directions. The row now states the fact at the details.reason level, keeps the usual dispatched reading, and treats only absence (logs/network/audio) as no guarantee. Co-Authored-Worked-On-By: Claude
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
…admission (#3674) The device-runtime lane re-read the session store and re-ran the naming check after awaiting device resolution, so a store change mid-await could move the request into a different error family than the one it was admitted into, and the classifier was spelled out twice. Admission now carries one typed namedSessionMissing fact, decided from the same store read as the admission session itself. A bare --udid still resolves its stated target first and answers the typed reason from the resolved device — pinned by asserting the resolution call, not just the payload — and the store-changes-during-resolution case pins the snapshot semantics. The manifest-first docstrings say what actually happens: the manifest's identity is re-resolved against the host for the live device fields, with the session/device assertions failing closed. Co-Authored-Worked-On-By: Claude
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
…two doc claims (#3674) The bare-udid pin built its runtime owner with `ios`, which is a public selector, not a Platform — typecheck TS2345 at the lint-package gate. The fixture device's own family, `apple`, is the value the owner takes. Alongside it, two docstrings said more than the code does: the manifest lane asks the host only for the manifest's device where the family has an identity flag (families without one keep the request's own selection, proven by the manifest assertion), and the admission-carried fact keeps the refusal stable once a device resolves — resolution itself can still fail first with the device's own error. Co-Authored-Worked-On-By: Claude
This branch has not been deployed
1 participant
Summary
Closes #3674. Explicitly named missing sessions refuse before unconstrained device selection. Named
record stoprecovery uses the durable manifest's device identity. Apple appstate preserves the typed named-session reason, including the bare-UDID route, using session state captured at admission.Validation
Published head:
2d3838f77db1c1b72925fece0f5d8ef18a9a3cf3.ios→apple) and qualified the two comment claims. All 15 review threads are resolved; the preceding head's Typecheck failure remains historical.run-bBSfwXpassed withexactCommit: true, exit 0 and matching clean checkout fingerprints: 11 passed, nine explicitly skipped GitHub or parked lanes, one deduplicated check. The old67e439713invocation stopped at dirty-checkout preflight; the superseded633115a56waiter was cancelled with authorization.633115a56build. Parent raw verification remains pending; this head changes a test fixture and two comments.633115a56review is retained as input. A new final-head review is required before completion, even with unchanged production behavior.Current CI: 16 successes, one skip, three smoke lanes pending and no failures. The historical Android infrastructure retry succeeded; its single retry budget is used.