Repository navigation
Conversation
connection-runtime.ts crossed the 1,000-line architecture-debt line. Split it along the seams the file already had, with no behavior change: - connection-lease-policy.ts: which lease a provider's connection allocates, its TTL, and the id it names a new lease with - connection-lease-transaction.ts: the acquisition transaction — resolve the device, allocate or renew the lease, bind it to the record, and release it - connection-metro-runtime.ts: the deferred Metro runtime preparation the record carries, and the tunnel teardown - connection-previous-lease.ts: releasing the lease a reconnect replaces, with the previous endpoint's own credential - connection-runtime.ts: the materialization that sequences those three and persists the record Importers now name the module that owns each declaration.
…r persisted `materializeRemoteConnectionForCommand` allocated a lease and then could fail before `persistMaterializedConnection` wrote the record that would name it — a rejected `prepareConnectedMetro` was the live path. The only release path ran when the state-file write itself failed, so the new lease was neither saved nor ended: no record named it, so `disconnect` could not reach it, and it held the device until its TTL lapsed. The call now owns a lease it allocated until persistence transfers ownership, and every earlier exit ends it. A lease the call renewed was already named by the stored record, so this path never releases it — disconnect still does. The failure that ended the command stays the thrown error. A release that failed too is recorded beside it as typed secondary evidence under `details.leaseRollback` (ADR 0010 as amended by ADR 0019), never in its place.
The split left every module under the 1,000-line limit, so the 1,019-line legacy cap for connection-runtime.ts has no subject.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 13 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Needs changes: three ownership gaps remain against the coordinator’s requirement.
-
P1 — Do not equate an allocation RPC with a newly created lease.
src/cli/commands/connection-lease-transaction.ts:62(classification originates at:349–363). If session A has persisted a lease, session B uses the same tenant/run/backend with no lease ID, and B’s Metro preparation fails,leases.allocate()can return A’s existing lease.LeaseRegistry.allocateLease()explicitly reuses that run binding (packages/daemon/src/resources/lease-registry.ts:93–94,192–209), and the handler already computesreusedbut does not expose it in the lease response. This client nevertheless returnsacquired: true, so B’s rollback terminates A’s persisted lease. I confirmed that two allocations for the same scope return the same lease ID. Fix: carry the authoritative new/reused allocation outcome through the client and release only a lease actually created by this transaction. Add a two-session regression case. -
P2 — Start ownership before acquisition’s post-allocation validation.
src/cli/commands/connection-runtime.ts:106–115andsrc/cli/commands/connection-lease-transaction.ts:64–69. The outer rollback learns the lease only afteracquireConnectionLease()returns. A proxy allocation forios-instancethat returnsdeviceKey: android:mobile:emulator-5554is bound, then rejected bycarriedConnectionPlatform()withCONNECTION_PLATFORM_CONFLICT; the new lease never reachesunrecordedLease, no release occurs, and no stored record names it. An isolated head probe reproduced this. Binding’s own release paths also sit before this ownership handoff. Fix: register each genuinely new lease immediately when allocation succeeds, keep ownership across binding and validation, and make binding cleanup consume that same ownership so a failure cannot leak or release twice. Preserve the primary error and attach failed cleanup as typed secondary evidence. -
P2 — Transfer ownership at the actual durable record write.
src/cli/commands/connection-runtime.ts:131–143,171.writeRemoteConnectionState()publishes the session record and then.active-session.json(packages/remote/src/remote-connection-state.ts:254–255). If the second publication fails, the catch releases the lease even though the session record already names it. I reproduced this by making.active-session.jsona directory: the call rejects, the persisted session containslease-1, and rollback releaseslease-1. Fix: expose/observe the session-record commit point and clear rollback ownership there, or make persistence restore its previous record before reporting failure. Do not treat a later pointer-write failure as proof that no ownership transfer occurred. Add a failure between the two publications.
Review evidence at head b4deffa99966087e2576ed9519606bc102497222, merge-base f7879cf2d266ee1dd476d0342b2819bdeceb647b:
- Mechanical move verified with
git diff -M90% --stat d8e81888^ d8e81888(817 additions / 785 deletions) plus a parser-based content comparison: all 44 original declarations are byte-identical after excluding imports/export modifiers. Importer changes only redirect ownership. The gross churn is the requested split; no hidden behavior change found. chore(gates)removes only the obsolete 1,019-line exemption. The five production modules are 234/95/441/217/148 lines. No gate weakening, new allowlist, or unrelated baseline regeneration found.- One focused Vitest invocation in a disposable checkout after frozen install/build: the PR’s three tests pass at head; the same tests against the exact pre-fix runtime/transaction declarations at
d8e81888b0064aa5de5df4b707c9bc89296e1dc6produce two intended failures and one pass. The pre-fix copies redirect only their module imports; the expected rollback reason comes from head. Both additional head probes above fail their ownership assertions. Overall: four passed / four failed, with the four failures expected from these probes. This is focused source evidence, not CI readiness. - Ordinary heartbeat reuse remains excluded from rollback. The previous write-failure lease rollback was removed, so that direct path does not release twice. Allocation cancellation before a result is handled by the existing daemon allocation handler (
packages/daemon/src/sessions/handlers/lease.ts:117–140); no changed cancellation behavior found. The new outer rollback’s primary AppError identity/reason and structureddetails.leaseRollbackconform to ADR 0010/0019 for the covered failure path.
No reviewed source files were edited. CI was not awaited.
Apex SentinelVerdict: Since the last review: 7 new · 0 still open · 22 no longer raised 7 finding(s): 2 minor, 5 nit. Verdict: approve. reached max rounds (1). 🟡 Minor (2)
🔵 Nit (5)
Reply under an inline comment with |
![apex-sentinel-staging[bot]](https://avatars.githubusercontent.com/in/5196457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Apex Sentinel: 7 new findings on b4deffa. The verdict and the full list are in the summary comment.
|
I found no blocking problem in b4deffa. The fix ends a lease that a failed materialization allocated but never saved, and I read no path that skips it. Smoke Tests is still running. Those jobs drive local devices without Not blocking, and you can take or leave these. First, On the open threads: the P1 about the silently dropped release failure at proxy-lease-device.ts:135 still applies (#4135 (comment), release failure dropped). The fix is to record the evidence locally, as I did not run the regression tests, so their pre-fix and post-fix results come from reading the code. The live Limrun run and the check:affected receipt are your reports, and I could not reproduce them. A SIGINT or SIGKILL during Metro preparation bypasses the catch, and I could not tell whether a signal handler turns SIGINT into a rejection before exit. |
…he durable record The independent review found three ownership gaps in the lease rollback. - The daemon now answers each lease_allocate with whether its request created the lease (reused on the wire, Lease.created on the client). A run's repeat allocation gets the run's existing lease back, and that lease belongs to whoever holds it: this path ends only a lease the daemon says it created. - A lease enters the new ledger the moment allocation returns, before any later validation or binding can fail, so a platform conflict after a successful allocation can no longer leak it. Binding cleanup consumes the same ownership, so a lease is never released twice. - Ownership transfers at the durable session record, not at the whole write: the commit-point callback runs between the record and the active-session pointer, so a pointer failure leaves a named lease to disconnect. A release that did not end the lease keeps it owned, and the failure path reports it beside the command's own failure as typed details.leaseRollback.
|
All three needs-changes findings accepted and fixed in 1. P1 — allocation ≠ newly created lease. ACCEPTED. The authoritative answer now comes from the call, not from the returned lease. 2. P2 — ownership started too late. ACCEPTED. New 3. P2 — ownership transferred at the wrong point. ACCEPTED. Cubic violations — dispositions:
Verification at head
|
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Needs changes at 3df3025ce73a41330213c38a85d7c246cae2975d (round-2 range b4deffa9..3df3025c).
-
P2 — Stop fallback when retiring its first lease fails.
src/cli/commands/proxy-lease-device.ts:163–167. When the supported keyless-lease fallback receives a newly created lease withoutdeviceKey,leaseLedger.end()now catches a throwing release and returns{ released: false }. This caller ignores that outcome and allocates another lease. If the second allocation and persistence succeed, only the replacement is transferred; the first stays in the local ledger, which is then discarded without failure cleanup. Previously a throwing release stopped this path. A head probe reproduced 2 allocations, 1 throwing release, successful materialization, and onlylease-2persisted;lease-1has no durable owner. This also applies when the first release is canceled/rejected but a subsequent allocation can proceed. Fix: require a confirmed successful retirement before entering fallback, or fail with typed release evidence and let the outer ledger perform cleanup. Do not return success while another unrecorded lease remains owned. Cover the fallback's throwing/refused release case. -
P2 — Correct and document the published
Lease.createdcontract.packages/contracts/src/client-lease.ts:18–22. The shipped JSDoc saysfalsemeans newly created andtruemeans reused, whilenormalizeLease()implements the opposite.Leaseis publicly exported throughsrc/client/client-types.ts→src/sdk/index.ts→ theagent-deviceentrypoint; I also confirmed the reversed prose survives in the built.d.ts. A consumer following this guidance can release another session's reused lease and keep its own newly created lease. Fix: documenttrue = created by this allocation,false = reused, andundefined = unknown / not an allocation; show thecreated === truerelease guard in the owning lease documentation atwebsite/docs/docs/client-api.md:347. That page currently has no explanation of the new field. No CLI flag/help syntax changed, so an unrelated help rewrite is unnecessary.
Round-1 disposition check:
- P1 resolved for the reported two-session failure: the handler returns authoritative
reused; the client maps it tocreated; the CLI tracks onlycreated === true. A missingreusedleavescreatedundefined, which is conservatively excluded from release, never treated as created. - P2a's late-registration scenario resolved: tracking is immediately after allocation returns, before binding/platform validation. Binding and outer rollback share the ledger. Successful
endremoves its entry, preventing a second release from the later catch; failedendremains owned for a retry. Finding 1 is the remaining exit where ownership is abandoned. All productionendcallers await it; the newinFlightguard concerns a pending client promise, not proof that a canceled transport request has stopped executing remotely. - P2b resolved:
onSessionRecordCommittedexecutes immediately after the first session-record publication returns and before active-pointer publication. A session-record write failure does not transfer ownership; a pointer-write failure does. The existing pointer-failure regression passes.
Wire/public surface assessment:
reusedis an additive optional response field. ADR 0006 explicitly excludes such additions from protocol bumps. Existing clients ignore it; the new client handles omission conservatively, so neither a protocol bump nor a capability gate is needed for safe ownership classification. Older daemons cannot supply affirmative ownership evidence, so failure cleanup may leave an unrecorded lease to expire rather than risk releasing someone else's lease.- ADR 0007's lazy allocation and durable connection ownership remain consistent in the reported scenarios, subject to finding 1. The public API runtime addition is optional and compatible; its documentation is unsafe until finding 2 is fixed.
- No contracts export subpath changed.
scripts/layering/contracts-exports.snapshot.jsonsnapshots package export keys, not fields insideLease, so leaving it unchanged is correct. The wire manifest/ledger tracks shared envelope declarations, not this untyped command-result property ornormalizeLease; none of its tracked declarations changed. No unrelated snapshot regeneration or gate weakening appears in this round.
Evidence: reviewed production source stayed read-only and the checkout is clean at the named SHA. In a disposable archive of that SHA, pnpm install --frozen-lockfile && pnpm build passed. One focused Vitest invocation ran the allocation handler, real-client lease wire test, rollback tests, proxy allocation tests, and two scratch probes: 46 passed / 1 failed, with the sole failure proving finding 1 (45 existing tests passed; boolean/omission normalization probe passed). Command: pnpm exec vitest run --project unit-core src/__tests__/r4135b-probe.test.ts src/cli/commands/connection-runtime-lease-rollback.test.ts packages/daemon/src/sessions/handlers/__tests__/lease.test.ts src/__tests__/daemon-client-lease-device-spec.test.ts src/__tests__/remote-connection-proxy-allocate-by-spec.test.ts. No CI wait and no live-device claim.
There was a problem hiding this comment.
All reported issues were addressed across 12 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.
Apex Sentinel: 9 new findings on 3df3025. The verdict and the full list are in the summary comment.
…es, and document created correctly The round-2 review found two gaps. - The keyless-lease fallback released the daemon-granted lease and ignored whether that release ended it. A throwing or refused release left the first lease owned by no durable record while the fallback allocated a replacement that became the connection. The fallback now requires a confirmed retirement; otherwise the command fails with PROXY_LEASE_RETIREMENT_FAILED, the ledger keeps the lease, and the failure path retries the release and reports it as details.leaseRollback beside the refusal. - The published JSDoc on Lease.created stated the mapping backwards; it now reads true = created by this allocation, false = an existing lease, absent = the daemon did not answer. website/docs/docs/client-api.md describes the field and the release guard: release only when created is true, and a missing answer is not permission to release.
|
Thanks for the update. At 3df3025 the lease is now tracked at allocation, so a binding failure ends it, and a release that returns A rebind can still leave the first lease running. proxy-lease-device.ts:163 ignores the release outcome and allocates a replacement at :167. Because An allocate answer without Could the ledger be one owned slot instead of a Map? Keep the two facts apart: Not blocking: Open threads: the two P1 threads on the rebind release still apply (#4135 (comment), #4135 (comment)). The P2 threads on the reversed The only check not passing is the review gate, which reflects the open threads above, including the one on lease-client.ts:96. No test or smoke job is failing. The next step before merge is the two fixes above, then a push so the open threads can be re-checked. I did not run the regression tests. I judged they fail before the change and pass at 3df3025 by reading the code. The |
|
Both round-2 findings accepted and fixed in 1. P2 — fallback abandoned a lease it could not retire. ACCEPTED. Tests (
The existing success-path test still proves a confirmed retirement enables the fallback (2 allocations, 2. P2 — published Owning user docs at Verification at head
|
|
Coordinator check of round 3 (
No blocking findings. All findings from rounds 1–2 are resolved. The wire addition was already judged backward compatible in round 2. |
There was a problem hiding this comment.
All reported issues were addressed across 4 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.
Apex Sentinel: 6 new findings on cffa60d. The verdict and the full list are in the summary comment.
|
This is a follow-up on the earlier review (#4135 (comment)). The lease release now goes through the ledger, the rebind no longer allocates a replacement while the first lease is unended, and the connection-lease-transaction.ts:450 returns Once that is done, could the ledger be a single slot, On the other threads, these still apply: the
I did not run any tests, and I judged the old regression tests by reading the code against them. I could not reproduce the author's |
|
Addressed the 2026-11-01 12:40 maintainer review in
Regression evidence: before production edits, the same focused Vitest command against production at pnpm exec vitest run --project unit-core \
src/__tests__/remote-connection-proxy-allocate-by-spec.test.ts \
src/cli/commands/__tests__/proxy-lease-device-spec.test.ts \
src/cli/commands/connection-runtime-lease-rollback.test.ts \
packages/daemon/src/sessions/handlers/__tests__/lease.test.tsLive Limrun: loaded only LIMRUN_API_KEY with the requested extraction command; used this worktree's Validation: Cold self-review found no additional issue. This implementation sub-agent did not start an independent reviewer; the coordinator owns that review. Not merged; labels unchanged. |
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
|
Addressed r4437455877 in Mutation proof: clearing the materialized binding fails the new assertion ( |
There was a problem hiding this comment.
Apex Sentinel: 7 new findings on c36e0f1. The verdict and the full list are in the summary comment.
|
Thanks for the update. The earlier findings from cffa60d are fixed, but c36e0f1 still has two cases where the CLI abandons a lease it allocated itself. The code change needs another pass. Smoke Tests is still running with no failure recorded, and I expect no overlap because the change only touches the remote lease path. There are no conflicts. The refusal paths now skip the release when The keyless fallback has the same gap (proxy-lease-device.ts:158). Against a pre-#3487 daemon, which also omits Not blocking, and fine to take or leave: when the keyless retirement release throws, the raw provider error is now the primary error and The open inline thread on the pre-pick test (#4135 (comment)) is fixed at head and can be resolved. The thread on the spec-refusal release error showing up twice (#4135 (comment)) still applies as a low-priority note. These do not apply and can be resolved: the retry after a thrown release (#4135 (comment)) keeps the slot by design, so it is not a double release; attaching I read the head code and main's release sites but did not run tests, the mutation proof, the Limrun run, a pre-#3487 daemon or a third-party proxy service. I could not tell whether production proxy services forward |
|
Settled the seven Apex threads in
Regression proof: the spec suite at parent production had 2 failed / 13 passed, detecting duplicate flat evidence and stale failure evidence after recovery. The same spec command now has 15 passed. Fixed focused suites: 33 passed across spec refusal, general rollback, and keyless fallback. The release sequence is unchanged; this round changes error evidence only. Cold self-review found no additional issue; coordinator review remains separate.
|
|
This follow-up covers 8ee7290. The new commit fixes the false "releasing the lease failed" text and the open threads below, but the two release findings from the earlier review (#4135 (comment)) are still open. First, the spec refusal and the identity refusal still leave the lease held (proxy-lease-device.ts:178). The ledger tracks a lease only when Second, the keyless replacement leaks the first lease (proxy-lease-device.ts:158). A pre-#3487 daemon never picks a device and never sends Not blocking: if the first release throws and the rollback retry succeeds, the refusal carries neither Hosted CI is green, with 20 checks and none failing at 8ee7290. There are no conflicts. I did not run the focused Vitest suites, a mutation, or a pre-#3487 daemon. I judged the two new spec tests by reading the parent code. I found no reply that accepts or rebuts the two earlier findings, so I treat them as unaddressed. Before merge, both findings need the fix above or the documented non-release, plus the regression tests. On the inline threads: the Apex thread about the second release attempt (#4135 (comment)), the |
Summary
A failed remote materialization releases leases proven created by that command before a durable session record takes ownership. A single slot covers allocation, binding, Metro preparation, and persistence. Every allocate response remains eligible for binding; omitted ownership evidence never authorizes rollback.
Spec refusal retains one rollback retry after a thrown release. Failed cleanup now has one typed shape:
details.leaseRollbackwith reasonLEASE_RELEASE_FAILED. A successful retry leaves no stale failure evidence.26 files overall, including the mechanical split. This round touches 5: 2 production, 2 tests, 1 doc. No scope expansion or linked issue (SM-2).
Validation
Head
8ee729017c8863d052e3771c2a48ac7a956c16f9, parentc36e0f1e21ca5e66eac07ad8e1400fd73cc7178c.pnpm formatandpnpm check:affected --run --head HEADpassed (41 runnable checks, 25 explicitly skipped),exactCommit=true,checkoutChanged=false; receipt.tmp/validation/run-YCAgiS/receipt.json. Hosted coverage/device smoke and current-head review remain pending.4472e078: forced Metro failure followed successful allocation/release; instanceandroid_euna_01m6cfayamfbv8evtry0dze6smterminated; isolated daemon stopped without pending releases. This round preserves release ordering.