Skip to content

fix(desktop): bind imported sessions to selected workspace - #5552

Merged
me2seeks merged 4 commits into
apache:mainfrom
me2seeks:fix/5551-import-workspace
Sep 22, 2026
Merged

me2seeks merged 4 commits into
apache:mainfrom
me2seeks:fix/5551-import-workspace

Conversation

@me2seeks

@me2seeks me2seeks commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Refs #5551

  • Resolve the Desktop-selected workspace on the target Runtime Host before importing an external Session.
  • Carry the validated WorkspaceTarget through the external-session import protocol.
  • Persist the Host-resolved canonical cwd and projectId instead of the source adapter cwd.
  • Keep source-cwd fallback for older protocol callers that do not provide a target.
  • Keep destination-resolution failures distinct from an uncertain import commit.

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-host
  • Runtime Host external-session protocol and coordinator tests: 38 passed.
  • Desktop external-session IPC test, isolated against the current source with esbuild: 12 passed.

npm run typecheck --workspace @maka/desktop remains blocked by the current dependency/dist snapshot: unrelated missing UI/runtime exports and unavailable packages such as virtua and AI SDK modules. The affected Desktop IPC test was compiled and executed independently.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex authored the reproduction, implementation, tests, and local validation under human direction.

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jackwener

Copy link
Copy Markdown
Member

Seat: kabi-opus-review-orchestrator — re-bind after head drift.

My earlier review was bound to a74cb8e43. The head is now f86a7a3e8eb6332301236b81f556f97df151c906, so I re-checked rather than let the old conclusion stand on code it no longer describes.

The P3 is closed. f86a7a3e8 adds exactly the entry that was missing, and in the right shape — it names the change and then says who rejects it, mirroring how the 169 entry ends:

// 170: External Session import input may carry an optional Host-resolved
// workspace target. Epoch-169 peers reject the unknown `workspace` key.

Everything I verified still holds, and I checked that rather than assumed it. The new head is a rebase onto 1c18700a7 plus the docs commit. Both patch sets touch the identical ten files, and byte-comparing each one across the two heads, the only author-side difference is protocol/index.ts. external-session.ts (the requireExactRecordrequireShapedRecord relaxation) and external-session-coordinator.ts (resolving the target through #workspaceResolver rather than trusting the client) are unchanged. runtime-host-client.ts and runtime-host-boot.ts differ only because the rebase picked up main's own changes.

One scheduling note. PR #5554 also bumps to epoch 170, for an unrelated reason — it removes deep-research.query. Both pass CI today because each is measured against a base at 169. Whichever merges second will fail scripts/protocol-epoch-check.mjs, which runs on the merge result and will say the epoch must move past the base it now sees. I exercised that check in both directions to be sure it fires rather than assuming it would. This PR is the one with its entry already written, so nothing is owed here — it is just worth knowing that the two are racing for the same number, and the loser rebases.

简体建议

重新绑定 head。 我之前的评审绑的是 a74cb8e43,现 head 为 f86a7a3e8,所以我重新核了一遍,而不是让旧结论停留在它已不再描述的代码上。

那条 P3 已关闭。 f86a7a3e8 补上的正是缺的条目,而且形状对 —— 先说改了什么,再说谁会拒绝它,与 169 条目的收尾写法一致。

我核过的东西依然成立,而且这一点我是验的、不是假设的。 新 head = rebase 到 1c18700a7 + 那个 docs 提交。两份补丁集触及完全相同的十个文件;逐文件跨 head 做字节比对,作者侧唯一的差异就是 protocol/index.tsexternal-session.ts(requireExactRecordrequireShapedRecord 的放宽)与 external-session-coordinator.ts(经 #workspaceResolver 解析目标而非信任客户端)字节未变runtime-host-client.tsruntime-host-boot.ts 的差异仅来自 rebase 带进来的 main 自身改动。

一条排期提示。 PR #5554 也把 epoch 提到 170,理由完全无关(它移除 deep-research.query)。两者今天都能过 CI,因为各自对照的 base 都还在 169。后合的那个会在 scripts/protocol-epoch-check.mjs 上失败 —— 该检查跑在合并结果上,会要求 epoch 越过它此时看到的 base。我把这个检查两个方向都实际跑了一遍,确认它确实会触发,而不是想当然。本单条目已经写好了,所以这边不欠什么;只是值得知道两者在抢同一个号,输的那个要 rebase


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.

@me2seeks
me2seeks force-pushed the fix/5551-import-workspace branch from aeb24da to 5adb9e8 Compare September 22, 2026 12:39
@me2seeks
me2seeks force-pushed the fix/5551-import-workspace branch from 5adb9e8 to ce14467 Compare September 22, 2026 12:40

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@me2seeks
me2seeks merged commit 0052f1c into apache:main Sep 22, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants