feat(desktop): add Work Board Phase 3 start-task spike - #4598
feat(desktop): add Work Board Phase 3 start-task spike#4598somewan820 wants to merge 14 commits into
Conversation
ca71f0c to
4daa66c
Compare
28eadb3 to
be39b1b
Compare
be39b1b to
4a5f222
Compare
github-actions
Bot
added
effort/XL
8857838 to
d057740
Compare
liugddx
left a comment
liugddx
left a comment
There was a problem hiding this comment.
Reviewed exact head d057740da4d4fcf8c54d0f3876d0881683293299.
The target-resolution direction is sound: this reuses Task Entry and the existing Composer first-send path, keeps Work Board out of Runtime/Task Ledger, and correctly limits the spike to project-scoped items. I ran npm run build:test and the focused Work Board/first-send suites (38/38 passed); the current CI test check is also green.
Three blocking correctness issues remain.
P1 — The pending Work Board claim is not bound to the new-task surface
apps/desktop/src/renderer/features/workbar/controller/use-workbar-controller.ts:194-243 stores only { itemId, target }. app-shell.tsx:2143-2146 passes the same onSessionResolved callback to every ordinary new-task first send, and use-workbar-controller.ts:261-284 links whichever projected Session arrives.
A user can start item A, leave that new-task surface, create/send task B, and have B linked to A.
Bind the pending claim to the draft/surface identity and clear it when the target or surface owner changes. The callback must only accept the first send belonging to that claim.
P1 — A failed link cannot be retried against the same Session
use-workbar-controller.ts:261-265 clears the pending claim before the asynchronous workBoard.linkSession() completes. On failure, :287-300 only shows a toast and loses the created Session id.
A transient Host reconnect, SQLite busy error, or IPC failure therefore creates this state:
Session S created and first send succeeded
→ linkSession fails
→ S has no linkedSessions
→ retrying Start task creates Session S2
→ S becomes orphaned
This violates the #2560 requirement that link failure must retry the same Session without creating a duplicate.
Keep { itemId, target, sessionId } as retryable state, or persist a pending-link claim. Retry must call linkSession for S rather than invoke newTasks.create again.
P1 — The Host validator does not verify project ownership
apps/desktop/src/main/runtime-host-boot.ts:927-938 checks only the profile, host incarnation, and whether the Session exists on that Host. work-board-ipc-main.ts:168-176 checks only that the board item is project-scoped.
A Session from project B on the same Host can therefore be linked to an item belonging to project A. The validator receives only the link, so it lacks the item project context required to enforce this invariant.
Pass the canonical board project into the validation path and require the Session's project to match it. Keep the check at the main-process mutation boundary, not only in the renderer.
P2 — linkedSessions is unbounded
packages/core/src/work-board.ts:427-442 and packages/storage/src/work-board-store.ts:274-286 append links without a count or record-size limit. A repeatedly started item can grow indefinitely, and every Work Board list returns the full array.
Restore a clear bound or explicitly define a single-link contract for this spike.
P2 — The development-only flag is not enforced as development-only
use-workbar-controller.ts:172-174 checks only VITE_MAKA_WORK_BOARD_START_TASK. A production Vite build with that variable set would expose the experimental path. Existing linked-session buttons in work-board-panel.tsx:122-129 are also not gated by the flag.
Use import.meta.env.DEV && ... and gate the complete experimental path.
The target resolver and isolated store/contract tests should stay. The missing tests are the real wiring cases: Start A then send B, link failure followed by same-session retry, same-Host cross-project linking, and restart/reopen of a successfully linked Session.
The smallest correct version keeps the current Host target resolution and Composer reuse, but makes the pending claim surface-bound, makes link failure recoverable with the same Session, and validates the Host/project/Session relationship in one main-process authority.
| return; | ||
| } | ||
| input.openNewTaskSurface?.(); | ||
| pendingWorkBoardStartRef.current = { itemId: item.id, target: result.target }; |
There was a problem hiding this comment.
P1: this pending claim is global to the controller, not tied to the new-task surface or draft key. A later ordinary first send can consume the claim and link the wrong Session. Bind the claim to the surface/draft identity and reject callbacks from other sends.
| (sessionId: string) => { | ||
| const pending = pendingWorkBoardStartRef.current; | ||
| if (!pending) return; | ||
| pendingWorkBoardStartRef.current = undefined; |
There was a problem hiding this comment.
P1: clearing the claim before linkSession resolves loses the Session id on any transient link failure, so retry creates a duplicate Session. Retain a retryable { itemId, target, sessionId } claim and retry this same link.
| const current = runtimeHostManager?.current(normalized.value.profileId); | ||
| if (!current?.candidate || current.hostId !== normalized.value.hostId) return false; | ||
| const sessions = await current.candidate.client.listSessions(); | ||
| return sessions.some((session) => session.id === normalized.value.sessionId); |
There was a problem hiding this comment.
P1: existence on this Host is not enough to prove project ownership. The IPC layer currently validates the item only as kind: project, so a Session from project B on this Host can be attached to project A. Pass the canonical item project into this check and compare it with the Session project.
liugddx
left a comment
liugddx
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 4267661e86dc0665f5c32a8a2c38da754ace82c7.
The follow-up addressed most of the previous review: the DEV gate is now enforced, the link is project-checked, same-renderer link failures retain the Session id for retry, and the single-link bound is explicit. Two blocking correctness issues remain.
P1 — draftKey is still not a New Task surface identity
use-workbar-controller.ts:220-225, 371-375 treats draftKey as the identity of the surface that owns the claim, but task-entry-selection.ts:72-75 derives it only from (profileId, hostId, projectId). That is a stable target-scoped draft bucket, not a unique surface instance.
Concrete repro:
Start Work Board item A in project P
→ abandon that New Task surface
→ open a normal New Task again with the same Host/project
→ send unrelated task B
→ both surfaces have the same draftKey
→ B's Session is accepted as A's linked Session
The new wiring test only changes project A to project B, so it misses this same-target case. Use a real per-surface owner nonce/generation and carry it through first-send; do not reuse the target draft key as a surface identity.
P1 — Project validation still has a TOCTOU race
work-board-ipc-main.ts:168-182 reads the item, awaits Host/session validation for its project, and then calls store.linkSession. packages/storage/src/work-board-store.ts:270-292 re-reads and writes the item without checking that the scope is still the one validated.
A concurrent move from project A to project B during listSessions() can therefore write an A Session into the now-B item. Pass the read revision as expectedRevision and make linkSession enforce that CAS inside the write transaction, or move the whole invariant into one main-process mutation authority.
P2 — Retry state is still volatile across renderer restart
pendingWorkBoardStartRef is an in-memory renderer ref (use-workbar-controller.ts:220-225). If the Session is created, linking fails, and the renderer reloads or the app restarts before the user retries, the claim and Session id disappear. Starting the item again creates a second Session and leaves the first orphaned.
#2560 explicitly calls out the return-after-restart loop. Persist a pending-link intent in the main process/storage, or explicitly narrow the documented guarantee to the current renderer lifetime and record restart/reopen as an unresolved spike limitation.
Test/scope notes
The cross-project IPC test uses a mock validator based on sessionId.split('-')[0]; it verifies argument plumbing, not the production-shaped runtime-host-boot.ts project comparison. Add a validator test with a realistic Session workspace projection.
The single-link replacement is bounded, but it discards previous Session history; document that contract in docs/work-board-contract.md. The textarea/composer styling changes remain unrelated to this Phase 3 spike and would be cleaner as a separate change.
Not yours: the current GitHub test check is still running, and the latest commit is CI-only.
The smallest correct version keeps the existing target resolver and Composer reuse, but uses a unique surface owner token, performs link validation with revision-checked main-process commit, and makes the retry durability boundary explicit.
| // and refuse to link the wrong Session. | ||
| if ( | ||
| input.newTaskDraftKey !== undefined && | ||
| input.newTaskDraftKey !== pending.draftKey |
There was a problem hiding this comment.
P1: draftKey identifies the target, not this New Task surface instance. Two different surfaces for the same Host/project share the same key, so an unrelated first send can still consume this Work Board claim. Use a per-surface owner nonce/generation.
| 'Only project-scoped Work Board items can link a Session', | ||
| ); | ||
| } | ||
| if (!(await input.validateLinkedSession(link, item.scope.projectId))) { |
There was a problem hiding this comment.
P1: the item/project is read before an async Host validation, then linkSession re-reads and writes without a revision/scope CAS. A concurrent move can make a Session validated for project A get stored on a project-B item. Pass the read revision and enforce it in the store transaction.
liugddx
left a comment
liugddx
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 07065c5f2768c869e7f16d2a1ea81740c0ba9bf6.
The follow-up closes the two blocking findings from the previous round:
- New Task ownership is now bound to a per-surface nonce incremented by
openNewTaskSurface, not the target-scoped draft key; the same-Host/project reopen regression is covered. - The main-process link path now carries the item revision read before asynchronous Host validation into
store.linkSession, where the write transaction enforces the CAS; the race is covered by a fail-closed test.
The accepted renderer-lifetime limitation for retry durability is now explicit in the Work Board contract, and the project-ownership test uses a production-shaped Session workspace projection. I found no remaining blocking correctness issue in this intentionally thin Phase 3 spike.
APPROVE.
Not yours: GitHub currently reports the branch as conflicting and has not started a fresh CI check. Please rebase onto the current base and wait for the green CI result before merging. The single-link replacement and the existing Work Board composer styling remain intentional/non-blocking follow-up decisions.
liugddx
left a comment
liugddx
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 07065c5f2768c869e7f16d2a1ea81740c0ba9bf6 against the #2560 scope and maintainer constraints.
The scope is now clear and the core path is close to the smallest correct design: project-scoped item → explicit Host/project target resolution → existing New Task + Composer first-send → main-process Host/project validation → revision-CAS link → reopen through the existing Session identity path. It does not introduce Task Ledger writes, Inbox target inference, execution projection, result refs, or a parallel Session-creation flow.
I am changing my review to a non-blocking comment rather than an approval. The remaining points are design/ablation follow-ups, not blockers for this thin validation spike:
- The target resolver, Composer callback, surface owner nonce, Host/project validation, revision CAS, persistent link, and DEV gate are the parts that must stay. Removing any one should fail a concrete #2560 acceptance test (wrong target, wrong Session, cross-project link, race, lost reopen link, or production exposure).
- The Work Board textarea/card styling is presentation-only and can be deleted or split without affecting the start-task loop.
- The single-link replacement policy is bounded and now documented, but it intentionally discards prior Session history; keep it only as an explicit spike policy or revisit it before productising.
- The current tests are useful for the individual contracts, but a later validation pass should still exercise the real panel → AppShell → first-send → link → reopen path rather than only calling controller callbacks and injecting validators.
- Retry after renderer reload/restart is explicitly out of scope for this spike and is documented as a later persistence phase.
For ablation, the useful success criteria are: zero wrong-item links under same-target surface switching, zero cross-project links under concurrent item mutation, zero duplicate Sessions after an in-lifetime link retry, successful reopen of the persisted Host/session link, and no Start-task entry in a production renderer. The safe removal candidates are the UI polish and any compatibility layer proven unnecessary by migration evidence.
Overall: the implementation now matches the approved issue scope and its authority boundaries. The branch still needs to resolve the current merge conflict and run a fresh green CI check before merge.
Superseded by the non-blocking re-review comment on the same head.
07065c5 to
2a26aba
Compare
首发送结果未知时不再触发 onSessionResolved，避免误建 Work Board 持久链接；将 Work Board 编排迁入 workbar 功能，app-shell 仅通过 props 注入；workbar 控制器改用 toastApi 输入以通过主进程 tsconfig 编译；重新生成 renderer-architecture.json。
taskEntryDraftKey 的消费者均从 model/task-entry-selection.js 直接导入，feature index 的 re-export 无外部使用，knip 报未使用导出，删除之。
- work-board-target: 移除未使用的 preferredHost 参数 - use-workbar-controller: 删除 resolveWorkBoardStartTask 包装，透传底层解析 - work-board-panel: 每行只解析一次 start-task 目标 - use-task-entry-controller: 移除 prepareWorkBoardDraft 中的重复查找与校验
- IPC linkSession 增加 scope 校验，仅允许 project 项（防御纵深） - core 解码改为宽容模式：丢弃损坏/重复的链接条目，不再隐藏整条记录 - 补充 ambiguous/archived/scope 校验与宽容解码测试 - 修复 ipc-main 测试缩进，操作行支持换行不溢出
rebase 冲突归并时误将 app-shell-chat-actions.ts 整体回退到 apache#4630 之前的版本， 重新引入了既有会话发送也会触发 onSessionResolved 的回归。恢复为上游 main 的 实现：onSessionResolved 仅在新建会话首次消息投射时触发，Work Board 链接只绑定 到已投射的新会话。
… SessionSummary 导入）
… ledger（rebase 后对齐 apache#4384）
按 review（3 个 P1 + 2 个 P2）修正 Work Board start-task 实现： - P1-1: pending claim 绑定到 new-task surface 的 draftKey；其它 surface 的 首次发送不消费该 claim，避免误链接到错误的 Session - P1-2: 仅当 linkSession 成功后才清除 claim；失败保留 sessionId，同一 item 再次 Start 时重试复用同一 Session，不再创建重复（孤儿）Session - P1-3: linkSession 在 main 进程侧校验 board item 的 project 所有权， Host validator 收到 expectedProjectId 并比对 Session 目标 project - P2-1: linkedSessions 改为单链接契约（WORK_BOARD_MAX_LINKED_SESSIONS=1）， 新链接替换旧链接，不再无限追加；读取路径保持对旧多条目容错 - P2-2: start-task 开关同时要求 DEV 环境与 VITE_MAKA_WORK_BOARD_START_TASK=1， 生产构建即使设置 flag 也不暴露实验路径 测试：workbar-controller 新增 4 个 wiring 场景（surface 绑定、同 surface 链接、失败重试、打开已链接 Session）；work-board-ipc-main 新增 project 所有权校验 2 例；work-board-store 新增单链接契约 1 例。
第二轮 review（5108707232）的 2 个 P1 + 1 个 P2： - P1-1: draftKey 只是 target-scoped draft bucket，不能作为 surface 身份。 改为 app-shell 在每次打开 New Task surface 时递增 owner nonce （newTaskSurfaceNonceRef），claim 绑定该 nonce；同 Host/project 重开 surface 的首次发送不再误消费 claim。 - P1-2: project 校验存在 TOCTOU——linkSession 在异步 Host 校验后重写 item 时不检查 scope 是否仍被校验过。现在 IPC 把校验前读到的 item.revision 作为 expectedRevision 传给 store.linkSession（事务内 CAS，operation_conflict 失败关闭），并发 move 无法写入错误 project。 - P2: retry 状态（pendingWorkBoardStartRef）在 renderer 重启后丢失为 spike 已接受限制——在 docs/work-board-contract.md 明确收窄保证并 记录重启/重开为未解决限制。 测试：workbar-controller 新增同 target 重开 surface 不误链接场景； work-board-ipc-main 的 cross-project 测试改为 production 形状的 Session workspace 投影校验，并新增 revision CAS 失败关闭测试。
The second review round's surface-ownership fix added a useRef to app-shell, tripping the renderer-architecture debt ratchet (hookCalls 102->103, nonTriviaTokens 15620->15636 vs base c59f11d). Track the surface owner nonce as a module-scope counter instead, pass its render-time value into the workbar controller, and have the controller backfill a claim's surfaceNonce on the first render after the claim is created (the open itself already bumped the counter past the value captured at claim time). First-send ownership is then compared against that render-time nonce. app-shell stays at 102 hookCalls / 15620 tokens, exactly the base budget.
The renderer-architecture ratchet also counts browser environment capabilities, so turning window.requestAnimationFrame into a bare requestAnimationFrame call in openNewTaskSurface pushed the bare-global count above base. Restore the window. prefix and instead reclaim the same two tokens (and more) by destructuring resolveWorkBoardTarget / prepareWorkBoardDraft from taskEntry.commands alongside selectLocalProject, the established pattern on the same line. app-shell drops to 15614 tokens (<= base 15620) with an unchanged capability fingerprint.
2f417c9 to
05635d6
Compare
2 participants
Summary
Implements the thin Work Board Phase 3 validation spike for #2560.
Start taskpath for project-scoped Work Board items whose target resolves to one available, non-archived Host/project target.linkedSessionsreference only after a projected first-send outcome.The experiment is enabled only in a development renderer with:
This is intentionally not a persisted user setting. The goal is maintainer/contributor dogfooding before deciding whether to productize the entry and continue later Work Board phases.
Verification
npm test: 1580 passed, 12 skipped; runtime-host and other relevant suites passed.npm --workspace @maka/desktop run build:main: passed.git diff --check: passed.The storage workspace test run remains affected by the existing sandbox
EPERM chmodrestriction under~/Library/Application Support/Maka/~/Library/Caches/Maka; this is environmental and unrelated to the changed Work Board behavior.Generated-by: Codex
AI use
Codex assisted with implementation, review, and test execution. The contributor reviewed the resulting changes and owns the final PR.
Review follow-up
Addressed the review comments (3 P1 + 2 P2):
draftKey). A first send from any other surface no longer consumes the claim, so it can never link the wrong Session.linkSessionsucceeds. On failure the Session id is retained, so pressing Start on the same item retries the same Session instead of creating a duplicate.workBoard:linkSessionnow resolves the board item's project and passes it to the Host validator, which requires the Session's workspace target to be a project matching that id. Cross-project links on the same Host are rejected.linkedSessionsis now a single-link contract (WORK_BOARD_MAX_LINKED_SESSIONS = 1): a freshly started Session replaces any previous link, so the array stays bounded. Read paths remain tolerant of legacy multi-entry arrays.Start taskexperiment now requiresimport.meta.env.DEV === truein addition toVITE_MAKA_WORK_BOARD_START_TASK=1, so it is unreachable in production builds.Tests added: 4 wiring scenarios in
workbar-controller.test.ts(surface-bound claim, same-surface link, failed-link retry reusing the Session, opening a linked Session), 2 project-ownership cases inwork-board-ipc-main.test.ts, and a single-link contract case inwork-board-store.test.ts. Desktop main-process suite: 2074 passed / 0 failed; typecheck, biome, and both architecture checks pass.Second review follow-up
Addressed the second review round (2 P1 + 1 P2):
draftKeyis a target-scoped draft bucket, not a surface identity. The app now assigns each freshly opened New Task surface a monotonic owner nonce (newTaskSurfaceNonceRef); the pending claim binds to that nonce. A New Task reopened on the same Host/project (identical draft key) is a distinct surface and can no longer consume another claim's Session. Added a same-target regression test.workBoard:linkSessionnow passes theitem.revisionread before Host validation asexpectedRevisiontostore.linkSession, which enforces the CAS inside its write transaction (operation_conflicton mismatch). A concurrent mutation duringlistSessions()can no longer write a Session validated for one project into an item that has moved to another. Added a revision-CAS fail-closed test; the cross-project test now uses a production-shaped Session workspace projection.docs/work-board-contract.md.Desktop main-process suite: 2076 passed / 0 failed; typecheck, biome, and architecture checks pass.