feat(acp): emit plan session updates from todos store - #1030
Conversation
ACP clients (Zed renders these natively) receive a stable-v1 plan sessionUpdate with the full PlanEntry snapshot whenever a todo write lands in the session store, plus one initial snapshot on resume. Blocked todos map to a neutral [blocked] marker (no failed status in v1); scope metadata rides Plan._meta["kimchi.dev"].scope. Co-Authored-By: Kimchi <noreply@kimchi.dev>
Kimchi Code Review
Summary📊 Review Score: 92/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Strong coverage across three layers: unit tests for 📝 Found 1 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
There was a problem hiding this comment.
📊 Review Score: 92/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 3/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Strong coverage across three layers: unit tests for buildPlanEntries/buildPlanUpdate in plan-mapper.test.ts, integration tests in server.test.ts for emission, session isolation, close/shutdown, and loadSession resume behavior, plus an E2E test in tests/e2e/acp/plan-updates.test.ts exercising create_todos/update_todos/clear_todos tool calls and full-replacement plan snapshots.
📝 Found 1 issue(s). See inline comments for details.
| models: buildSessionModelState(configOptions), | ||
| } | ||
| } catch (err) { | ||
| unregisterAcpPrompter(session.sessionId) |
There was a problem hiding this comment.
The newSession error handler calls unsubscribePlanSync() but does not call the session's base unsubscribe() before disposing. The loadSession error handler correctly unsubscribes both, so this asymmetry is inconsistent and can leave a live session subscription attached to a disposed session if creation fails after session.subscribe() is wired.
💡 Suggestion: Store the session record in a local before cleanup (or retrieve it once) and call both record.unsubscribe() and record.unsubscribePlanSync() in the newSession catch block, matching the cleanup in the loadSession error path.
|
Documentation Changes Added
Actions
If neither actions are selected, on PR close/merge the docs branch in ReadMe will remain open. |
|
📚 No documentation changes were needed for this PR. |
There was a problem hiding this comment.
Simpler model looks correct. A few edge cases worth considering:
-
No dedupe. The todo-sync bridge writes multiple scopes/no-op updates in quick succession; clients may get flooded with identical full-replacement plans. The old tracker JSON-deduped consecutive identical plans.
-
Empty-scope flashes. When the bridge clears a step scope on
STEP_COMPLETED, the tracker emitsentries: []even if the phase scope still has content. -
Restore snapshot picks one scope.
restoredPlanSnapshotemits only the most specific non-empty scope, then live writes from other scopes can switch the client's plan view unexpectedly. -
Missing Ferment-lifecycle E2E coverage. The old
tests/e2e/acp/plan-updates.test.tsdrove the real binary throughactivate_ferment_phase → start_ferment_step → complete_ferment_step → complete_ferment_phase; the new version only exercises directcreate_todos/update_todos/clear_todoscalls. Given the architecture now treats the todo store as the source of truth, this is a coverage gap rather than a logic bug, but worth adding back if you want integration confidence for the bridge-to-tracker path.
What
Emits stable-v1 ACP
plansession updates (sessionUpdate: "plan") directly from the Todo store:create_todos/update_todos/mark_todo/clear_todos, including Ferment-synced Todos) recomputes the most-specific non-empty scope as a full-replacementPlanEntry[]snapshot.loadSessionuses the same scope selection for restored Todos; sessions without restored Todos emit nothing.pending | in_progress | completed):in_progressprefersactiveForm;blockedmaps topendingwhilePlanEntry._meta["kimchi.dev"]preservestodoStatus: "blocked"and the optional note.Plan._meta["kimchi.dev"].scope; spec-compliant clients ignore it.Why
Zed renders ACP
planupdates natively as a live checklist in the agent panel, so IDE users can watch ordinary, plan-mode, and Ferment-generated Todo progress without separate lifecycle-specific ACP integrations. The Todo store remains the single checklist source; ACP v2 can replace only the wire encoder withplan_update.Example
Validation
src/modes/acp/plans.test.ts: 11 tests covering mapping, dedupe, scope fallback, session isolation, restore, teardown, and send-failure isolation.src/modes/acp/server.test.ts: 227/227 passed, including empty-session reload without JSONL junk and invalid-ID rejection.tests/e2e/acp/plan-updates.test.ts: 4 real-binary scenarios covering create/update/clear, cross-session isolation, ordinary Todos under--plan, and the Ferment phase/step lifecycle.pnpm run checkandpnpm run build:binarypassed.Co-Authored-By: Kimchi noreply@kimchi.dev