feat(workbench): make Todos the durable project work anchor - #11
Conversation
Make Todos the durable intent anchor while preserving Sessions and Automations as first-class execution inventories. Add global Home and work search, project-level navigation, composed Run now, batched inventories, and derived Todo work status. Hard-cut root Session source and Automation origin contracts and remove the legacy Dashboard and Sidebar implementation. BREAKING CHANGE: Historical project runtime records using the removed Session and Automation source fields are no longer accepted. Delete old project runtime data before launch; no migration or fallback is provided.
Replace persisted title and body fields with one Markdown content field, derive compact display labels, and add an independent Todo detail workspace. BREAKING CHANGE: Historical Todo runtime data using title and body is unsupported and must be deleted before launch.
Persist Todo-owned references and project them into Todo-origin Sessions, Automations, and tools. Add native PDF text reading and the References UI. BREAKING CHANGE: Todo and Automation source schemas now require attachmentIds and todoId, and attachments use .archcode/runtime/attachments without legacy path compatibility.
Project canonical Session sources and current Todo/Plan facts at each model boundary. Clarify planning before workspace inspection and treat missing glob search roots as empty results.
Keep mobile workbench controls visible and clarify status and empty-state copy.\n\nCoalesce text and reasoning SSE deltas into bounded Session store updates while preserving event order and recovery.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesProject Workbench
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
This PR has 17,381 reviewable changed lines after ignored/generated files are excluded, above this repository's 10,000-changed-line automatic review limit. The raw diff is 21,349 lines before ignored/generated files are excluded. Most of the diff comes from:
Comment |
|
@cubic-dev-ai review this |
@boh5 I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 252 files
Note: This PR contains a large number of files. cubic only reviews up to 200 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/features/ChatInput.tsx (1)
89-99: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset the focus guard for each session. The session route reuses
SessionRoute, andSessionComposerDockhas no session key. If the dock stays mounted,ChatInputretainsfocusOnReadyAppliedRef.current === true, so a later session withfocusComposerenabled will not focus whencanComposebecomes true. Key the dock byrootSessionIdor reset the guard whensessionIdchanges.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/features/ChatInput.tsx` around lines 89 - 99, Reset focusOnReadyAppliedRef when the active session changes in ChatInput, using the sessionId prop as the dependency, so a reused composer can focus once when canCompose becomes true for each session. Preserve the existing focus guard behavior within a session.
🟡 Minor comments (15)
packages/agent-core/src/todos/attachments.ts-117-131 (1)
117-131: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThe compensation path can throw and hide the activation error.
Line 118 calls
readTodoinside thecatchblock. If the Todo disappeared meanwhile, for example through the run-now compensation delete,readTodothrowsProjectTodoNotFoundError. That error replaces the real activation failure, and the uploaded object is never cleaned up. Treat a failed re-read as "not activated" and continue with cleanup.🛡️ Proposed fix
- const attachmentWasActivated = (await this.#state.readTodo(input.todoId)) - .attachmentIds.includes(input.attachmentId); + const attachmentWasActivated = await this.#state.readTodo(input.todoId) + .then((todo) => todo.attachmentIds.includes(input.attachmentId)) + .catch(() => false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/todos/attachments.ts` around lines 117 - 131, Update the catch path in the attachment activation method around readTodo so a failed re-read, including ProjectTodoNotFoundError, is treated as not activated rather than escaping. Preserve cleanup of the uploaded unreferenced object and rethrow the original activation error, while still aggregating any cleanup failure with it.packages/agent-core/src/todos/state-manager.ts-166-208 (1)
166-208: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win为附件引用变更加上
assertMutable。 已归档 Todo 仅允许通过archived: false恢复；当前两个方法仍可修改attachmentIds，因此附件上传和删除会绕过该约束。增加已归档 Todo 的回归测试。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/todos/state-manager.ts` around lines 166 - 208, 在 addAttachmentReference 和 removeAttachmentReference 的变更回调中，在修改 attachmentIds 前调用现有的 assertMutable(todo) 校验，使已归档 Todo 只能通过 archived: false 恢复后再变更附件引用；补充覆盖两个方法对已归档 Todo 拒绝修改的回归测试。packages/agent-core/src/attachments/service.ts-258-263 (1)
258-263: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd
todoowner cleanup to run-now compensation.#compensateRunNowremoves the Todo without callingremoveOwner, so itstodos/<todoId>directory can remain on disk. Add cleanup and test directory removal. Archive is not deletion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/attachments/service.ts` around lines 258 - 263, The `#compensateRunNow` flow must invoke AttachmentService.removeOwner for the removed Todo so its todos/<todoId> directory is deleted during run-now compensation. Add the cleanup using the existing Todo owner representation, and add or update a test to verify the directory is removed; do not treat archive behavior as deletion.packages/agent-core/src/attachments/read-paths.ts-21-21 (1)
21-21: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win将新增注释改为中文。
Line 21、Line 54 和 Line 59 的新增注释为英文。代码标识符和 API 名称可以保持英文。
As per coding guidelines, "
**/*: Speak Chinese while writing code in English, including comments."Also applies to: 54-55, 58-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/attachments/read-paths.ts` at line 21, 将 read-paths.ts 中新增的英文注释改为中文，包括第 21、54-55 和 58-59 行附近的注释；保留代码标识符、API 名称及注释含义不变。Source: Coding guidelines
apps/web/src/components/features/TodoReferences.tsx-160-163 (1)
160-163: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDerive the size message from the limit constant.
The message hardcodes "50 MiB". If
MAX_ATTACHMENT_SIZE_BYTESchanges, the text becomes wrong.formatAttachmentSizeis already imported.♻️ Proposed fix
if (file.size > MAX_ATTACHMENT_SIZE_BYTES) { - setNotice("Files larger than 50 MiB cannot be added."); + setNotice(`Files larger than ${formatAttachmentSize(MAX_ATTACHMENT_SIZE_BYTES)} cannot be added.`); continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/features/TodoReferences.tsx` around lines 160 - 163, Update the size-limit notice in the file validation flow to derive its displayed limit from MAX_ATTACHMENT_SIZE_BYTES using the existing formatAttachmentSize helper, instead of hardcoding “50 MiB”.packages/agent-core/src/main.test.ts-2014-2054 (1)
2014-2054: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winShut down the runtime in this test.
This test creates a runtime at Line 2016 but never calls
runtime.shutdown(). The neighbouring tests at Lines 2056-2142 and 2144-2206 both shut the runtime down in afinallyblock. A leaked runtime keeps timers, the fake MCP manager, and the persistence queue alive, which can hang the suite or leak state into later tests.🔒️ Proposed fix
- expect(todo.attachmentIds).toEqual([attachmentId]); - const opened = await todos.openAttachment({ todoId: todo.id, attachmentId }); - expect(await Bun.file(opened.contentPath).text()).toBe("durable Todo reference"); - }); + try { + expect(todo.attachmentIds).toEqual([attachmentId]); + const opened = await todos.openAttachment({ todoId: todo.id, attachmentId }); + expect(await Bun.file(opened.contentPath).text()).toBe("durable Todo reference"); + } finally { + await runtime.shutdown(); + } + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/main.test.ts` around lines 2014 - 2054, Update the test around the runtime created by createRuntime so its session, Todo, and attachment assertions execute within a try/finally block, and call runtime.shutdown() in the finally block. Preserve the existing test behavior while ensuring shutdown runs even when an assertion or operation fails.packages/agent-core/src/tools/builtins/background-output.test.ts-63-69 (1)
63-69: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCreate a valid child identity in
child(). A child cannot havesourceor the root-only"lead"agent name. PassparentSessionIdandrootSessionIdtocreate, use a child agent name such as"explore", and omitsource.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/tools/builtins/background-output.test.ts` around lines 63 - 69, Update the child() helper to create a valid child identity by passing parentSessionId and rootSessionId in the create call, using a child agent name such as "explore", and omitting the source field. Remove the subsequent identity-setting calls that become redundant after create receives these values.AGENTS.md-114-118 (1)
114-118: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win将 Global Home 原型重命名为
home.html。Line 114 仍引用
design-system/prototypes/dashboard.html。Dashboard 已替换为 Home。该引用保留了旧页面身份，并且不符合当前页面原型命名规则。将原型重命名为design-system/prototypes/home.html，并同步更新所有引用。As per coding guidelines,
design-system/prototypes/*.htmlrequires one prototype nameddesign-system/prototypes/<page>.html.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` around lines 114 - 118, 将 AGENTS.md 中 Global Home 原型的引用从 design-system/prototypes/dashboard.html 更新为 design-system/prototypes/home.html，并同步检查和更新仓库内所有指向旧 dashboard.html 路径的引用，确保原型文件使用 home.html 命名。Source: Coding guidelines
apps/web/src/routes/root-layout.interaction.tsx-77-100 (1)
77-100: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a focused regression test for
hitl-live-toast
No test covers thehitlNoticesrendering path. Add a test that supplies a HITL notice and asserts the toast content.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/routes/root-layout.interaction.tsx` around lines 77 - 100, Add a focused test in the RootLayout global shell suite covering the hitlNotices rendering path: provide a HITL notice through the same input or context consumed by RootLayout, render the layout, and assert that the hitl-live-toast element displays the notice content.apps/web/src/routes/automation-detail.tsx-40-40 (1)
40-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRoute path segments are interpolated without
encodeURIComponent. Three new navigation targets build URLs fromslug,automation.id, andsession.sessionIdwithout encoding, while neighbouring code in the same cohort does encode them (automation-detail.tsxlines 98, 105, and 266). A value that contains#,?, or%produces a broken link or a wrong route match.
apps/web/src/routes/automation-detail.tsx#L40-L40: wrapsluginencodeURIComponentwhen buildingautomationsHref; this value also serves as the post-delete navigation target at line 73.apps/web/src/routes/automations.tsx#L157-L157: wrapslugandautomation.idinencodeURIComponentin the detailtoprop.apps/web/src/routes/project-sessions.tsx#L222-L222: wrapslugandsession.sessionIdinencodeURIComponentin the Sessiontoprop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/routes/automation-detail.tsx` at line 40, Encode all interpolated route path segments before constructing navigation URLs: in apps/web/src/routes/automation-detail.tsx lines 40-40, encode slug in automationsHref; in apps/web/src/routes/automations.tsx lines 157-157, encode slug and automation.id in the detail to prop; and in apps/web/src/routes/project-sessions.tsx lines 222-222, encode slug and session.sessionId in the Session to prop. The post-delete navigation using automationsHref requires no separate change.design-system/prototypes/todos.html-1476-1486 (1)
1476-1486: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHeadings render with their literal
#markers.
contentBodyHtmlsplits content on blank lines, then treats a block as a heading only when the block has exactly one line. Every seeded sample puts the heading and its body in the same block. For example lines 1297-1298 contain## Problemfollowed immediately by the paragraph, so the block falls through to line 1484 and renders<p>## Problem<br>Projects often prefer…</p>. The same happens for## Expected behaviorat lines 1300-1304 and## Acceptanceat lines 1306-1309, where the bullet and ordered-list branches also fail because the first line is not a list item.The result is that the Brief/PRD surface shows raw Markdown markers for all six samples.
🐛 Proposed fix: peel a leading heading off each block
function contentBodyHtml(content) { const blocks = (content || "").split(/\n\s*\n/).filter(Boolean); return blocks.map((block) => { const lines = block.split(/\r?\n/).map((line) => line.trim()).filter(Boolean); - const heading = lines[0]?.match(/^#{1,6}\s+(.+)$/); - if (heading && lines.length === 1) return `<h3>${escapeHtml(heading[1])}</h3>`; - if (lines.every((line) => /^[-+*]\s+/.test(line))) return `<ul>${lines.map((line) => `<li>${escapeHtml(line.replace(/^[-+*]\s+/, ""))}</li>`).join("")}</ul>`; - if (lines.every((line) => /^\d+[.)]\s+/.test(line))) return `<ol>${lines.map((line) => `<li>${escapeHtml(line.replace(/^\d+[.)]\s+/, ""))}</li>`).join("")}</ol>`; - return `<p>${lines.map(escapeHtml).join("<br>")}</p>`; + const heading = lines[0]?.match(/^#{1,6}\s+(.+)$/); + const headingHtml = heading ? `<h3>${escapeHtml(heading[1])}</h3>` : ""; + const body = heading ? lines.slice(1) : lines; + if (body.length === 0) return headingHtml; + if (body.every((line) => /^[-+*]\s+/.test(line))) return `${headingHtml}<ul>${body.map((line) => `<li>${escapeHtml(line.replace(/^[-+*]\s+/, ""))}</li>`).join("")}</ul>`; + if (body.every((line) => /^\d+[.)]\s+/.test(line))) return `${headingHtml}<ol>${body.map((line) => `<li>${escapeHtml(line.replace(/^\d+[.)]\s+/, ""))}</li>`).join("")}</ol>`; + return `${headingHtml}<p>${body.map(escapeHtml).join("<br>")}</p>`; }).join(""); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@design-system/prototypes/todos.html` around lines 1476 - 1486, Update contentBodyHtml to recognize and render a leading Markdown heading even when the block contains following body or list lines. Peel the first heading line from each block, render it as an h3, then process the remaining lines through the existing unordered-list, ordered-list, or paragraph branches so heading markers are not emitted literally.apps/web/src/routes/automation-detail.tsx-86-100 (1)
86-100: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShow a loading state for the linked Todo.
todos.dataisundefinedwhileuseProjectTodosloads. Line 99 then renders${todoId} · unavailablefor a Todo that exists. The Session branch at lines 102-110 handlesisLoading; the Todo branch does not.🐛 Proposed fix
- <Link className="text-brand hover:underline" to={`/projects/${encodeURIComponent(slug)}/todos/${encodeURIComponent(automation.origin.todoId)}`}> - Todo · {linkedTodo === undefined ? `${automation.origin.todoId} · unavailable` : projectTodoContentExcerpt(linkedTodo.content)} - </Link> + <Link className="text-brand hover:underline" to={`/projects/${encodeURIComponent(slug)}/todos/${encodeURIComponent(automation.origin.todoId)}`}> + Todo · {todos.isLoading + ? "loading…" + : linkedTodo === undefined + ? `${automation.origin.todoId} · unavailable` + : projectTodoContentExcerpt(linkedTodo.content)} + </Link>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/routes/automation-detail.tsx` around lines 86 - 100, Update the Todo rendering in the automation detail component to handle the loading state from useProjectTodos before treating linkedTodo as unavailable. Use the hook’s isLoading state to show the established loading UI while todos are loading, then retain the existing excerpt for a found Todo and “unavailable” fallback only after loading completes.apps/web/src/api/mutations.ts-129-131 (1)
129-131: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign the session query test with the inventory shape. The endpoint returns
ProjectSessionInventoryItem[], andqueryKeys.sessionsuses nestedsession.sessionId. No production writer storesSessionSummary[]under this key. Updateapps/web/src/api/queries.test.tsand add a deletion-cache test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/api/mutations.ts` around lines 129 - 131, Update the session query tests in queries.test.ts to use the ProjectSessionInventoryItem shape and nested session.sessionId values expected by queryKeys.sessions, rather than SessionSummary entries. Add a deletion-cache test covering the mutation’s setQueryData filtering behavior and confirming the deleted root session is removed while other inventory items remain.design-system/prototypes/todos.html-1465-1474 (1)
1465-1474: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win让
contentExcerpt与projectTodoContentExcerpt使用相同的截断规则。 原型按 UTF-16 的length/slice截断，协议 helper 按 Unicode code point 计数。包含😀的内容可能被错误截断并保留孤立 surrogate，导致文本不一致。按 code point 截断，并增加 Unicode 回归用例。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@design-system/prototypes/todos.html` around lines 1465 - 1474, Update contentExcerpt to truncate by Unicode code points, matching projectTodoContentExcerpt instead of using UTF-16 length/slice semantics; preserve the 80-character limit and ellipsis behavior while preventing isolated surrogates. Add a regression case covering content containing 😀 and verify both excerpt helpers produce identical results.apps/web/src/routes/project-todos.tsx-235-263 (1)
235-263: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEncode URL segments in the Run now success navigation.
Line 250 builds the destination URL with raw
slugandsession.sessionId, unlike every other navigation in this file (lines 202, 229, 365-366), which useencodeURIComponent. IfslugorsessionIdcontain characters that need encoding, this produces an inconsistent or broken route. No test inproject-todos.test.tsxexercises this success path's navigation target, so this gap is not caught.🐛 Proposed fix
- navigate(`/projects/${slug}/sessions/${session.sessionId}`); + navigate(`/projects/${encodeURIComponent(slug)}/sessions/${encodeURIComponent(session.sessionId)}`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/routes/project-todos.tsx` around lines 235 - 263, Update the Run now success navigation in the run function to apply encodeURIComponent to both slug and session.sessionId when constructing the project session route, matching the encoding used by the other navigation paths in this file.
🧹 Nitpick comments (21)
packages/agent-core/src/todos/state-manager.ts (1)
94-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
createRunNowTododuplicatescreateTodoexcept for one status value.Consider one private builder that takes the initial status, so future Todo fields cannot drift between the two creation paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/todos/state-manager.ts` around lines 94 - 110, Refactor the todo creation flow so createRunNowTodo and createTodo share one private builder that accepts the initial status, keeping all field initialization in that builder and passing "in_progress" for createRunNowTodo while preserving createTodo’s existing status.packages/agent-core/src/todos/state-manager.test.ts (1)
206-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive attachment fixtures and revisions from
MAX_ATTACHMENTS_PER_TODO. This keeps the limit and revision assertions valid when the protocol cap changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/todos/state-manager.test.ts` around lines 206 - 217, Update the attachment-limit test around manager.addAttachmentReference to derive the fixture count from MAX_ATTACHMENTS_PER_TODO instead of hardcoding 10, and derive the expected todo.revision from that count plus the initial revision. Keep the subsequent over-limit mutation assertion intact.packages/protocol/src/project-todos.ts (1)
32-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
attachmentIdsa readonly array.
readonly attachmentIds: string[]protects the property but not the array contents. Consumers can mutate a shared Todo projection. Other collection fields in this package usereadonly T[].♻️ Proposed change
- readonly attachmentIds: string[]; + readonly attachmentIds: readonly string[];Note:
packages/agent-core/src/todos/state-manager.tspushes totodo.attachmentIdson its own mutable draft, so verify that this narrowing does not break that path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/protocol/src/project-todos.ts` around lines 32 - 42, Update the ProjectTodo interface’s attachmentIds property to use a readonly string array, and adjust the state-manager draft path around its attachmentIds push so mutation occurs through an appropriately mutable draft type without weakening the public ProjectTodo contract.packages/protocol/src/workbench.ts (1)
45-54: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
status: stringweakens the Home contract.The producer emits a fixed set of values: HITL kinds, goal statuses, terminal execution statuses, family activities,
"ready_to_review", and"scheduled". Astringtype lets the server and the web surface drift without a compile error. Consider a union type perkind, or at least a named status union.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/protocol/src/workbench.ts` around lines 45 - 54, Strengthen the HomeSummaryItem status contract by replacing the broad status string with a named union covering the producer’s emitted HITL, goal, terminal execution, family activity, “ready_to_review”, and “scheduled” values. Apply the appropriate status narrowing for each kind while preserving the existing HomeSummaryItem shape and producer behavior.apps/server/src/routes/todos.test.ts (1)
22-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
__test_tmp__/for temporary directories.The suite creates roots under the OS temp directory. The coding guidelines require
__test_tmp__/for temporary directories, cleaned inafterAll. Cleanup is already present; only the base path needs to change.♻️ Proposed change
- workspaceRoot = await mkdtemp(join(tmpdir(), "archcode-todos-route-")); + await mkdir("__test_tmp__", { recursive: true }); + workspaceRoot = await mkdtemp(join("__test_tmp__", "archcode-todos-route-"));As per coding guidelines: "Colocate tests as
<name>.test.ts; use__test_tmp__/for temporary directories and clean them inafterAll."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/routes/todos.test.ts` around lines 22 - 29, Update the temporary-directory base used in the beforeEach setup around workspaceRoot and mkdtemp to use the repository’s __test_tmp__/ directory instead of the OS tmpdir(); preserve the existing per-test directory creation and afterAll cleanup via roots.Source: Coding guidelines
packages/agent-core/src/automations/state-manager.ts (1)
321-331: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate
originwith a Zod schema.
validateAutomationOriginhand-rolls the discriminated union. An unknownkindfalls into thetodobranch and fails with a misleadingorigin.todoId must be a UUIDmessage. Az.discriminatedUnionwith.strict()object members would reject unknown variants and extra keys directly, and would match how the rest of this file validates input.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/automations/state-manager.ts` around lines 321 - 331, Replace the hand-written branching in validateAutomationOrigin with a Zod discriminated union keyed by kind, using strict object schemas for direct, session, and todo variants and UUID validation for the relevant IDs. Parse the origin through that schema so unknown kinds and extra keys are rejected directly, while preserving the AutomationOrigin return shape and existing validation behavior for valid inputs.Source: Coding guidelines
packages/agent-core/src/todos/service.ts (1)
318-332: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the recovery cases that leave orphaned entities.
ProjectTodoServiceOptionsaccepts alogger, but the service does not use it. When compensation fails, or when durable acceptance cannot be determined, a Todo and possibly a Session stay behind. Only the caller sees the error. Add a log record withtodoIdandsessionIdat the throw sites so operators can find the orphans without a client stack trace.♻️ Retain the logger and record the recovery case
readonly `#attachments`: ProjectTodoAttachmentService; + readonly `#logger`?: Logger;async `#compensateRunNow`(todoId: string, sessionId: string | undefined, cause: unknown): Promise<void> { try { if (sessionId !== undefined) { await this.#sessions.deleteSession({ workspaceRoot: this.workspaceRoot, sessionId }); } await this.#state.deleteRunNowTodo(todoId); } catch (error) { + this.#logger?.error("Run now compensation failed", { todoId, sessionId }); throw new ProjectTodoRunNowRecoveryError(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/todos/service.ts` around lines 318 - 332, Use the configured logger from ProjectTodoServiceOptions in ProjectTodoService and record recovery failures at both the `#compensateRunNow` throw path and the durable-acceptance-undetermined throw path. Include todoId and sessionId in each log record, then preserve the existing error propagation unchanged.packages/agent-core/src/store/store.test.ts (1)
35-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the new
createSessionStorehelper.The helper at lines 35-40 already encodes
{ agentName: "lead", source: { kind: "direct" } }. Most call sites still repeat that literal. Route the same-workspace cases through the helper and keep explicit options only where the workspace root or agent differs.Also applies to: 402-431
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/store/store.test.ts` around lines 35 - 41, Update the same-workspace call sites in the test, including the occurrences around lines 402-431, to use the existing createSessionStore helper instead of repeating the lead/direct options literal. Retain explicit storeManager.create options only when the workspace root or agent differs.packages/agent-core/src/runtime.ts (1)
789-837: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the top-level
AttachmentDescriptorimport.Lines 818 and 824 use inline
import("@archcode/protocol").AttachmentDescriptor. The file already imports protocol types at the top. AddAttachmentDescriptorthere and drop the inline forms.Separately,
deleteSessionat line 794 closes overexecutionManager, whichlet executionManager!declares at line 838. The closure only runs after assignment today, so there is no temporal dead zone hit. Keep that ordering invariant in mind ifcontextResolveris ever resolved during construction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/runtime.ts` around lines 789 - 837, Update the top-level protocol import to include AttachmentDescriptor, then replace both inline import("`@archcode/protocol`").AttachmentDescriptor annotations in resolveCurrentTodoAttachments with the imported type. Preserve the existing executionManager initialization ordering around deleteSession and contextResolver construction.apps/server/src/routes/todos.ts (1)
288-337: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm the plan path guard covers the intended threat model.
The
lstatcheck, therealpathcomparison, and the secondhandle.stat()check are layered correctly. Two notes:
- After
relativeCandidate !== \${todoId}.md`returns false, thestartsWith("..")andisAbsolute` checks are unreachable.join(".archcode", "plans")produces a platform separator inplan.path, which the Web client receives verbatim.Neither breaks the current flow. Confirm the client tolerates the returned
pathvalue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/routes/todos.ts` around lines 288 - 337, Review the client consumers of readTodoPlan’s returned path and confirm they tolerate the platform-specific separator produced by join(".archcode", "plans"). If the web client expects URL-style paths, normalize path before returning it; otherwise preserve the current value and leave the existing layered path checks unchanged.packages/agent-core/src/store/helpers.ts (1)
1124-1137: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCentralize
ProjectTodoSessionEntryvalues.Export a runtime tuple from
packages/protocol/src/project-todos.tsand use it forz.enum(...)here and inProjectTodoSessionEntrySchema. Duplicate lists can reject persisted sessions when the protocol union changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/store/helpers.ts` around lines 1124 - 1137, Export a runtime tuple containing the ProjectTodoSessionEntry values from project-todos.ts, then reuse it in the z.enum(...) for the todo source schema and in ProjectTodoSessionEntrySchema. Remove the duplicated literal lists while preserving the existing discussion, work, and automation values and inferred types.packages/agent-core/src/tools/builtins/lsp/lsp-diagnostics.test.ts (1)
13-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated durable-session helper.
createDurableTestSessionContextis duplicated in all four LSP unit tests. Move it to a shared test utility and retain an optionalcwdparameter forlsp-diagnostics.test.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/tools/builtins/lsp/lsp-diagnostics.test.ts` around lines 13 - 37, Extract createDurableTestSessionContext into a shared test utility used by all four LSP unit tests, preserving its existing session/store/projectContext setup. Update lsp-diagnostics.test.ts and the other test files to import the shared helper, and retain the optional cwd parameter so callers can override the workspace root.Source: Path instructions
packages/agent-core/src/tools/builtins/lsp/lsp-find-references.test.ts (1)
17-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
createDurableTestSessionContexthelper across two LSP test files. Both files define the identical 15-line helper that creates a session store withagentName: "lead"andsource: { kind: "direct" }, then flushes it. The shared root cause is the removal of a previously shared durable-session test helper, now re-inlined independently in each file.
packages/agent-core/src/tools/builtins/lsp/lsp-find-references.test.ts#L17-L31: extract this helper into a shared test utility (for example alongsidecreateTestProjectContext) and import it here.packages/agent-core/src/tools/builtins/lsp/lsp-goto-definition.test.ts#L17-L31: import the same shared helper instead of keeping a second copy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/tools/builtins/lsp/lsp-find-references.test.ts` around lines 17 - 31, Extract the duplicated createDurableTestSessionContext helper from packages/agent-core/src/tools/builtins/lsp/lsp-find-references.test.ts lines 17-31 into a shared test utility alongside createTestProjectContext, then import and use it there. Apply the same replacement in packages/agent-core/src/tools/builtins/lsp/lsp-goto-definition.test.ts lines 17-31, removing its local copy.design-system/prototypes/app.js (1)
180-182: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueHand-rolled HTML escaping feeds
innerHTMLin the search dialog and attention inbox.
escapePrototypeHtml(Line 180) escapes&,<,>,',", and every interpolated value passed into theinnerHTMLassignments at Lines 283-296, 327-328, and 415-427 goes through it, so the flagged pattern does not appear exploitable as written. A vetted encoder is still preferable to a hand-rolled one to guard against future call sites that forget to escape.Since this file is a design prototype and the data source is either static records or the user's own
localStorage, this is optional and low priority.Also applies to: 283-296, 312-329, 413-429
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@design-system/prototypes/app.js` around lines 180 - 182, Replace the hand-rolled escaping in escapePrototypeHtml with the project’s vetted HTML encoder, preserving escaped output for every interpolated value used by the listed innerHTML assignments. Reuse the existing encoder if one is already available rather than adding another implementation.Sources: Learnings, Linters/SAST tools
packages/agent-core/src/tools/builtins/lsp/lsp-symbols.test.ts (1)
16-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated test-session helper.
createDurableTestSessionContextis duplicated in the symbol, goto-definition, and find-references tests. Move the shared logic to a common test utility. Support the diagnostics test’s additionalcwdparameter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/tools/builtins/lsp/lsp-symbols.test.ts` around lines 16 - 30, Extract createDurableTestSessionContext into a shared test utility reused by the symbol, goto-definition, and find-references tests, preserving its existing session setup and returned context. Extend the shared helper to accept and apply the diagnostics test’s additional cwd parameter, then remove the duplicated local implementations and update all callers.packages/agent-core/src/tools/builtins/pdf-read.test.ts (1)
196-227: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing test for the oversized-file rejection path.
No test in this file exercises
TOOL_PDF_TOO_LARGE(thefile.size > MAX_ATTACHMENT_SIZE_BYTESbranch in pdf-read.ts). Every other classified error path (TOOL_PDF_NOT_PDF,TOOL_PDF_CORRUPT,TOOL_PDF_PASSWORD_REQUIRED,TOOL_PDF_NO_TEXT,TOOL_FILE_NOT_FOUND,TOOL_PDF_PAGE_OUT_OF_RANGE) has coverage. Add a test that writes a file larger thanMAX_ATTACHMENT_SIZE_BYTES(or a sparse/truncated file with a size check) and asserts theTOOL_PDF_TOO_LARGEcode.Also applies to: 254-261
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/tools/builtins/pdf-read.test.ts` around lines 196 - 227, Add a test alongside the existing classified PDF error tests that creates a file whose size exceeds MAX_ATTACHMENT_SIZE_BYTES, invokes pdfReadTool.execute, and verifies the result is an error with code TOOL_PDF_TOO_LARGE. Use the existing workspace/write helpers and test context patterns without changing the other error-path coverage.design-system/prototypes/todos.html (1)
1461-1463: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCoerce the value before escaping.
escapeHtmlcallsvalue.replace(...)directly. The sibling prototypes useString(value).replace(...):automations.htmlline 760,sessions.htmlline 383, andsession.htmlline 1838. A non-string reference field throws aTypeErrorhere.♻️ Proposed change
function escapeHtml(value) { - return value.replace(/[&<>'"]/g, (character) => ({ "&": "&", "<": "<", ">": ">", "'": "&`#39`;", '"': """ })[character]); + return String(value).replace(/[&<>'"]/g, (character) => ({ "&": "&", "<": "<", ">": ">", "'": "&`#39`;", '"': """ })[character]); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@design-system/prototypes/todos.html` around lines 1461 - 1463, Update escapeHtml to coerce value to a string before invoking replace, matching the sibling prototype implementations and ensuring non-string reference fields do not throw.design-system/prototypes/sessions.html (2)
451-451: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAlign the truncation cap for the shared session store.
This page keeps 12 records under
archcode-prototype-sessions.design-system/prototypes/automations.htmlline 920 keeps 6, anddesign-system/prototypes/todos.htmlline 1512 keeps 6. The three pages write the same key, so a session created here disappears after a write from another page. Use one shared cap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@design-system/prototypes/sessions.html` at line 451, Update the records truncation in the sessions page’s localStorage write to use the shared six-record cap, matching the limits used by the automations and todos pages for the archcode-prototype-sessions key.
355-357: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDistinguish a true empty inventory from a filtered no-results state.
The single empty state always says "No Sessions match this filter." The implementation contract separates the two cases:
apps/web/src/routes/project-sessions.tsxlines 68-72 return "No Sessions yet. Start one directly or run work from a Todo." for a zero-total inventory, andapps/web/src/routes/inventory-classification.test.tsline 27 asserts that text. Add the true-empty copy so the prototype matches the specified behavior.Also applies to: 438-439
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@design-system/prototypes/sessions.html` around lines 355 - 357, Update the sessions empty-state markup identified by data-sessions-empty to distinguish zero-total inventory from filtered no-results: add the specified “No Sessions yet. Start one directly or run work from a Todo.” copy for the true-empty case, while retaining “No Sessions match this filter.” for filtered results. Ensure the prototype’s state handling exposes the appropriate message in each case.design-system/prototypes/styles.css (1)
111-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the now-duplicated per-page
[hidden]rules.This shared rule makes the identical inline copies redundant:
design-system/prototypes/automations.htmlline 10,design-system/prototypes/sessions.htmlline 10, anddesign-system/prototypes/todos.htmllines 10-12. Keep the rule only here.Based on learnings: keep
design-system/prototypes/styles.csslimited to CSS shared across multiple page prototypes, and keep page-only CSS in that page's HTML.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@design-system/prototypes/styles.css` around lines 111 - 114, Remove the duplicated [hidden] CSS declarations from the per-page prototypes automations.html, sessions.html, and todos.html, keeping the shared [hidden] rule only in styles.css. Preserve styles.css as the source for CSS shared across multiple prototypes and leave page-specific styles in their respective HTML files.Source: Learnings
design-system/prototypes/dashboard.html (1)
6-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRename this prototype to
design-system/prototypes/home.html.The file now describes the Home page: the description, title, body class, and
<main aria-label="Global Home">all say Home. The filename still saysdashboard. The path instruction requires one current effective prototype per page named<page>.html, and this PR removes the Dashboard surface as a hard cut.The rename also touches the rail link in
automations.html(line 477),session.html(line 1384),sessions.html(line 303), andtodos.html(line 953), which all point at./dashboard.html?v=home-shell-1.As per path instructions: "Keep one current effective HTML prototype per page named
design-system/prototypes/<page>.html".Also applies to: 255-259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@design-system/prototypes/dashboard.html` around lines 6 - 7, Rename the prototype file from dashboard.html to home.html to match its Home page identity and the required one-file-per-page naming convention. Update the rail links in automations.html, session.html, sessions.html, and todos.html from ./dashboard.html?v=home-shell-1 to the renamed home prototype, preserving the existing query parameter.Source: Path instructions
|
Addressed the actionable Cubic and CodeRabbit findings in 112add9. The fixes cover run-now atomicity and recovery, Plan path safety, realtime form/revision races, live attachment projection, inventory status semantics, search cancellation, prototype/spec consistency, and worktree error mapping. Non-actionable findings were answered inline with the relevant architecture or current-code evidence. Local typecheck, full tests, build, and diff checks pass. |
|
@cubic-dev-ai review this @coderabbitai review |
|
✅ Action performedReview finished.
|
@boh5 I can't start this review because you've reached your trial's review limit. Trial plans have lower review limits than paid plans. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/agent-core/src/todos/service.ts (2)
352-369: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win不要重新赋值
catch参数。Biome 的
lint/suspicious/noCatchAssign会报错。用局部变量承载聚合后的原因。♻️ 建议改动
} catch (error) { + let removalFailure: unknown = error; try { await this.#state.markRunNowRecoveryRequired(clientRequestId); } catch (recoveryError) { - error = new AggregateError([error, recoveryError]); + removalFailure = new AggregateError([error, recoveryError]); } throw new ProjectTodoRunNowRecoveryError( todoId, sessionId, "Run now failed and its partial entities could not be fully removed", - { cause: new AggregateError([cause, error]) }, + { cause: new AggregateError([cause, removalFailure]) }, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/todos/service.ts` around lines 352 - 369, 在运行清理逻辑的 catch 块中，避免重新赋值 catch 参数 error。使用局部变量保存原始错误，并在 recoveryError 发生时更新该局部变量为 AggregateError；随后将该变量用于 ProjectTodoRunNowRecoveryError 的 cause 聚合，同时保持现有错误处理行为不变。Source: Linters/SAST tools
260-277: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
attachRunNowSession失败会泄漏已创建的 Session。
createRootSession成功后sessionId已赋值，因此第 272 行的条件为假，不执行补偿。此时收据仍是preparing且没有sessionId，Session 没有任何引用。用同一clientRequestId重试时，第 252 行走deletePendingRunNow并新建 Todo 与 Session，先前那个 Session 永久残留，也不会被标记为recovery_required。建议把绑定单独包裹补偿，保留外层
catch处理创建 Session 前的失败。🐛 建议修复
- await this.#state.attachRunNowSession(request.clientRequestId, sessionId); + try { + await this.#state.attachRunNowSession(request.clientRequestId, sessionId); + } catch (attachError) { + await this.#compensateRunNow(request.clientRequestId, todo.id, sessionId, attachError); + throw attachError; + } return await this.#acceptPreparedRunNow(request.clientRequestId, todo.id, sessionId);请补充一条测试：
attachRunNowSession抛错时，Session 被删除或收据进入recovery_required。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/todos/service.ts` around lines 260 - 277, 在包含 createRootSession、attachRunNowSession 和 _acceptPreparedRunNow 的流程中，将 attachRunNowSession 单独包裹补偿逻辑：绑定失败时，删除已创建的 Session，或将收据标记为 recovery_required；保留外层 catch 仅处理 Session 创建前的失败，避免重复补偿。补充测试覆盖 attachRunNowSession 抛错，并断言 Session 被删除或收据进入 recovery_required。
🧹 Nitpick comments (2)
packages/agent-core/src/todos/service.ts (1)
236-253: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win用显式校验替换
sessionId!断言。
accepted收据的sessionId只由completeRunNow的运行时不变量保证，schema 仍允许缺失。持久化状态被外部修改时，undefined会传入readRootSession。♻️ 建议改动
- if (receipt.status === "accepted") return await this.#readRunNowResponse(receipt.todoId, receipt.sessionId!); + if (receipt.status === "accepted") { + if (receipt.sessionId === undefined) { + throw new ProjectTodoRunNowRecoveryError( + receipt.todoId, + undefined, + "Run now receipt is accepted but has no Session", + ); + } + return await this.#readRunNowResponse(receipt.todoId, receipt.sessionId); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/todos/service.ts` around lines 236 - 253, Replace the non-null sessionId assertion in the accepted branch of the run-now receipt handling with an explicit presence check before calling readRunNowResponse. Preserve the existing accepted-response behavior when sessionId exists, and throw the appropriate recovery/error path when persisted state is missing it instead of passing undefined.packages/agent-core/src/todos/state-manager.ts (1)
100-130: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win为
runNowReceipts增加保留上限。收据只在补偿路径中被删除。
accepted与recovery_required收据永久累积。#load与每次#mutate都会解析并整体重写state.json，因此长期使用会持续放大所有 Todo 写入的开销。建议在
beginRunNow提交时按时间或数量裁剪accepted收据（recovery_required保留待人工处理），并补充一条断言裁剪行为的测试。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/todos/state-manager.ts` around lines 100 - 130, 在 StateManager 的 beginRunNow 提交流程中为 runNowReceipts 增加保留上限，仅裁剪较旧的 accepted 收据，保留所有 recovery_required 收据及未完成收据；按现有时间或数量约定稳定排序后执行裁剪，并补充测试断言 accepted 收据会被裁剪而 recovery_required 不会被删除。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/agent-core/src/todos/service.ts`:
- Around line 352-369: 在运行清理逻辑的 catch 块中，避免重新赋值 catch 参数 error。使用局部变量保存原始错误，并在
recoveryError 发生时更新该局部变量为 AggregateError；随后将该变量用于 ProjectTodoRunNowRecoveryError
的 cause 聚合，同时保持现有错误处理行为不变。
- Around line 260-277: 在包含 createRootSession、attachRunNowSession 和
_acceptPreparedRunNow 的流程中，将 attachRunNowSession 单独包裹补偿逻辑：绑定失败时，删除已创建的
Session，或将收据标记为 recovery_required；保留外层 catch 仅处理 Session 创建前的失败，避免重复补偿。补充测试覆盖
attachRunNowSession 抛错，并断言 Session 被删除或收据进入 recovery_required。
---
Nitpick comments:
In `@packages/agent-core/src/todos/service.ts`:
- Around line 236-253: Replace the non-null sessionId assertion in the accepted
branch of the run-now receipt handling with an explicit presence check before
calling readRunNowResponse. Preserve the existing accepted-response behavior
when sessionId exists, and throw the appropriate recovery/error path when
persisted state is missing it instead of passing undefined.
In `@packages/agent-core/src/todos/state-manager.ts`:
- Around line 100-130: 在 StateManager 的 beginRunNow 提交流程中为 runNowReceipts
增加保留上限，仅裁剪较旧的 accepted 收据，保留所有 recovery_required
收据及未完成收据；按现有时间或数量约定稳定排序后执行裁剪，并补充测试断言 accepted 收据会被裁剪而 recovery_required 不会被删除。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b8e54d6c-a48a-485a-ae1c-45614eeafb4c
📒 Files selected for processing (43)
apps/server/src/global-work-read-service.tsapps/server/src/routes/automations.test.tsapps/server/src/routes/automations.tsapps/server/src/routes/global-work.test.tsapps/server/src/routes/todos.tsapps/web/src/api/mutations.tsapps/web/src/api/queries.tsapps/web/src/components/features/EditAutomationDialog.test.tsapps/web/src/components/features/EditAutomationDialog.tsxapps/web/src/components/features/ProjectBar.test.tsxapps/web/src/components/features/ProjectBar.tsxapps/web/src/components/features/TodoReferences.tsxapps/web/src/routes/inventory-classification.test.tsapps/web/src/routes/project-sessions.tsxapps/web/src/routes/project-todo-detail.tsxapps/web/src/routes/project-todos.tsxapps/web/src/routes/project.tsxapps/web/src/routes/root-layout.tsxdesign-system/pages/automations.mddesign-system/pages/dashboard.mddesign-system/pages/sessions.mddesign-system/prototypes/automations.htmldesign-system/prototypes/dashboard.htmldesign-system/prototypes/session.htmldesign-system/prototypes/sessions.htmldesign-system/prototypes/styles.cssdesign-system/prototypes/todos.htmlpackages/agent-core/src/agents/configured-agent.test.tspackages/agent-core/src/agents/configured-agent.tspackages/agent-core/src/agents/factory.test.tspackages/agent-core/src/agents/factory.tspackages/agent-core/src/attachments/model-projector.test.tspackages/agent-core/src/attachments/model-projector.tspackages/agent-core/src/automations/state-manager.test.tspackages/agent-core/src/automations/state-manager.tspackages/agent-core/src/store/session-store-manager.tspackages/agent-core/src/todos/schema.tspackages/agent-core/src/todos/service.test.tspackages/agent-core/src/todos/service.tspackages/agent-core/src/todos/state-manager.test.tspackages/agent-core/src/todos/state-manager.tspackages/agent-core/src/tools/builtins/glob.tspackages/agent-core/src/tools/builtins/pdf-read.ts
🚧 Files skipped from review as they are similar to previous changes (34)
- design-system/pages/automations.md
- apps/web/src/routes/project.tsx
- design-system/prototypes/sessions.html
- apps/web/src/routes/inventory-classification.test.ts
- apps/web/src/routes/root-layout.tsx
- design-system/pages/sessions.md
- packages/agent-core/src/agents/configured-agent.test.ts
- apps/server/src/routes/automations.ts
- apps/web/src/components/features/TodoReferences.tsx
- apps/server/src/routes/todos.ts
- design-system/prototypes/automations.html
- apps/web/src/routes/project-sessions.tsx
- apps/server/src/routes/global-work.test.ts
- apps/web/src/components/features/ProjectBar.tsx
- packages/agent-core/src/agents/factory.test.ts
- apps/web/src/routes/project-todo-detail.tsx
- packages/agent-core/src/attachments/model-projector.test.ts
- apps/web/src/api/mutations.ts
- packages/agent-core/src/tools/builtins/pdf-read.ts
- apps/server/src/global-work-read-service.ts
- packages/agent-core/src/tools/builtins/glob.ts
- packages/agent-core/src/todos/state-manager.test.ts
- packages/agent-core/src/attachments/model-projector.ts
- apps/web/src/routes/project-todos.tsx
- packages/agent-core/src/automations/state-manager.ts
- packages/agent-core/src/todos/service.test.ts
- apps/web/src/api/queries.ts
- packages/agent-core/src/store/session-store-manager.ts
- design-system/prototypes/dashboard.html
- packages/agent-core/src/todos/schema.ts
- design-system/prototypes/styles.css
- packages/agent-core/src/agents/configured-agent.ts
- design-system/prototypes/todos.html
- packages/agent-core/src/automations/state-manager.test.ts
2 participants
Summary
Product and compatibility
This is the approved hard cut to the new workbench model. It intentionally does not add legacy UI/data fallbacks, compatibility paths, or migration tombstones.
Validation
bun run typecheckbun run testbun run buildgit diff --check origin/main...HEADSummary by CodeRabbit
New Features
Improvements