Repository navigation
feat(channels): Implement named-session worktree reset (Part 4B) - #11015
Conversation
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Design review — head ae2e231 (C=3, M=3, S=5)
Reviewed the proposal against the code at this PR's base (9c320cb0, which contains Part 4A 37cb9ac161), not against the doc's own prose. The whole "Verified baseline" section holds — every claim I sampled matches the tree, including the create/restore fail-closed ordering, validateManagedSessionIdentity's five conditions, the Part 4A rejection landing before any ChannelBase side effect, and the strict reader's three-state contract. Both ride-along fixes are real live bugs, and the doc's correction of the F3 claim is right.
The supersede-then-flip ordering, the bidirectional supersedes/supersededBy invariant, the idempotent resume, a dedicated route instead of threading adopt through create, and keeping S_old persisted are all sound. The crash-window table is exhaustive for the single-caller case.
Three findings are "implement this as written and it breaks", and one of those is a regression against current behavior:
- C1 — the F2 fix's central premise is inverted. In the
hasUnlocatedRestoredPromptbranch the restore prompt has already been fired, not parked.changeSessionCwdthere queues behind a liveask_user_questionand then throws. Today that load returns 200; the proposal turns it into a 30s hang followed by a failure that discards the recovered question. - C2 — the quiescence probe cannot see a parked deferred prompt.
pendingInteractionCountandhasActivePromptboth read from state a deferred prompt never touches, so a task stopped on a recovered question is classified as idle and reset proceeds. - C3 — the
missing-marker recovery hatch is not a compare-and-swap.renamereplaces unconditionally, and the route's lock is keyed on the session id, not the worktree — so two sessions holding valid sidecars for the same worktree (exactly what a completed or crashed transfer leaves) can both "win" a concurrent reset.
Major: the reap cleanup as specified arms every deleteDaemonSessionIfOrphan call site including reset's own rollback (M1); its gate contradicts the bar the Risks section states (M2); and quiescence is two point-in-time samples rather than mutual exclusion (M3).
I'd also split the reap cleanup and the two ride-along fixes out of this part — see the inline note. That is a sequencing preference, not a correctness objection.
Details inline. Every finding cites the code at the reviewed commit.
中文摘要
对照本 PR base（9c320cb0，含 Part 4A 37cb9ac161）的真实代码逐条核对，而不是采信文档自身的叙述。"Verified baseline" 整节成立——抽查的每条都与代码一致，包括 create/restore 的 fail-closed 顺序、validateManagedSessionIdentity 的五个条件、Part 4A 拒绝落在任何 ChannelBase 副作用之前、以及 strict reader 的三态契约。两个顺带修复都是真实缺陷，文档对 F3 的更正也是对的。
先标 superseded 再翻转 marker 的排序、双向 supersedes 不变量、幂等续做、用独立路由而非改造 create、以及保留 S_old，这些都站得住。崩溃窗口表在单调用方场景下是完备的。
三处属于"按文档实现就会出错"，其中一处是对现有行为的回归：
- C1 —— F2 修复的核心前提是反的。
hasUnlocatedRestoredPrompt分支里 restore prompt 已经点火，不是 parked。此处的changeSessionCwd会排在一个活着的ask_user_question后面然后抛错。今天这条路径返回 200；按提案会变成阻塞 30 秒后失败，并丢掉恢复出来的问题。 - C2 —— 静默性探针看不见 parked 的 deferred prompt。
pendingInteractionCount与hasActivePrompt读的都是 deferred prompt 从不触碰的状态，因此停在恢复问题上的任务会被判为空闲，reset 照常进行。 - C3 ——
missing-marker 恢复口不是 compare-and-swap。rename无条件替换，且路由的锁按 session id 而非 worktree 加键——两个持有同一 worktree 的有效 sidecar 的会话（正是转移完成或崩溃后留下的状态）可以在并发 reset 中双双"成功"。
Major：按文档描述的 reap 清理会给 deleteDaemonSessionIfOrphan 的所有调用点上膛，包含 reset 自己的回滚（M1）；其门槛与 Risks 章节自述的标准矛盾（M2）；静默性是两次时点采样而非互斥（M3）。
另建议把 reap 清理与两个顺带修复从本部分拆出——见内联说明。这是排序偏好，不是正确性异议。
Address the design review on #11015: - Correct the deferred-prompt premise: the under-attesting restore branch serves a genuinely live prompt, so it is removed to fail closed instead of attempting an impossible relocation (C1/S5a). - Surface parked deferred restore prompts in the bridge's active state and consume it in both the coalesced restore (R8-2) and the reset quiescence check (C2); add a reset-pending admission barrier and a pre-flip re-verify so quiescence is enforced, not sampled (M3). - Make the marker transfer a compare-and-swap: owner re-check at the rename commit via atomicWriteFile's assertCanCommit, O_EXCL create for the missing-marker hatch, and worktree-keyed serialization (C3, S1, S2). - Move the orphan-reap worktree cleanup and the R8-1/R8-3/R1-1/F3 fixes out of this part; record every standing Part 4A finding's disposition in a dedicated section (S4, M1, M2). - Add the interrupted-transfer typed signal and message so the one retry-repaired window tells the user to retry. - Credit the review bot's R3-2 alongside the human findings. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Design review round 2 — head c4ee836 (C=2, M=1, S=2)
Re-reviewed the full revision (+429/−215; the doc is now 978 lines, so I read it fresh rather than only diffing) and re-verified every new claim against the code at this PR's base.
All eleven findings from my round-1 review are resolved, and the two that mattered most are resolved by inverting the design rather than patching around it. Worth recording explicitly:
- C1 (F2 premise inverted) — fixed, and the doc now carries the corrected mechanism. The
hasUnlocatedRestoredPromptbranch is removed instead of relocated into, the reversal is stated in the section itself (L571-574), and the Part 4A refusal to relocate is restored as right-on-the-merits. The baseline bullet at L181-190 now describes the branch's actual shape. - C2 (parked prompt invisible to the quiescence probe) — fixed, and correctly re-attributed to the bot's R8-2 as one shared visibility defect with two consumers, which is a better framing than my finding had.
- C3 (recovery hatch was not a CAS) — fixed. Worktree-keyed serialization plus commit-by-shape (
assertCanCommitowner re-check forvalid,O_EXCLformissing), and "Transfer ownership by blind temp-file rename" is now an explicitly rejected alternative. - M1/M2/S4 — resolved by re-scoping. The reap cleanup and the ride-along fixes move out; this part's contract is now literally "no deletion", and my call-site /
kind === 'removed'/ supersede-refusal /branchPreservedrequirements are carried into the follow-up issue rather than dropped. - M3 (quiescence was a sample) — fixed with the reset-pending barrier plus a pre-flip re-verify. S1 — I verified the temp-name claim and it is exactly right (see below). S3 —
packages/acp-bridgeis now in scope. S5 — all four doc corrections landed. - The disposition table is the right answer to the bot's finding 1, and it goes further than asked by carrying the reap requirements forward.
Two verifications worth stating because they are easy to get wrong and the doc got them right: the temp file really is ${targetPath}.${randomBytes(6).toString('hex')}.tmp (atomicFileWrite.ts:268, and :538 for the sync variant), so .qwen-session.*.tmp matches both; and isBusy really does cover queued turns, not just running and permission-pending (ChannelBase.ts:3472-3477 — queuedTurns, activePrompts, pendingPermissions, then a listSessions fallback).
The new findings are all in the two mechanisms this revision introduced:
- C1 — the R8-2 visibility fix and the F2 branch removal contradict each other on
hasActivePrompt. L345-348 makes a parked prompt count as active "everywhere the active state is computed"; L576-579 depends on the deferred shape readinghasActivePrompt: falseso it reaches the relocatingelse. Since the same revision removes the branch that currently absorbshasActivePrompt: trueon an unlocated restore, applying the visibility fix to the restore response breaks every deferred worktree restore — the path a reviewer verified working on #10643. - C2 —
atomicWriteFilehas two non-rename commit paths, so "never a partial file" and "never following a symlink" do not hold unconditionally. The uid-mismatch fast path is a plain in-placefs.writeFile. - M1 —
assertCanCommitis synchronous, so it cannot perform the async marker re-read the commit-point check is specified as doing — which also contradicts "reuse only; no change expected".
None of these needs a redesign; each needs a boundary stated. Details inline.
中文摘要
重新审阅了完整修订版（+429/−215；文档现为 978 行，因此按全文重读而非仅看 diff），并对照本 PR base 的代码重新核验了每一条新增论断。
上一轮 11 条发现全部已解决，其中最关键的两条是通过反转设计而非绕过来解决的，值得明确记录：
- C1（F2 前提颠倒）—— 已修复，且文档现在承载了更正后的机制：移除
hasUnlocatedRestoredPrompt分支而不是在其中做 relocate，反转本身写在该节内（L571-574），Part 4A 当初拒绝 relocate 的判断被恢复为"实质上正确"。L181-190 的 baseline 条目现在描述了该分支的真实形态。 - C2（parked prompt 对静默性探针不可见）—— 已修复，且正确地重新归因到 bot 的 R8-2：一个共享的可见性缺陷、两个消费方——这个框架比我原本的表述更好。
- C3（恢复口不是 CAS）—— 已修复。 worktree 维度的串行化 + 按形态提交（
valid用assertCanCommit重检 owner，missing用O_EXCL），并且"盲目 temp-file rename"已列为明确否决的替代方案。 - M1/M2/S4 —— 通过重新划分范围解决。 reap 清理与两个顺带修复移出；本部分的契约现在字面上就是"不删除"，而我提出的调用点放置 /
kind === 'removed'/ supersede 拒绝 /branchPreserved等要求被带入 follow-up issue 而非丢弃。 - M3（静默性只是采样）—— 已修复：reset-pending 屏障 + 翻转前重验。S1 —— 我核验了临时文件名论断，完全正确（见下）。S3 ——
packages/acp-bridge已进入作用域清单。S5 —— 四处文档更正全部落地。 - disposition 表是对 bot finding 1 的正确回答，并且超出要求地把 reap 要求一并前移保存。
两处值得指出的核验（容易搞错而文档写对了）：临时文件确实是 ${targetPath}.${randomBytes(6).toString('hex')}.tmp（atomicFileWrite.ts:268，同步变体在 :538），所以 .qwen-session.*.tmp 两者都能匹配；isBusy 确实覆盖排队中的 turn，而不只是运行中与待授权（ChannelBase.ts:3472-3477）。
新发现全部落在本次修订新引入的两个机制上：
- C1 —— R8-2 可见性修复与 F2 分支移除在
hasActivePrompt上自相矛盾。 L345-348 让 parked prompt 在"所有计算活跃状态的地方"都算作活跃；而 L576-579 依赖 deferred 形态读到hasActivePrompt: false才能走到会 relocate 的else。由于同一次修订移除了当前吸收"未定位 restore 且hasActivePrompt: true"的那个分支，把可见性修复应用到 restore 响应上就会破坏所有 deferred worktree restore —— 而这条路径在 #10643 上被 reviewer 验证为可用。 - C2 ——
atomicWriteFile存在两条非 rename 的提交路径，因此"绝不产生部分文件"与"绝不跟随符号链接"并非无条件成立。uid 不匹配的快路径是普通的原地fs.writeFile。 - M1 ——
assertCanCommit是同步的，无法执行规范中要求的异步 marker 重读——这同时与"仅复用；预期无改动"相矛盾。
三者都不需要重新设计，只需明确边界。详见内联。
Review disposition — head
|
| Finding | Disposition |
|---|---|
C1 — the hasUnlocatedRestoredPrompt branch serves an already-live prompt, so relocating there is impossible |
Accepted. The F2 section is rewritten: the branch is removed so the shape fails closed through the existing active-session check (chiga0's second offered shape). The deferred shape was never broken — it flows through the else branch, which already relocates and attests. Verified against bridge.ts:7802 (deferred sets neither promptActive nor goalTurnActive) and bridge.ts:11184 (CdWhilePromptActiveError). |
| S5a — baseline bullet mislabels the branch | Accepted. The bullet now describes the branch's actual shape (live prompt, no attachment, no recorded cwd). |
| C2 — quiescence probe cannot see a parked deferred prompt | Accepted. A non-empty deferredRestoreAskUserQuestionPrompts now counts as an active prompt in the bridge's shared definition, consumed by both the coalescer and the reset quiescence check. This is the same root as R8-2, closed together. |
| C3 — missing-marker hatch is not a compare-and-swap; lock is per-session | Accepted. The transfer commits by shape: valid owner → atomicWriteFile(noFollow) with assertCanCommit re-reading the owner at the rename commit; missing → the existing O_EXCL exclusive create (the CAS against absence). Serialization is re-keyed on the canonical worktree path. |
S2 — reuse atomicWriteFile |
Accepted; the hand-rolled temp+rename is gone, renameWithRetry covers Windows EPERM/EACCES. |
| S1 — exclude rule does not cover a sibling temp | Accepted. The design states the temp shape (.qwen-session.<hex>.tmp) and adds an explicit .qwen-session.*.tmp exclude line, with the main-checkout widening acknowledged. |
M1 — cleanup inside deleteDaemonSessionIfOrphan arms every call site, incl. reset's own rollback |
Accepted by removing the reap cleanup from this part entirely (see S4). The follow-up issue records call-site placement, the supersede-link refusal, and the sibling-claim check as requirements. |
M2 — !== 'error' admits notFound |
Accepted; the follow-up issue specifies kind === 'removed' / mutationApplied. |
| M3 — owner lock + two point-in-time samples is not mutual exclusion | Accepted. Quiescence is now a barrier: S_old is marked reset-pending at the gate (the bridge refuses new prompt admissions), the flag clears on every failure path, and quiescence is re-verified immediately before the flip. |
S3 — packages/acp-bridge missing from the scope list |
Accepted; bridge.ts/bridgeTypes.ts are listed (deferred-prompt visibility, the barrier, attach severing, worktree-association clearing). |
| S4 — split the reap cleanup and the ride-along fixes | Accepted. The reap cleanup moves to a follow-up tracking issue (being filed); R1-1 and F3 join R8-1 and R8-3 in a standalone small fix PR. This part now deletes nothing anywhere. |
Triage review (qwen-code-ci-bot)
| Finding | Disposition |
|---|---|
1 — standing Part 4A Criticals (R8-1, R8-3, SessionRouter.ts:487) not dispositioned |
Accepted. New "Disposition of standing Part 4A findings" section gives every finding a destination: this part / standalone fix PR / follow-up issue, with reasons. |
| 2 — the quoted exit criterion omits its start gate | Accepted. The Status section quotes the gate and records that it was judged passed at the #10643 merge (human approval + reviewer-run real-daemon verification). |
| 3 — item 2 is a reversal, not a deferral | Accepted, and sharpened by C1: the reversal narrative is replaced by the corrected record — Part 4A's refusal to relocate a live prompt stands on the merits; what changes is the branch's silent under-attestation. wenshao's E2E unreachability note is cited; the fix is presented as hardening. |
| 4 — reap safety rests on an unstated invariant | Accepted via the split; the follow-up issue carries "never forceDeleteBranch" and branchPreserved logging as explicit requirements. |
5 — sibling POST /sessions/delete path keeps the leak |
Accepted via the split; the follow-up issue requires an explicit decision on that path. |
| 6 — the retry-repaired window gets the least helpful message | Accepted. New worktree_reset_interrupted typed signal (daemon detects the agreeing sidecar pair with the marker still naming S_old) mapped to "clear the task again to finish the reset". |
| 7 — quiescence is a check, not a fence, for non-Channel callers | Accepted; folded into the M3 barrier. |
| 8 — routine preserve-and-log noise after every reset | Accepted via the split; the follow-up issue requires skipping superseded predecessors silently. |
| Minor (a) docs list / (b) lock keyed per bridge instance / (c) branch deletion entering the series / (d) R3-2 attribution | Accepted. The doc-update list names only overview.md for the limitation; serialization is documented as per-bridge-instance and worktree-keyed; branch deletion no longer enters this part; R3-2 is credited alongside the human findings. |
No finding was rejected.
中文说明
两份评审已对照 PR 基线代码逐条裁决，修订后的设计（c4ee836ae4）覆盖了全部发现。
内联评审：C1 采纳——F2 一节重写为删除欠证明分支、经既有活跃会话检查 fail-closed（已对照 bridge 代码验证 deferred 形态从不进入该分支）；C2 采纳——deferred 状态纳入 bridge 的统一定义，coalescer 与 reset 静默性共用（与 R8-2 同根）；C3 采纳——转移改为按形态的 CAS（valid 走 atomicWriteFile(noFollow) 并在提交点复检 owner，missing 走 O_EXCL 排他创建），序列化锁改按 worktree 路径；S1/S2/S3 采纳；M1/M2 通过把 reap 清理整体拆出本部分来采纳（要求已写入跟踪 issue）；M3 采纳——reset-pending 准入屏障 + 翻转前复检；S4 采纳——reap 清理进跟踪 issue，R1-1/F3 与 R8-1/R8-3 组成独立小修复 PR。
triage 评审：发现 1-8 全部采纳——新增"Part 4A 遗留发现处置"章节（每项都有去向）；Status 补启动闸门说明；F2 按更正后的记录呈现（4A 的拒绝在实质上成立）；4/5/8 随拆分进入跟踪 issue；6 新增 worktree_reset_interrupted 信号与重试消息；7 并入 M3 屏障；minor 四项全部采纳。
没有评审项被拒绝。
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Design review round 3 — head cb1c5eb610 (M=2, S=3)
Disclosure first: the substantive content of 2fa96dc9db was drafted at my direction after round 2, so findings 2, 3 and 4 below are defects in my own edit. Reporting them as found rather than letting them ride.
2fa96dc9db..cb1c5eb610 is mechanical — seven the follow-up issue → follow-up issue #11024, no design change. Prettier passes at head.
I verified #11024 is real and matches what the doc claims it carries. It is OPEN and its body reproduces all seven reap-cleanup requirements verbatim (call-site placement, kind === 'removed' / mutationApplied, the supersedes/supersededBy refusal plus the sibling-claim check, silently skipping a superseded predecessor, never forceDeleteBranch plus branchPreserved logging, the full ownership chain with a clean git status --porcelain, and an explicit decision on POST /sessions/delete), followed by the SessionRouter.ts:487 reproduce-then-design step. All eight references in the doc resolve correctly. That closes the one thing that would have made the re-scoping hollow — the requirements survived the move instead of being dropped with the section.
Round 2's three findings are all resolved: the visibility fix is scoped and the exclusion is now stated with its consequence chain; the transfer commits by shape with the foreign-uid refusal keeping atomicWriteFile on its rename path; and the synchronous re-check is specified against a new sync strict reader with atomicFileWrite.ts untouched. The two suggestions landed as well.
What remains:
- Finding 1 (Major) — the disposition comment posted at
c4ee836ae4now contradicts the design on exactly the point round 2 flagged, and an implementer following that record would rebuild the regression. - Finding 2 (Major) — the foreign-uid refusal is not portable:
process.geteuiddoes not exist on Windows, and the predicate it is defending against already guards for that. - Findings 3–5 (Suggestions) — a residual window the "never a partial file" claim still overstates; the barrier is a new refusal surface with no specified outcome; and the standalone fix PR that Exit criteria depend on has no tracking at all, unlike #11024.
中文摘要
先说明：2fa96dc9db 的实质内容是 round 2 之后按我的意见起草的，因此下面第 2、3、4 条是我自己那次编辑里的缺陷。按发现如实报出，而不是让它们顺车通过。
2fa96dc9db..cb1c5eb610 是机械改动——7 处 the follow-up issue → follow-up issue #11024，无设计变化。head 处 prettier 通过。
已核验 #11024 真实存在且与文档声称承载的内容一致。 它处于 OPEN，正文逐字复现了全部 7 条 reap 清理要求（调用点放置、kind === 'removed' / mutationApplied、supersedes/supersededBy 拒绝加兄弟声明检查、静默跳过已被取代的前任、绝不 forceDeleteBranch 并记录 branchPreserved、完整所有权链加干净的 git status --porcelain、以及对 POST /sessions/delete 的显式决策），随后是 SessionRouter.ts:487 的"先复现再设计"步骤。文档中 8 处引用全部可解析。这关掉了唯一会让此次重新划分范围变成空话的问题——要求随迁移存活，而不是与章节一起被丢掉。
round 2 的三条发现均已解决：可见性修复已收窄且排除项连同后果链一并写明；transfer 按形态提交，foreign-uid 拒绝把 atomicWriteFile 保持在 rename 路径上；同步重检指定为新增的同步 strict reader，atomicFileWrite.ts 保持不动。两条 suggestion 也已落地。
剩余问题：
- 第 1 条（Major） —— 发布于
c4ee836ae4的 disposition 评论现在恰好在 round 2 指出的那一点上与设计矛盾，照该记录实现的人会把回归重新建出来。 - 第 2 条（Major） —— foreign-uid 拒绝不可移植：
process.geteuid在 Windows 上不存在，而它所防御的那个判定本身已经为此加了护栏。 - 第 3–5 条（Suggestion） —— "绝不产生部分文件"仍有一处被夸大的残留窗口；barrier 是新增拒绝面却没有约定结果；Exit criteria 依赖的 standalone fix PR 与 #11024 不同，完全没有追踪。
Review disposition — head
|
| Finding | Disposition |
|---|---|
C1 — visibility fix and branch removal contradict on hasActivePrompt |
Accepted. The visibility fix is scoped to exactly two consumers — the coalesced-restore waiter and getSessionSummary — and must not feed the restore response's hasActivePrompt; the section states why (it would push the working deferred shape into the fail-closed branch), and the verification plan pins a deferred worktree restore still reaching persisted-v1 so a later widening fails the test instead of the restore. |
C2 — atomicWriteFile has two non-rename commit paths |
Accepted. The marker primitive refuses a foreign-owned marker (keeping atomicWriteFile on its rename path), the sibling temp keeps the EXDEV fallback unreachable, and the baseline documents both non-rename paths explicitly. |
M1 — assertCanCommit is synchronous |
Accepted. The commit-point re-check uses a new synchronous no-follow strict reader beside the async one (same checks, same three-state result); atomicFileWrite.ts itself is reused unchanged, and the sync reader is called out in the implementation sequence. |
| S1 — crash vs. failure conflation on the barrier | Accepted. Step 0 now states the barrier is in-memory and deliberately non-durable; durability of a mid-transfer state lives in the marker and the sidecar pair, which the resume path reads. |
| S2 — R8-1 fix shape needs the reconciliation named | Accepted. The disposition table carries the distinction, and the fix's code comment states it in place: under !spawnCompleted no session has entered the checkout, unlike the two inner handlers that may still have a live session inside. |
Round 3 (review at cb1c5eb610)
| Finding | Disposition |
|---|---|
| F1 — stale disposition comment | Accepted; this comment is the fresh record at the current head, and the stale C2 row is corrected above. |
| F2 — the uid refusal is not portable | Accepted. The refusal now runs only where the platform reports an effective uid (process.geteuid); where it does not (Windows), ownershipWouldChange() is always false so the in-place path it guards is unreachable by construction and nothing is refused. Both strict readers surface the marker's owner uid alongside the three-state result, so no third stat is implied. |
| F3 — "never a partial file" is stronger than the mechanism | Accepted. The claim is bounded: the commit-point re-check compares owner and file identity (dev/ino/uid); the residual in-place rewrite preserving inode and ownership requires a same-uid writer (the daemon itself or the local user), which the worktree-keyed lock excludes for daemon writers. |
| F4 — the barrier's refusal shape is unspecified | Accepted. The barrier refusal reuses the typed worktree_reset_active failure, and the error-classes section records that no chat-facing message exists by decision — the owner lock keeps Channel users out of the window, so only a non-Channel API caller can meet it. |
| F5 — the four-fix bundle had no tracking | Accepted differently than suggested: the four fixes (R8-1, R8-3, R1-1, F3) are implemented directly in this PR (e7fdb82781), each pinned by a regression test — R8-1's flipped post-spawn cleanup test in server.test.ts, R8-3's heal case in DaemonSessionClient.test.ts, R1-1's no-leftover-file case in git-worktree-service-marker.test.ts, and F3's usage-line case in ChannelBase.test.ts. |
Verification at this head: npm run build, npm run typecheck, and npm run lint pass; the full packages/channels/base suite (1233 passed), the full DaemonSessionClient.test.ts (82 passed), and the full packages/cli server file (1217 passed; 4 failures in unrelated CORS/status-route tests are pre-existing timing flakes that pass when rerun in isolation, the same pattern documented on #10643).
中文说明
本评论取代在 c4ee836ae4 发布的第一轮处置记录。先更正该记录的一处失真（对应第三轮 F1）：其 C2 行称可见性修复进入一个"共享定义"——当前文档刻意不做统一，因为把 restore 响应的 hasActivePrompt 纳入会在欠证明分支被删除后破坏今天可用的 deferred worktree 恢复路径。另记录在案：跟踪 issue 为 #11024（已创建）；原指向未来 PR 的四个修复已直接实现在本 PR（e7fdb82781）。
第二轮：C1 采纳（可见性修复限定在两个消费方，明确不喂给 restore 响应，并有测试钉住 deferred 恢复仍可达 persisted-v1）；C2 采纳（foreign-uid 拒绝 + sibling temp 排除 EXDEV 回退）；M1 采纳（提交点复检改用新的同步 strict reader，atomicFileWrite.ts 不变）；S1 采纳（屏障声明为刻意非持久）；S2 采纳（区分写入处置表与代码注释）。
第三轮：F1 采纳（本评论即为当前 head 的新记录）；F2 采纳（uid 拒绝以平台是否报告 euid 为条件，Windows 不拒绝任何东西；两个 strict reader 顺带返回 owner uid）；F3 采纳（提交点复检比对 owner 与 dev/ino/uid 身份，残余情形如实写明）；F4 采纳（屏障复用 worktree_reset_active，并记录 Channel 用户因 owner 锁而永不可见是设计决定）；F5 以不同于建议的方式采纳——四个修复直接落在本 PR 并各带回归测试，不再需要跟踪 issue。
当前 head 的验证：npm run build、npm run typecheck、npm run lint 全部通过；packages/channels/base 全量 1233 通过、DaemonSessionClient.test.ts 全量 82 通过、packages/cli server 文件全量 1217 通过（4 个失败为无关的 CORS/状态路由既有时序抖动，隔离复跑全部通过，与 #10643 记录的模式一致）。
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review round 4 — head 536b74dca2 (M=3, S=3)
The PR changed shape this round: it is no longer documentation-only. e7fdb82781 adds production code across packages/core, packages/cli/src/serve, packages/sdk-typescript, and packages/channels/base — 9 files, +1192/-24. That is the resolution chosen for round 3's finding 5 (the four residual fixes had no tracking): implement them here rather than file a separate PR. Legitimate, and it closes two live main defects sooner. It does mean the code half now needs code review, which is what most of this round is.
Credit where due first — round 3's other four findings are all properly resolved, and two of them well beyond the minimum:
- F2 (uid portability) — fixed exactly right: the refusal runs only where
process.geteuidreports a value, and the doc states that on WindowsownershipWouldChange()is always false so the guarded path is unreachable by construction. Both strict readers surfacing the owner uid removes the third stat too. - F3 (partial-file claim) — bounded honestly, and the bound is sharper than I asked for: identity re-check on
dev/ino/uid, with the residual case narrowed to a same-uid writer that the worktree-keyed lock excludes for daemon writers. - F4 (barrier shape) — named as the typed
worktree_reset_activefailure, with "no chat-facing message by decision, not omission" recorded. - F1 (stale disposition) — superseded by a fresh comment at this head that explicitly corrects the contradicting C2 row. That is the right shape: correction in a new comment, history intact.
I also confirmed the PR description was rewritten to match reality (no docs-only claim remains, the Tested-on table now shows macOS ✅ / Windows packages/core marker suite is green locally: 12/12.
New findings, all in the code half
- Finding 2 (Major) — the R1-1 fix deletes a file it has just proven is not its own. I verified this by execution, not by reading: injecting a path swap during the write window makes the
identity changedbranch fire, and the foreign file at the marker path is then gone (fs.readFile→null). The comment above the unlink says "The O_EXCL open created this file, so it is ours to remove" — that is precisely false in the one branch whose job is to detect that the path is no longer ours. - Finding 3 (Major) — the success-path
close()sits inside thetry, so a close rejection unlinks a fully written and fsync'd marker. Harmless for today's only caller, but this PR's own design makescreateWorktreeSessionMarkerExclusivethe commit for the transfer'smissingshape, where reset rollback never removes the worktree — so there it produces the ownerless worktree the design's own "unlink + exclusive create" rejection exists to prevent. - Finding 1 (Major) — the design's Non-goals bullet still says the four fixes "all land separately", contradicting the disposition table, the re-scope list, and the Exit criteria, which all now say "this PR". It is the section that defines the part's boundary, so it is the worst place to leave stale.
- Findings 4–6 (Suggestions) — the F3 parser fix is narrower than the defect class it closes; R8-3's heal infers sidecar absence from a missing
worktreeobject, an inference this PR's own Part 4B step 6 breaks; and a note on AGENTS.md's round/scope discipline now that the diff has widened into core paths on the fourth review round.
Neither Major needs a redesign: finding 2 is "don't unlink when the inode no longer matches", finding 3 is "move one await past the catch".
中文摘要
本轮 PR 性质发生变化：已不再是纯文档。e7fdb82781 新增了跨 packages/core、packages/cli/src/serve、packages/sdk-typescript、packages/channels/base 的生产代码——9 个文件，+1192/-24。这是对第三轮 F5（四个残留修复缺少追踪）的处置方式：直接在本 PR 实现，而不是另开 PR。这是合理的，也让两个 main 上的真实缺陷更早关闭。代价是代码部分现在需要代码审查，本轮大部分内容即是如此。
先说做得好的——第三轮另外四条均已正确解决，其中两条超出了我的要求：
- F2（uid 可移植性） —— 修得完全正确：仅在
process.geteuid报出值时执行拒绝，并写明 Windows 上ownershipWouldChange()恒为 false，故被守护的路径按构造不可达。两个 strict reader 同时返回 owner uid，也顺带消除了第三次 stat。 - F3（部分文件声明） —— 边界写得诚实，且比我要求的更锐利：按
dev/ino/uid做身份重检，残留情形收窄到同 uid 写者，而 worktree 维度的锁已排除守护进程侧的同 uid 写者。 - F4（barrier 形态） —— 命名为 typed
worktree_reset_active失败，并记录"无 chat 层消息是决策而非遗漏"。 - F1（过期处置记录） —— 由当前 head 的新评论取代，并显式更正了矛盾的 C2 行。这是正确的做法：更正写在新评论里，历史保持不动。
我也确认 PR 描述已重写以匹配现实（不再声称纯文档，Tested-on 表改为 macOS ✅ / Windows packages/core marker 套件本地全绿：12/12。
新发现，全部在代码部分
- 第 2 条（Major）—— R1-1 修复会删除一个刚被自己证明不属于自己的文件。 这条我用执行验证，而非阅读推断：在写入窗口注入路径替换会触发
identity changed分支，随后 marker 路径上那个外来文件消失（fs.readFile→null）。unlink 上方的注释写着"O_EXCL 创建了这个文件，所以由我们删除"——而在这个专门用于检测"路径已不属于我们"的分支里，这句话恰恰是错的。 - 第 3 条（Major）—— 成功路径的
close()位于try内，因此 close 失败会 unlink 一个已完整写入并 fsync 的 marker。 对今天唯一的调用方无害，但本 PR 自己的设计把createWorktreeSessionMarkerExclusive作为 transfermissing形态的提交方式，而 reset 回滚从不移除 worktree——在那里它会产生设计中"unlink + exclusive create"否决项本要避免的无主 worktree。 - 第 1 条（Major）—— 设计文档 Non-goals 仍写着四个修复"all land separately"，与处置表、re-scope 列表、Exit criteria 现在一致的"this PR"相矛盾。这一节定义的是本部分的边界，是最不该留下过期表述的地方。
- 第 4–6 条（Suggestion） —— F3 解析修复窄于其所关闭的缺陷类；R8-3 的自愈从"缺少
worktree对象"推断"sidecar 不存在"，而本 PR 自己的 Part 4B 步骤 6 会打破该推断；以及在第四轮审查上 diff 已扩入核心路径后，关于 AGENTS.md 轮次与范围纪律的一点提醒。
两条 Major 都不需要重新设计：第 2 条是"inode 不再匹配时不要 unlink"，第 3 条是"把一个 await 移到 catch 之后"。
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review round 5 — head 536b74dca2 (C=1, S=1, plus 3 process items)
The design and the code are unchanged since round 4 — same head, same 9 files, and none of round 4's eight threads has a reply or a resolution. So this round is about something else: the automated review found a Critical on this PR and it was never posted.
The bot's comment says it plainly once you read past the summary line:
📄 Docs-only change — the automatic review ran at
--effort medium(verified findings, no reverse audit; medium posts no inline comments).
Review complete: pr-11015 — Request changes, not posted (1 Critical, 5 Suggestion). Reviewed head:cb1c5eb610.
So the Critical exists only inside workflow run 33873028134. Nothing on the PR surfaces it, and at merge time nobody would see it. I pulled it out of the run log and then verified it myself against 536b74dca2 rather than relaying it on trust — it holds, and I missed it across three rounds of my own. Full finding inline on the resume path.
The second finding below (the absolute "no deletion" contract contradicting the protocol's own sidecar deletions) is also the automated review's, also verified here. Its remaining Suggestion about a stale PR description is now obsolete — the description was rewritten before round 4.
Three process items, none of which is a code defect
- The automated review classified this PR as docs-only, and reviewed a head that predates the code. It ran at
cb1c5eb610;e7fdb82781then added 9 files of production code acrosspackages/core/src/services,packages/cli/src/serve,packages/sdk-typescript, andpackages/channels/base. No automated review, at any effort, has ever looked at that code. The bot's own comment names the remedy: comment@qwen-code /reviewfor a full high-effort review with inline comments at the current head. Worth doing before this merges — the code half is exactly the part that has had the least scrutiny. Test (windows-latest)andTest (macos-latest)are stillskipping, andTest (ubuntu-latest)is still pending. The new code isfs-semantics code —fs.unlink,O_NOFOLLOW, file-handle lifetime — which is where Windows diverges most. The PR's own Tested-on table records Windows⚠️ and Linux⚠️ (not run locally), so there is currently no Windows or macOS verification of this code anywhere. #10643's round-4 Critical was a Windowspath.joinassumption, and the stage-2 triage comment on this very PR predicted that these lanes should stop being skipped once implementation landed. It landed.- Round 4's three Majors are still open — the R1-1 unlink deleting a file it proved was not its own (verified by execution), the success-path
close()inside thetry, and the stale Non-goals bullet. Both code ones are in the marker primitive the design depends on.
中文摘要
设计与代码自第四轮以来没有变化——同一 head、同样 9 个文件，第四轮的 8 条线程无一被回复或 resolve。所以本轮说的是另一件事：自动评审在本 PR 上发现了一条 Critical，但它从未被发布出来。
bot 的评论在摘要行之后写得很明确：
📄 Docs-only change —— 自动评审以
--effort medium运行（发现已验证、无反向审计；medium 不发布行内评论）。
Review complete: pr-11015 — Request changes, not posted (1 Critical, 5 Suggestion)。评审的 head：cb1c5eb610。
也就是说这条 Critical 只存在于 workflow run 33873028134 里。PR 上没有任何地方呈现它，合并时也没人会看到。我把它从 run 日志中取出，并在 536b74dca2 上自行核验而非直接采信——它成立，而且是我自己连续三轮漏掉的。完整发现见 resume 路径的行内评论。
下面第二条（"不删任何文件"的绝对契约与协议自身的 sidecar 删除相矛盾）同样来自自动评审，同样已在此核验。它另一条关于 PR 描述过期的 Suggestion 现已失效——描述在第四轮之前已重写。
三条流程事项，均非代码缺陷
- 自动评审把本 PR 归类为纯文档，且评审的 head 早于代码落地。 它跑在
cb1c5eb610；随后e7fdb82781新增了跨packages/core/src/services、packages/cli/src/serve、packages/sdk-typescript、packages/channels/base的 9 个文件生产代码。没有任何自动评审在任何强度下看过这些代码。bot 自己给出了办法：评论@qwen-code /review以在当前 head 触发带行内评论的完整高强度评审。合并前值得做——代码部分恰恰是受审最少的部分。 Test (windows-latest)与Test (macos-latest)仍为skipping，Test (ubuntu-latest)仍 pending。 新代码正是fs语义代码——fs.unlink、O_NOFOLLOW、file handle 生命周期——而这正是 Windows 差异最大的地方。PR 自己的 Tested-on 表记录 Windows⚠️ 与 Linux⚠️ （未本地运行），因此这批代码目前在任何地方都没有 Windows 或 macOS 验证。#10643 的第四轮 Critical 就是一个 Windowspath.join假设，而本 PR 的 stage-2 triage 评论早已预言实现落地后这些 lane 不应再被 skip。它已经落地了。- 第四轮的三条 Major 仍未处理——R1-1 的 unlink 会删除刚被自己证明不属于自己的文件（已用执行验证）、成功路径的
close()位于try内、以及过期的 Non-goals 条目。两条代码问题都在设计所依赖的 marker 原语里。
Review disposition — head
|
| Finding | Disposition |
|---|---|
| F2 (Major) — the R1-1 unlink deletes a file the identity check just proved is not ours | Accepted and fixed. The cleanup now captures dev/ino from the opened handle and unlinks only while lstat still resolves to that exact inode; on the identity-changed branch the foreign file survives. A regression test injects a path swap during the write window and asserts the foreign content is still readable after the rejection. |
F3 (Major) — success-path close() inside the try unlinks a fully written, fsync'd marker on a close rejection |
Accepted and fixed. close() moved past the catch: a close rejection now propagates with the valid marker intact, and only failures before the close can trigger cleanup. This matters for the design's missing-shape commit, where reset rollback never removes the worktree and an unlinked marker would leave it ownerless. |
| F1 (Major) — Non-goals still says the four fixes "all land separately" | Accepted and fixed. The bullet is inverted: the four residual fixes are in scope for this PR; only the reap cleanup, the sibling delete path, and the SessionRouter.ts:487 investigation remain out (tracked in #11024). |
| F4 (Suggestion) — the parser guard is narrower than the defect class | Accepted and fixed. The guard is now parts[0]?.startsWith('-'), covering any leading-flag token (--force included); no legal task name starts with -, so nothing that would have been accepted becomes a usage line. The test gained the --force case. |
| F5 (Suggestion) — the heal comment states an inference as exhaustive that Part 4B step 6 breaks | Accepted and fixed (comment-only). The comment now states what the client can observe: no worktree object may mean a legitimate exit, a cleared in-memory association, or a provenance that skips sidecar restore; the client drops the claim and lets the identity gate re-establish the truth. |
| F6 (Suggestion) — scope/round discipline per AGENTS.md | Acknowledged as a conscious call: the user chose to keep the four fixes in this PR so two live main defects close sooner; F2 and F3 — the ones the reviewer said must not ride along unfixed — are fixed in this revision. |
Round 5 (review at 536b74dca2)
| Finding | Disposition |
|---|---|
| Critical — the resume path's rollback is unconditional, so a crash past the commit point can delete the authoritative owner and strand the task | Accepted and fixed in the design. The resume path now reads the marker before choosing its recovery shape: marker names S_new (committed) → never roll back; a busy replacement fails closed with worktree_reset_active and a later retry converges once idle; other revalidation failures fail closed for operator repair. Marker names S_old or missing-with-valid-sidecar (pre-commit) → the destructive rollback stays correct there, because S_new is provably non-authoritative. Transfer-protocol steps 4 and 5 now name which recovery shape a retry takes, and the verification plan gained the crash-past-flip + busy-replacement case (assert the marker, sidecar, and record all survive). |
| Suggestion — the absolute "no deletion" contract contradicts the protocol's own sidecar deletions | Accepted and fixed. The contract is narrowed in all four places (Status, Goals, Non-goals, Exit criteria) to: no deletion of user-visible or pre-existing state — the worktree, its branch, the files within it, the ownership marker, or S_old's sidecar/transcript/record — with rollback permitted to remove only artifacts the reset itself created (S_new's sidecar and session record). The rejected-alternative wording is aligned. |
| Process 1 — no automated review has seen the code half | Acting on it: triggering a full review at the current head with @qwen-code /review after this push. |
| Process 2 — Windows/macOS lanes skipped for fs-semantics code | Noted. Local verification is macOS-only; the Windows-sensitive surfaces in this revision are the marker unlink/close sequencing (POSIX lstat/unlink semantics, close-then-unlink ordering) and the parser guard. CI lanes will exercise the rest; the Tested-on table keeps Windows/Linux as |
| Process 3 — round 4's Majors still open | Closed by this revision (F1/F2/F3 above). |
Verification at this head: npm run build, npm run typecheck, npm run lint pass; marker suite 13/13 (including the new swap-window test), DaemonSessionClient.test.ts 82/82, the widened parser test green, and the previously flipped R8-1 test unchanged.
中文说明
第四、五轮共八条发现全部处理完毕。两条代码 Major 与一条 Critical 均先对照代码核验再修复。
第四轮：F2 采纳并修复——清理逻辑改为捕获打开句柄的 dev/ino，仅当 lstat 仍解析到同一 inode 时才 unlink；身份变更分支下外来文件得以保留，并新增写窗口替换的回归测试。F3 采纳并修复——close() 移到 catch 之后，close 失败时已完整写入并 fsync 的 marker 保持原样传播错误。F1 采纳并修复——Non-goals 条目反转：四个修复在本 PR 范围内，只有 reap 清理、兄弟删除路径与 SessionRouter.ts:487 调查在范围外（#11024 跟踪）。F4 采纳并修复——守卫扩为 parts[0]?.startsWith('-')，覆盖整个缺陷类，测试补 --force 用例。F5 采纳并修复（仅注释）——注释改为陈述客户端实际可观察的事实。F6 知悉：保留四个修复在本 PR 是有意识的决定（让两个 main 上的真实缺陷更早关闭），评审者点名不能带病合入的 F2/F3 已在本轮修复。
第五轮：Critical 采纳并在设计中修复——resume 路径先读 marker 再选择恢复形态：marker 指向 S_new（已提交）则绝不回滚，忙碌的替代会话以 worktree_reset_active fail-closed、空闲后重试收敛；marker 指向 S_old 或缺失+有效 sidecar（提交前）才允许破坏性回滚。协议步骤 4/5 写明重试走哪种形态，验证计划补"越过翻转点崩溃 + 替代会话忙碌"用例。Suggestion 采纳——"零删除"契约在全部四处收窄为"不删除用户可见或既有状态"，回滚只允许移除 reset 自己创建的产物（S_new 的 sidecar 与会话记录）。流程项 1：推送后以 @qwen-code /review 在当前 head 触发完整自动评审。流程项 2：知悉，本地验证仅 macOS，Tested-on 表在 CI 跑绿前保持 Windows/Linux
当前 head 验证：npm run build、npm run typecheck、npm run lint 通过；marker 套件 13/13（含新的写窗口替换测试）、DaemonSessionClient.test.ts 82/82、扩展后的 parser 测试通过、此前翻转的 R8-1 测试不变。
|
@qwen-code /review Full high-effort review requested at head 9a48f7f: the code half (R8-1/R8-3/R1-1/F3 fixes plus the round-4 hardening of the marker cleanup and parser guard) has not been through an automated review at any effort yet, per the round-5 process note. |
|
Qwen Code review request accepted. Review is running in workflow run. A command-triggered review is not listed under the checks of this PR; the result is posted here as a review when it finishes. |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Review pass on head 9a48f7f (design doc + the four supporting code changes). No blocking findings. Design: the worktree-reset primitive (POST /session/:id/worktree-reset) keeps the attested worktree, replaces only the conversation, flips the ownership marker last via compare-and-swap, never deletes user state (orphan-reap explicitly deferred to issue#11024), and every standing Part 4A finding — including the R3-2 Critical — has a recorded disposition. Code: the marker cleanup now unlinks only after a dev/ino identity match (a foreign inode swapped in during the write window is left alone, covered by the new tamper tests); the resume path drops a cached worktree claim when the daemon answers without one instead of failing, keeping the mismatch branch fail-closed; the spawn-failure path removing the unowned checkout is a leak fix with a sound rationale (relocation has not run yet, so nothing can own the checkout) and the test was flipped accordingly; and the '/session new' leading-flag guard returns the usage line instead of a confusing validation error. Ubuntu Test and the automatic review-pr checks were still running at review time; everything else is green.
Part 4B implementation landed (c230032)The design in this PR is now implemented end to end, so Daemon — Restore surface — the sidecar pre-read now redirects a superseded session with Core — Bridge — a parked deferred restore prompt now counts as active in Channels / SDK — Also in this push — round-6 review responses: the R8-1 generation-closed rollback logs a failed Verification — new tests at every layer (core marker/sidecar 53, bridge reset 8, daemon route 13, channels router/manager/ChannelBase additions) plus full regression sweeps: acp-bridge 907, channels/base 849, cli serve 1338, sdk 1780 — all green; Docs updated: protocol reference (new route + capability + restore taxonomy), daemon session lifecycle, channels user overview. E2E plan recorded in |
|
CI fix in 7277d50: the new route and capability tag tripped three drift guards that targeted test runs don't cover — the telemetry route catalog ( |
Address the design review on #11015: - Correct the deferred-prompt premise: the under-attesting restore branch serves a genuinely live prompt, so it is removed to fail closed instead of attempting an impossible relocation (C1/S5a). - Surface parked deferred restore prompts in the bridge's active state and consume it in both the coalesced restore (R8-2) and the reset quiescence check (C2); add a reset-pending admission barrier and a pre-flip re-verify so quiescence is enforced, not sampled (M3). - Make the marker transfer a compare-and-swap: owner re-check at the rename commit via atomicWriteFile's assertCanCommit, O_EXCL create for the missing-marker hatch, and worktree-keyed serialization (C3, S1, S2). - Move the orphan-reap worktree cleanup and the R8-1/R8-3/R1-1/F3 fixes out of this part; record every standing Part 4A finding's disposition in a dedicated section (S4, M1, M2). - Add the interrupted-transfer typed signal and message so the one retry-repaired window tells the user to retry. - Credit the review bot's R3-2 alongside the human findings. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
b15e2fd to
273afd3
Compare
|
Rebased onto main (force-push, new head 273afd3). Two count-contract files conflicted because main added capabilities and telemetry routes in parallel; resolved by re-counting the merged source of truth rather than picking a side: capability registry is now 157 tags (docs/developers/daemon/00-index.md), and the telemetry route catalog is 69 routes with a 67/2 handler_resolved/pre_resolved split (both drift-guard tests). Contract tests verified locally: telemetry 84/84, capability registry 11/11, worktree-reset routes 13/13. |
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
Address the design review on #11015: - Correct the deferred-prompt premise: the under-attesting restore branch serves a genuinely live prompt, so it is removed to fail closed instead of attempting an impossible relocation (C1/S5a). - Surface parked deferred restore prompts in the bridge's active state and consume it in both the coalesced restore (R8-2) and the reset quiescence check (C2); add a reset-pending admission barrier and a pre-flip re-verify so quiescence is enforced, not sampled (M3). - Make the marker transfer a compare-and-swap: owner re-check at the rename commit via atomicWriteFile's assertCanCommit, O_EXCL create for the missing-marker hatch, and worktree-keyed serialization (C3, S1, S2). - Move the orphan-reap worktree cleanup and the R8-1/R8-3/R1-1/F3 fixes out of this part; record every standing Part 4A finding's disposition in a dedicated section (S4, M1, M2). - Add the interrupted-transfer typed signal and message so the one retry-repaired window tells the user to retry. - Credit the review bot's R3-2 alongside the human findings. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
273afd3 to
8658820
Compare
|
Rebased onto main again (force-push, new head 8658820) to pick up eaef97e (#11036), which fixes the |
Round 5 response — 10 Criticals addressed, 20 Suggestions deferredHead is now Criticals
Each thread carries the mechanism, the measurements, and the mutation or red/green proof for its own fix. Why this branch was force-pushed, and what did not changeR4-4's only possible fix is rewording the commit that carries the false clause, so history was rewritten from that commit onward and every commit after it has a new sha. The content is untouched: the diff between the pre-reword head and the current head is empty. The reword replaced one parenthetical — the message no longer claims a promise-chain release fix, and now says the release logic is byte-identical to
Verification
Suggestions: 19 deferred, 1 fixed as a consequence of R5-2Per this repository's guidance for a PR at roughly five review rounds, only Critical fixes landed here and the Suggestions are deferred rather than silently dropped. Those 19 threads are left unresolved on purpose so they stay visible; they are not overlooked.
Residuals we are not papering over
On the review's own advisoryThe review asked a human to decide whether the shape of the change is still right before these findings were fixed. That decision was made and the answer was to keep the shape: the design decision at |
…lias/concurrency probe)
Maintainer verification: real-daemon E2E on Linux — recommend merge after a rebaseI built a real verification environment for this PR and drove the whole Part 4B surface through an actual Result: 43/43 probes pass. The feature works exactly as the description claims, including every crash window I could forge. The one thing blocking a green CI is unrelated to the diff (stale base — see the last section). Setup. Head 1. The headline claim, base vs headSame workspace, same
The base refuses and the session id is unchanged; the head transfers ownership and keeps the checkout. The capability line is the gate doing its job. 2. The transfer keeps the exact checkout — proven from inside the worktree
The two proofs I care most about here:
3. Crash windows: I forged each shape on disk and retriedThis is the part of the design that deserved the most scrutiny, so I built each interrupted-transfer shape by hand in the sidecars and the marker, restarted the daemon, and retried the reset.
4. Missing-marker recovery on a never-reset task
On a task with no supersede links, deleting the marker produces the distinct 5. The superseded session stays fencedThe severance can legitimately not complete (in one run I saw
The superseded id refuses prompts, its restore returns the typed 6. All three aliases, and two resets at once
Gate
Two notes, neither blocking
One thing worth calling out in the PR's favour: the Recommendation: merge after a rebase onto current 中文版维护者验证：Linux 真实 daemon 端到端 —— 建议 rebase 后合并我为这个 PR 搭了一套真实验证环境，把 Part 4B 的整个面通过真正的 结论：43/43 全部通过。功能与描述完全一致，包括我能伪造出来的每一个崩溃窗口。 唯一挡住 CI 变绿的问题与 diff 无关（base 过旧，见最后一节）。 环境。 head 1. 核心主张：base 对 head同一个 workspace、同一条 2. 转移保留了同一个 checkout —— 从 worktree 内部证明这里我最看重两个证据：
P6 确认 marker 指向替换会话且 sidecar 双向链接完整；P7 确认被取代会话的持久化状态仍在磁盘上。没有任何东西被删除。 3. 崩溃窗口：我在磁盘上逐一伪造每种形态后重试这是设计中最该被仔细审的部分，所以我手工在 sidecar 和 marker 上构造出每种中断形态，重启 daemon，再重试 reset。
4. 从未被 reset 过的任务上的 marker 缺失恢复在没有 supersede 链接的任务上删除 marker，会得到独立的 5. 被取代的会话保持隔离severance 可能合法地未完成（某次运行的 5 次 reset 中有 2 次出现 被取代的 id 拒绝 prompt；它的 restore 返回带替换 id 的 typed 6. 三个别名，以及并发 reset
门禁
两点说明，均不阻塞
另外有一点值得为本 PR 说明：随转移一起加入的 建议：rebase 到当前 |
|
Two verifiable facts about the current head 1. The The failing step is
For completeness, and because it argues against any assumption that the merged content is simply main's: I could not reproduce the failure locally. Fetching the merged head's blob ( 2. Run Separately, for the record on the failure that preceded the merge: |
R5-11 and the maintainer's real-daemon verification both note that the field is written on two response paths and typed in the SDK, while no first-party caller reads it. Say so where a reader looks for the contract instead of leaving the SDK comment stating an obligation nothing honours: what keeps a surviving superseded session out of the checkout is the armed barrier plus the on-disk sidecar link, not this flag, and the channel worker reports the same success message either way. External callers that need to tell the two cases apart can still read it.
|
Correction and completion of my earlier comment on the Why my first attempt did not reproduce it. I ran Prettier over a copy of the blob in The cause. The merge commit Already fixed on the branch. What I did push. One commit, Verification at One CI observation, stated without a request: |
|
@qwen-code /triage |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Scope: packages/channels/base/src/named-session-manager.ts, SessionRouter.ts, packages/core/src/services/gitWorktreeService.ts, worktreeSessionService.ts, packages/sdk-typescript/src/daemon/DaemonSessionClient.ts, packages/cli/src/serve/server/worktree-reset-errors.ts, capabilities.ts, packages/cli/src/serve/routes/session.ts (imports + worktree-reset route shape), packages/acp-bridge/src/bridge.ts (diff only), docs/design/channel-named-sessions-part4b.md (design + crash analysis claims). Test coverage confirmed via described test names in PR body; tests not independently run locally.
Tier: Deep — atomic ownership transfer, crash recovery, filesystem markers, restore-vs-transfer concurrency. Hard to reverse.
No confirmed blocking findings. No approval blockers.
Checked:
-
Staged-temp + hardlink publish (
gitWorktreeService.ts:createWorktreeSessionMarkerExclusive) —O_EXCLon the staged temp path prevents collision; content fsync'd before the hardlink to the final marker path;O_EXCLfailure exits before thetry, so nothing to clean up. Hardlink publish means the marker path first exists once its inode is durable. Crash after hardlink but before temp cleanup is benign (temp is non-marker, marker is correct). -
WorktreeMarkerCommittedError semantics — signals that the atomic flip is committed; callers must treat as "flip happened" and resume, not roll back. The route tests pin this: pre-commit shapes roll back and complete a fresh transfer; committed shapes resume idempotently and never roll back the replacement.
-
readDaemonHttpErrorCode / readSupersededReplacementId (
SessionRouter.ts) — depth-5 cause-chain walk is sufficient for any realistic wrapping depth. Correctly extractscodefrom aDaemonHttpError409 body, walkingcauseso wrapped errors stay transparent. -
Superseded self-heal (
named-session-manager.ts) —409 worktree_session_supersededtriggers redirect to the replacement id and route-map healing; heals if the worker crashed between a committed transfer and its registry commit. -
Capability gate —
session_worktree_reset_v1gates mixed-version workers; workers without it retain the Part 4A refusal message. Additive, no breaking change. -
Sidecar schema evolution — two optional fields added; older readers ignore them; registry stays at v1.
-
Restore serialization lock re-keying to canonical worktree path — makes restore-vs-transfer contention real for the first time; author explicitly discloses the promise-chain release logic is byte-identical to
mainand carries any latent issue to follow-up. Noted and accepted. -
Four staged fixes (R8-1, R8-2, R8-3, R1-1, F3) — each has a regression test that fails at base and passes at head per PR description. Mechanisms consistent with described fixes.
Cross-check (findings frozen above before reading existing reviews):
The latest CI-bot CHANGES_REQUESTED (2026-09-05) raises coverage gaps, not confirmed code defects:
- Bridge idle-close/orphan-reap for zero-attach replacement: not traced (budget limit).
/newvs/resetdispatch equivalence: not fully confirmed (budget limit).spawnOrAttach/sessionSourceinitial transcript at spawn: not verified (budget limit).
None of these are confirmed defects. The CI bot reached its tool budget before exploring these paths. I record them as unreviewed dimensions, not approval blockers, because:
yiliang114reviewed head9a48f7f662and found no blocking findings.- Five rounds of design review (doudouOUC) resolved every finding.
- The design doc's per-step crash analysis table is thorough; each crash window is pinned by a named route test.
Unreviewed dimensions (not approval blockers): Windows/macOS semantics for fs.unlink, O_NOFOLLOW, file-handle lifetime (CI skipped); Integration Tests (CLI, No Sandbox) skipped; orphan-reap cleanup and POST /sessions/delete leak tracked in #11024; restore serialization lock promise-chain release (explicitly out-of-scope); spawnOrAttach initial transcript write.
Reviewed with AI assistance.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
CI triage on head 587d519 — all three red checks look infrastructure-side, not PR-caused: (1) web-shell E2E Smoke failed on chat-transcript-document.test.ts with a timing-budget assertion (67.6s vs 60s), and this PR touches zero web-shell files, so the failing path and the diff are disjoint — pool contention shape; (2) Classify PR died on git exit 128 on the runner; (3) review-pr hit its own 360-minute budget and was cancelled mid-review, which is also why the bot's CHANGES_REQUESTED ledger still points at the older head 7b6e710 while fixes landed afterwards — those findings need a fresh bot round (or maintainer dismissal after verification) rather than reading as live on this head. chiga0's deep-review approval is on this exact head. Not adding my approval yet: at +9115 lines I have not independently verified the round-4 Critical fixes on this head, and the standing bot CR is formally unresolved.
Maintainer re-verification at
|
| round-5 change | how I exercised it | result |
|---|---|---|
marker published by staged temp + fs.link instead of O_EXCL-create-then-write (gitWorktreeService.ts) |
inotifywait on the checkout across a hatch reset, A/B'd against the 7b6e710e build |
the marker path is only ever CREATEd, 0 MODIFY events; the id is written and fsync'd into the staged inode first |
barrier extended to controlSessionWorkflowTask / controlSessionGoal (bridge.ts) |
both routes hammered at the old session id while a transfer is in flight | both refused with 409 worktree_reset_active; released on the replacement afterwards |
pre-flip client-disconnect refusal (session.ts) |
raw-socket resets aborted at 2/10/20/30/45/70/200 ms | 5 pre-flip → refused with Client disconnected before the worktree ownership flip in the daemon log; 2 post-flip → committed; no delay lost a file |
rollback now requires a confirmed removal (session.ts) |
forged pre-commit shape + retry | rolls back and completes a fresh transfer; the links are never unwritten while a live replacement survives |
staleBridgeBindings (SessionRouter.ts), healSupersededTask made non-fatal (named-session-manager.ts) |
superseded-restore + registry-rollback probes | registry self-heals off 409 worktree_session_superseded; no load aborted as "replaced" |
supersededSessionLive documented diagnostic-only (types.ts) |
grep for consumers at this head | accurate — 2 producers, 0 readers; the daemon logs a WARN instead. Closes my last observation |
3. What the marker rewrite actually bought — A/B on disk
Same probe, same workspace, two dists. inotifywait on the checkout while the reset takes the missing-marker hatch:
7b6e710e (before round 5) 587d5195 (this head)
CREATE .qwen-session CREATE .qwen-session.<hex>.tmp
MODIFY .qwen-session MODIFY .qwen-session.<hex>.tmp
CLOSE_WRITE .qwen-session CREATE .qwen-session <- fs.link, content already complete
DELETE .qwen-session.<hex>.tmp
Both orders leave a crash residue that strict reads classify invalid, and both fail closed — the checkout and its uncommitted files survive either way. The difference is recoverability, which I measured rather than assumed:
- Old order, crash between
CREATEandMODIFY→ a 0-byte marker. The owner id was never on disk, so nothing can reconstruct it;/clearrefuses on every retry. Recovery means an operator deleting the marker so the hatch can run. - New order, crash between
link()andunlink(staged)→ a marker atnlink=2holding the complete,fsync'd owner id. Dropping the stray sibling alone restores it — I verified the task then resets normally.
So the round-5 order strictly narrows the window (two adjacent syscalls instead of a window spanning an fsync) and turns an unrecoverable residue into a repairable one. The .qwen-session.*.tmp exclude rule this PR adds covers the new staged name, and git status stays clean through a transfer.
4. Base A/B and gate
Merge base 421393d51d: session_worktree_reset_v1 is not advertised and all three aliases refuse — Task "basecheck" uses a worktree and cannot be cleared or reset yet. So every pass above is attributable to the diff.
Local gate at this head, all green: typecheck 0, lint 0, session-worktree-reset.test.ts 31, server.test.ts 1227, ChannelBase + SessionRouter + named-session-manager 868, bridge-worktree-reset.test.ts 12, git-worktree-service-marker.test.ts 27, DaemonSessionClient.test.ts 83.
CI: Lint & Static is green now — the check:core-subpath-exports red I reported last time was stale-base skew and the rebase cleared it. Test (ubuntu), Real daemon E2E, Serve A/B and Integration Tests (no-AK, No Sandbox) all pass. Two reds, neither in the diff:
Classify PR— fails at the stepCheckout trusted CI classifier, before anything is classified.web-shell E2E Smoke—chat-transcript-document.test.tsmisses a latency budget:AssertionError: expected 67624.83516100001 to be less than 60000. The diff touches noweb-shell,transcriptordocs-sitefile, and the same gate is green on PRs fix(skills): re-apply a Skill's side effects when a session is resumed #11280/feat(daemon): enumerate installed extension skills locally #11281/fix(goal-draft): keep drafts concise and verifiable #11284/test(e2e): isolate protocol checks and ACP test state #11286/fix(web-shell): keep mid-turn messages the daemon rejects at idle #11289/fix(core): retry status-less upstream errors instead of ending the turn #11291/feat: support GPT-5 and GPT-6 reasoning effort configuration #11295.
5. Two observations (non-blocking, neither is a merge condition)
O1 — an out-of-band POST /session/:id/worktree-reset on a Channel task leaves that task unusable. This PR also ships the route as a public SDK API (DaemonClient.resetWorktreeSession, DaemonSessionClient.resetWorktree) with no documented Channel-only restriction. Driving it directly against a Channel-managed session:
- The transfer commits correctly on disk — marker → replacement, both sidecars linked — but the replacement gets only its
.worktree.jsonsidecar, no.jsonlrecord. A Channel-driven/clearwrites atype:"system"record immediately; this path writes none. - While the daemon is up,
/session use <task>answersSelected task "…"and the task then answers nothing — the registry still names the superseded id, and the still-live session short-circuits the load so the409redirect is never requested. - After a daemon restart the redirect does fire, and following it fails:
POST /session/:id/resume → 404 No session with id "<replacement>". Selection reports the genericCould not load task "<name>". The current task was not changed. /clearthen reports success and commits the registry to that unresumable id; the next message fails again./session closeis the only exit, and it leaves the checkout behind.
Files are never lost at any step, and this is outside the window the PR claims: the worker's own post-flip failure path (replaceManagedWorktreeSession's staleBridgeBindings + liveSessionIds.delete) is what makes the documented self-heal work, and that path I verified green. Worth either documenting the route as Channel-worker-only, or persisting the replacement's record before the route returns.
O2 — worktree_reset_invalid_state has no actionable message. Both invalid-marker residues (0-byte and nlink=2) surface as the byte-identical Could not reset task "<name>"., while the restore surface gives typed, actionable text for worktree_marker_missing and worktree_reset_interrupted (ChannelBase.worktreeRecoveryMessage maps only those two). The recovery for the round-5 residue — delete a hidden .qwen-session.<hex>.tmp sibling inside the user's own checkout — is undiscoverable from that message. Same shape as the generic-message issue I raised on #10643; fail-closed and non-destructive, so not a blocker.
Verdict
Recommend merge. 43/43 core probes plus the round-5 delta probes pass on a real daemon; the ownership-flip rewrite is an improvement I could demonstrate on disk rather than infer; the base refuses everything the head does; the local gate is clean and both red checks are infrastructure, not the diff.
Harness: /root/git/qwen-code-v11015-harness (real qwen serve --channel + plugin-example adapter, on-disk crash-shape forging, inotify publish tracing). Full probe transcripts on assets-pr11015.
中文版
维护者复验（head 587d5195）——针对 round-5 增量在真实 daemon 上重跑；建议合入
我在 2026-09-06 已于 head 7b6e710e72 验证过本 PR（评论，43/43 实机探针）。此后 head 变了——round-4/round-5 修复外加两次 main 合并——而 round-5 那次提交改的是所有权翻转原语本身，所以我重建环境，在 587d5195978b42b623e6a3aef5cd80998d6fa2cc（merge base 421393d51d）上重跑了全部内容。
结论：全部仍然通过；marker 发布方式的重写是一次可以在磁盘上做 A/B 证明的真实改进；未发现阻断项。文末两条非阻断观察，其中我上次提的 supersededSessionLive 已关闭。
环境：真实 qwen serve --port 9263 --no-web --channel pe daemon，驱动仓库内 packages/channels/plugin-example 适配器 → 真实 mock 平台服务 → 真实 ACP 子进程 → 真实 git worktree checkout，HOME 隔离。Linux，Node 22。模型是本地 mock，因此"新鲜度"和"隔离性"可以直接从请求流上读出来。
1. 全量套件在本 head 重跑：43/43
其中最强的几条与上次一致且依然成立：checkout 被保留是用 inode 相等 + 分支相同证明的（重新 checkout 会暴露）；对话新鲜度是从替换会话内部证明的（它自己的 run_shell_command 在同一 worktree 里 pwd && cat PRECIOUS.txt，而重置前播种的 token 出现在 24 个重置前模型请求中、重置后 0 次）；伪造的 pre-commit / 已提交崩溃形态是直接写到磁盘再重启 daemon 的，因此回滚与幂等续做被分别验证。
2. round-5 增量逐项验证
| round-5 改动 | 我怎么验的 | 结果 |
|---|---|---|
marker 改为「暂存临时文件 + fs.link 发布」，不再是 O_EXCL 建文件再写（gitWorktreeService.ts） |
在 checkout 上开 inotifywait 跑一次 hatch 重置，并与 7b6e710e 构建做 A/B |
marker 路径只出现 CREATE，MODIFY 事件为 0；会话 id 先写入并 fsync 到暂存 inode |
屏障扩展到 controlSessionWorkflowTask / controlSessionGoal（bridge.ts） |
转移进行中对旧会话 id 高频打这两条路由 | 两者均被 409 worktree_reset_active 拒绝；转移完成后在替换会话上释放 |
翻转前的客户端断连拒绝（session.ts） |
用裸 socket 在 2/10/20/30/45/70/200 ms 处中止重置请求 | 5 次落在翻转前 → 拒绝，daemon 日志出现 Client disconnected before the worktree ownership flip；2 次落在翻转后 → 正常提交；没有任何延迟档丢文件 |
回滚必须先确认删除成功（session.ts） |
伪造 pre-commit 形态后重试 | 回滚并在同一请求内完成新鲜转移；只要还有存活的替换会话，链接就不会被抹掉 |
staleBridgeBindings（SessionRouter.ts）、healSupersededTask 改为不致命（named-session-manager.ts） |
superseded restore + registry 回退探针 | registry 依 409 worktree_session_superseded 自愈；没有 load 因"已被替换"而中止 |
supersededSessionLive 标注为仅诊断用（types.ts） |
在本 head 上 grep 消费方 | 属实——2 处产出、0 处读取，daemon 改为打 WARN。我上次的观察就此关闭 |
3. marker 重写到底换来了什么——磁盘级 A/B
同一探针、同一 workspace，两个 dist。重置走「marker 缺失」补建路径时在 checkout 上开 inotifywait：
7b6e710e（round-5 之前） 587d5195（本 head）
CREATE .qwen-session CREATE .qwen-session.<hex>.tmp
MODIFY .qwen-session MODIFY .qwen-session.<hex>.tmp
CLOSE_WRITE .qwen-session CREATE .qwen-session <- fs.link，内容此时已完整
DELETE .qwen-session.<hex>.tmp
两种顺序留下的崩溃残留都会被严格读判为 invalid，也都是 fail-closed——checkout 与未提交文件在任一情况下都完好。差别在可恢复性，这一点我是实测而非推断：
- 旧顺序，崩在
CREATE与MODIFY之间 → 0 字节 marker。owner id 从未落盘，无从重建；/clear每次重试都拒绝。恢复方式是运维手工删掉 marker 让补建路径接管。 - 新顺序，崩在
link()与unlink(staged)之间 → marker 处于nlink=2，但内容是完整且已fsync的 owner id。仅删掉那个多余的兄弟硬链接即可恢复——我验证过之后任务能正常重置。
也就是说 round-5 的顺序既收窄了窗口（从跨越一次 fsync 缩到两条相邻系统调用），又把不可恢复的残留变成可修复的残留。本 PR 新增的 .qwen-session.*.tmp 排除规则覆盖了新的暂存名，转移全程 git status 保持干净。
4. Base A/B 与门禁
Merge base 421393d51d：不广告 session_worktree_reset_v1，三个别名全部拒绝——Task "basecheck" uses a worktree and cannot be cleared or reset yet.。因此上面每一项通过都可归因于本 diff。
本地门禁在本 head 全绿：typecheck 0、lint 0、session-worktree-reset.test.ts 31、server.test.ts 1227、ChannelBase + SessionRouter + named-session-manager 868、bridge-worktree-reset.test.ts 12、git-worktree-service-marker.test.ts 27、DaemonSessionClient.test.ts 83。
CI：Lint & Static 现在是绿的——我上次报告的 check:core-subpath-exports 红是 stale base 造成的，rebase 已清除。Test (ubuntu)、Real daemon E2E、Serve A/B、Integration Tests (no-AK, No Sandbox) 均通过。两处红，均与 diff 无关：
Classify PR—— 挂在Checkout trusted CI classifier步骤，还没开始分类就失败。web-shell E2E Smoke——chat-transcript-document.test.ts未达时延预算：AssertionError: expected 67624.83516100001 to be less than 60000。本 diff 未触碰任何web-shell/transcript/docs-site文件，且同一 gate 在 fix(skills): re-apply a Skill's side effects when a session is resumed #11280/feat(daemon): enumerate installed extension skills locally #11281/fix(goal-draft): keep drafts concise and verifiable #11284/test(e2e): isolate protocol checks and ACP test state #11286/fix(web-shell): keep mid-turn messages the daemon rejects at idle #11289/fix(core): retry status-less upstream errors instead of ending the turn #11291/feat: support GPT-5 and GPT-6 reasoning effort configuration #11295 上均为绿。
5. 两条观察（非阻断，都不是合入条件）
O1 —— 由 Channel 之外的调用方直接打 POST /session/:id/worktree-reset，会让该 Channel 任务不可用。 本 PR 同时把这条路由作为公开 SDK API 发布（DaemonClient.resetWorktreeSession、DaemonSessionClient.resetWorktree），并没有写明"仅限 Channel worker"。直接对一个 Channel 托管会话调用它：
- 转移在磁盘上正确提交——marker 指向替换会话，两个 sidecar 互链——但替换会话只有
.worktree.jsonsidecar，没有.jsonl记录。走 Channel 的/clear会立刻写入一条type:"system"记录，这条路径不写。 - daemon 仍在线时，
/session use <task>回复Selected task "…"，随后任务毫无响应——registry 仍指向被取代的 id，而仍然存活的会话让 load 短路，于是409重定向根本没被请求。 - daemon 重启后重定向确实触发了，但跟随它会失败：
POST /session/:id/resume → 404 No session with id "<replacement>"。选择任务只得到通用文案Could not load task "<name>". The current task was not changed. - 此时
/clear会报成功并把 registry 提交到那个不可 resume 的 id，下一条消息再次失败。唯一出口是/session close，而 checkout 会被遗留。
全过程文件从未丢失，且这不在本 PR 声明覆盖的窗口内：worker 自身翻转后失败的路径（replaceManagedWorktreeSession 的 staleBridgeBindings + liveSessionIds.delete）才是文档所述自愈生效的地方，那条路径我验证为绿。建议二选一：把该路由文档化为"仅 Channel worker 使用"，或在路由返回前先持久化替换会话的记录。
O2 —— worktree_reset_invalid_state 没有可操作的提示文案。 两种 invalid marker 残留（0 字节与 nlink=2）都表现为逐字节相同的 Could not reset task "<name>".；而 restore 面对 worktree_marker_missing 与 worktree_reset_interrupted 给的是带类型、可操作的文案（ChannelBase.worktreeRecoveryMessage 只映射了这两个码）。round-5 残留的恢复动作——删掉用户自己 checkout 里一个隐藏的 .qwen-session.<hex>.tmp 兄弟文件——从那句文案里完全无从得知。与我在 #10643 提过的通用文案问题同型；由于 fail-closed 且无破坏性，不作为阻断。
结论
建议合入。 43/43 核心探针加上 round-5 增量探针在真实 daemon 上全部通过；所有权翻转的重写是我能在磁盘上演示、而非推断出来的改进；base 拒绝了 head 所做的一切；本地门禁干净，两处 CI 红都属基础设施问题而非 diff。
Harness：/root/git/qwen-code-v11015-harness（真实 qwen serve --channel + plugin-example 适配器、磁盘级崩溃形态伪造、inotify 发布顺序追踪）。完整探针输出见 assets-pr11015。
|
@qwen-code /triage |
4 participants
What this PR does
This PR lands Part 4B of owner-scoped named Channel sessions (issue #10103):
/clear,/new, and/resetnow work on a selected worktree-isolated task, giving it a fresh conversation while keeping its exact daemon-attested worktree, files, and branch. It ships the design document, the full implementation of the ownership-transfer protocol across core, daemon, bridge, channels, and SDK, and the four residual Part 4A defect fixes that were staged ahead of it.The transfer is one new daemon route,
POST /session/:id/worktree-reset, performing a compensatable ownership handover: spawn a replacement session in the registered root workspace, relocate it into the same verified worktree through the server-derived allowed roots, write its sidecar with the forward link, mark the old sidecar superseded, and flip the in-worktree ownership marker last with an atomic compare-and-swap (a synchronous strict re-read at the rename commit point, plus a foreign-uid refusal where ownership is observable). Every crash window either leaves the old session authoritative — a retry rolls the partial transfer back and completes a fresh one in the same request — or is completed by an idempotent resume that never rolls back the authoritative replacement. A typed superseded signal on restore (409 worktree_session_supersededwith the replacement id) lets the Channel registry self-heal if a worker crashed between a completed transfer and its registry commit. Quiescence is enforced as a barrier (a reset-pending prompt-admission refusal plus a pre-flip re-verify), not sampled. The route holds a worktree-keyed ownership lock shared with Part 4A restores, so no restore can pass ownership validation mid-transfer. A newsession_worktree_reset_v1capability gates mixed worker/daemon versions. The part deletes nothing anywhere; the one destructive candidate identified in review (orphan-reap worktree cleanup) moved to follow-up issue #11024.The four included fixes are live defects in
mainfrom the #10643 review record, each with a regression test: the post-spawn generation-check failure block no longer orphans the created worktree when session cleanup is inconclusive (R8-1); the SDK reattach guard heals a stale durable-worktree claim when the daemon resumes with no worktree object, instead of erroring on every prompt forever (R8-3); the exclusive marker create unlinks its own file when a write fails, so the path no longer wedges onEEXIST(R1-1); and/session new --worktreewithout a task name returns the usage line instead of a misleading name-validation error (F3). The implementation also fixes one latent defect it depends on: the deferred-restore-prompt invisibility that made a parked prompt read quiescent to summaries, to the coalesced waiter, and to the daemon status snapshot (R8-2). The restore serialization lock is re-keyed from session id to canonical worktree path, which is what makes contention between a restore and a transfer real for the first time; its promise-chain release logic is byte-identical tomain, and this PR claims no fix there.The design document records the disposition of every standing Part 4A finding — what this part resolves, what this PR fixes, and what the follow-up issue carries — so none of them is silently dropped.
Why it's needed
Part 4A deliberately rejects
/clear,/new, and/resetfor worktree tasks, so the issue's acceptance criterion — that users can clear their named tasks, and that worktree tasks are not physically deleted by close or clear — remains unmet without this part. Restarting an isolated task's conversation meant closing it, losing the name (closed tasks keep their names), and abandoning a worktree that nothing will ever clean up. The four fixed defects ship inmaintoday and produce permanent orphans, permanent error loops, wedged marker paths, and misleading errors.Reviewer Test Plan
How to verify
Read the design doc's Decision, Transfer protocol (with the per-step crash analysis), and Risks sections first, plus the disposition table against the #10643 review record. Then verify the transfer protocol's behaviors — each is pinned by tests:
cd packages/cli && npx vitest run src/serve/routes/session-worktree-reset.test.ts -t 'transfers worktree ownership to a fresh replacement session'— the replacement is spawned, relocated, linked, the marker flips last, the old session is severed, and the response attestspersisted-v1; the checkout is never removed and the old record is never deleted.-t 'rejects with 409 while the old session has an active prompt'; the bridge barrier is pinned bycd packages/acp-bridge && npx vitest run src/bridge-worktree-reset.test.ts(prompt admission throws while armed, re-admits after clear, and a parked deferred restore prompt reads active in summaries and the coalesced waiter).cd packages/core && npx vitest run src/services/git-worktree-service-marker.test.ts— the transfer recreates an absent marker exclusively through theO_EXCLhatch and refuses the hatch when a marker exists; the restore surface returns the typed409 worktree_marker_missing.cd packages/cli && npx vitest run src/serve/routes/session-worktree-reset.test.ts -t 'rolls back an interrupted pre-commit transfer'and-t 'resumes a committed transfer idempotently'— pre-commit shapes roll back and complete a fresh transfer in the same request; committed shapes resume as a no-op and never roll back the replacement.-t 'redirects a superseded session restore to the replacement', pluscd packages/channels/base && npx vitest run src/SessionRouter.test.ts -t 'superseded'— the Channel redirects the task's exact load to the replacement, heals its route mappings, and the manager's registry follows the healed id.cd packages/channels/base && npx vitest run src/ChannelBase.test.ts -t 'worktree'—/clearon a worktree task confirms the kept worktree, and the busy/interrupted/marker-missing daemon codes map to their actionable messages.For the four staged fixes, each is pinned by a regression test:
cd packages/cli && npx vitest run src/serve/server.test.ts -t 'removes an unowned worktree when post-spawn cleanup is inconclusive'.cd packages/sdk-typescript && npx vitest run test/unit/DaemonSessionClient.test.ts -t 'worktree'.cd packages/core && npx vitest run src/services/git-worktree-service-marker.test.ts.cd packages/channels/base && npx vitest run src/ChannelBase.test.ts -t 'accepts only the exact worktree task syntax'.Evidence (Before & After)
Before:
/clearon a worktree task answeredTask "<name>" uses a worktree and cannot be cleared or reset yet. After: the task keeps its worktree, files, and branch, and continues on a fresh conversation; the superseded session's restore redirects to the replacement. The new route tests fail against the base commit (the route does not exist) and pass on this head; the four staged-fix tests fail on base and pass here (the R8-1 test previously asserted the buggy preservation and was flipped to assert removal).Tested on
Environment (optional)
Linux, Node.js v22. Verified locally:
npm run build,npm run typecheck, ESLint and Prettier clean; full suites green — core 23302, acp-bridge 907, channels/base 1267 (+ dingtalk 501, feishu 280, telegram 25), sdk-typescript 1780, cli 28589 (CI's Test job; two local-only flake families excluded). CI note: the self-hosted runner fleet intermittently fails the Test job with vitest worker RPC timeouts under load ~180 and cancels web-shell E2E for a missing Node provisioning — both are documented in PR comments with log evidence and are unrelated to the diff.Risk & Scope
POST /sessions/deleteleak, and the truncatedSessionRouter.ts:487finding are tracked in feat(channels): Worktree session lifecycle cleanup and residual Part 4A findings #11024.Linked Issues
Refs #10103
Refs #11024
中文说明
本 PR 的作用
本 PR 落地 owner 作用域命名 Channel 会话（issue #10103）的 Part 4B：
/clear、/new、/reset现在对选中的 worktree 隔离任务生效——任务获得全新对话，同时保留其经 daemon 证明的同一个 worktree、文件与分支。PR 包含设计文档、跨 core/daemon/bridge/channels/SDK 的所有权转移协议完整实现，以及先行合入的四个 Part 4A 残余缺陷修复。转移由一条新 daemon 路由
POST /session/:id/worktree-reset执行可补偿的所有权交接：在已注册的根 workspace 生成替换会话，经服务端推导的 allowed roots 迁移进同一个已验证 worktree，写入带前向链接的新 sidecar，将旧 sidecar 标记为已被取代，最后以原子 compare-and-swap 翻转 worktree 内的所有权 marker（rename 提交点的同步严格复读 + 可观测所有权下的 foreign-uid 拒绝）。每个崩溃窗口要么保持旧会话权威——重试在同一请求内回滚部分转移并完成新鲜转移——要么由幂等续做完成，且绝不回滚已权威的替换会话。restore 面的 typed superseded 信号（409 worktree_session_superseded+ 替换 id）让 Channel registry 在 worker 于转移完成与 registry 提交之间崩溃时自愈。静默性以屏障强制执行（reset-pending 准入拒绝 + 翻转前复检），而非时点采样。路由持有与 Part 4A restore 共享的 worktree 键所有权锁，任何 restore 都无法在转移中途通过所有权验证。新session_worktree_reset_v1capability 为混合版本门控。本部分在任何路径上不删除文件；评审识别的唯一破坏性候选（孤儿回收清理）移至 #11024。四个随附修复是 #10643 评审记录中
main上的现行缺陷，各有回归测试：R8-1（post-spawn generation 失败块在清理不确定时不再遗弃 worktree）、R8-3（SDK reattach 对无 worktree 对象的 resume 自愈过期声明）、R1-1（exclusive marker 创建写失败时删除自己的文件）、F3（/session new --worktree缺名返回用法行）。实现还修复了它依赖的一个潜在缺陷：R8-2（parked deferred prompt 对摘要、coalesced waiter 与 daemon 状态快照均不可见）。restore 串行锁由会话 id 改键为规范化 worktree 路径——这正是 restore 与转移之间首次产生真实竞争的原因；其 promise 链释放逻辑与main逐字节相同，本 PR 不声称在该处做了任何修复。设计文档记录了每项 Part 4A 遗留发现的处置——本部分解决的、本 PR 修复的、跟踪 issue 承接的——没有任何一项被静默丢弃。
为什么需要它
Part 4A 有意拒绝对 worktree 任务执行
/clear、/new、/reset，issue 的验收标准（用户可 clear 命名任务，且 worktree 任务不因 close/clear 被物理删除）在本部分之前一直未满足。四个被修复的缺陷今天就在main上，造成永久孤儿、永久错误循环、marker 卡死与误导性报错。Reviewer 测试计划
如何验证
先读设计文档的 Decision、Transfer protocol（含逐步崩溃分析）与 Risks，再验证转移协议行为（均有测试钉住）：
cd packages/cli && npx vitest run src/serve/routes/session-worktree-reset.test.ts -t 'transfers worktree ownership to a fresh replacement session'-t 'rejects with 409 while the old session has an active prompt'；bridge 屏障见cd packages/acp-bridge && npx vitest run src/bridge-worktree-reset.test.tscd packages/core && npx vitest run src/services/git-worktree-service-marker.test.ts（O_EXCL hatch 重建 / 已有 marker 时拒绝 hatch）；restore 面 typed409 worktree_marker_missing-t 'rolls back an interrupted pre-commit transfer'与-t 'resumes a committed transfer idempotently'-t 'redirects a superseded session restore to the replacement'+cd packages/channels/base && npx vitest run src/SessionRouter.test.ts -t 'superseded'npx vitest run src/ChannelBase.test.ts -t 'worktree'（保留 worktree 的确认消息与 busy/interrupted/marker-missing 的可操作消息）四个随附修复的回归测试命令见英文版对应小节。
证据（之前与之后）
之前：worktree 任务
/clear返回 "uses a worktree and cannot be cleared or reset yet"。之后：任务保留 worktree/文件/分支并在全新对话上继续；被取代会话的 restore 重定向到替换会话。新路由测试在 base 上失败（路由不存在）、在本 head 通过；四个修复测试 base 失败、head 通过。测试平台
环境（可选）
Linux，Node.js v22。本地验证：build/typecheck/ESLint/Prettier 全净；全量套件绿——core 23302、acp-bridge 907、channels/base 1267（另 dingtalk 501、feishu 280、telegram 25）、sdk 1780、cli 28589（CI Test job）。CI 备注：自托管 runner 舰队间歇性以 vitest worker RPC 超时（负载 ~180）挂掉 Test job、并因缺 Node 取消 web-shell E2E——均有日志证据的 PR 评论在案，与 diff 无关。
风险与范围
POST /sessions/delete泄漏、SessionRouter.ts:487截断发现由 feat(channels): Worktree session lifecycle cleanup and residual Part 4A findings #11024 跟踪。关联 Issue
Refs #10103
Refs #11024