fix(desktop): snapshot linked children for side conversations - #3669
Conversation
There was a problem hiding this comment.
Reviewed at exact head e078acdbcbeb73bb51c3ead08580659964ea8911 (36 files, +1550/-146, MERGEABLE).
No findings (P0–P3). The three questions this change most needs answered all check out:
1. When does the snapshot happen? Inside the existing admission lease, which covers source + target + revision root plus every linked-child lane (agentGraphRevisionAdmissionSessionIds feeds the retry_admission pass before prepareAgentGraphRevisionReferences runs). While that lease is held, a retained linked child that is still active fails with session_busy, and a retained live Agent Graph fails the same way (session-revision-graph-references.ts). The snapshot therefore cannot read a mid-flight child mutation.
2. Can the copy drift from the source afterwards? By design it neither tracks nor aliases: snapshot mode strips childSessionId / runId / resumedFromRunId from subagent results and swarm items, rewrites child artifact references to copies owned by the target session, rehydrates archived linked-child results with ownership removed, and drops child_agent ownership in the copied task ledger (task-ledger-store.ts). The result is reference-only frozen context, exactly what a side conversation wants.
3. Can a linked child be missed by enumeration? directChildren not represented in the copied slice are caught explicitly: active → session_busy, terminal-but-unrepresented → fail closed with operation_unavailable. A child created after the source turn is refused rather than silently dropped.
The ordinary paths are preserved by explicit gates: intent is rejected by the revision operation's decoder (decodeSessionRevisionCopyInput), kind !== 'side_conversation' keeps the old conversation-owned-reference hard failure for branch/revision, the fingerprint namespace is versioned (v1 vs v2) so a side-conversation retry can never resolve against an ordinary branch's in-flight copy, and the wire change is epoch-gated (44 → 45; current main is 44, no collision).
Executed evidence (real Windows x64, this head): runtime-host session-revision-protocol + session-revision-graph-references 17/17; storage artifact-store + task-ledger-authority 46/46 (5 skips are platform gates); desktop quote-companion-{retry,cleanup,disposal} + runtime-host-session-execution-ipc-main 28/28; runtime conversation-copy 10/15 — the 5 failures are EBUSY: unlink runtime.sqlite in temp dirs and reproduce identically on the PR's base commit (84ed9a31) on this machine, i.e. a pre-existing Windows-only suite flake, not this PR. All four new side-conversation snapshot tests pass. (Non-blocking note for the repo, not this PR: the runtime conversation-copy suite has never been green on a real Windows developer machine.)
Checks: path-filtered as expected — only test applies to this diff (no manifest/lockfile/sandbox paths), completed/success on the exact head. audit correctly absent.
简体中文
零 finding。三个关键问题的答案:快照发生在覆盖源/目标/修订根/全部 linked child 通道的 admission lease 内,运行中的 child 或存活 graph 会以 session_busy 拒绝;快照剥离 childSessionId/runId/resumedFromRunId、把 child artifact 复制进目标命名空间、任务台账丢弃 child_agent 归属,因此快照之后源端再变动不会影响副本(这正是设计意图);slice 里没引用到的 direct child 会被显式枚举检查拦下(活跃→busy,未表示→fail closed)。普通 branch/revision 路径由 intent 拒绝、指纹 v1/v2 分域、epoch 44→45 三道门保持原样。Windows 真机执行:17/17 + 46/46 + 28/28 绿,conversation-copy 的 5 个失败在 base commit 上同样复现(既有的 Windows EBUSY 测试 flake,与本 PR 无关)。
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at e078acdb. No P0–P3. Approving.
The three things worth checking on a snapshot change all hold:
- When the snapshot is taken — it runs inside the admission lease covering the source, the target, the revision root and every linked-child channel, so a live graph or an active child resolves to
session_busyrather than a torn snapshot. - Divergence afterwards — that is the intent, not a gap: ownership ids are stripped, artifacts are copied into the target namespace, and the ledger drops
child_agentattribution, leaving a reference-only relationship. - Enumeration gaps —
session-revision-graph-references.ts:101-105explicitly filters children not represented in the request and fails withsession_busyif any is active. The retained-graph path fails closed too: an unreadable graph state returnsoperation_unavailablerather than proceeding, and ordinary branches are rejected outright before any of this.
test is terminal green on this exact head, and it is the only check the path filter should trigger here.
One non-blocking note for the repo rather than this PR: on a real Windows machine the conversation-copy suite fails 5 of 15 on EBUSY during unlink, and it reproduces identically on the base commit. It is a pre-existing Windows-only flake, unrelated to this change, but it appears never to have been green there.
中文
在 e078acdb 上审,无 P0–P3,approve。三个关键点都成立:快照发生在覆盖源、目标、修订根和全部 linked child 通道的 admission lease 之内,活跃 child 或 live graph 会直接 session_busy;快照之后父子失同步是设计本身(剥 ownership id、artifact 复制进目标命名空间、台账丢 child_agent 归属,只保留引用关系);枚举缺口由 session-revision-graph-references.ts:101-105 显式拦截,未表示且仍活跃的 child 一律 session_busy,retained graph 读不到状态时返回 operation_unavailable(fail closed),普通 branch 更是提前拒掉。
另有一条与本 PR 无关、供仓库参考的非阻塞信息:真 Windows 机器上 conversation-copy 套件 15 条挂 5 条(unlink 时 EBUSY),在 base commit 上同样复现,属既有的 Windows-only flake。
There was a problem hiding this comment.
Review at exact head e078acdbcbeb73bb51c3ead08580659964ea8911 against merge-base 84ed9a31018ce31aee228473af4d03c35def0f3f. No P0–P2. Three P3 design/documentation opportunities, inline. The exact-head test check is green.
Verdict
- Problem: valid. A disposable Side Conversation needs a historical child result, not shared lifecycle/control authority over the source child. The existing ordinary-branch rejection was a correct invariant; the missing distinction was copy purpose.
- Mechanism: addresses the root cause end to end. Purpose reaches the Host and the durable fingerprint; the Host admits the source/target/root/child lanes, refuses live graphs or children, validates terminal run/artifact provenance, copies referenced artifacts, strips child control identities from typed results, removes child task ownership, and publishes only after all ledgers commit.
- Design scope: an ownership-free terminal snapshot is smaller and safer than cloning/reparenting children or weakening the branch invariant. Runtime Host remains the authority, Runtime owns typed reference rewriting, and Storage owns byte/task persistence; those seams pass the deletion test. Epoch 45 correctly closes the incompatible-peer boundary.
- Correctness: I found no missing acceptance criterion or scope creep. I specifically challenged archived-result hydration and opaque
providerOutput: neither yielded a reachable deterministic failure or ownership leak. Structured busy/unsupported results and the latest-settled-turn retry path are covered. - Optimality: the behavior is right, but the representation can be made deeper. The inline P3s cover the scattered copy-purpose vocabulary, a duplicate artifact map, and missing UI evidence.
Further mechanical deletion opportunities: classify each archived body once instead of separately deserializing it for ownership, linked-child detection, and reference collection; and move the busy→retry→success transition into a pure companion-core state module so the 186-line Linkedom test can become a small wiring smoke plus a table test (roughly 120–140 harness lines deletable).
简体中文
问题定义成立:Side Conversation 需要的是子会话的历史结果,不应继承对子会话的生命周期/控制权;普通 branch 的拒绝规则本身没有错,缺的是明确的 copy purpose。当前实现从 Renderer 一直把 purpose 送到 Host 和持久化指纹,并在 admission lease 内验证 graph/child/run/artifact 的终态一致性,再复制 artifact、剥离 childSessionId/runId/resumedFromRunId、去掉 child task ownership,所有 ledger 成功后才发布,因此解决的是根因而非 UI 症状。没有 P0–P2。三个 P3 分别是:缺少 CONTRIBUTING 要求的 UI 前后证据;同一 purpose 被 boolean、optional intent、semantic kind、label 多套表示;snapshot 中重复保存同一个 artifact 映射。另可把 archive body 分类集中一次,并把 186 行 Linkedom retry 测试拆成纯状态表测 + 小型 wiring smoke,预计可删除约 120–140 行 harness。
There was a problem hiding this comment.
English
Reviewed independently at exact head e078acdbcbeb73bb51c3ead08580659964ea8911. GO: no P0–P2 findings. I independently retained two non-blocking P3 cleanup/documentation items that already have current-head threads, so I am not duplicating them inline: add the CONTRIBUTING.md-required UI screenshot/recording, and delete the nested snapshot artifactIds alias because the outer exact reference map already carries the identical source-to-target mapping.
First-principles verdict
The problem definition is correct. An ordinary branch must not share lifecycle/control authority over linked child Sessions, while a Side Conversation needs terminal child output only as immutable reference context. The existing rejection was the right invariant; the missing distinction was an explicit copy purpose.
The implementation addresses that root cause at the existing authority seam:
side_conversationtravels from Renderer through Desktop IPC and the Runtime Host protocol, participates in a versioned idempotency fingerprint, and is persisted before publication;- the Host admits the source, target, and all retained child lanes before checking terminality and provenance, so live source/child/graph state returns structured
session_busyinstead of producing a torn snapshot; - snapshot mode removes
childSessionId,runId, andresumedFromRunId, copies validated child artifacts into target-owned IDs, rewrites archived linked results inline, and removes child task ownership/run references; - ordinary branches retain their fail-closed rejection, revisions retain validated-reference semantics, and failed creation does not leave a visible/preparing target;
- Desktop retries a busy source from the newest settled turn using a fresh copy identity and clears the stale error after success.
This is more minimal than cloning/reparenting child Sessions or weakening the ordinary-branch invariant. The 36-file spread is one typed semantic propagated through protocol, storage, runtime, and Desktop boundaries, not a second copy authority. A separate operation would duplicate more of the existing copy stack.
Validation: Core, Storage, Runtime, Runtime Host, MCP, Computer Use, UI, Desktop main, and Desktop preload builds passed. Focused suites completed 114 passed / 2 platform skips / 0 failed across 116 tests. git diff --check and Biome over all changed TS/TSX files passed. The exact-head hosted test is terminal green; the PR is OPEN, non-draft, MERGEABLE/clean. Fresh current main 04836d3b8053c68b8d0b5e4101608bf66f5d1020 has zero changed-file overlap with this PR since its base, and synthetic merge tree 0afc7721937880ff8294eb7388a9ad8c1649cfa8 materializes cleanly. All unresolved review threads are current-head P3s; there is no unresolved P0–P2 thread and no stale-head approval.
中文
在 exact head e078acdbcbeb73bb51c3ead08580659964ea8911 上独立审查。GO:没有 P0–P2。 我独立保留了两条非阻塞 P3,而且它们已有当前 head 的 thread,因此不重复发 inline:补上 CONTRIBUTING.md 要求的 UI 截图/录屏;删除 snapshot 内层的 artifactIds 别名,因为外层 exact reference map 已经持有同一个 source→target 映射。
第一性原理结论
问题定义是对的:ordinary branch 不能共享 linked child Session 的生命周期/控制 authority;Side Conversation 只需要把 terminal child 输出作为不可变的参考上下文。原来的拒绝规则没有错,缺的是显式 copy purpose。
实现是在现有 authority seam 上解决根因:
side_conversation从 Renderer 贯穿 Desktop IPC 与 Runtime Host protocol,进入版本化 idempotency fingerprint,并在发布前持久化;- Host 先锁住 source、target 和全部 retained child lane,再检查终态与 provenance,因此 live source/child/graph 会返回结构化
session_busy,不会读出撕裂快照; - snapshot mode 删除
childSessionId/runId/resumedFromRunId,把验证后的 child artifact 复制为 target-owned ID,inline 重写 archived linked result,并清掉 child task ownership/run reference; - ordinary branch 继续 fail closed,revision 保留原来的 validated-reference 语义,失败创建不会留下可见/preparing target;
- Desktop 对 busy source 会重读最新 settled turn、使用新的 copy identity 重试,成功后清除旧错误。
这比 clone/reparent child Session 或放宽 ordinary-branch invariant 更小、更安全。36 个文件是在 protocol、storage、runtime、Desktop 边界传播同一个 typed semantic,不是增加第二套 copy authority;另开 operation 反而会复制更多现有 copy stack。
验证:Core、Storage、Runtime、Runtime Host、MCP、Computer Use、UI、Desktop main/preload build 通过;focused tests 共 116 条,114 pass / 2 platform skip / 0 fail;全部 changed TS/TSX 文件通过 Biome,git diff --check 通过。exact-head hosted test 终态绿;PR OPEN、非 draft、MERGEABLE/clean。Fresh current main 04836d3b8053c68b8d0b5e4101608bf66f5d1020 自 PR base 起与本 PR changed-file 0 overlap,synthetic merge tree 0afc7721937880ff8294eb7388a9ad8c1649cfa8 clean。所有 unresolved review thread 都是当前 head 的 P3;没有 unresolved P0–P2,也没有 stale-head approval。
e078acd to
a21b599
Compare
af3b5bc to
aa66984
Compare
There was a problem hiding this comment.
Walked all 36 files at head aa669841781090db196b1bff9ad6ea9ee67a1ad2. No P0-P2; one P3, inline below. CI is still queued, so I am not looking at approval yet.
For context: the two approvals on e078acdb were dismissed because they were bound to a stale head. One of them was M4n5ter's own review and was dismissed by our account, which was our process mistake — we have already explained it to him.
简体中文
在当前 head aa669841781090db196b1bff9ad6ea9ee67a1ad2 上把 36 个文件都走了一遍,没有 P0–P2,只有下面这一条 P3。CI 目前还在排队,等它绿了再看批准的事。
另外说明一下,之前 e078acdb 上的两条 approve 已经因为绑定的 head 过期被撤掉了,其中一条是我们账号误撤了 @M4n5ter 本人的,这点是我们的流程问题,已经跟他说明。
6551f9b to
7c62833
Compare
Closes apache#3654 Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Avoid restarting eager setup when the source Session projection refreshes. Allow Side Conversations to snapshot a settled boundary while newer source work continues, while still waiting for retained child and Graph state by exact identity. Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
7c62833 to
5f3c31f
Compare
|
Updated on top of current main and addressed the latest review follow-ups:
Validation on head 5f3c31f:
The conditional 2-second fallback remains intentional. It starts only after session_busy, stops after success or unmount, and covers linked child or Graph completion that is not guaranteed to arrive through the source Session subscription. |
Astro-Han
left a comment
There was a problem hiding this comment.
Approving at 5f3c31fb6. CI is terminal green on this exact head (run 32717978917), and I have no blocking findings. One non-blocking note is below.
This was reviewed independently along two tracks and I checked the substantive claims against the code myself. The storage and protocol work holds up: intent is an optional field that older records simply omit, the strict decoder accepts it only on a branch copy and rejects it on a revision, and nothing in the copy, rebuild, repair, backfill, or terminal-read paths drops it. Snapshotting a linked child correctly strips child owner and run identity while keeping main-owned task refs, and the archived result is hash- and size-checked before it is inlined, with a rollback across the sidecars and target on failure.
The epoch guard is right, and worth calling out because it is easy to get wrong: the new assertion is > 47, so it actually fails if the bump to 48 is reverted, and the comment records why 47 was unavailable. That is the standard the other epoch tests in that file follow.
[P3] Legacy Quote Companion creation leases stay stuck after upgrade.
cleanupOnce replays the persisted lease verbatim at apps/desktop/src/main/quote-companion-cleanup.ts:205. intent is new here and optional, so a phase: "creating" row written by an earlier Desktop carries none; recovery therefore resumes it as an ordinary branch, the Host applies the linked-child rejection, the row is retained, and the failure repeats on every launch. The new restart test seeds only a current-format lease with intent: "side_conversation", so this path is untested.
To be precise about ownership, because it matters here: this is not a regression you introduced. The ordinary-branch rejection already exists at the merge-base in session-revision-graph-references.ts, so such a lease was already looping on every launch before this change. What this PR does is fix that loop for newly written leases and leave the old ones where they were. Recognising or migrating a legacy creating lease as a Side Conversation would close it, as would retiring the lease outright once the Host has proven the target absent. A follow-up is fine by me.
Verification on my side, beyond the PR's own CI: merged into current main (8801edc32, two commits ahead of your base) — clean, with the epoch landing at 48 over main's 47. On that merged tree the runtime-host session-revision graph-reference, session-revision protocol, and bootstrap protocol suites pass 67/67, and storage artifact-store and task-ledger-authority pass 49/49 with 2 skipped. @maka/runtime fails to build in my checkout with five errors, but byte-identically on a clean main checkout in the same environment — missing @slack packages and an incompatible proxy agent — so none of it is attributable here.
One thing to be aware of when you read CI: main is currently red on Check ASF source headers, from five plugin files added by #3250. It has nothing to do with this branch, and #3708 addresses it. I would rather land this after main is green again so that the merge result is readable — happy to do that once it is, unless you would prefer otherwise.
简体中文
在 5f3c31fb6 上给出 approve。这个 exact head 的 CI 终态绿(运行记录 32717978917),我没有阻塞性问题。下面有一条不阻塞的说明。
这个 PR 由两条独立线各自复审,我自己也对照代码核过其中的实质论断。存储和协议部分是站得住的:intent 是可选字段,旧记录不带它即可;严格解码器只在 branch 复制上接受它、在 revision 上拒绝;copy、rebuild、repair、backfill 和终态读取各条路径都没有把它丢掉。linked child 的快照正确地剥离了 child 的 owner 与 run 身份,同时保留了 main 持有的 task refs;archived result 在内嵌前会校验哈希与大小,失败时会跨 sidecars 和 target 回滚。
epoch 守卫写对了,值得单独提一句,因为这一点很容易写错:新增断言是 > 47,因此一旦把提升到 48 的改动回退,它确实会失败;注释里也记录了 47 为何不可用。这正是该文件里其它 epoch 测试遵循的写法。
[P3] 升级后,旧的 Quote Companion creation lease 会一直卡住。
cleanupOnce 在 apps/desktop/src/main/quote-companion-cleanup.ts:205 原样回放持久化的 lease。intent 是本次新增且可选的,因此旧版 Desktop 写入的 phase: "creating" 记录不带它;恢复时便会把它作为普通 branch 续做,Host 施加 linked-child 拒绝,记录被保留,此后每次启动都重复这次失败。新增的重启测试只写入了带 intent: "side_conversation" 的当前格式 lease,因此这条路径没有被覆盖。
归属上我要说准确,因为这里很关键:这不是你引入的回归。 普通 branch 的那条拒绝在 merge-base 上就已存在(session-revision-graph-references.ts),所以这样的 lease 在本次改动之前就已经每次启动都在循环失败了。本 PR 所做的是为新写入的 lease 修好了这个循环,而把旧的留在了原处。把旧的 creating lease 识别或迁移为 Side Conversation 即可闭合这一点;或者在 Host 已证明目标不存在时直接终止该 lease。放到后续 PR 处理我也没有意见。
除 PR 自身 CI 外,我这边的验证:合入当前 main(8801edc32,比你的 base 新两个提交)——合并干净,epoch 在 main 的 47 之上落到 48。在合并后的树上,runtime-host 的 session-revision graph-reference、session-revision protocol 与 bootstrap protocol 三套 67/67 通过,storage 的 artifact-store 与 task-ledger-authority 49/49 通过、2 项跳过。@maka/runtime 在我的检出上构建失败、报 5 个错,但在同一环境下干净 main 检出上逐字节一致(缺少 @slack 相关包以及代理 agent 版本不匹配),因此与本次改动无关。
读 CI 时请注意一点:main 目前在 Check ASF source headers 上是红的,来自 #3250 新增的五个 plugin 文件。这与本分支无关,#3708 正在处理。我倾向于等 main 恢复绿之后再合入本 PR,这样合并结果才可读——届时我很乐意来合,除非你有别的偏好。
|
LGTM. 简体中文已确认。当前 head 上 |
Summary
Closes #3654
Verification
session_busywhile a Side Conversation can snapshotturn-2during a newer active turn, and that the active turn is not copied@maka/runtime-hostand all Desktop targets after rebuilding workspace dependenciesgit diff --check origin/main...HEADorigin/main@d77e854f7before

after

AI use
Select exactly one:
Tool(s) and scope: Codex contributed implementation, tests, verification, and review; Claude performed an additional review of the completed change.
Checklist
Does this PR entail a change in behavior?