feat(workhub): restore chat identity and reuse standard composer - #5003
Conversation
c255bda to
55d492f
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for bringing the standard Composer and Work identity back into WorkHub. Reusing the existing UI and keeping execution in ordinary Sessions is the right direction. I reviewed 55d492ffac9bd79c7257cb6167ea522e85feefd5 against #3492 and the accepted Coordination Session decision.
The inline comments cover three bounded fixes, including a P1 that currently prevents attachment-bearing delegation from committing. Two broader input/configuration contracts also need to converge before this is ready:
[P1] Keep configuration defaults and explicit choices in the owning Host/draft scope. On a normal path, default Host A uses ask, while the ordinary new-task picker is on Host B with bypass. Opening WorkHub preserves that TaskEntry selection, but WorkHub executes on A. The shell supplies B's permission as a WorkHub default; the Composer sends it as an explicit override, so A creates a bypass Session without a WorkHub permission-change confirmation. A choice made for an ordinary draft can leak the same way.
The local defaults snapshot and model fallback add another divergence (P2): mounting before the catalog arrives can select the first connection's default instead of the configured connection. Could we keep only explicit WorkHub-scoped choices, derive displayed defaults from the Coordination Host through the existing resolver, and omit untouched overrides so that Host inherits its own policy? Please preserve the separate ordinary draft. A two-Host ask/bypass case and delayed-catalog case would protect this boundary.
[P2] Preserve the complete original input through clarification and recovery. The attachment reference currently lives in staging state, an uploaded Map and local submissionContext, while the persisted send lease retains only the action ID/text. After a Host claim followed by failure, leaving and reopening WorkHub loses the original payload; retrying that ID then conflicts with the durable claim. Separately, clarify omits attachments from the persisted message (the protocol rejects them), yet clears staging as a successful send. Returning to the conversation cannot recover those attachments.
Please converge this through the existing Coordination Session input/recovery authority, covering all four dispositions, rather than adding a separate renderer outbox. The useful acceptance chains are “clarify → leave → return → delegate” and “claim → failure → reopen → same-request retry.”
I did not find a new durable Work/Turn execution authority. The existing Coordination Turn-admission follow-up and #4868's queue/Stop proposals remain separate; this feedback does not ask this PR to absorb them.
Validation: exact-head production-path review with Codex, two reviewer agents and one deep reviewer, plus four focused source probes; no full Electron verification. CI currently fails the locale-hygiene guard at workhub-composer.tsx:76. Please also complete the native navigation/retry checks and light/dark wide/narrow visual acceptance, with before/after evidence.
简体中文
谢谢把标准 Composer 和 Work 身份带回 WorkHub。复用现有 UI、让普通 Session 继续拥有执行权威,这个方向是合适的。本次对照 #3492 与已采纳的 Coordination Session 决定评审了上述精确 head。
三条行内意见对应局部修复,其中有一项 P1 会让带附件的委派无法提交。另有两组需要从根因收敛:
[P1] 配置默认值和显式选择必须属于正确的 Host/草稿作用域。 默认 Host A 为 ask,普通新任务选中的 Host B 为 bypass;打开 WorkHub 后仍保留 TaskEntry 选择,但执行发生在 A。Shell 将 B 的权限传给 WorkHub,随后被当作显式 override 提交,A 的新 Session 因而获得 bypass,而且没有经过 WorkHub 的权限修改确认。普通草稿的一次性选择也会泄漏到这里。
本地 defaults 快照和另一套模型 fallback 还带来 P2:目录尚未返回时挂载,之后可能选中第一个连接的默认模型,而不是配置指定的连接。建议只保留明确的 WorkHub 选择,从 Coordination 所属 Host、通过已有 resolver 派生显示默认值;未修改的字段不提交 override,让 Host 继承自己的策略。请保留普通草稿,不通过清空它来规避。回归覆盖双 Host ask/bypass 和异步目录加载。
[P2] 原始输入在澄清与恢复中必须完整保留。 附件引用分散在暂存、uploaded Map 和本地 submissionContext 中,而持久发送凭据只保留 action ID/文字。Host claim 已保存、随后失败时,离开再返回会丢失原 payload,同 ID 重试因此与持久 claim 冲突。另一个分支是 clarify:持久消息没有附件,协议也不接受附件,但界面按成功发送清空暂存,返回后无法恢复。
建议收敛到已有 Coordination Session 的输入与恢复权威,贯通四种 disposition,不另加 Renderer outbox。验收应覆盖“澄清→离开→返回→委派”和“claim→失败→重新打开→原请求重试”。
没有发现新增的持久 Work/Turn 执行权威。既有 Coordination Turn admission 欠账和 #4868 的队列/Stop 后续提议仍单独处理,不要求本 PR 全部承担。
验证范围为 Codex、两位 reviewer 与一位 reviewer_deep 的精确版本生产链审查,以及四个定向源码探针;没有完整 Electron 验收。当前 CI 在 workhub-composer.tsx:76 的 locale hygiene 检查失败。还需完成原生导航/重试和 light/dark、宽窄布局验收,并补前后对比证据。
| const targetAttachments = !durable && input.attachments?.length | ||
| ? await copyWorkHubAttachmentsToTarget(openedArtifactStore, artifacts, input.targetSessionId, input.attachments) | ||
| : input.attachments; | ||
| const content = normalizeMessageContent({ text: input.userText, ...(targetAttachments ? { attachments: targetAttachments } : {}) }); |
There was a problem hiding this comment.
[P1] Extend the atomic assignment contract for attachment content
On the normal delegate_existing/create_new/replace path, this now puts target-owned attachments into admission.content, but SqliteSessionMetadataStore.assignWorkHubMessage still compares that content with normalizeMessageContent({ text: assignment.userText }). messageContentsEqual compares attachments, so every nonempty attachment array causes Invalid WorkHub assignment identity before the transaction commits.
Could we extend the existing store invariant to validate the Coordination-source → target-copy attachment relationship, rather than dropping the identity check? A regression through the real assignment store should verify both the source refs in the Coordination link and target refs in the pending Message. The new copy-helper test does not reach this consumer.
简体中文
[P1] 将附件内容贯通到原子 assignment 契约
正常 delegate_existing/create_new/replace 路径已把目标 Session 的附件放入 admission.content,但真实 SQLite 存储入口仍将它与纯文字内容比较。messageContentsEqual 会比较附件,所以任何非空附件都会在提交前触发 Invalid WorkHub assignment identity。
建议扩展现有存储不变量,校验 Coordination 源附件与 target 复制引用的合法关系,不直接删除身份校验。回归应经过真实 assignment store,同时验证 Coordination link 的源引用和 pending Message 的目标引用;新增复制 helper 测试没有走到这个消费者。
| if (attachment.ref.kind !== 'session_file') throw new Error('Invalid WorkHub attachment'); | ||
| return attachment.ref.relativePath; | ||
| }); | ||
| const copied = await store.copyConversationArtifacts({ |
There was a problem hiding this comment.
[P2] Make the pre-assignment copy replayable
This is called before the assignment transaction and even before the target's reserved check. If copying succeeds and admission then returns session_busy, persistence fails, or the Host exits before assignment, retry still sees no durable assignment and calls this again. The existing copy API derives a deterministic destination ID but rejects an existing destination with Artifact target already exists.
This recovery defect remains after fixing the text-only assignment guard. Please make identical copies converge through the existing Artifact ownership/replay seam, coordinated with target admission/deletion, without accepting mismatched content. A “copy committed → assignment interrupted → restart → same action retry” regression should finish with one assignment.
简体中文
[P2] 让 assignment 之前的附件复制可以重放
复制发生在 assignment 事务之前,甚至早于目标 reserved 状态检查。复制成功后若 admission 返回 session_busy、持久化失败,或 Host 在 assignment 前退出,重试仍读不到 assignment,会再次复制。现有复制 API 使用确定性的目标 ID,却在目标已存在时报 Artifact target already exists。
即使修复纯文字 assignment 校验,这个恢复问题仍然存在。建议通过已有 Artifact 所有权/重放接缝让相同复制收敛,并与目标 admission/删除协调;内容不一致仍须拒绝。回归覆盖“复制已提交→assignment 中断→重启→同 action 重试”,最终只产生一条 assignment。
| : defaultHostConnections.snapshot; | ||
| } | ||
| const activeConnectionSnapshot = activeId | ||
| const activeConnectionSnapshot = workHubActive || activeId |
There was a problem hiding this comment.
[P2] Invalidate the connection projection WorkHub now consumes
On a normal configuration-change path, WorkHub can be open with no ordinary ownerActiveId. It now reads sessionHostConnections, but refreshConnectionProjections and handleConnectionEvent below still skip that projection without an owner. Adding/removing a connection therefore refreshes only the default/new-task projections, leaving WorkHub's model choices stale.
The Coordination lifecycle subscription does not cover this: its availability callback only retries a failed generation, not a healthy resolved one. Please include the active Coordination Host in the existing refresh/event routing. A connection-change regression with WorkHub open and no ordinary active Session should observe the updated choices.
简体中文
[P2] 让失效事件覆盖 WorkHub 当前读取的连接投影
正常配置变更路径中,WorkHub 可以在没有普通 ownerActiveId 时打开。这里改为读取 sessionHostConnections,但下面的 refreshConnectionProjections 和 handleConnectionEvent 仍在没有 owner 时跳过它。新增/删除连接只刷新 default 和 new-task 投影,WorkHub 保留过期模型选项。
Coordination lifecycle 的 availability callback 只重试失败代际,不能刷新健康且已解析的 WorkHub。建议把当前 Coordination Host 纳入已有刷新/事件转发;回归覆盖“WorkHub 打开、无普通活动 Session、连接目录变化”。
09386ff to
61c972b
Compare
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
61c972b to
d2f7b96
Compare
M4n5ter
left a comment
There was a problem hiding this comment.
Codex-assisted review, submitted at M4n5ter's explicit request.
No blocking findings in the reviewed diff: attachment source ownership and target copying, action identity/retry behavior, creation defaults versus existing Work configuration, scoped IPC, and conversation identity/navigation.
Rebased the six commits onto main 9ba2283. Range comparison confirms the business patches are unchanged; only the protocol epoch/history was adjusted to 133. Root build/typecheck/lint/format, Desktop/UI knip, renderer architecture and surface inventory pass. All 167 focused tests and both native Electron WorkHub reconstruction/replacement tests pass on the rebased head.
Remote Host behavior was not exercised locally. Merge remains subject to the required CI check.
Summary
WorkHub now reuses the standard Composer with attachments, model and permission controls, with aligned conversation and Composer layouts on wide and narrow screens. Selecting a Work applies configuration to that Work; without a selection, settings become defaults for new Work. Automatically routed existing Work retains its configuration.
Messages show Project / Work identity with a right-side user rail, left-side assistant rail and colored Work metadata. Hovering a message rail, navigation entry or prompt anchor highlights the same Work across the conversation.
Attachment staging supports file-only sends and preserves uploads on retry. Runtime Host validates source ownership and copies attachments into the target Session. Protocol epoch increases from 132 to 133 for the new wire fields.
Refs #3492
Verification
tests 179 / pass 179 / fail 0; the separate real artifact-store test adds one passing test.9ba2283a5; the six business patches are preserved, with only the protocol epoch and its history adjusted to 133 after main occupied 132. The Storybook runs above predate this rebase.AI use
Tool(s) and scope: OpenAI Codex implemented the changes and validation. Affected commits include
Generated-by: Codex; retain the trailer when squashing.Checklist
Does this PR entail a change in behavior?