Populate workspace todos from Claude's TaskCreate/TaskUpdate calls - #9475
austinywang wants to merge 19 commits into
Conversation
Claude Code replaced TodoWrite with TaskCreate/TaskUpdate, which arrive as PreToolUse events and mutate one task per call. WorkstreamStore has no mapping for them, so the todo list never fills. Refs #8960 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude Code no longer emits TodoWrite; it drives its plan through TaskCreate/TaskUpdate, which reach cmux as ordinary PreToolUse events (the wrapper's PreToolUse hook uses matcher ""). Nothing mapped those tool names, so the Feed never produced a .todos payload and the workspace checklist stayed empty. Two parts: - WorkstreamTaskToolTodos accumulates a per-workstream task list across the delta-shaped task tools: TaskCreate appends (taking the next sequential id, since PreToolUse fires before Claude assigns one), TaskUpdate retargets by taskId, and status "deleted" removes the row. TodoWrite still replaces the whole list, and now works whether it arrives as a hook event name or as a PreToolUse tool name. - FeedCoordinator folds any .todos payload into the event's workspace checklist through WorkspaceAgentChecklistSync, so the sidebar summary, the todo pane, and No todo items. all follow the agent's plan. The sync keeps user-authored rows, drops agent rows the agent stopped reporting, treats an empty report as a no-op rather than a wipe, and skips the write when nothing changed. Task-tool PreToolUse events are promoted to session-critical ingress importance: they are deltas, so a dropped TaskCreate would leave the accumulated list permanently wrong. Fixes #8960 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughClaude ChangesAgent todo synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Claude
participant cmuxClaudeWrapper
participant WorkstreamStore
participant FeedCoordinator
participant WorkspaceAgentChecklistSync
participant WorkspaceChecklist
Claude->>cmuxClaudeWrapper: emit PostToolUse task event
cmuxClaudeWrapper->>WorkstreamStore: forward tool response
WorkstreamStore->>FeedCoordinator: emit todo payload
FeedCoordinator->>WorkspaceAgentChecklistSync: create checklist replacement
WorkspaceAgentChecklistSync->>WorkspaceChecklist: update checklist
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (21 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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.
Inline comments:
In
`@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamStore.swift`:
- Around line 50-54: Update the .sessionEnd handling in WorkstreamStore to
remove the taskToolTodosByWorkstream entry for event.sessionId, matching the
cleanup already performed in the .todoWrite case. Ensure sessions ending after
TaskCreate/TaskUpdate activity no longer leave stale dictionary entries.
In
`@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolTodos.swift`:
- Around line 38-57: Update apply’s TaskCreate handling so id-less creates
cannot publish a synthetic ID that conflicts with an existing real task ID after
resume. Reconcile such creates with the next matching TaskUpdate using the real
ID or task position, or reject them until a real task ID is available; preserve
explicit TaskCreate IDs and existing updates through the TaskCreate/TaskUpdate
flow.
In
`@Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/Workstream/WorkstreamTaskToolTodoTests.swift`:
- Around line 36-103: Add a test alongside task accumulation tests that first
ingests a TaskUpdate for taskId "7" without a prior TaskCreate, then ingests a
TaskCreate without an id for the same workstream. Assert the latest todos
contain a single row for that task rather than duplicate rows, covering resumed
sessions with a pre-existing high task ID.
In
`@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Values/WorkspaceAgentChecklistSync.swift`:
- Around line 30-71: Update replacementItems to rebuild items by walking
existing in order: retain user rows and refresh matching agent rows at their
original positions, then append only normalized agent tasks not already present.
Track consumed agent IDs so new tasks are not duplicated, preserve existing
ordering for matchesExisting, and apply the checklist cap without reordering or
dropping preserved user rows where possible.
In
`@Packages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/Values/WorkspaceAgentChecklistSyncTests.swift`:
- Around line 26-37: Add a test alongside agentTasksAppendAfterUserItems using
existing items ordered as agent, user, agent, then call
WorkspaceAgentChecklistSync.replacementItems and assert the user item remains
between the agent rows, preserving each row’s expected IDs, text, state, and
origin.
In `@Sources/Feed/FeedCoordinator`+AgentTodos.swift:
- Around line 15-41: Refactor the applyAgentTodos flow so
resolveAttentionTarget(event:) and resolveTodoWorkspace(for:) perform their
synchronous session-file resolution before entering the `@MainActor` section. Then
dispatch the resolved workspace back to the main actor for checklist mapping and
replacement, preserving the existing guard behavior and
WorkspaceTodoFeature.markUsed() call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 50275b67-5c65-48ef-a93c-0b035628c7b1
📒 Files selected for processing (9)
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamStore.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolTodos.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/Workstream/WorkstreamTaskToolTodoTests.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Values/WorkspaceAgentChecklistSync.swiftPackages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/Values/WorkspaceAgentChecklistSyncTests.swiftSources/Feed/FeedCoordinator+AgentTodos.swiftSources/Feed/FeedCoordinator.swiftSources/Feed/WorkstreamEvent+FeedIngress.swiftcmux.xcodeproj/project.pbxproj
| @Test func agentTasksAppendAfterUserItems() { | ||
| let userItem = WorkspaceChecklistItem(text: "mine", state: .completed, origin: .user) | ||
| let a = UUID(), b = UUID() | ||
| let items = WorkspaceAgentChecklistSync.replacementItems( | ||
| existing: [userItem], | ||
| agentTasks: [task(a, "plan", .completed), task(b, "build", .inProgress)] | ||
| ) | ||
| #expect(items?.map(\.text) == ["mine", "plan", "build"]) | ||
| #expect(items?.map(\.id) == [userItem.id, a, b]) | ||
| #expect(items?.map(\.state) == [.completed, .completed, .inProgress]) | ||
| #expect(items?.map(\.origin) == [.user, .agent, .agent]) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add coverage for a user row interleaved between agent rows.
Add a test with existing containing an agent row, then a user row, then another agent row, and assert the user row keeps its position relative to the agent rows after replacementItems. This directly exercises the ordering issue raised in WorkspaceAgentChecklistSync.swift.
🤖 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/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/Values/WorkspaceAgentChecklistSyncTests.swift`
around lines 26 - 37, Add a test alongside agentTasksAppendAfterUserItems using
existing items ordered as agent, user, agent, then call
WorkspaceAgentChecklistSync.replacementItems and assert the user item remains
between the agent rows, preserving each row’s expected IDs, text, state, and
origin.
… bounded state Five fixes from local review of the task-tool checklist path: - TaskCreate ids are no longer guessed from call order. PreToolUse creates the row under a namespaced provisional id, and the matching PostToolUse tool result (newly forwarded by the wrapper and the feed hook) supplies the authoritative id, which the accumulator adopts. A failed create or a session whose counter already ran ahead can no longer desynchronize later TaskUpdates onto a phantom row. - The checklist sync now retires only rows the reporting workstream owns, tracked as every task id that workstream ever minted. Two agents sharing a workspace keep each other's rows instead of erasing them on every update. - Cap enforcement trims only the incoming agent portion, so a long agent plan can never delete user-authored rows; if user rows already fill the cap the agent report is dropped instead. - Accumulators are bounded (oldest tasks evicted at the checklist cap) and retired on SessionEnd, so a long-running app cannot accumulate them. - Deleting the last task now publishes an empty list rather than falling through to tool telemetry, so the final agent row is actually removed. Also splits the new types into one file each, moves pure helpers to file scope, and adds DocC to the new public symbols, clearing all 22 Aziz policy findings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/Workstream/WorkstreamTaskToolTodoTests.swift (1)
128-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the mid-run adoption and mismatched-response paths.
The suite exercises the happy lifecycle well. Two branches that decide task identity have no coverage:
applyLines 80-91: aTaskUpdatefor an id that was never created, which a resumed session or a mid-run hook install produces. Assert that one row appears and that a laterTaskCreatewithout an id does not duplicate it.adoptTaskIdLines 128-135: aPostToolUseresponse whose subject matches no provisional row. Assert the intended outcome, which changes if the oldest-row fallback is removed.A test for an unrecognized
statusvalue on an existing completed row would also lock in the state-preservation behavior discussed ontaskState.🤖 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/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/Workstream/WorkstreamTaskToolTodoTests.swift` around lines 128 - 180, Extend the WorkstreamTaskToolTodoTests suite to cover apply handling of an unknown TaskUpdate id, asserting it creates one row and a later id-less TaskCreate does not duplicate it. Add an adoptTaskId test for a mismatched PostToolUse subject that asserts the intended non-fallback identity outcome. Also test taskState preservation when an existing completed row receives an unrecognized status.
🤖 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.
Inline comments:
In
`@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamStore.swift`:
- Around line 389-392: Remove the taskToolTodosByWorkstream[event.sessionId]
reset in the .todoWrite branch of the WorkstreamStore event handling, preserving
previously accumulated TaskCreate and TaskUpdate ownership while returning the
parsed TodoWrite list.
In
`@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskTodo`+ChecklistIdentity.swift:
- Around line 39-49: Update derivedChecklistUUID to require CryptoKit and always
derive the 16-byte UUID seed from SHA256.hash; remove the `#if`
canImport(CryptoKit) conditional and the additive zero-filled fallback branch,
preserving the existing deterministic UUID derivation.
In
`@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolTodos.swift`:
- Around line 32-46: The ownedIds tracking in WorkstreamTaskToolTodos is
unbounded even though retained todos are capped. Add insertion-order tracking
for ownedIds and enforce a bounded retention policy, such as a multiple of
maxRetainedTodos, removing the oldest entries when the limit is exceeded while
preserving IDs still needed for checklist synchronization and existing todo
trimming behavior.
- Around line 52-59: The TodoWrite handling in the switch currently treats an
intentionally empty list the same as malformed input. Update the
parsing/validation around parseWorkstreamTodoWriteList and the "TodoWrite"
branch to distinguish a missing or unparseable todos value from a present empty
array, returning .ignored only for invalid or absent input and publishing
.list([]) while clearing ownership state for an intentional clear.
- Around line 8-13: Move the ambient functions onto their owning types: in
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolTodos.swift#L8-L13,
make isWorkstreamTaskTool and parseWorkstreamTodoWriteList static members of
WorkstreamTaskToolTodos; in
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskTodo+ChecklistIdentity.swift#L35-L37,
make workstreamChecklistItemId a static member of WorkstreamTaskTodo and update
stableChecklistItemId(workstreamId:) plus FeedCoordinator+AgentTodos.swift to
call it through that type, preserving existing behavior.
- Around line 205-213: Update taskState(in:) so its default branch returns nil
for unrecognized status strings instead of .pending. Preserve the existing
mappings for completed, in-progress, and pending statuses, allowing apply to
retain the existing todo state via its taskState(in: input) ??
todos[index].state fallback while using .pending only for new rows.
- Around line 128-135: Update the provisional selection in
WorkstreamTaskToolTodos so provisionalIds.first is used only when
taskContent(in: task) returns no subject. When a subject exists, require a
provisional row whose content exactly matches it and return .ignored if none
matches; do not bind the task to an unrelated oldest row.
In `@Resources/bin/cmux-claude-wrapper`:
- Line 943: The HOOKS_JSON configuration currently delivers
PostToolUse(TaskCreate) asynchronously, allowing it to arrive after later events
or session termination. Update the TaskCreate entry in PostToolUse to use the
synchronous path, matching the generic PreToolUse ingress, while preserving the
existing command and timeout.
In `@Sources/Feed/FeedCoordinator`+AgentTodos.swift:
- Around line 22-26: Update the ownership flow surrounding ownedIds and
adoptTaskId so provisional task IDs remain included until checklist
synchronization consumes them, or pass a separate retired-ID set for
reconciliation. Ensure the replacement receives both provisional and
authoritative checklist UUIDs while retaining a single ownership source of truth
and authoritative IDs from matching PostToolUse results.
---
Duplicate comments:
In
`@Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/Workstream/WorkstreamTaskToolTodoTests.swift`:
- Around line 128-180: Extend the WorkstreamTaskToolTodoTests suite to cover
apply handling of an unknown TaskUpdate id, asserting it creates one row and a
later id-less TaskCreate does not duplicate it. Add an adoptTaskId test for a
mismatched PostToolUse subject that asserts the intended non-fallback identity
outcome. Also test taskState preservation when an existing completed row
receives an unrecognized status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cedd6543-0c81-4f2c-ae7d-17225250cd0b
📒 Files selected for processing (13)
CLI/cmux.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamEvent.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamStore.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskTodo+ChecklistIdentity.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolOutcome.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolTodos.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/Workstream/WorkstreamTaskToolTodoTests.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Values/WorkspaceAgentChecklistSync.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Values/WorkspaceAgentChecklistTask.swiftPackages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/Values/WorkspaceAgentChecklistSyncTests.swiftResources/bin/cmux-claude-wrapperSources/Feed/FeedCoordinator+AgentTodos.swiftSources/Feed/WorkstreamEvent+FeedIngress.swift
| private func derivedChecklistUUID(from seed: String) -> UUID { | ||
| let data = Data(seed.utf8) | ||
| var bytes: [UInt8] | ||
| #if canImport(CryptoKit) | ||
| bytes = Array(SHA256.hash(data: data).prefix(16)) | ||
| #else | ||
| bytes = Array(repeating: 0, count: 16) | ||
| for (index, byte) in data.enumerated() { | ||
| bytes[index % 16] = bytes[index % 16] &+ byte &+ UInt8(truncatingIfNeeded: index) | ||
| } | ||
| #endif |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Drop the non-CryptoKit fallback instead of keeping a weak digest.
This package lives under Packages/macOS/, and CryptoKit is available on every platform it builds for, so the #else branch never compiles. The fallback also folds bytes additively into 16 slots, which collides easily. A collision would map two different (workstreamId, taskId) pairs onto one checklist row, and the identity is persisted across restarts. Remove the branch and require CryptoKit, or replace the fold with a real digest if a non-Apple platform is actually a target.
🤖 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/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskTodo`+ChecklistIdentity.swift
around lines 39 - 49, Update derivedChecklistUUID to require CryptoKit and
always derive the 16-byte UUID seed from SHA256.hash; remove the `#if`
canImport(CryptoKit) conditional and the additive zero-filled fallback branch,
preserving the existing deterministic UUID derivation.
| public func isWorkstreamTaskTool(_ toolName: String) -> Bool { | ||
| switch toolName { | ||
| case "TodoWrite", "TaskCreate", "TaskUpdate": return true | ||
| default: return false | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
New public top-level free functions add ambient module API. Both files introduce public top-level free functions under a production Sources/ path. The shared root cause is placing behavior at module scope instead of on an owning type. Move each onto the type it belongs to and keep the call sites unchanged in meaning.
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolTodos.swift#L8-L13: moveisWorkstreamTaskToolto astaticmember ofWorkstreamTaskToolTodos, and moveparseWorkstreamTodoWriteListat Line 216 alongside it.Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskTodo+ChecklistIdentity.swift#L35-L37: moveworkstreamChecklistItemIdto astaticmember onWorkstreamTaskTodo, then call it fromstableChecklistItemId(workstreamId:)and fromFeedCoordinator+AgentTodos.swift.
As per coding guidelines: "In production Swift code, avoid ambient global state and behavior: do not add public or internal top-level free functions … Put state and behavior on a constructable, injectable owning type."
📍 Affects 2 files
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolTodos.swift#L8-L13(this comment)Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskTodo+ChecklistIdentity.swift#L35-L37
🤖 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/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolTodos.swift`
around lines 8 - 13, Move the ambient functions onto their owning types: in
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolTodos.swift#L8-L13,
make isWorkstreamTaskTool and parseWorkstreamTodoWriteList static members of
WorkstreamTaskToolTodos; in
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskTodo+ChecklistIdentity.swift#L35-L37,
make workstreamChecklistItemId a static member of WorkstreamTaskTodo and update
stableChecklistItemId(workstreamId:) plus FeedCoordinator+AgentTodos.swift to
call it through that type, preserving existing behavior.
Source: Coding guidelines
Dogfooding the tagged build showed each task landing twice in the sidebar: once under its provisional id and again under the authoritative one. Adopting the id dropped the provisional id from the owned set, and only owned rows may be retired, so the row written under it was left behind as an orphan. Keep it owned instead. Bounds the owned-id set while here, since it now holds two ids per task plus deleted ones; oldest ids are forgotten first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rage - An unrecognized TaskUpdate status no longer resets the row to pending. It now leaves the state alone, so a status cmux does not model cannot silently un-complete a finished task. - Corrects the sync's doc comment: rows the agent does not own keep their relative order, and the agent's rows are re-emitted as a block after them in the order the agent reports, which is not what "position" implied. - Adds coverage for a resumed session whose first event is a TaskUpdate for an id cmux never minted, and for an unknown status arriving after a completed one. CodeRabbit also flagged the accumulator not being cleared on session end; that already landed in the previous review round (WorkstreamStore clears the entry on .sessionEnd). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/Workstream/WorkstreamTaskToolTodoTests.swift (1)
176-181: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the ownership-cap test exceed the cap.
The test creates 70 tasks. Each task claims a provisional and authoritative ID, so it reaches at most 140 IDs. An unbounded implementation would pass this assertion.
Create at least
WorkstreamTaskToolTodos.maxOwnedIds / 2 + 1tasks before asserting the bound.Proposed fix
- let overflow = WorkstreamTaskToolTodos.maxRetainedTodos + 20 + let overflow = max( + WorkstreamTaskToolTodos.maxRetainedTodos + 20, + WorkstreamTaskToolTodos.maxOwnedIds / 2 + 1 + )🤖 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/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/Workstream/WorkstreamTaskToolTodoTests.swift` around lines 176 - 181, Update the ownership-cap test around the overflow task creation loop to create at least WorkstreamTaskToolTodos.maxOwnedIds / 2 + 1 tasks, accounting for each task registering two IDs. Keep the existing maxRetainedTodos assertion and verify ownedTaskIds(forWorkstream:) remains within WorkstreamTaskToolTodos.maxOwnedIds.
🤖 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.
Inline comments:
In
`@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolTodos.swift`:
- Line 91: Update the task-update handling around claimId(id) so ignored
status-only updates for unknown tasks do not claim ownership. Claim IDs only
after an existing task is found or a content-bearing update is accepted, and
re-claim the ID immediately before processing a valid deletion; use the
canonical task state as the ownership source and fail closed when no valid task
state is established.
---
Outside diff comments:
In
`@Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/Workstream/WorkstreamTaskToolTodoTests.swift`:
- Around line 176-181: Update the ownership-cap test around the overflow task
creation loop to create at least WorkstreamTaskToolTodos.maxOwnedIds / 2 + 1
tasks, accounting for each task registering two IDs. Keep the existing
maxRetainedTodos assertion and verify ownedTaskIds(forWorkstream:) remains
within WorkstreamTaskToolTodos.maxOwnedIds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ad947a84-29ce-4900-886f-f2eed49a237e
📒 Files selected for processing (3)
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolTodos.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/Workstream/WorkstreamTaskToolTodoTests.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Values/WorkspaceAgentChecklistSync.swift
A status-only TaskUpdate for a task cmux never saw returns .ignored, but still claimed its id. Enough of those would evict real ids from the bounded owned set, and rows those ids own could then never be retired. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolTodos.swift`:
- Around line 93-96: Update the valid deletion branch in the task-processing
flow around taskContent and todos.removeAll to call claimId(id) after confirming
the task exists and before removing it. Add a regression test that retains a
todo, evicts its ID from ownedIdSet, then deletes the todo and verifies
checklist synchronization retires the row.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 14a01ab1-bc69-418a-a907-83e621532fe4
📒 Files selected for processing (2)
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolTodos.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/Workstream/WorkstreamTaskToolTodoTests.swift
Review round 2 found the state machine trusting PreToolUse. A create that is denied or errors left a permanent phantom row, a speculative delete dropped a row that still existed, and because PreToolUse carries no task id, results had to be correlated back to provisional rows by subject and arrival order — which two async hooks can reorder. Applying every mutation from PostToolUse removes the whole class: - The tool has already run, and its tool_response carries the authoritative id, so there is no provisional row and no correlation heuristic. A create whose result has no id is ignored outright. - is_error calls never mutate the list. - The wrapper now matches PostToolUse for TaskCreate|TaskUpdate|TodoWrite, and the feed hook forwards tool_response for all three. Also from that round: - A TaskUpdate that describes its task lets a resumed session re-adopt tasks created before cmux was watching, instead of ignoring deltas for ids it never saw. - Whole-list TodoWrite snapshots now go through the same accumulator, so ownership unions across snapshots and a task dropped from a later report can still have its row retired. - The per-workstream accumulator map is bounded with LRU eviction. SessionEnd is not guaranteed: an agent that crashes never sends it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolTodos.swift`:
- Around line 107-109: Update the task ID selection in WorkstreamTaskToolTodos
so that when both input and result IDs are present they must match, returning
.ignored on conflict; otherwise prefer the available response/result ID and fall
back to the request/input ID. Add a regression test covering conflicting IDs and
confirming no task mutation occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 96e56799-2c9d-48a7-a45e-129a905cd6bb
📒 Files selected for processing (6)
CLI/cmux.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamStore.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolTodos.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/Workstream/WorkstreamTaskToolTodoTests.swiftResources/bin/cmux-claude-wrapperSources/Feed/WorkstreamEvent+FeedIngress.swift
| guard let id = input.flatMap(taskId(in:)) ?? resultTask.flatMap(taskId(in:)) else { | ||
| return .ignored | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail closed when request and response task IDs conflict.
Line 107 selects the request ID before the completed response ID. If the IDs differ, the code can apply response content or a deletion to the request task instead of the task returned by PostToolUse.
When both IDs exist, require them to match. Otherwise, use the response ID when available. Add a regression test for conflicting IDs.
Proposed fix
- guard let id = input.flatMap(taskId(in:)) ?? resultTask.flatMap(taskId(in:)) else {
+ let inputID = input.flatMap(taskId(in:))
+ let resultID = resultTask.flatMap(taskId(in:))
+ guard inputID == nil || resultID == nil || inputID == resultID,
+ let id = resultID ?? inputID
+ else {
return .ignored
}As per path instructions: use authoritative structured signals as the single source of truth and fail closed when they disagree.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| guard let id = input.flatMap(taskId(in:)) ?? resultTask.flatMap(taskId(in:)) else { | |
| return .ignored | |
| } | |
| let inputID = input.flatMap(taskId(in:)) | |
| let resultID = resultTask.flatMap(taskId(in:)) | |
| guard inputID == nil || resultID == nil || inputID == resultID, | |
| let id = resultID ?? inputID | |
| else { | |
| return .ignored | |
| } |
🤖 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/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamTaskToolTodos.swift`
around lines 107 - 109, Update the task ID selection in WorkstreamTaskToolTodos
so that when both input and result IDs are present they must match, returning
.ignored on conflict; otherwise prefer the available response/result ID and fall
back to the request/input ID. Add a regression test covering conflicting IDs and
confirming no task mutation occurs.
Source: Path instructions
…ails-only updates Review round 3: - SessionEnd no longer discards the accumulator. A Claude session can be resumed under the same id, and a resumed status-only TaskUpdate carries no subject, so dropping the ownership map left those checklist rows permanently stale; delayed async PostToolUse hooks raced it the same way. Growth stays bounded by the LRU on the workstream map. - An empty TodoWrite snapshot now clears the list instead of being conflated with a malformed payload, so clearing a legacy todo list no longer leaves stale rows. A payload with no list at all is still ignored. - A TaskUpdate carrying only a description no longer overwrites the row's subject. Subject and description are parsed separately; description is used only when adopting a task that has no subject at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…restarts Round 4 review: the accumulator is process-local, so after an app restart, a dropped event, or LRU eviction, an ordinary status-only TaskUpdate named an id cmux had never seen and was dropped. The checklist rows were still on disk but nothing recorded who owned them. Each row now carries a WorkspaceAgentTaskRef (workstream id + agent task id), persisted with the checklist and in the session manifest. Ownership is read from the rows themselves, so: - The sync retires exactly the reporting agent's rows without an in-memory set, and rows from other agents or the user are untouched. - FeedCoordinator reseeds the store from those rows before applying a delta, so a resumed session's status-only update lands. The field is optional and omitted when nil, so existing manifests decode unchanged and their rows are simply unowned. Also stops using resolveAttentionTarget for checklist writes. It falls back to retained hook-session JSON on disk, which is fine for routing a transient notification but could write rows into the workspace a moved session used to live in, and put a synchronous file read on the main actor for every task event. The event's own workspace_id is now required. Verified on the tagged build: after killing and relaunching the app, a plain status-only TaskUpdate marked the right row completed, which the previous build could not do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…failures Round 5 review, all on the newly enabled whole-list path: - Entries with no id of their own (OpenCode's todo.updated carries none) now get an identity derived from their text instead of their array index. Positional ids meant a removal or reorder handed one task's identity — and the checklist row and attachments the merge binds to it — to a different task. - Cancelled entries are dropped from the snapshot rather than defaulting to pending, which would have counted abandoned work as outstanding in the progress readout. - An unparseable TodoWrite payload now falls through to tool telemetry instead of publishing an empty list, which the checklist sync would have read as an authoritative cleared plan and used to retire every row the workstream owns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 6 review: - The PostToolUse task hook is now synchronous. As an async hook each invocation ran as an independent background process, so a TaskCreate and its following TaskUpdate could reach cmux inverted, leaving the checklist permanently stale. Claude runs a synchronous hook to completion before the next tool, which is what gives these stateful deltas their per-session order. Task tools fire a few times per turn, so the added latency is negligible. - Identical id-less whole-list entries now get distinct ids. They shared one content-derived id, and replaceChecklist rejects a whole replacement on a repeated id, so a valid OpenCode snapshot listing the same wording twice silently left the checklist stale. Not changed: the review also asked to route checklist writes through resolveAttentionTarget to survive a pane move. That resolver prefers event.workspaceId exactly as this code does and only adds an on-disk session-store fallback, so it would not re-home anything; it is also what round 4 of this review asked to stop using here, for the stale-write and main-actor disk I/O reasons documented at the call site. Live re-homing needs the surface probe the claude hook path uses, which is a separate change to the feed hook. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 7 review: - A TaskUpdate result can report failure in its own body (success:false, e.g. the task vanished or a dependency check rejected it) without the event-level is_error flag being set. Applying such a delta mutated the checklist as though the call had succeeded, leaving it permanently inconsistent with Claude's task store. The response's success flag is now honored. - Dedicated .todoWrite events from the OpenCode producer are now session-critical alongside the PostToolUse task tools. They are authoritative whole-list snapshots, so dropping the final clear or completion under ingress pressure left checklist rows stale with nothing later to correct them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 8 review: - Rewording a task from a producer that reports no ids (OpenCode) minted a new content-derived id, so the checklist merge replaced the row and dropped the attachments bound to the old one. When a snapshot differs from the previous one by exactly one derived id on each side, that pairing is unambiguous and the old identity is reused. A removal or any less certain diff keeps the derived id rather than guessing, so identity is never transplanted onto a surviving task. - seedTaskTodos appended to the recency list without running eviction, so a restored workstream whose next delta was ignored stayed retained past the documented 64-workstream cap. Both paths now share one helper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
…ss snapshots Round 9 review: - A hook process inherits CMUX_WORKSPACE_ID at spawn, so moving a surface to another workspace left task updates writing into the old one (or dropping them if it had closed) while the agent ran visibly elsewhere. The feed hook now re-homes task-tool events through the app's live surface-ownership probe, the same mechanism the claude hook path already uses. The surface id travels with the terminal, so it is the only stable handle. Non-task feed traffic is untouched. - Restart recovery keyed on a task tool name, but the OpenCode producer's dedicated .todoWrite events carry none, so an id-less snapshot after a restart skipped seeding and a reword then replaced the row instead of keeping it. The hook event name now counts as a task event too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
…eft behind Round 10 review: - Reverts the reword reconciliation added last round. One departed and one arrived derived id is not an unambiguous reword: [alpha, beta] to [beta, gamma] is equally a removal plus an addition, and the heuristic would graft alpha's attachments onto gamma. Without a producer id, minting a new identity on a text change is the strictly safer failure, and that is now documented at the derivation site. - When a surface moves between workspaces, the agent's rows are recreated in the new one; the copies left in the old workspace are now retired rather than lingering there forever, skewing its progress with tasks no later delta will touch. Not changed, with reasons: - Acknowledged blocking ingestion for task hooks. That is the same best-effort telemetry transport every other feed producer uses; converting it is a transport-wide change that would block Claude on each task call. The hook is synchronous, so deltas are written to the socket in order. - Adopting an unknown task from a status-only TaskUpdate. That fallback needs text the real payload may not carry, which is why the primary recovery path is the persisted agentTaskRef on each row rather than the update payload. The fallback stays as a best effort for producers that do describe the task. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…persisted owners
Round 11 review found the round-9 re-homing was dead code: the normal
'cmux hooks feed' dispatch passes a socket path and no client, so the
guard requiring one never ran and task events kept their inherited,
spawn-time workspace id.
Fixed by routing task tools through the connected path Pi already uses:
- The hook now awaits ingestion instead of firing a swallowed one-way
send, so a dropped TaskCreate or final TaskUpdate no longer vanishes
silently. These deltas are authoritative and cannot be reconstructed
from later traffic.
- That path owns a live client, which is what re-homing needed: the app
is asked which workspace owns this surface now, so a moved pane writes
where it lives rather than where its terminal started.
- Failures stay neutral. With the app down the hook still prints {} and
exits in well under a second, verified against the tagged build.
Retirement no longer depends on a process-local map. Rows record their
owning workstream, so the rows themselves are scanned across open
workspaces; a surface that moves while cmux is restarting can no longer
strand a full copy of the plan in its old workspace.
Not changed: OpenCode's TodoWrite events carry no surface identity, so
they cannot be re-homed the same way, and failing closed would drop
OpenCode todos entirely rather than routing them slightly wrong after a
move. Re-homing them needs a surface id from that producer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…owledgment Round 12 review: - allWorkspacesForAgentTodoRetirement missed ordinary registered windows: registration forgets a window's recoverable route, so an inactive window's workspaces were never visited and rows a workstream left there were never retired. It now enumerates mainWindowContexts as well. - Restart recovery searched only the workspace the event resolved to, so rows sitting in a workspace the surface had left produced an empty seed. It now takes that workspace first and then every other one it can enumerate, deduplicating by workspace and task id. - The task hook treated any socket response as admission. feed.push answers ok:false while starting up, when saturated, or when the target is gone, and losing a TaskCreate is permanent because later status-only updates cannot reconstruct its subject. The response is now parsed and retried once within the hook's remaining deadline. - The event carries surface_id so the app can revalidate ownership at its own acceptance boundary rather than trusting a probe that races a move landing between it and the push. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude runs concurrent TaskUpdate calls, so their PostToolUse hooks can reach cmux out of order: a delayed in_progress delta can land after the completion that superseded it and permanently regress the checklist row, since no authoritative snapshot follows to correct it. The result's statusChange reports the transition the task store actually made. When a row we already hold as completed receives a delta whose transition did not start from completed, that delta predates our view and is dropped. Scoped to the terminal state deliberately. cmux's view legitimately lags the task store for intermediate states, because it never sees a transition whose hook did not reach it, so rejecting every mismatch would drop ordinary updates. Verified both ways on the tagged build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 14: every admitted task delta rebuilt the list of all workspaces across all windows and scanned each checklist, widening a per-workstream status event into O(workspaces x checklist) main-actor work. Steady state is now one dictionary check: once a workstream has been seen writing to a workspace, there is nowhere else its rows can be stranded. The full scan runs only on its first delta of a process or after it moves — exactly when rows can be elsewhere. The index is a fast path only. Retirement correctness still comes from the agentTaskRef persisted on each row, so a restart that empties the map costs one sweep rather than a missed retirement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fixes #8960
Claude Code no longer calls
TodoWrite. It drives its plan throughTaskCreate/TaskUpdate, and nothing in cmux recognized those tool names, so the Feed never produced a.todospayload. Separately, even a.todospayload only ever rendered inside the Feed panel, so the workspace checklist could only be filled by an explicitcmux todo set.Mechanism
WorkstreamTaskToolTodosaccumulates a per-workstream task list across the delta-shaped task tools. Every mutation is applied fromPostToolUse, neverPreToolUse: a pre-execution event only states intent, so a denied create would leave a phantom row and a speculative delete would drop a live one.PostToolUsealso carriestool_response, which is where Claude reports the id it assigns during execution, so ids are read from the result rather than inferred from call order. A create whose result has no id, a call flaggedis_error, and a result reportingsuccess: falseall leave the list untouched.TaskUpdateretargets bytaskId;status: "deleted"removes the row and publishes an empty list, which is a real transition rather than a parse failure. A details-only update does not overwrite the row's subject, and an unrecognized status leaves a known state alone. The whole-listTodoWriteshape still replaces the list and goes through the same accumulator, dropping cancelled entries and giving id-less entries a content-derived identity (stable across reorder; a text change deliberately mints a new id rather than guessing whether it was a reword or a replacement).Two plumbing additions make the result available: the wrapper matches
PostToolUseforTaskCreate|TaskUpdate|TodoWritesynchronously, so these stateful deltas keep their per-session order, and the feed hook forwardstool_responsefor those tools. It rides the existing passthrough bag, so there is no wire-schema change. The feed hook also re-homes task events against the surface's current owner, so a pane moved between workspaces routes to where it lives now rather than the workspace its terminal inherited at spawn.FeedCoordinatorfolds any.todospayload into the workspace checklist throughworkspaceAgentChecklistReplacement, so the sidebar summary, the todo pane, andcmux todo listall follow the agent's plan. Each agent row persists aWorkspaceAgentTaskRef(workstream id + task id), so ownership is read from the rows themselves and survives an app restart, a dropped event, or LRU eviction: an agent may retire only rows it owns, two agents in one workspace never erase each other, user rows are always carried through, and the store reseeds from those rows so a resumed status-only update still lands. Rows left in a workspace the surface has moved away from are retired. The cap trims only the incoming agent portion, and the sync skips the write when nothing changed.State is bounded at three levels: retained tasks per workstream, remembered owned ids per workstream, and tracked workstreams (LRU) —
SessionEndcannot be relied on, since a crashed agent never sends it, and a session can resume under the same id. Task-tool results and whole-list snapshots are session-critical ingress, because they are authoritative and cannot be reconstructed from later traffic.Testing
WorkstreamTaskToolTodoTests.swift(new, 24 tests) andWorkspaceAgentChecklistSyncTests.swift(new, 11 tests) cover the accumulation, ownership, bounding, recovery, and failure-rejection behavior described above. Commit 1 adds the failing tests alone so CI goes red, then green — see the Commits tab.WorkspaceTodoSnapshotTests.swiftpin the session-manifest round trip for the persisted task reference, including that older manifests still decode and that user rows do not gain the key.swift test: 306/306 (CMUXAgentLaunch) and 211/211 (CmuxWorkspaces). Local$autoreviewpolicy check clean../scripts/reload.sh --tag sym8960, driven over/tmp/cmux-debug-sym8960.sockwith Claude-shaped hook payloads throughcmux hooks feed --source claude. Before:cmux todo list→No todo items.After:1/4 completedwith correct states beside an untouched hand-added user row. Also verified that aPreToolUse-only create changes nothing, a failed delete changes nothing, asuccess: falseresult changes nothing, a second agent session coexists, one agent deleting every task retires only its own rows, and — after killing and relaunching the app — a plain status-onlyTaskUpdatestill marks the right row completed. The dogfood loop caught three defects the unit tests missed (duplicate rows per task, ownership lost on restart, and the session snapshot dropping the task reference), all fixed here.