feat(workhub): persist delegation linkage - #3935
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
What this PR does
Slice 5's first cut: it makes "this delegation happened" a durable fact. On top of #3818's non-destructive typed Action Gate, it appends an immutable delegation_intent before the target effect and a delegation_committed after it, both into the existing WorkHub Coordination Session. Around that it adds a renderer send lease binding an action identity to the retained Composer draft per Runtime Host, three-source recovery of the accepted target Turn after a Host restart, a persisted first Coordination summary so retries stay byte-identical, and revision-guarded session.remove compensation for a create_new Session whose submit was definitively rejected. It deliberately excludes replace, Stop ownership, target lifecycle state, supersession, and a general workflow phase machine.
Reviewed at exact head 1b352a04f9944fbd3acc0a21039f6aa300ab1e79. test and windows_recovery are green.
Verdict
Requesting changes: 4 × P1, 5 × P2, 4 × P3, all inline.
The design choice in the PR description is right, and I want to say so before the findings. A self-contained intent written before the effect is genuinely stronger than a committed-only journal, candidateRef really is the wrong thing to key recovery on, and the lock order [Coordination lease] → target effect → [Coordination lease] is clean with no nesting and no deadlock. Crash-after-effect/before-commit is correctly idempotent through #queryDurableSubmitProof. The three recovery sources are not redundant — each covers a disposition the others cannot prove.
Nothing is P0: workHub.enabled defaults to false (packages/core/src/settings.ts:560), so none of this blocks a release. The four P1s must be fixed before merge.
Root causes
Most of the findings are not independent. Four choices produce almost all of them, and fixing the first and last collapses most of the list.
1. The action identity was built on the wrong component. WorkHubSendLease is simultaneously the ComposerDraftPersistence implementation (read/write on key 'workhub'), the owner of the action identity, and the store for the frozen summary — all in one blob whose identity is inferred from draft-text equality. Those two things have opposite lifecycles. A draft changes on every keystroke and is cleared on send; an action identity must survive edits and retire only on an explicit protocol event. Hosting the identity inside the draft component makes every keystroke an identity event, and makes any non-Composer action (a clarification click) unable to own an identity at all.
This one choice produces P1 #1, P1 #2, P2 #8, and P3 #11, and it is why settle/complete/acquire never settled into a clean shape. The smaller design: keep requestId under its own key scoped only by Host, retire it with an explicit release(requestId), and leave the draft on the existing ComposerDraftPersistence seam. preservesIdentity, summary's owner check, and the whole read/write surface on this class then disappear.
2. The record state machine has no terminal state. The journal models intent → committed and nothing else, so "this intent can never succeed" is represented as "stay in intent forever". Every path where the effect becomes permanently impossible therefore becomes a permanent trap rather than a clean failure: the tombstoned create_new id (P1 #3), a fingerprint mismatch on a durable identity (P2 #6), and an edited target message whose digest no longer matches. A third terminal record — delegation_abandoned, meaning "this actionId is spent, mint a new one" — closes all three at once and is the missing piece rather than three separate patches.
3. A parallel path instead of the closest existing seam. workhub-delegation-journal.ts reimplements workhub-coordination-coordinator.ts#record point for point: header validation, derived message ids, high-water + messageIds snapshot read, content-equality idempotency, admission.run + refreshCanonical, drain-on-error. AGENTS.md's first principle applies. Beyond the duplication (P3 #12), the two copies independently decided what an unexpected error means — which is where the drain escalation in P2 #5 comes from.
4. Typed failures still have no end-to-end channel. Server-side codes exist (WorkHubActionGateFailureCode, WorkHubActionEffectFailureCode), are collapsed at the coordinator, dropped by ipcMain.handle, and reconstructed in the renderer by regex over English prose. This PR adds a fifth message that matches no pattern. It is not just cosmetic: it is precisely why P1 #1 and P1 #2 reach the user as "delivery failed, please retry" — an invitation to duplicate the delegation — instead of "already delivered" or "this can never succeed". Carried from #3818 and extended here.
One smaller point that is not a defect but shapes the design: the durable journal's primary key is minted and stored only by the renderer (sessionStorage), so the Host owns the authority but cannot enumerate, repair, or GC it, and every recovery must be client-driven (P2 #9). The PR rejected "an autonomous startup executor", which is reasonable — but Host-minted identity is not the same thing as a startup executor, and would remove the dependency on renderer storage surviving.
AI use: Claude Code assisted with source investigation and ran adversarial passes over the record layer, Action Gate, recovery, renderer, and architecture slices; every graded finding was re-verified against the exact head by me, and the analysis, grading, and conclusions are my own.
简体中文
这个 PR 在做什么：Slice 5 的第一刀，把"这次委派发生过"变成持久事实。在 #3818 的 typed Action Gate 之上，于同一个 WorkHub Coordination Session 内在目标副作用前后各追加一条不可变记录（delegation_intent / delegation_committed），配套渲染进程按 Host 作用域的 send lease、Host 重启后的三源恢复、首次 Coordination summary 持久化，以及 create_new 提交被定性拒绝后用 revision-guarded session.remove 回收空 Session。
结论：request changes，4 × P1 / 5 × P2 / 4 × P3，全部在行内。
先说对的部分：副作用前写自包含 intent 确实强于只写 committed，candidateRef 也确实不该作为恢复的键；锁顺序 [协调会话租约] → 目标副作用 → [协调会话租约] 干净、无嵌套、无死锁；副作用后崩溃前提交这一刀经 #queryDurableSubmitProof 是幂等的；三源恢复不冗余，各自覆盖别的源证明不了的 disposition。
没有 P0：workHub.enabled 默认 false，不阻塞发布。四条 P1 是合并前必须修的。
根因（多数发现并不独立，四个选择产生了几乎全部问题，修掉第 1 和第 2 条能一次收掉大半）：
- action identity 建在了错误的组件上。
WorkHubSendLease同时是ComposerDraftPersistence实现、action identity 的持有者、以及冻结 summary 的存储，全部塞在同一个 blob 里，且身份由草稿文本相等性推断。这两者生命周期正好相反：草稿每次按键都变、发送后清空；action identity 必须跨编辑存活、只能由显式协议事件退休。把身份寄生在草稿组件里，就等于让每一次按键都成为身份事件，也让任何非 Composer 发起的动作（点击澄清选项）根本无法拥有身份。这一个选择直接产生 P1#1、P1#2、P2#8、P3#11。更小的设计：requestId单独一格、只按 Host 作用域、由显式release()退休，草稿继续走既有 seam；preservesIdentity、summary的 owner 校验、这个类上的read/write全都可以删。 - 记录状态机没有终态。 只有
intent → committed，于是"这条 intent 永远不可能成功"被表示成"永远停在 intent"。所有效果已永久不可能的路径都变成死锁而不是干净失败：墓碑化的create_newid（P1#3）、fingerprint 不匹配（P2#6）、目标消息被编辑后 digest 对不上。加第三条终态记录delegation_abandoned（含义：此 actionId 已作废，请重新取号）能一次关掉三处，而不是打三个补丁。 - 建了平行路径而没有扩展最近的 seam。 journal 逐点复刻了
workhub-coordination-coordinator.ts#record。除重复本身（P3#12）外，两份副本各自独立决定"未预期错误意味着什么"，这正是 P2#5 里 drain 升级的来源。 - typed failure 仍然没有端到端通道。 服务端有类型码，在 coordinator 被压平、被
ipcMain.handle丢弃、再由渲染进程用英文正则重建；本 PR 又新增了一条不匹配任何模式的消息。这不只是观感问题：它正是 P1#1 和 P1#2 到用户面前变成"投递失败，请重试"——一句诱导重复委派的提示——而不是"已投递"或"这条永远不会成功"的原因。
另有一条不算缺陷但塑造了整个设计：持久 journal 的主键由渲染进程生成并只存在 sessionStorage，于是 Host 拥有权威却无法枚举、修复或回收它，所有恢复都必须由客户端驱动（P2#9）。PR 拒绝"自主启动执行器"是合理的，但由 Host 分配并持久化身份并不等于启动执行器，那样就不必依赖渲染进程存储存活。
Downgrading to a non-blocking comment review. The findings and inline comments stand as advisory; they are not a merge block.
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Recover accepted submissions before retrying, retry pristine create cleanup after unknown outcomes, and keep waiting responses from consuming the final coordination summary. Add focused coverage and a renderer reload E2E for the durable action identity flow. Generated-by: Codex
Persist definitive abandonment and deterministic cleanup state, preserve retry identity across renderer drafts and Host scopes, and recover accepted steering from durable proof. Carry typed failures across Desktop IPC, avoid unnecessary Host drains, and extend regression coverage for crash and retry seams. Generated-by: Codex
04b17f4 to
dc25b43
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Makes "a delegation happened" durable so one piece of user work reaches a target Session at most once. Problem correctly defined, and delegation_abandoned closes the biggest gap in the original design. Net improvement over main — not blocking.
Nine findings last round: five fixed, three partial, one unchanged, seven new. That ratio is the review.
Root cause (architecture)
"This send" has three state machines with different values, transitions, and non-equivalent terminals, and nothing binds them:
| Authority | Values | Terminal |
|---|---|---|
Renderer lease (localStorage) |
active | settled — workhub-send-lease.ts:35 |
the 4 codes in workhub-surface.tsx:454 |
| Renderer turn list (React state) | routing | settled | failed — workhub-surface.tsx:47 |
— |
| Host journal | intent | committed | abandoned |
delegation_abandoned |
P1 — a non-terminal failure makes the next, rewritten message send the old text. workhub-send-lease.ts:84-90 returns existing.action.text unconditionally while the action is active; the guard at workhub-surface.tsx:188 only runs under preserveDraft, which send (:335-354) does not pass. Send "check the payment regression" → route throws persistence_failed → turn becomes failed, lease stays active → the user clears the box and types something else → the old text is delivered, and the turn list shows it. A renderer reload mid-send leaves the same permanently-active state. This is the first two rows disagreeing about one event: failed exists in one machine and not the other.
P2 — operation_conflict is too coarse to be a renderer-side terminal. workhub-coordination-coordinator.ts:209 collapses candidate_set_stale / candidate_unavailable / self_route / action_conflict into one code, and the renderer abandons the identity on sight. After a successful submit followed by commit_outcome_unknown, a retry whose candidate set has changed raises action_conflict → identity discarded → the user sends again → the target receives the same work twice. The distinction that matters — "this identity is empty, remint it" vs "this identity may already have delivered" — exists only in the Host and never crosses IPC.
The convergent fix is to delete state, not add a check: make the journal the single authority and reduce the lease to requestId only. Dropping text/draft/state removes both of the above, plus preserveDraft and the last P3 below, and it stays inside workhub-send-lease.ts and workhub-surface.tsx — the Host side does not move. A text-equality guard would add a fourth cell to those tables, and this is already the second round.
Local — worth fixing in this PR
P1 — actionFingerprint omits the target, so a retry can be delivered to the wrong Session with nobody detecting it. workhub-coordination-action-gate.ts:564-572 covers {userText, disposition, assistantText?} but not targetSessionId / create.workspace. First delivery to A fails with session_busy (non-terminal, identity retained) → the user retries the same text → policy now routes to B → the fingerprint matches → #act short-circuits into #executeDelegation(durable) and still delivers to A, discarding the freshly resolved candidate. No party considers this an error, so delegation_abandoned does not cover it.
P2 — the epoch bump does not close the downgrade path it was raised for. RUNTIME_HOST_COMPATIBILITY_EPOCH 53 → 54 guards the Client↔Host handshake, but decodeMessage still throws on an unknown type and SQLITE_SESSION_METADATA_SCHEMA_VERSION is still 32. An older build opening a profile that already holds delegation_* records runs the same build on both sides, so the handshake never fails and recoverInterruptedSessionsStrict fails at startup instead. Either tolerate unknown message types on read, or reject the profile explicitly.
Ungraded: epoch 54 is also claimed by #3751 and #3390 against main = 53. scripts/protocol-epoch-check.mjs only compares head against origin/main, so it structurally cannot see sibling branches. Whichever lands first keeps 54; the other two need a rebase.
P3
session-catalog-coordinator.ts:151STABLE_SESSION_INITIAL_REVISION = 1is a second authority for "initial revision is 1" (sqlite-session-metadata-store.ts:917is the first), and#creategrows three WorkHub-specific callbacks, pushing a WorkHub policy into the generic catalog. Related: could you state whererevision === 1 ⇒ pristinecomes from? If a header is not written between message admission and Turn start, a qualified submit failure during recovery would remove a Session holding a pending admission.'unauthorized'inisTerminalWorkHubSurfaceFailureis dead —:209already maps it tooperation_unavailable.workHubActError(runtime-host-workhub-ipc-main.ts:88-122) enumerates the codes a third time and silently degrades unknown ones tointernal_failure, with no compile-time signal when a code is added.acquireAttempt:106writes already-sent text back into the persisted draft underpreserveDraft, so a failure or reload makes a sent message reappear in the composer.
Simplification
workhub-delegation-journal.ts still reproduces workhub-coordination-coordinator.ts#record point for point and grew another 104 lines this round; one shared CoordinationRecordWriter covers both, and the split already showed as the read-path drain being fixed while the three write-path drains were not. Once delegation_abandoned exists, the whole create_new compensation chain guards a cosmetic leak rather than a correctness problem, and it is the heaviest runtime mechanism this PR adds. workhub-target-submission-recovery.ts reinterprets three kinds of evidence message-coordinator already owns; the minimal form is for turn.message.submit to be idempotent per messageId and self-describing about disposition.
AI-assisted review: a Claude Code subagent did adversarial analysis of the 1b352a04f→dc25b4320 delta; I verified every graded finding against the new head myself — read acquireAttempt / send / isTerminalWorkHubSurfaceFailure for the first P1, enumerated the three state machines from source, and confirmed the epoch value and the sibling-branch collision. No tests, typecheck, or format:check run; revision === 1 ⇒ pristine unverified. AI review is not independent human review.
简体中文
问题定义正确,相对 main 是净改善,不阻塞。上一轮九条:五修、三部分修、一未动,新增七条——修复在一张张状态表上分别进行,这是根因的症状。
根因:"这次发送"有三个状态机(渲染 lease、turn 列表、Host journal),取值与终态定义都不等价且互不绑定。P1-1(旧文本复发)是 failed 只存在于其中一个状态机;P2-1(operation_conflict 过宽)是两侧终态定义不等价。收敛做法是删状态——让 journal 成为唯一权威,lease 只留 requestId——而不是补一处文本校验。
本 PR 内值得修:actionFingerprint 缺 targetSessionId(会静默错投且无人察觉);epoch 54 没关上它想关的降级路径,且与 #3751、#3390 撞号。
Architecture question: can Slice 5 be one atomic assignment followed by asynchronous consumption?After tracing the current storage boundaries, I think this PR may be solving a local transactional problem as a distributed saga. The important boundary does not appear to be "Coordination Session versus target Session". Within one Runtime Host, Session metadata/transcripts, message admissions, AgentRun admissions, and runtime-event authority are all persisted in the same operational-state database (runtime.sqlite) through leases of the shared OperationalStateDatabaseOwner. Cross-Host coordination is explicitly deferred. The genuinely non-transactional effect is waking or continuing the in-memory executor, not recording facts for two Sessions. That suggests a smaller causal spine: Proposed atomic primitiveInstead of: could the Host expose one storage-level operation such as: assignWorkHubMessage({
actionId,
coordinationSessionId,
targetSessionId,
userText,
disposition,
createContext?,
})Under the target/Coordination admission authorities, its single SQLite transaction would:
Only after commit would MessageCoordinator wake the target Session. If the Host dies after commit and before wake, normal recovery sees the durable pending admission and wakes it later. If it dies before commit, neither Session observes the assignment and retry is safe. The resulting authoritative state machine is much smaller: Target execution lifecycle remains owned by the ordinary Session and is projected into WorkHub; WorkHub does not create another active/settled/failed state machine for it. Crash cuts
There is still a need for a small client idempotency key until the Host acknowledges the request, but it can be only { hostScope, actionId }. Composer draft persistence remains on its existing seam. The renderer should not need to persist action text, active/settled state, or a frozen summary in the same blob. Coordination transcript projectionThe typed delegation_assigned record can itself project as the visible WorkHub turn: If the transcript contract requires ordinary user/assistant/state messages, those messages can instead be appended in the same transaction. Either form avoids the current post-commit renderer write and therefore removes the "target committed but Coordination summary failed" cut. This also avoids paying five Coordination messages per successful delegation (intent + committed + user + assistant + state) unless all five have independently justified semantics. create_new compensation and retirementThe current synchronous compensation chain appears unnecessary under this boundary. Ideally, Session metadata creation and target admission are in the same transaction, so a failed assignment rolls the new Session back and there is no empty Session to discard. If database-external resources have already been materialized, logical retirement should still be synchronous while physical cleanup is asynchronous: From the coordinator's perspective, committing the tombstone/outbox notification finishes the operation. It should not synchronously own discardRevision, retry session.remove, interpret an unknown cleanup outcome, or drain the Host because physical cleanup did not finish inline. For this PR specifically, that would remove or sharply reduce:
Why this may be the more Occam-shaped boundaryThis is not necessarily the smallest diff: it requires a real transactional admission API below the current coordinator methods. But it has fewer independent authorities and fewer non-equivalent state machines. The current design has renderer lease state, local React turn state, a Host intent/commit/abandoned journal, target admission state, and a separately persisted Coordination summary. The proposed design has one atomic assignment fact plus the target Session's existing execution state. The questions I would like to resolve before adding more retry states are:
I am not suggesting that runtime execution itself belongs inside a SQLite transaction. The proposal is specifically to atomically commit the assignment and durable admission, then let execution consume that committed fact asynchronously. |
3 participants
Summary
Persist the first Slice 5 delegation authority in the existing WorkHub Coordination Session:
delegation_intentanddelegation_committedrecords aroundcreate_newanddelegate_existingcreate_newtitle/workspace in the immutable intentwaiting_for_useras a local, retryable result so it cannot consume the immutable acceptance summarysession.remove; stable-create replay retains the guarded cleanup revision while the Session remains pristine so an unknown first discard can be retried safelyThis supersedes #3930, which GitHub automatically closed when its non-conforming fork branch was renamed. It intentionally does not add
replace, Stop ownership, target lifecycle state, supersession, or a general workflow phase machine.Refs #3492
Proposal: #3286
Design choice: retry-driven immutable intent
A committed-only journal is insufficient because
candidateRefis opaque and scoped to one candidate snapshot. If the target effect succeeds but the committed link is not appended before a Host restart, a later candidate set cannot reconstruct the selected Session.The Action Gate therefore appends one self-contained immutable intent before the target effect, followed by one immutable commit containing the accepted target Turn. Its fingerprint covers stable user intent rather than ephemeral candidate IDs; once prepared, the intent owns the resolved target and creation context.
Recovery is driven by an explicit retry carrying the same production action identity. The renderer persists that identity with the retained Composer draft and scopes it to the active Runtime Host Coordination Session. It retires the identity only after target admission and the Coordination summary have both settled.
Before a retry can perform another target effect, the gate first checks durable admission evidence. An existing root receipt, pending message admission, or immutable steering-message proof is treated as accepted and is committed instead of re-submitted. A temporary
waiting_for_userresult stays local and does not reserve the immutable summary turn; the same action identity can later record the actual acceptance.For
create_new, cleanup remains deliberately narrow. The catalog returns a discard revision only for the newly created or exact stable-replay Session while it is still pristine. This lets a retry repeat revision-guarded cleanup after an unknown discard outcome, but any metadata or execution mutation revokes cleanup authority.This deliberately avoids an autonomous startup executor or mutable workflow phases while closing the following failure cuts:
waiting_for_usercreate_newsucceeding before a definitive target-submit rejection, including an unknown first cleanup outcomeUnknown submit outcomes remain recoverable and are never compensated destructively.
UI behavior evidence
Before
After
Failure → reload → same-identity retry
pr3935-workhub-failure-reload-retry.mp4
The screenshots and recording come from the Electron WorkHub E2E flow. The recording shows a post-admission Coordination-summary failure retaining the draft, a renderer reload retaining the same action identity, and a successful retry producing one accepted request rather than a duplicate delegation.
Verification
npm run lint— PASSnpm run format:check— PASSnpm run build— PASSnpm run typecheck— PASSnpx knip --workspace apps/desktop— PASSnpx knip --workspace packages/ui— PASSgit diff --check— PASSVerified against exact head
04b17f4caf35f0f358f8ce4de62d28e654e861e2.AI use
Select exactly one:
Tool(s) and scope: Codex implemented the typed delegation journal, production retry identity and recovery wiring, immutable steering-proof recovery, per-Host send leases, stable summary replay, revision-guarded
create_newcompensation, regression tests, and ADR updates. It also ran the verification listed above. All commits include the requiredGenerated-by: Codextrailer.Checklist
Does this PR entail a change in behavior?