feat(runtime): bind new sessions to connection identity - #3864
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found a blocking schema issue.
[P2] handoff → transferring rename without migration
local-deployment-owner.ts:72-90 changes persisted state.kind but keeps schemaVersion=1 and no decode/migration. Interrupted handoff records on upgrade become invalid_record and cannot be recovered.
Fix: keep backward-compatible decode or add migration with test.
Hosted test: SUCCESS (32932642459).
简体中文
状态名称变更缺少迁移。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
fd3b116 to
2edb541
Compare
|
Thanks for flagging this. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found a blocking issue.
[P1] ScheduledTask binds by slug, allows credential re-binding after reuse
Agent-run tasks freeze only llmConnectionSlug; if connection A is deleted and slug B reuses same name, task silently binds B's credential, bypassing immutable-ID binding.
Fix: freeze and validate bound Connection ID, test delete-reuse path.
Hosted test: SUCCESS (32935683031) and windows_recovery: SUCCESS.
Old owner-schema P2 no longer applies to this head.
简体中文
调度任务未绑定具体连接。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
|
Addressed the current-head ScheduledTask account-rebinding review:
Validation: Core 16/16; Storage workflow 14/14; focused Runtime Host protocol/coordinator 59/59; real UDS slug-reuse 1/1; Core/Storage/Runtime/Runtime Host typechecks; Biome and Generated-by: Codex |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found no code blocking issues.
Current head ad22301 closes prior ScheduledTask slug-reuse (ID+slug validation, fail-closed) and bumps epoch 50→51 correctly. Hosted test: SUCCESS (32942342339) and windows_recovery: SUCCESS.
No P0-P3 on code. Note: feature gate (task #334 is code only) remains CONDITIONAL GO per @feature杀手 — legacy wording, Scope/epoch declaration, and single pre-AgentRun migration authority need closure before merge.
简体中文
代码上无阻断，功能闸仍有条件。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
|
Closed the remaining feature-gate items on head
Validation on the final diff: Runtime Host composition 45/45, Runtime pre-AgentRun ordering 3/3, typecheck, Biome, and diff-check. The PR body now records the exact limitations and scope. |
|
CI freshness note for head
I cannot rerun the original Apache job without repository admin permission, so the #3864 check remains visibly red and still needs a maintainer rerun if branch-level green status is required. No code claim is being made from the downstream check alone; this records the exact failure and independent freshness evidence. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found a blocking issue.
[P2] Adoption without AbortSignal — cannot cancel pre-AgentRun
session-connection-adoption.ts:35-57 has no AbortSignal, but runtime-kernel.ts:661-663 runs it before the execution abort controller exists (1477-1492). If catalog/CAS hangs, stop/drain cannot interrupt, and may still commit ID after cancellation. Prior backend path passed abortSignal per read.
Fix: carry signal into adoption or make pre-AgentRun cancelable.
Hosted test: SUCCESS, but feature gate still conditional on doc/migration items.
简体中文
绑定阶段缺少可中断信号。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
844ecff to
661e28a
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found blocking issues.
[P2] Non-model config updates miss llmConnectionId
Desktop/Bot updates for permissions etc. send only slug+model, not immutable llmConnectionId; reused slug silently rebinds Session to new account.
Fix: carry bound Connection ID for all Session mutations.
[P2] Legacy adoption lacks AbortSignal
Adoption runs before abort controller, cannot be canceled on stop/drain and may commit after cancellation.
Fix: make adoption cancelable.
简体中文
连接标识与中断信号缺失。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
1ee78dd to
ca59f6b
Compare
|
Addressed both current-head review findings on
Validation: Storage/Runtime/Runtime Host builds, CLI and Desktop-main typechecks, CLI driver 53/53, focused cancellation/adoption/protocol/Desktop tests, protocol epoch guard, Biome, and diff-check. Generated-by: Codex |
|
Follow-up on head Fixed by preserving synchronous owner registration while retaining pre-admission cancellation, queued tombstone ordering, and post-admission real-outcome semantics. Synchronous executor rejection now releases the lane before propagating, so successors cannot hang. Validation: Storage runtime-policy stores 54/54; serialized lane 4/4; the original synchronous drain regression; Storage build; Biome and diff-check. Two independent re-reviews returned GO. Generated-by: Codex |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found a blocking issue.
[P1] assertExecutionAdmissionOpen bypasses durable cancelled-run convergence
Stop arriving between preflight and new AgentRun reservation throws directly, bypassing cancelled-run ledger; session-manager 8/211 fail.
Fix: keep Host adoption cancel but make runtime entry durable convergence.
Previous 2×P2 (immutable ID retention, cancellable adoption) now closed.
Hosted test: QUEUED, windows_recovery: SUCCESS — not green.
简体中文
取消路径缺少持久收敛。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found no blocking code issues.
Removes pre-AgentRun abort throws so stop intent attaches to pending claim and settles as durable cancelled Run via existing fences. Hosted windows_recovery: SUCCESS, test: IN_PROGRESS.
No P0-P3 on code. Note: feature gate still CONDITIONAL GO per @feature杀手 — funnel and legacy explicit-target fixes remain before merge.
简体中文
代码上无阻断，功能闸仍有条件。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
8300b26 to
26bf00b
Compare
|
Rebased the reviewed stack onto current The prior P1/P2 code findings remain closed: cancellable pre-AgentRun adoption, durable cancelled-Run convergence, and immutable identity on non-model configuration updates are unchanged. The six-commit range-diff is patch-equivalent after the final rebase; compatibility epoch remains 52 over main epoch 51. The remaining CLI creation-funnel adoption is intentionally carried by the dependent #3882 head, which now passes the installed release-package controlled model-turn smoke. Scoped validation here: protocol epoch guard, Runtime/Storage/Runtime Host typechecks, and SessionManager 212/212. Generated-by: Codex |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found the overall approach is not merge-ready.
The underlying immutable connection identity is necessary, but this PR in its current shape is NO-GO — please freeze the charter and do not continue via rolling fixes / repeated rebases refreshing old GO verdicts.
Why:
- Scope has expanded far beyond the original issue (which explicitly listed wire model/catalog, Desktop/CLI picker and epoch bump as non-goals). Current head is ~87 files / +2382 -179 across 6 commits covering wire shapes, epoch 50→52, ScheduledTask, Desktop/CLI configuration, legacy adoption/cancellation and Storage admission.
- Fix chain has repeatedly grown from slug-reuse into dual/late migrations, cancellation propagation, config-update ID loss, abort-fence breaking durable cancelled runs, Storage owner admission and multiple rebases — this is delivery-boundary loss, not closable with one more test.
- Two structural knots remain: six Runtime provider-bearing paths still manually call an optional
prepare*hook instead of a single encoded funnel through the commonenterExecutionClaim(); Desktop rejects legacynullIDs before merging an explicitmodelTarget, so the "Select an account" action itself is unreachable. - The PR body claims centralization and multiple review rounds fully GO while the next piece (CLI creation-funnel adoption) is delegated to stacked #3882 — this proves the PR is not a self-contained vertical slice and the evidence narrative is not trustworthy.
Recommendation: freeze the charter now and choose one path — rebuild a clean self-contained PR from current main that completes the core invariant without depending on #3882, or split into independently verifiable, self-safe vertical slices. Either path must rewrite the true scope, remove stale GO claims, and then get a single final review on one frozen head. Until then, keep as not mergeable.
Hosted checks do not substitute for this.
简体中文
底层不可变连接标识必要，但当前 PR 已扩散失控，建议冻结范围后重整或拆分为自闭合竖切再审。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
|
Acknowledged. I am freezing head I will rebuild from current
The current lossy adoption/cancellation/Storage-admission chain will not be carried into the Session slice. I will rewrite the PR body to its true frozen scope, remove stale GO claims, run the required two independent reviews on the rebuilt head, and then request one final freshness review. |
26bf00b to
04686a0
Compare
me2seeks
changed the title
|
Rebuilt and frozen at The old six-commit stack has been replaced by one self-safe non-wire slice: new Session/AgentRun durable identity, bound ID+slug execution resolution, and delete/reuse protection across backend, OAuth reconciliation, auxiliary models, configuration updates, and Session-derived tool surfaces. Protocol/epoch, CLI/Desktop, ScheduledTask, legacy adoption, cancellation, and Storage admission are absent from the diff. During the required two independent pre-push reviews, both reviewers found the same tool-surface slug fallback. The final head closes it and adds a real A-delete/same-slug-B composition regression. Both final reviews are GO with no P0-P3 findings. The extracted follow-ups are #3926 (exact Session wire/surface recovery) and #3927 (ScheduledTask identity). The PR body and tracker #3852 now reflect the exact scope. CI is fresh on this head; requesting one final review on this frozen commit. Generated-by: Codex |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found blocking issues.
[P2-1] Legacy Session without llmConnectionId exposes model picker but Host rejects all updates
session-catalog-coordinator.ts:730-734 rejects all config patches when llmConnectionId === null, but Desktop app-shell.tsx:2925-2935 / @maka/ui still shows model switcher and sends request — users see choosable model but it always fails, and wire has no recovery path.
[P2-2] Bound Session with modelTarget=default silently replaces with stale model
session-catalog-coordinator.ts:746-753 coalesces default to legacy model instead of resolving current canonical default; out-of-limit default should be rejected but regresses to "success but not updated".
Hosted windows_recovery: SUCCESS, but test: FAILURE (host-kernel ENOTEMPTY) + 2×P2 ⇒ NO-GO.
简体中文
两个遗留与默认模型处理问题阻断。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
|
Addressed the current-head review on 16841bd.
Validation: Runtime Host 1260 passed / 0 failed / 9 skipped; build, Biome, and diff-check pass. Two independent final code reviews reported GO with no P0-P3 findings. |
16841bd to
bad6984
Compare
|
Addressed on |
bad6984 to
751b196
Compare
|
CI's Windows inventory gate identified one omitted generated row for the new UDS recovery test. Regenerated with |
|
This is a synthesis of the independent blind review by @Luna-Deep-Qronos at exact head What I checked myself:
Findings from Luna-Deep's review: Overall — CODE GO — 0 P0/P1/P2/P3.
Verification: Runtime Host/Storage focused 148/148 PASS, Runtime SessionManager 247/247 PASS, Desktop focused IPC/session 68/68 PASS, CLI Runtime Host run/session-driver 95/95 PASS; relevant builds and What I did not judge: a full Desktop end-to-end session creation with concurrent connection switch was not executed beyond the inspected driver and projection paths — verification was by code inspection and the focused suites noted above. Gate: no P0–P2,
|
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
I re-reviewed exact head 979a4f55a87b82a0e4fe345eefe31f37f5a0e734.
The earlier legacy-Session model-picker problem is closed. An identity-free configuration patch still fails closed, but an explicit model selection now resolves and persists the exact Connection identity. The production coordinator tests cover both sides of that contract, and the focused Runtime Host and CLI suite passed 242/242 locally, including deleted-account recovery and two-client continuity.
I cannot approve this head because it is no longer mergeable into current main. The branch assigns Runtime Host compatibility epoch 54, while main is already at epoch 55. A current-main synthetic merge conflicts in packages/runtime-host/src/protocol/index.ts; selecting the branch side would both move the epoch backward and attach the wrong meaning to epoch 54. Please rebase onto current main, preserve the intervening epoch ledger, assign this wire change a fresh epoch (56 with the current ledger), and rerun the exact-head checks. Because this is a feature, the merge decision remains with a human committer after the new head is reviewed.
Two small cleanup opportunities remain. The second legacy-Session defensive throw in #mergeConfigurationPatch is unreachable after the earlier guard, and the other configuration controls remain enabled even though they intentionally fail until the user first restores an account binding through the model picker. Neither changes the conclusion above.
Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查，也不能替代人类审查。有人类对本条评论负责，如有错误请直接指出。
6770d7e to
29a6381
Compare
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
I re-reviewed exact head 29a638195c619356acbdb9c61e9f091718a1f53a. The two earlier P2 findings are closed, and I found no new P0–P2 issue.
The legacy-Session recovery path is now enforced at the Runtime Host rather than only by a client projection. A non-fake Session without llmConnectionId is blocked at both normal execution and safe-boundary continuation, so another client cannot bypass the visible model picker and resume slug-only resolution. Explicit configuration updates remain available, allowing the user to bind a complete connection ID, slug, and model tuple and recover the Session.
The compatibility update is also correct on the code path: current main is epoch 56 and this head is epoch 57, with the existing epoch ledger preserved. The branch merges cleanly with current main (10470e620304a7887a46e896feaa508f6165e9f4), producing tree 487bad2c056b1b1881234924d2beef02a7075ea1.
I found three non-blocking code/UX cleanups:
- [P3]
session-catalog-coordinator.tsretains a second throw for an absent model target after an earlier guard has already rejected that same state. The later branch is unreachable and can be removed to keep one recovery authority. - [P3] Legacy Sessions still expose non-model controls even though the Host now blocks all non-fake execution until an account is selected. Disabling those controls, or explaining that account recovery comes first, would avoid presenting actions that can only fail.
- [P3] The helper input types for
runtimeHostExecutionUnavailableReasonandruntimeHostSafeBoundaryContinuationUnavailableReasonmakebackendoptional even though the implementation uses it for the fake-backend exemption. Every current caller passes a complete Session header, so this is not a current runtime defect; making the field required would keep the type contract aligned with the decision.
There is also one documentation cleanup before merge: the PR description still says epoch 53 → 54 and cites validation on the old frozen head 751b196f6. The landing record should state 56 → 57 and reflect the current-head verification.
Exact-head verification is now green. The first test attempt failed only one Desktop prompt-rail end-to-end assertion after 1320 unit tests passed; the rerun of the same exact head completed successfully, as did windows_recovery. The PR does not change the prompt-rail production path. There are no review threads or current-head approvals.
Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查，也不能替代人类审查。有人类对本条评论负责，如有错误请直接指出。
29a6381 to
b1ea437
Compare
|
Rebased the identity slice onto current |
|
Current head The hosted |
Persist canonical Connection identity for newly created Sessions and propagate it into AgentRun and derived execution state. Bound execution resolves by ID plus slug and never follows a reused slug. Existing and legacy Sessions reject ambiguous configuration rebinding until an exact-identity wire slice lands. Generated-by: Codex
Generated-by: Codex
ChatModelChoice and DesktopConnectionSnapshot now require immutable Connection identity; carry connectionId through the story fixtures so the storybook typecheck matches the bound session identity contract.
…jection The Host default-target projection now carries each Connection's immutable connectionId (the same IdentifiedLlmConnection contract the rest of the suite asserts); the without-inventing-a-second-authority expectation was the one row left comparing the pre-identity shape.
b1ea437 to
91ce5be
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
NO-GO on the current head.
[P1] Scheduled AgentRuns can still bind to a different account after slug reuse
A ScheduledTask still persists only llmConnectionSlug. When it fires, the coordinator looks up the current connection by slug and stamps that connection ID onto the new Session.
On the normal supported path “create task with A → delete A → create B with the same slug → task fires”, the task silently executes with B. This directly violates the immutable-identity invariant claimed by this PR.
Changing the PR wording is not enough because the unsafe behavior remains. The smallest safe fix is to fail closed for slug-only ScheduledTasks until #3927 lands. Alternatively, persist and validate the immutable Connection ID here, but I recommend keeping that work in #3927.
简体中文
当前版本不建议合并。
[P1] 定时 Agent 任务仍可能在 slug 被复用后切换账号
ScheduledTask 目前只保存连接 slug。任务触发时，代码会按 slug 找到当前账号，再把它的 ID 写入新 Session。
因此在“用账号 A 创建任务 → 删除 A → 创建同名账号 B → 任务触发”这条正常路径下，任务会静默改用 B，违反本 PR 的不可变账号绑定原则。
只修改 PR 描述无法解决实际风险。最小方案是让只有 slug 的旧任务直接拒绝执行，等 #3927 再完整支持不可变连接 ID。
|
Fixed the current-head ScheduledTask slug-reuse P1 in Slug-only The production Host regression creates and triggers a slug-only Agent task, then verifies a durable failed result with the immutable-identity diagnostic and no |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The earlier P1 is closed on the current head. Slug-only ScheduledTask Agent runs now fail before Session or AgentRun identity is allocated, so a deleted account cannot silently rebind to a same-slug replacement. Exact-head checks are green.
Non-blocking suggestion: reject new slug-only AgentRun tasks at creation time as well, instead of accepting a task that is guaranteed to fail when it fires. Existing persisted tasks should keep the current fail-closed behavior until #3927 lands. This does not need to block this PR.
简体中文
之前的 P1 已关闭。只有 slug 的 ScheduledTask Agent 任务会在分配 Session 或 AgentRun 身份前失败，不会静默切换到同名新账号。当前 head 的 CI 已全部通过。
非阻塞建议：新建这种任务时就直接拒绝，不要先创建成功、等触发时再失败。已持久化的旧任务继续保持当前 fail closed，完整支持留给 #3927。这个建议不阻塞本 PR。
3 participants
Summary
Persist immutable Connection identity on Sessions and AgentRuns, then carry that exact identity through Runtime Host protocol, execution, CLI/TUI, and Desktop model recovery.
This is the self-contained identity vertical slice for #3852. It now includes the exact recovery wire and surfaces because the previous split left upgraded legacy Sessions with a visible model control that could never succeed.
Fixes #3926.
Safety invariant
llmConnectionId + llmConnectionSlug + model.connectionId + connectionSlug + modelselection and commits that exact tuple atomically.Scope
Included:
Excluded:
Compatibility
This changes the Runtime Host wire shape and advances
RUNTIME_HOST_COMPATIBILITY_EPOCHfrom current-main epoch 60 to 61.The Session projection now requires nullable
llmConnectionId; an explicit model target requires ID, slug, and model. Older peers fail the existing compatibility handshake rather than interpreting a partial identity.Persisted Desktop/CLI data remains readable. Legacy-null and deleted-account Sessions do not silently recover; the surface explains the state, and only an explicit current-account selection restores execution.
Validation
590d37cea; synthetic merge is clean.60 -> 61.git diff --checkpassed.Generated-by: Codex