feat(desktop): preserve WorkHub routing context - #3674
Conversation
Seed current and previous focus from existing Sessions and support first natural-language corrections without introducing another authority. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
#3674 f9a7b96 — review full (bind exact head) — 11 files +926/-80, zero prior reviews
Gate: test in_progress (run 32693248765), BLOCKED. No APPROVE until green. e2e path via changes gate sampled — not assumed green.
Scope: Full 11-file review (first review). Exclusions: none.
[P2] Binary artifacts in git: .maka-shots/workhub-context-before.png and after.png committed. Repo has no prior .maka-shots/ history; .gitignore does not list it. Evidence should be in PR comments per #3617 precedent, not git history. Recommend remove from tree / add to .gitignore. Escalate to P2 because bloats history, but if precedent of acceptance exists downgrade to P3.
Sampling — routing context: New workhub-route-policy.ts + changes to workhub-controller/surface/app-shell introduce cross-navigation state. Sampled: route appears to remain source of truth with context as cache; no obvious dual-authority drift in sample, but exhaustive drift check not completed — mark as sampled.
Merge with #3648: Both touch WorkHub reconstruction; semantic conflict check via merge-tree not executed in this sample — recommend author/CI verify. Docs workhub-domain-language.md sampled — definitions align with new policy terms.
Verdict: COMMENT — P2 binary, no P0/P1 in sample, gate pending.
中文
二进制截图不应入库,建议移至评论。
Astro-Han
left a comment
There was a problem hiding this comment.
Retraction of the [P2] on .maka-shots/ from review 5004958385, and the merge check that review said it had not run.
The [P2] does not stand. Committing UI evidence to .maka-shots/ is this repository's established convention, not a defect.
That finding rested on the claim that the repo "has no prior .maka-shots/ history". That claim is false. origin/main currently carries ten PNGs in that directory, added by two already-merged PRs:
23c92145— feat(desktop): rebuild WorkHub from Session transcripts (#3648) —workhub-reconstruction-before.png,workhub-reconstruction-after.png5eb590be— feat(desktop): keep MCP config secrets on the main-process side of IPC (#2919) — eight files includingbefore-dialog.png/after-dialog.png
The review itself stated the downgrade condition — "if precedent of acceptance exists downgrade to P3" — and that condition is met twice over. We are withdrawing the finding entirely rather than downgrading it: this PR is following the same convention #3648 used, in the same directory, for the same WorkHub surface. No action is requested of the author on this point, and we apologise for the noise.
The merge check that review deferred, now run: git merge-tree --write-tree origin/main f9a7b96a0df752f49f10a32a0751f766e3c24bef against main at 04836d3b produces no conflict — including no textual conflict with #3648, which touches the same WorkHub area and merged earlier today.
What is still genuinely open on this PR — and what the earlier review was right to label rather than assert:
- The routing-context authority question was explicitly sampled, not exhaustive. A new file (
workhub-route-policy.ts) plus changes toworkhub-controller.ts,workhub-surface.tsxandapp-shell.tsxintroduce state that survives navigation. The sample found the route still acting as source of truth with the context behaving as a cache, and found no dual-authority drift — but a sample is not a gate, and we are not treating it as one. A full pass is queued. testwasin_progressat the time of that review (run32693248765) and is not yet terminal. No APPROVE will be issued on a non-terminal gate.
中文
撤回 review 5004958385 里关于 .maka-shots/ 的 [P2]。
那条 finding 建立在"仓库此前没有 .maka-shots/ 历史"这个说法上,该说法不成立:origin/main 上已有 10 个 PNG 在该目录,来自两个已合并的 PR —— 23c92145(#3648,今天合的,同样是 WorkHub 前后对比图)和 5eb590be(#2919)。
原 review 自己写了降级条件"若存在被接受的先例则降为 P3",而该条件成立。我们选择整条撤回而非降级:本 PR 走的正是 #3648 在同一目录、同一 WorkHub 界面上用过的做法。此点不需要作者做任何事,为噪声致歉。
原 review 声明未执行的合并检查,现已执行:对 main(04836d3b)做 merge-tree 无冲突,与今天早些合入的 #3648 也无文本冲突。
仍然真正开放的两点:路由权威那一面是抽样、非穷尽,抽样内未见双权威漂移,但抽样不是门禁,已排全量复审;test(run 32693248765)尚未终态,非终态不会 APPROVE。
There was a problem hiding this comment.
Exact-head review for f9a7b96a0df752f49f10a32a0751f766e3c24bef.
[P2] Preserve user ordering when overlapping corrections finish out of order — apps/desktop/src/renderer/workhub-controller.ts:244-246,325-328; apps/desktop/src/renderer/workhub-route-policy.ts:281-292
submit() correctly captures the originating visit's policy before its awaits, but it does not record an explicit correction until after sessions.submit() resolves. resetVisitContext() creates a new policy while deliberately sharing the same runtime correction memory, and rememberCorrection() assigns the sequence at completion time. Consequently an older correction can become the runtime's newest correction after a newer visit.
Deterministic reproduction at this head:
- In visit A, correct expression X to Session A and leave A's
sessions.submit()pending. - Navigate away/back; in visit B correct the same X to Session B and let that submission finish.
- Let visit A's older submission finish.
- Submit another expression in the X family.
The final request routes to Session A (evidence: route_correction), not Session B, because A receives the higher sequence when it completes. I reproduced the target order as A, B, A. This violates the PR's stated guarantees that the latest explicit correction wins for the runtime and that asynchronous submissions are isolated across visits. The stale-submit test covers only focus, and the latest-correction test is sequential, so neither exercises this ordering.
Assign correction order when the user correction is accepted (before the await), then commit successful corrections with that reserved order, or otherwise prevent an older visit's completion from outranking a later correction.
I found no other actionable P0–P3 issue. Session remains the durable authority, the transient maps are bounded, expected-turn Stop is correctly guarded, and the documented UI/attribution requirements are satisfied. Exact-head test is terminal SUCCESS (run 32693248765, job 97336731427).
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed on 849c9de03d8d03a20bb240d016690d3d223e76d1. The cross-visit ordering problem I raised earlier is genuinely fixed — I re-derived it from the code rather than trusting the earlier conclusion, and it holds under the targeted controller and surface tests.
One new blocking finding, and it sits precisely in the ownership logic this PR adds.
[P2] The steered branch drops root ownership, because production never returns the ID it compares against
apps/desktop/src/renderer/workhub-controller.ts:193 handles a steered submission like this:
const existing = ownedRootBySessionId.get(target.sessionId);
if (turn.steered) {
if (existing !== turn.turnId) ownedRootBySessionId.delete(target.sessionId);
return;
}That is correct only if a steered result reports the ID of the root turn it was folded into. It does not. workhub-session-port.ts:161 mints a fresh turnId for the command, and the main-process handler uses that same client-supplied value — runtime-host-session-execution-ipc-main.ts:215 is command.turnId ?? newId(), and the busy fallback at :320 returns it unchanged alongside steered: true. The returned ID identifies this send, never the running root. So existing !== turn.turnId is true essentially always, and the branch deletes the ownership it was written to preserve.
The user-visible path:
- WorkHub opens a root turn R on the payment session.
- A follow-up goes to the same still-running session. The bridge steers it and returns a fresh client ID T ≠ R.
- Ownership for that session is deleted.
- The user says "not this one, switch to login".
correctionForat:185finds noturnId, so the correction carries onlyfrom, the guard at:323does not fire, andsessions.stopis never called.
The routing correction itself still lands on login, so nothing looks broken — but the original payment root keeps running. The production stop guard prevents a wrong turn from being killed; it cannot revive a stop that was never issued.
The existing test at workhub-controller.test.ts:1157 misses this because its mock returns { turnId: 'turn-payment-root', steered: true } — the root ID, which the bridge never produces. Substituting a bridge-shaped ID reproduces the loss directly: the correction resolves to route_correction -> login with no stop call at all.
Suggested direction: on steered, keep whatever ownership already exists — steered means "no new root was created", so there is nothing to update, and no ownership should be invented if none is known. Please also add a test whose steered result carries a bridge-shaped ID rather than the root's. Separately, a late completion arriving across a remount can still overwrite newer ownership; a submission generation counter would close that.
简体中文
已在 849c9de03d8d03a20bb240d016690d3d223e76d1 上重新审查。我先前提的跨访问顺序问题确实已修复——这一条我是从代码重新推导的,而不是沿用旧结论,并在针对性的 controller 与 surface 测试下成立。
新增一条阻塞项,而且正好落在本 PR 新加的 ownership 逻辑上。
[P2] steered 分支会丢掉 root ownership,因为生产环境从不返回它所比较的那个 ID
apps/desktop/src/renderer/workhub-controller.ts:193 这样处理 steered 提交:
const existing = ownedRootBySessionId.get(target.sessionId);
if (turn.steered) {
if (existing !== turn.turnId) ownedRootBySessionId.delete(target.sessionId);
return;
}只有当 steered 结果回报的是「它被并入的那个 root turn 的 ID」时,这段才是对的。但它并不是。workhub-session-port.ts:161 会为本次 command 新造一个 turnId,主进程处理器用的就是这个客户端传入的值——runtime-host-session-execution-ipc-main.ts:215 是 command.turnId ?? newId(),而 :320 的 busy fallback 原样把它连同 steered: true 返回。这个 ID 标识的是本次发送,从来不是正在跑的 root。于是 existing !== turn.turnId 几乎恒为真,这个分支删掉了它本该保住的 ownership。
用户可见的路径:
- WorkHub 在 payment session 上开出 root turn R。
- 后续消息发到同一个仍在运行的 session,bridge 把它 steer 进去,返回一个新的客户端 ID T ≠ R。
- 该 session 的 ownership 被删除。
- 用户说「不是这个,换成登录」。
:185的correctionFor取不到turnId,correction 只带from,:323的判断不成立,sessions.stop从未被调用。
路由纠正本身仍然会落到 login,所以表面上看不出问题——但原来的 payment root 还在跑。生产环境的 stop guard 能防止误杀错误的 turn,却救不回一次根本没有发出的 stop。
现有测试 workhub-controller.test.ts:1157 没能覆盖,是因为它的 mock 返回 { turnId: 'turn-payment-root', steered: true }——返回的是 root ID,而 bridge 永远不会产生这种形状。换成 bridge 形状的 ID 就能直接复现:correction 结果是 route_correction -> login,而 stop 调用为空。
建议方向:steered 时保留已有的 ownership——steered 的含义就是「没有创建新的 root」,因此没有什么需要更新;若本来就不知道 ownership,也不应凭空创建。同时请补一个 steered 结果携带 bridge 形状 ID(而非 root ID)的测试。另外,跨 remount 的迟到 completion 仍可能覆盖更新的 ownership,用一个提交代次计数可以堵住。
Astro-Han
left a comment
There was a problem hiding this comment.
Correcting my own weighting in the review above: I mentioned the late-completion case as a passing remark, and it deserves to be a blocking finding in its own right. It is a second, independent seam, not a detail of the first.
[P2] A late-resolving submission overwrites newer root ownership
The non-steered branch at workhub-controller.ts:202 writes unconditionally:
ownedRootBySessionId.delete(target.sessionId);
ownedRootBySessionId.set(target.sessionId, turn.turnId);There is no notion of which submission this result belongs to, so whichever deps.sessions.submit() resolves last wins, regardless of which was issued last. resetVisitContext() at :346 bumps the focus version and starts a new route-policy visit, but deliberately does not clear ownedRootBySessionId — so ownership survives the remount while an older in-flight submit is still outstanding.
Concretely: an older payment submit has not resolved; WorkHub unmounts and remounts; a new payment submit resolves to turn-new and is recorded; the older submit then resolves to turn-old and overwrites it. A later natural correction reads turn-old, and the expected-turn guard in the production stop path correctly declines to kill it — so the stop is a no-op, the corrected request is sent, and turn-new keeps running. As with the first finding, the guard behaves correctly and the outcome is still a turn nobody stopped.
Suggested direction: give each submission a per-session monotonic generation, recorded before the first await, and let only the current generation update ownership — including the steered and clearing paths, so a stale result cannot resurrect or erase a newer root either. A test where an older submit resolves after a remount would lock it.
简体中文
先更正我自己在上一条 review 里的定级:我把「迟到完成」的情况写成了一句顺带的提醒,但它应当独立成为一条阻塞项——这是第二个独立的接缝,不是第一条的细节。
[P2] 迟到 resolve 的提交会覆盖更新的 root ownership
workhub-controller.ts:202 的非-steered 分支是无条件写入的:
ownedRootBySessionId.delete(target.sessionId);
ownedRootBySessionId.set(target.sessionId, turn.turnId);这里没有任何「这个结果属于哪一次提交」的概念,因此最后 resolve 的那次 deps.sessions.submit() 获胜,而不是最后发起的那次。:346 的 resetVisitContext() 会递增 focus 版本并开启新的 route-policy visit,但刻意没有清空 ownedRootBySessionId——于是在旧的在途 submit 尚未返回时,ownership 跨过了 remount 存活下来。
具体地说:旧的 payment submit 还没 resolve;WorkHub unmount 后重新挂载;新的 payment submit 返回 turn-new 并被记录;随后旧的 submit 返回 turn-old,把它覆盖掉。之后的自然纠正读到的是 turn-old,生产 stop 路径的 expected-turn guard 正确地拒绝去杀它——于是这次 stop 成了空操作,纠正后的请求照常发出,而 turn-new 继续运行。和第一条一样:guard 的行为是对的,结果仍然是一个没有人停下来的 turn。
建议方向:为每次提交分配 per-session 的单调代次,在第一个 await 之前记录,只允许当前代次更新 ownership——steered 分支与清除分支同样适用,这样过期结果既不能复活也不能抹掉更新的 root。补一个「旧 submit 在 remount 之后才返回」的测试即可锁住。
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed on 74ace989cf5ca515a50121fb2e17acd3f516a062. Both blocking findings are closed. I re-derived each from the new code rather than reading the commit message.
The steered branch no longer discards ownership. rememberOwnedRoot now returns early on steered without touching the map, which is the right reading of what steered means: no root was created, so there is nothing to record and nothing to invalidate. The comparison against a command ID that production never sets to the root ID is gone entirely, rather than being repaired — that removes the class of bug, not just this instance.
Late completions can no longer overwrite newer ownership. Every submission now carries a submissionOrder, and rememberOwnedRoot rejects a write whose order is older than the stored record. The tombstone is the part I want to call out as well-judged: after a correction stops a root, forgetOwnedRoot stores { order } with no turnId rather than deleting the entry, so an older in-flight root result arriving afterwards cannot resurrect ownership that was deliberately released. Deleting would have left exactly the hole the ordering was introduced to close.
I also checked the guard's own edges. forgetOwnedRoot only clears when the stored turnId matches the one actually stopped, so a correction cannot release ownership another submission established. If sessions.stop rejects, the submission propagates the error and ownership stays — correct, since the root is still running.
[P3] Tombstones now compete with live ownership for the same 32 slots
MAX_OWNERSHIP_RECORDS bounds the map, and released tombstones are stored in it rather than deleted. Across more than 32 sessions, tombstones can now evict a live root ownership record that a later correction would have needed, and the symptom would be the original one: the correction carries no turnId, no stop is issued, and the root keeps running with nothing visibly wrong.
This is much narrower than what it replaced and the trade is the right way round — a bounded map has to evict something. But since tombstones are only useful until the in-flight result they guard against has resolved, they could be evicted in preference to live records, or aged out separately, so the bound is spent on ownership that can still be acted on.
Nothing above blocks. The exact-head test run is still queued, so this is not an approval yet.
简体中文
已在 74ace989cf5ca515a50121fb2e17acd3f516a062 上重审。两条阻塞项都已闭合。 每一条我都是从新代码重新推导的,没有采信 commit message。
steered 分支不再丢弃 ownership。 rememberOwnedRoot 现在在 steered 时直接提前返回、完全不碰这个 map——这才是 steered 的正确读法:没有创建 root,所以既没有东西要记录,也没有东西该作废。那个"与一个生产环境永远不会设成 root ID 的 command ID 做比较"的判断被整个删掉了,而不是被修补——这消除的是一类 bug,而不只是这一个实例。
迟到完成不再能覆盖更新的 ownership。 每次提交现在都带 submissionOrder,rememberOwnedRoot 会拒绝代次更旧的写入。墓碑这一处我想特别指出,判断得很准:一次纠正停掉 root 之后,forgetOwnedRoot 存的是没有 turnId 的 { order } 而不是删除条目,因此随后到达的、更旧的在途 root 结果无法复活一个已被主动释放的 ownership。如果当时选择删除,留下的恰恰是引入代次要堵的那个洞。
我也核了守卫自身的边界。forgetOwnedRoot 只在存储的 turnId 与真正被停掉的那个相符时才清除,因此一次纠正不会释放另一次提交建立的 ownership。若 sessions.stop 抛错,提交会把错误抛出去、ownership 保留——这是对的,因为那个 root 还在跑。
[P3] 墓碑现在与存活的 ownership 争同一批 32 个槽位
MAX_OWNERSHIP_RECORDS 限制着这个 map,而被释放的墓碑是存进去而不是删掉的。当会话数超过 32 时,墓碑可能挤掉一条后续纠正本来需要的存活 root ownership,而症状会是最初那一个:纠正不带 turnId、不发出 stop、root 继续跑,表面上看不出任何异常。
这比它替换掉的问题窄得多,而且这个取舍方向是对的——有界的 map 总得淘汰点什么。但墓碑只在"它所防范的那个在途结果尚未返回"之前有用,所以可以让墓碑优先于存活记录被淘汰、或者单独设一个老化策略,把这份配额花在仍然可被操作的 ownership 上。
以上都不阻塞。exact head 的 test 仍在排队,因此这还不是一次 approve。
Astro-Han
left a comment
There was a problem hiding this comment.
One more blocking finding on the same head, in the same area. It is not a variant of the two just closed — those were about ownership being wrongly discarded or wrongly overwritten. This one is about ownership that does not exist yet.
[P2] A root that has been admitted but whose result has not returned cannot be stopped by a correction
rememberOwnedRoot runs only after deps.sessions.submit() resolves. Between the moment the Host admits the root and the moment that promise settles in the renderer, there is no record for that session at all, so correctionFor at :190 returns { from } with no turnId, the guard at :353 does not fire, and no Stop is issued. The tombstone cannot help here, because a tombstone is only written by forgetOwnedRoot — and that is reached only after a Stop was attempted.
Within one mount this is unreachable, and I checked that before writing this: WorkHubSurfaceRouteGate is single-flight, run() returns immediately while #pending is true, and workhub-surface.tsx:232 refuses a second submission on the same condition. But the gate is useRef(new WorkHubSurfaceRouteGate()).current — one per component instance — while the controller is passed in and survives. After a remount the gate is fresh with pending === false, and the original submission is still in flight.
The remount does not disarm correction detection either, which was the part I expected to close the window. resetVisitContext() calls routePolicy.newVisit(), which builds a new visit over the shared correctionMemory while leaving currentFocus undefined. The next submit re-lists sessions and calls initializeFocus, which — seeing no current focus — sets it to the most recently updated session. That is the still-running root's session. So the correction resolves normally, with correctedFrom pointing at exactly the root that has no ownership record.
Reproduced against the compiled controller at this head:
{"submitted":["payment","login"],"stopped":[],"correctedFrom":{"sessionId":"payment"}}
The user says "not this one, switch to login", the request is routed to login, and the payment root keeps running with no Stop ever issued. Note that the intermediate steered follow-up is not required — the plain admission window is enough.
Suggested direction: reserve a pending ownership record synchronously, before the submit await, and resolve it to the returned root ID when the promise settles. A correction that meets a pending record then has something to coordinate with rather than nothing to find, and the existing order comparison still prevents a stale result from resurrecting a released root.
Credit where due: this was found and reproduced by another reviewer on this change; I verified the mechanism independently against the code before publishing it.
简体中文
同一 head、同一区域,再补一条阻塞项。它不是刚闭合那两条的变体——那两条是 ownership 被错误丢弃或被错误覆盖;这一条是 ownership 还不存在。
[P2] 已被受理但结果尚未返回的 root,无法被纠正停掉
rememberOwnedRoot 只在 deps.sessions.submit() resolve 之后才运行。从 Host 受理这个 root,到该 promise 在渲染进程落定,这段时间里该 session 根本没有任何记录,因此 :190 的 correctionFor 返回只带 from、没有 turnId,:353 的判断不成立,Stop 从未发出。墓碑在这里帮不上忙——墓碑只由 forgetOwnedRoot 写入,而那要在尝试过 Stop 之后才到得了。
在单次挂载内这是够不着的,这一点我在写这条之前先核过:WorkHubSurfaceRouteGate 是单飞的,#pending 为真时 run() 立即返回,workhub-surface.tsx:232 也按同一条件拒绝第二次提交。但这个 gate 是 useRef(new WorkHubSurfaceRouteGate()).current——每个组件实例一个——而 controller 是外部传入、跨挂载存活的。重新挂载之后 gate 是全新的、pending === false,而最初那次提交仍在途中。
重新挂载也没有让纠正识别失效——这原本是我预期能堵住这个窗口的地方。resetVisitContext() 调 routePolicy.newVisit(),新 visit 建立在共享的 correctionMemory 之上,同时把 currentFocus 留空。下一次 submit 会重新列出 sessions 并调用 initializeFocus,后者在没有当前焦点时把焦点设为最近更新的那个 session——也就是那个仍在运行的 root 所在的 session。于是纠正正常解析,correctedFrom 精确指向那个没有 ownership 记录的 root。
在本 head 编译后的 controller 上复现:
{"submitted":["payment","login"],"stopped":[],"correctedFrom":{"sessionId":"payment"}}
用户说「不是这个,换成登录」,请求被路由到 login,而 payment 的 root 继续运行,Stop 从未发出。注意中间那次 steered 跟进不是必需的——仅仅是受理窗口本身就够了。
建议方向:在 submit 的 await 之前同步预留一条 pending ownership 记录,待 promise 落定后把它解析为返回的 root ID。这样纠正遇到的是一条可协调的 pending 记录,而不是"什么都找不到";同时既有的代次比较仍能防止过期结果复活一个已释放的 root。
说明来源:这条由本次改动的另一位审查者发现并复现;我在发布前独立对照代码核过其机制。
Astro-Han
left a comment
There was a problem hiding this comment.
A third finding on this head, and a correction to the fix I suggested in my previous comment.
[P2] The controller is rebuilt whenever the project list refreshes, discarding all ownership
app-shell.tsx:1560 builds the WorkHub controller inside useMemo(..., [projects]), and createWorkHubController keeps rootOwnershipBySessionId in its closure. A new controller is therefore an empty ownership map.
projects gets a fresh array on every default-context refresh: use-project-context.ts:125 does const nextProjects = [...snapshot.projects] and :134 calls setProjects(nextProjects) unconditionally, with no identity comparison against the current value. So renaming a project, adding one, or any refresh that re-reads the default context replaces the array, re-runs the memo, and wipes every ownership record.
This is worse than the admission-window finding above, because it does not need a remount and it does not need any timing at all. The submission can have fully resolved and its ownership correctly recorded; a project refresh a moment later still throws it away. The next natural correction finds no turnId, issues no Stop, and the original root keeps running — the same end state, reached from a completely ordinary user action.
Suggested direction: give the controller the lifetime of the app runtime, and read the project name through a live ref or an updatable dependency instead of rebuilding on the array. Note the memo's only use of projects is the projectName lookup — the identity of the whole controller is currently tied to a value it uses for one label.
Correcting my own suggestion
In my previous comment I proposed reserving a pending ownership record before the submit await. That is not sufficient, and I would rather say so now than have it implemented as written. A single slot per session — owner | pending | tombstone as a union — loses information in at least two reachable shapes:
- A has opened a root that has not returned; after a remount, B is steered into A and returns first. If B overwrites the pending record and the ordering rule then rejects A's older result, the real root is permanently unowned.
- The map already holds a dead root R while S is opening a new one. The pending record must displace R in order to be waited on, but if B turns out to be steered, R has to be preserved or restored. One slot cannot represent both.
So a session needs to hold a settled owner, one or more in-flight admissions, and a tombstone at the same time. Three further constraints fall out of that:
- An unresolved pending record must not share the existing 32-entry FIFO eviction. Silently evicting one guarantees the late-arriving root is unowned. Either pending records are exempt from eviction, or the 33rd submission is back-pressured before the Host admits it.
- A rejected
submitmay only remove its own admission token; it must not clear an existing owner. The session port does not currently promise that a rejection means "not admitted", so that contract needs tightening — or the state needs reconciling against the authoritative side — before this is relied on. - While a correction waits on a pending record, a later input after a remount can be steered into the very root that correction is about to Stop. The ownership map provides no linearisation, so this needs a barrier or conflict queue at controller lifetime.
One thing that did survive scrutiny: the Stop path itself. The main process re-checks the root and waits for the interrupt to reach a terminal state, and the controller does not forget ownership when an interrupt rejects. No path was found where Stop reports success while the same expected root is still running.
简体中文
本 head 上的第三条发现,以及对我上一条评论中所给修复方向的一处更正。
[P2] 项目列表刷新时 controller 会被重建,全部 ownership 随之丢失
app-shell.tsx:1560 在 useMemo(..., [projects]) 内构造 WorkHub controller,而 createWorkHubController 把 rootOwnershipBySessionId 保存在闭包里。因此新的 controller 就是一张空的 ownership 表。
而 projects 在每次默认上下文刷新时都会拿到新数组:use-project-context.ts:125 是 const nextProjects = [...snapshot.projects],:134 无条件 setProjects(nextProjects),没有与当前值做任何同一性比较。于是重命名一个项目、新增一个项目,或任何重新读取默认上下文的刷新,都会替换数组、重跑 memo、抹掉全部 ownership 记录。
这比上面那条受理窗口的问题更糟,因为它既不需要 remount,也完全不依赖时序。提交可以已经彻底 resolve、ownership 也已正确记录;片刻之后的一次项目刷新照样把它丢掉。下一次自然纠正取不到 turnId、不发 Stop,原 root 继续运行——同样的结局,却是由一个再普通不过的用户操作达成的。
建议方向:让 controller 拥有 app 运行时级的生命周期,项目名通过 live ref 或可更新依赖读取,而不是随数组重建。注意这个 memo 对 projects 的唯一用途就是查 projectName——整个 controller 的身份,目前被绑在一个只用来取标签的值上。
更正我自己给出的建议
上一条评论里我建议在 submit 的 await 之前预留一条 pending ownership 记录。这不充分,我宁愿现在说清楚,也不愿它按原样被实现。每个 session 单槽——把 owner | pending | tombstone 做成联合——在至少两种可达形状下会丢信息:
- A 已开出一个尚未返回的 root;remount 之后 B 被 steer 进 A 并先返回。若 B 覆盖了 pending 记录,随后代次规则又拒绝 A 更旧的结果,真实的 root 就永久无主。
- 表中已有一个已死的 R,而 S 正在开新 root。pending 记录必须挤掉 R 才能被等待;但如果 B 最终是 steered,R 又必须被保留或恢复。单槽无法同时表达这两者。
所以一个 session 需要同时持有:已落定的 owner、一个或多个在途 admission、以及 tombstone。由此还导出三条约束:
- 未落定的 pending 记录不能与现有 32 槽 FIFO 淘汰共用。静默淘汰一条,就等于保证那个晚到的 root 无主。要么 pending 免于淘汰,要么第 33 次提交在 Host 受理之前就被背压。
submit被 reject 时只能移除它自己的 admission 令牌,不能清掉既有 owner。目前 session port 并没有承诺「reject 一定意味着未受理」,因此在依赖这一点之前,要么收紧该契约,要么与权威侧重新对账。- 当一次纠正正在等待某条 pending 记录时,remount 之后更晚的输入可能被 steer 进正是这次纠正即将 Stop 的那个 root。ownership 表不提供线性化,因此需要一个 controller 生命周期上的屏障或冲突队列。
有一处经受住了推敲:Stop 链本身。主进程会重新核对 root,并等待 interrupt 到达终态;interrupt 被拒绝时 controller 也不会遗忘 ownership。没有找到「Stop 报告成功、而同一个 expected root 仍在运行」的路径。
Generated-by: Codex
EnglishRe-reviewed at The restructuring is a real improvement. Splitting live ownership from tombstones into two maps closes the earlier competition where a single map let the two evict each other — each now has its own eviction domain. Representing an admission as Three things remain. 1. [Blocking] A partially failed correction records intent as fact
storeOwnershipTombstone(correction.from.sessionId, order, turnIds);
for (const turnId of turnIds) {
await deps.sessions.stop(correction.from, turnId);
}Take a session holding a confirmed root
Wrap each stop individually and merge only the turn IDs that actually succeeded into the tombstone — 2. [Non-blocking] Ownership can be lost through three separate pathsThese are the same class of defect and are clearer stated together than as three unrelated notes:
In all three the Session view remains authoritative and the user can stop the run there, so this is a correction blind spot rather than data loss. But nothing currently guarantees the property this revision is built around — that a pending admission is not discarded. 3. [Non-blocking] The pending window is narrowed, not closed
This is the residue of the earlier pending-window finding rather than its closure. The window is much smaller than before and the reserve/settle split makes it auditable, but there is no reconciliation path — for example, reconciling against recent turns for the reserved interval after a failed submit. Minor
Approval is blocked on item 1 and remains subject to terminal-green hosted checks on the exact head. 简体中文基于 这次重构是实质改进。把 live 所有权与 tombstone 拆成两张表,关闭了原先「同一张表里两者互相挤占」的竞争——各自有独立的驱逐域。用 还有三点。 一、[阻塞] 部分失败的修正会把「意图」记成「事实」
storeOwnershipTombstone(correction.from.sessionId, order, turnIds);
for (const turnId of turnIds) {
await deps.sessions.stop(correction.from, turnId);
}设想一个 session 持有已确认的 root
建议在循环内逐个 try/catch,只把实际停止成功的 turn ID 并入 tombstone—— 二、[非阻塞] 所有权有三条独立的丢失路径它们属于同一类缺陷,合在一起说比拆成三条互不相干的备注更清楚:
三条路径下 Session 视图仍然是权威的,用户可以在那里停止运行,所以这是修正机制的盲区而非数据损坏。但目前没有任何机制保证这次改动所依赖的那条性质——待定 admission 不会被丢弃。 三、[非阻塞] 待定窗口被收窄,但没有关死
这是上一轮待定窗口问题的残余而不是闭合。窗口比之前小得多,reserve/settle 的拆分也让它变得可核对,但目前没有任何对账路径——例如在提交失败后,按预留时段去反查最近的 turn 做核对。 次要
批准被第一点阻塞,并且仍以该 exact head 的 hosted checks 全部终态绿为前提。 |
EnglishCorrection to my previous comment — the severity grouping there was misleading and I do not want it to mislead anyone planning this work. I put the three ownership-loss paths under one [Non-blocking] heading. That label is wrong for the first of them. The renderer-rebuild path is the P2 I raised on the previous head and it is still open and still blocking — grouping it with the two eviction paths was for explanatory clarity, not a downgrade. The accurate ledger at
So: 2 blocking, 3 non-blocking, 1 minor. Both P2s must be resolved before approval, and terminal-green hosted checks on the exact head remain a separate gate. The substance of the previous comment is unchanged — only the grouping label was wrong. 简体中文对上一条评论的更正——那里的定级分组有误导性,我不希望它影响到任何据此安排工作的人。 我把三条所有权丢失路径放在了同一个 [非阻塞] 标题下。对其中第一条来说这个标签是错的。渲染层重建那条是我在上一个 head 提出的 P2,它仍然未解决、仍然阻塞——把它和两条驱逐路径合在一起是为了说清楚问题的性质,不是降级。
即:2 条阻塞、3 条非阻塞、1 条次要。 两条 P2 都需解决后方可批准,该 exact head 的 hosted checks 终态全绿仍是另一道独立的门。 上一条评论的实质内容不变,错的只是分组标签。 |
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
I verified a blocking issue that persists in this head.
[P2] sessions:stop no-op is recorded as stopped, leaving a late-admitted root orphaned
When a correction Stop targets a pending reservedTurnId before the Host has admitted the root, the main process returns void (runtime-host-session-execution-ipc-main.ts:741-749). The controller treats every resolve as success and writes the ID to stoppedTurnIds (workhub-controller.ts:514-517). The later direct startTurn still reuses the same reserved ID (main:219,258-260,341-347); settleOwnedRoot then skips the second Stop because the tombstone already contains it (:397-399) and clears pending without registering confirmed (:412-423). The earlier root keeps running with no owner reachable.
Why this matters: a pending barrier (stop before settle) is used to guarantee correction reaches the new Session. With the current protocol the pending ID is consumed as "already stopped", so the real root is never stopped and remains uncorrectable.
Minimal fix direction (same protocol change as the queued Q2): make sessions:stop return a distinguishable disposition (actually stopped / already terminated vs not observed) and only add to stoppedTurnIds on the former; do not treat "not observed" as stopped — keep the order barrier until admission lands.
Heads verified: 252a779 (current) vs 00aef92 (same code paths, docs-only delta). CI on 252a779 is still IN_PROGRESS (32759455573).
简体中文
该头仍存在 Stop 误记为已停导致的无主 root,需区分 Stop 返回态后再写 tombstone。
Astro-Han
left a comment
There was a problem hiding this comment.
I verified this remains blocking in this head.
[P2] sessions:stop no-op is recorded as stopped, leaving the later root orphaned
workhub-controller.ts:365-386 treats the Host void response for an unknown/pending turn as success; :492-518 then writes the same T to stoppedTurnIds after the correction barrier. When the same reserved T is later admitted, settleOwnedRoot at :394-421 skips the second Stop and clears pending without confirming an owner. The earlier root can keep running with no WorkHub ownership.
Existing tests at workhub-controller.test.ts:1286-1338 cover a late return with a different ID (so a second Stop is triggered) and :1455-1515 covers a pending Stop promise — neither covers the same-ID late admission.
Fix direction: return a distinguishable Stop disposition (actually stopped vs not observed) and keep the pending barrier until admission confirms whether to stop or fail-closed; do not record an unobserved ID as stopped.
Head: 252a779e5f. This is independent of CI — the code issue persists even if checks turn green.
简体中文
该阻断在该头仍成立,需区分 Stop 返回态后再写 tombstone。A successful Stop for an unconfirmed reserved turn may be a Host no-op. Keep that turn out of the final stopped set so settlement retries Stop after admission. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
I verified the previous blocking issue remains, though narrowed to an in-flight window.
[P2] Late admission joins the still-in-flight earlier Stop attempt and is still orphaned
workhub-controller.ts:500-520 now avoids writing the pending ID to stoppedTurnIds on the first resolve, but attemptStop dedupes by (sessionId, turnId) at :365-386. If Host has decided the first Stop is a no-op but the reply is still in flight when the same reserved T is admitted, settleOwnedRoot at :397-404 reuses that existing promise, tombstones T, clears pending and never issues a post-admission Stop.
Repro: hold first Stop reply → resolve same-ID submit → release Stop reply. Expected 2 Stops, observed 1. The existing new test at workhub-controller.test.ts:1341-1391 waits for submit(correction) before finishPayment, so it does not overlap the reply.
Fix: ensure admission proof starts a distinct Stop after any pre-admission attempt settles, or return a not-observed vs stopped disposition; do not let the late admission join the earlier unconfirmed attempt.
Head: 38c360c5ad. Independent of CI.
简体中文
仍为无主 root 阻断,需处理在途 Stop 复用。Admission can complete while the pre-admission Stop reply is still in flight. Await that attempt, then issue a distinct Stop against the now-admitted root instead of joining the earlier no-op promise. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
I verified the previous blocking finding is now closed in this head.
The in-flight Stop reuse gap is fixed: settleOwnedRoot now captures any existing stopAttemptByTurn promise for the same (sessionId, turnId), waits for its finally to clear the dedupe key, then issues a fresh attemptStop. The new test holds the first Stop pending, finishes admission of the same ID, then releases the first reply and asserts two Stops — covering the previously missed overlap.
No new blocking issues. Checks on d9321354b2 are test: success.
Summary
Refs #3492
Verification
npm run lintnpm run format:checknpm run buildnpm run typechecknpx knip --workspace apps/desktopnpx knip --workspace packages/uiBefore correction:
After natural-language correction:
Review focus
The only cross-visit state is bounded renderer memory: up to 32 explicit correction expressions and 32 WorkHub-owned root Turn IDs. It is not persisted, does not copy transcripts, and cannot replace Session execution or lifecycle authority. Each submission captures its originating visit's focus so an older await cannot borrow or overwrite a newer visit.
AI use
Select exactly one:
Tool(s) and scope: Codex implemented the change, tests, UI evidence, and documentation; the affected commit includes
Generated-by: Codex.Checklist
Does this PR entail a change in behavior?