fix(desktop): bind imported sessions to selected workspace - #5552
Conversation
jackwener
left a comment
There was a problem hiding this comment.
Seat: kabi-opus-review-orchestrator. Review of exact head a74cb8e43. Not a draft, MERGEABLE, all checks green or skipped. Merge-base feb9cf22f, behind main by 2. COMMENT only.
Verdict
No P0–P2. 1×P3. The wire change is shaped correctly and the epoch bump is warranted; what is missing is the record of why.
What I checked
The optional field is genuinely optional, and the decoder was widened to match. ExternalSessionImportInput.workspace is readonly workspace?: WorkspaceTarget, and decodeExternalSessionImportInput moves from requireExactRecord (which rejects unknown keys) to requireShapedRecord with workspace in the optional set. The comment states the fallback: older clients retain the source-cwd behaviour.
The epoch bump is warranted, and in one direction specifically. New Host plus old client is fine — the field is simply absent and the fallback applies. Old Host plus new client is not: the old decoder's requireExactRecord rejects the unknown workspace key outright. That asymmetry is exactly what the compatibility epoch exists to fence, so 169 → 170 is correct rather than defensive.
The coordinator honours the field through the resolver rather than trusting the client. input.workspace is passed through #workspaceResolver.resolve(...) and only the resolved cwd reaches the target, so a client cannot name an arbitrary path and have it used verbatim.
The tests include the failure direction, which is the part usually missing: alongside uses the Host-resolved workspace as the imported Session cwd there is does not turn a missing import destination into an uncertain commit — that second one is the interesting case, because a bad destination leaving an import half-committed is worse than refusing it.
P3 — epoch 170 has no changelog entry
protocol/index.ts keeps a descending list explaining why each epoch breaks interoperability. 170 has no entry. The entry for 169 sits directly beneath the bumped constant, ending "Epoch-168 peers reject the new state as an invalid frame" — which is the shape this one needs.
To be accurate about the convention rather than overstate it: entries exist for 160–167 and 169, and 168 is a prior lapse. So this is not unprecedented. But the list is the only place recording why a peer at 169 must be rejected, and without an entry the next person to debug a rejected frame has the number and no reason. Suggested text is already implied by the diff: the import input gains an optional workspace, and epoch-169 Hosts reject it as an unknown key.
未验证
I did not run any suite — the green above is hosted CI, not my own run. I did not exercise an actual import against a real Host, so the resolver behaviour is read from source rather than observed. I did not review the desktop-side IPC and boot changes in detail beyond confirming they thread the new field.
Cross-PR note: #5553 touches one file in common, apps/desktop/src/main/runtime-host-client.ts, and both add a type WorkspaceTarget import there. The two are otherwise disjoint — #5552 is protocol and Host, #5553 is the renderer repair path — and #5553 adds no protocol change and needs no epoch bump, because session.workspace.relocate already exists on main. Expect a small textual conflict on whichever lands second, not a semantic one. Neither branch has been tested against the other.
简体中文
结论:无 P0–P2,1×P3。 线协议改动的形状是对的,epoch 提升也有必要;缺的是「为什么」的记录。
已核:①可选字段是真可选,解码器也相应放宽 —— workspace?: WorkspaceTarget,解码从 requireExactRecord(拒绝未知键)改为 requireShapedRecord 并把 workspace 放进可选集;注释写明老客户端保留 source-cwd 回退。②epoch 提升有必要,且方向具体:新 Host + 老客户端没问题(字段缺失、走回退);老 Host + 新客户端不行 —— 老解码器的 requireExactRecord 会直接拒掉未知的 workspace 键。这种不对称正是兼容 epoch 要围住的东西,所以 169→170 是必要而非保守。③协调器是经 resolver 采信该字段,而不是信任客户端 —— input.workspace 走 #workspaceResolver.resolve(...),只有解析后的 cwd 进入 target,客户端无法指定任意路径直接生效。④测试包含了失败方向(通常缺的正是这个):除 uses the Host-resolved workspace as the imported Session cwd 外,还有 does not turn a missing import destination into an uncertain commit —— 后者才是有意思的用例,因为坏目的地把导入停在半提交状态,比直接拒绝更糟。
P3 —— epoch 170 没有变更说明条目。 protocol/index.ts 维护着一份倒序列表,解释每个 epoch 为何破坏互操作;170 没有条目。169 的条目就在被改的常量正下方,结尾是「Epoch-168 peers reject the new state as an invalid frame」——正是这条需要的形状。
为准确起见不夸大惯例:160–167 与 169 都有条目,168 是此前的一次遗漏,所以这并非史无前例。但那份列表是唯一记录「为什么 169 的对端必须被拒」的地方;没有条目,下一个排查被拒帧的人只有数字、没有原因。建议文字其实已由 diff 蕴含:导入输入新增可选 workspace,epoch-169 的 Host 会把它当未知键拒绝。
未验证:未跑任何套件(上面的绿是 CI 的);未对真实 Host 做过实际导入,resolver 行为是读源码而非观察到的;桌面侧 IPC 与 boot 的改动除确认它把新字段穿下去之外未细审。
跨 PR 提示:#5553 与本单共享一个文件 runtime-host-client.ts,两者都在其中新增 type WorkspaceTarget 导入。除此之外互不相交(本单是协议与 Host,#5553 是渲染端修复路径),而且**#5553 没有协议改动、也不需要 epoch 提升**,因为 session.workspace.relocate 在 main 上已经存在。后落地的那个预计只会有一处文本冲突,不是语义冲突。两条分支都没有相互测试过。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
96253b2 to
f86a7a3
Compare
|
Seat: kabi-opus-review-orchestrator — re-bind after head drift. My earlier review was bound to The P3 is closed. Everything I verified still holds, and I checked that rather than assumed it. The new head is a rebase onto One scheduling note. PR #5554 also bumps to epoch 170, for an unrelated reason — it removes 简体建议重新绑定 head。 我之前的评审绑的是 那条 P3 已关闭。 我核过的东西依然成立,而且这一点我是验的、不是假设的。 新 head = rebase 到 一条排期提示。 PR #5554 也把 epoch 提到 170,理由完全无关(它移除
|
aeb24da to
5adb9e8
Compare
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
5adb9e8 to
ce14467
Compare
jackwener
left a comment
There was a problem hiding this comment.
Approved. I found no P0–P2 issues in this head. The import path resolves the selected workspace on the target Runtime Host before committing the Session, rejects conflicting source and request destinations, and preserves the compatibility ledger by advancing the epoch from main's 176 to 177. The exact-head test check and protocol epoch check passed, the current-main merge is clean, and there are no unresolved review threads.
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Summary
Refs #5551
WorkspaceTargetthrough the external-session import protocol.cwdandprojectIdinstead of the source adaptercwd.cwdfallback for older protocol callers that do not provide a target.This is the import-time prevention half of #5551. A follow-up PR will repair already-imported Sessions by rebinding the same Session when its persisted workspace is unavailable.
Validation
npm run typecheck --workspace @maka/runtime-hostnpm run typecheck --workspace @maka/desktopremains blocked by the current dependency/dist snapshot: unrelated missing UI/runtime exports and unavailable packages such asvirtuaand AI SDK modules. The affected Desktop IPC test was compiled and executed independently.AI use
Tool(s) and scope: Codex authored the reproduction, implementation, tests, and local validation under human direction.