diff --git a/packages/cli/src/ui/contexts/TodoProvider.tsx b/packages/cli/src/ui/contexts/TodoProvider.tsx index 6fb4f957c6..12403400b1 100644 --- a/packages/cli/src/ui/contexts/TodoProvider.tsx +++ b/packages/cli/src/ui/contexts/TodoProvider.tsx @@ -5,7 +5,7 @@ */ import type React from 'react'; -import { useState, useEffect, useMemo, useCallback } from 'react'; +import { useState, useEffect, useMemo, useCallback, useRef } from 'react'; import { TodoStore, type Todo, @@ -23,7 +23,49 @@ interface TodoProviderProps { } /** - * Hook for managing task state and loading. + * Publish a provider-originated task-list replacement on the observation + * channel (issue #3052). Every observer of the singleton `todoEvents` + * emitter — external consumers (JSP/jefe, Zed) and any peer `TodoProvider` + * subscribed to the same channel — receives it. The originating provider's own + * listener is skipped by matching the exact event object against its + * per-instance publication ref (see `useTaskPersistence` / `useTaskUpdates`), + * so a provider-originated publication does not re-enter the origin. + * + * This mirrors the TodoWrite tool's canonical event shape and channel, not its + * call ordering: `updateTodos` stays a synchronous, optimistic API that applies + * local UI state and starts fire-and-forget persistence before emitting. The + * synchronous emit preserves fail-fast behavior — a throwing observer + * propagates to the caller. + */ +function publishTodos( + sessionId: string, + agentId: string | undefined, + todos: Todo[], + originPublicationRef: React.MutableRefObject, +): void { + const eventData: TodoUpdateEvent = { + sessionId, + agentId, + todos, + timestamp: new Date(), + }; + const previousPublication = originPublicationRef.current; + originPublicationRef.current = eventData; + try { + todoEvents.emitTodoUpdated(eventData); + } finally { + originPublicationRef.current = previousPublication; + } +} + +/** + * Hook for managing task state and the mount/session read. + * + * The read path deliberately does NOT publish. `TodoStore.readTodos` maps a + * parse or I/O failure to an empty list, so a refresh cannot distinguish an + * authoritative empty list from a load failure, and publishing it would risk + * advertising a stale or failed state as current. External TodoWrite events + * remain the authoritative source for external observers. */ function useTaskState(sessionId: string, agentId: string | undefined) { const [todos, setTodos] = useState([]); @@ -35,9 +77,7 @@ function useTaskState(sessionId: string, agentId: string | undefined) { setLoading(true); const store = new TodoStore( sessionId, - { - dataDirResolver: () => Storage.getGlobalDataDir(), - }, + { dataDirResolver: () => Storage.getGlobalDataDir() }, agentId, ); const loadedTodos = await store.readTodos(); @@ -65,16 +105,26 @@ function useTaskState(sessionId: string, agentId: string | undefined) { } /** - * Hook for listening to task update events. + * Hook for listening to task update events. An accepted external event (e.g. + * from the TodoWrite tool) is authoritative and mirrors the published list into + * local state. A provider-originated publication records the exact event object + * in its per-instance `originPublicationRef` before emitting (see + * `useTaskPersistence`); the origin's own listener skips only that event while + * external observers, nested external events, and matching peer providers still + * receive their events. */ function useTaskUpdates( sessionId: string, scopedAgentId: string, + originPublicationRef: React.MutableRefObject, setTodos: (todos: Todo[]) => void, setError: (error: string | null) => void, ) { useEffect(() => { const handleTaskUpdate = (eventData: TodoUpdateEvent) => { + if (originPublicationRef.current === eventData) { + return; + } if ( eventData.sessionId === sessionId && (eventData.agentId ?? DEFAULT_AGENT_ID) === scopedAgentId @@ -89,35 +139,83 @@ function useTaskUpdates( return () => { todoEvents.offTodoUpdated(handleTaskUpdate); }; - }, [scopedAgentId, sessionId, setTodos, setError]); + }, [scopedAgentId, sessionId, originPublicationRef, setTodos, setError]); } /** - * Hook for task persistence operations. + * Hook for task persistence operations — the single provider write path. It + * applies local UI state, starts fire-and-forget persistence, then publishes + * synchronously. The exact event object is recorded per provider so the + * origin's own `todoEvents` listener skips only that event (issue #3052), while + * external observers and any matching peer provider still receive it once. + * + * Persistence is ordered per provider: the first write starts immediately + * (before publication), and every subsequent write chains behind the in-flight + * one. This keeps `updateTodos` synchronous and fire-and-forget while ensuring + * writes reach disk in update call order — including a synchronously nested + * update invoked by a prepended `todoEvents` listener during the outer publish, + * which would otherwise race the outer write on the same store file. */ function useTaskPersistence( sessionId: string, agentId: string | undefined, + originPublicationRef: React.MutableRefObject, setTodos: (todos: Todo[]) => void, setError: (error: string | null) => void, ) { + // Tail of this provider's fire-and-forget persistence chain. `null` means no + // write is in flight, so the next write starts immediately. A pending tail + // makes the next write chain behind it so persistence follows update call + // order. A failed write reports the save error but does not poison later + // chained writes. + const inFlightWriteRef = useRef | null>(null); + const updateTodos = useCallback( (newTodos: Todo[]) => { setTodos(newTodos); - const store = new TodoStore( - sessionId, - { - dataDirResolver: () => Storage.getGlobalDataDir(), + + const writeNewTodos = (): Promise => { + const store = new TodoStore( + sessionId, + { dataDirResolver: () => Storage.getGlobalDataDir() }, + agentId, + ); + return store.writeTodos(newTodos); + }; + + const previous = inFlightWriteRef.current; + const next = + previous === null + ? writeNewTodos() + : previous.then(writeNewTodos, writeNewTodos); + + const tail = next.then( + () => { + if (inFlightWriteRef.current === tail) { + inFlightWriteRef.current = null; + } + }, + (err: unknown) => { + setError( + `Failed to save todos: ${err instanceof Error ? err.message : 'Unknown error'}`, + ); + if (inFlightWriteRef.current === tail) { + inFlightWriteRef.current = null; + } }, - agentId, ); - store.writeTodos(newTodos).catch((err) => { - setError( - `Failed to save todos: ${err instanceof Error ? err.message : 'Unknown error'}`, - ); - }); + inFlightWriteRef.current = tail; + + publishTodos(sessionId, agentId, newTodos, originPublicationRef); }, - [agentId, sessionId, setTodos, setError], + [ + agentId, + inFlightWriteRef, + originPublicationRef, + sessionId, + setTodos, + setError, + ], ); return { updateTodos }; @@ -128,13 +226,21 @@ function useTaskPersistence( */ function useTaskManagement(sessionId: string, agentId: string | undefined) { const scopedAgentId = agentId ?? DEFAULT_AGENT_ID; + const originPublicationRef = useRef(null); const state = useTaskState(sessionId, agentId); - useTaskUpdates(sessionId, scopedAgentId, state.setTodos, state.setError); + useTaskUpdates( + sessionId, + scopedAgentId, + originPublicationRef, + state.setTodos, + state.setError, + ); const persistence = useTaskPersistence( sessionId, agentId, + originPublicationRef, state.setTodos, state.setError, ); diff --git a/packages/cli/src/ui/contexts/__tests__/todoProvider.observation.bun.tsx b/packages/cli/src/ui/contexts/__tests__/todoProvider.observation.bun.tsx new file mode 100644 index 0000000000..54e05568c9 --- /dev/null +++ b/packages/cli/src/ui/contexts/__tests__/todoProvider.observation.bun.tsx @@ -0,0 +1,833 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Provider-to-canonical-observation-seam integration tests for issue #3052. + * + * A provider-originated mutation through `updateTodos` must reach the + * `todoEvents` observation channel exactly once, so external observers + * (JSP/jefe, Zed) stop retaining a stale list, while the originating + * provider's own `todoEvents` listener does not re-enter on its own echo + * (the origin suppresses it). External TodoWrite events remain authoritative. + * + * The wiring under test is real — no mocks on the observation seam: + * - The real `TodoProvider` is rendered and its real context value captured. + * - The real `createTodoObservationSubscription` (the exact seam + * `JspProducer` subscribes through in jspWiring) is subscribed, so a + * passing assertion proves the observation channel is actually reached. + * - The real `todoCommand` subcommands are driven with a `CommandContext` + * whose `todoContext` is the LIVE provider context. + * + * The provider, React, the event emitter, and the observation seam stay real. + * Storage is isolated to a per-process temp dir by the manifest preloads, so + * the real `TodoStore` disk I/O is sandboxed; each test uses a unique sessionId + * so its store file never collides with another test's. A peer provider is + * mounted in its own React root to assert that a provider-originated + * publication still reaches a matching peer while the origin suppresses its own + * echo. The mount/session read does not publish and is out of scope here. + */ + +import { describe, it, expect, afterEach } from 'bun:test'; +import React, { act } from 'react'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { + TodoStore, + todoEvents, + TodoEvent, + DEFAULT_AGENT_ID, + type Todo, + type TodoUpdateEvent, +} from '@vybestack/llxprt-code-core'; +import { Storage } from '@vybestack/llxprt-code-settings'; +import { renderHook, waitFor } from '../../../test-utils/render.js'; +import { TodoProvider } from '../TodoProvider.js'; +import { useTodoContext } from '../TodoContext.js'; +import { createTodoObservationSubscription } from '../../../observation/jspWiring.js'; +import { todoCommand } from '../../commands/todoCommand.js'; +import { shouldClearTodos } from '../../hooks/useTodoPausePreserver.js'; +import type { CommandContext } from '../../commands/types.js'; + +interface ObservedEvent { + agentId: string | undefined; + todos: readonly Todo[]; +} + +type TodoContextValue = ReturnType; + +interface MountedProvider { + readonly result: { + readonly current: TodoContextValue; + readonly all: readonly TodoContextValue[]; + }; + readonly rerender: () => void; + readonly unmount: () => void; +} + +const cleanups: Array<() => void> = []; + +afterEach(() => { + const errors: unknown[] = []; + while (cleanups.length > 0) { + try { + cleanups.pop()?.(); + } catch (error) { + errors.push(error); + } + } + if (errors.length > 0) { + throw new AggregateError(errors, 'TodoProvider test cleanup failed'); + } +}); + +function todosDir(): string { + return path.join(Storage.getGlobalDataDir(), 'todos'); +} + +/** + * Read a session's task file directly from disk, bypassing any active + * provider so persistence assertions are not perturbed. + */ +function readDiskTodos(sessionId: string, agentId?: string): Todo[] { + const scoped = agentId && agentId !== DEFAULT_AGENT_ID ? agentId : undefined; + const fileName = scoped + ? `todo-${sessionId}-${scoped}.json` + : `todo-${sessionId}.json`; + const filePath = path.join(todosDir(), fileName); + if (!fs.existsSync(filePath)) return []; + const raw = JSON.parse(fs.readFileSync(filePath, 'utf8')) as + | { todos?: Todo[] } + | Todo[]; + return Array.isArray(raw) ? raw : (raw.todos ?? []); +} + +function storeFor(sessionId: string, agentId?: string): TodoStore { + return new TodoStore( + sessionId, + { dataDirResolver: () => Storage.getGlobalDataDir() }, + agentId, + ); +} + +/** + * Narrow an outer-write capture from its nullable holder. Kept as a helper so + * the null guard does not appear directly inside a test body. + */ +function unwrapOuterCapture( + capture: { + store: TodoStore; + todos: Todo[]; + } | null, +): { store: TodoStore; todos: Todo[] } { + if (capture === null) { + throw new Error('outer TodoStore write was not captured'); + } + return capture; +} + +/** + * Drive the provider's refresh and settle its async read. `refreshTodos` + * returns a real promise at runtime, but the context type declares `void`, so + * it is wrapped to settle the read without an await-thenable violation. + */ +async function settleRefresh(refresh: () => void): Promise { + await Promise.resolve(refresh()); +} + +/** + * Subscribe to the REAL observation seam and collect every published event. + */ +function observeTodoChannel(): ObservedEvent[] { + const events: ObservedEvent[] = []; + const unsubscribe = createTodoObservationSubscription((agentId, todos) => { + events.push({ agentId, todos }); + }); + cleanups.push(unsubscribe); + return events; +} + +function mountProvider(sessionId: string, agentId?: string): MountedProvider { + const Wrapper = ({ children }: { children: React.ReactNode }) => ( + + {children} + + ); + const rendered = renderHook(() => useTodoContext(), { wrapper: Wrapper }); + cleanups.push(rendered.unmount); + return rendered; +} + +/** + * Build a `CommandContext` whose `todoContext` is the LIVE provider context, + * so task subcommands read and mutate the real provider state. The remaining + * fields are inert stubs; only `ui.addItem` is observable, to surface errors. + */ +function commandContextFrom( + live: TodoContextValue, + errors: string[], +): CommandContext { + return { + services: { + config: null, + agent: null, + settings: {} as unknown as CommandContext['services']['settings'], + git: undefined, + logger: { + error: (m: string) => errors.push(m), + } as unknown as CommandContext['services']['logger'], + }, + ui: { + addItem: ((item: { type?: string; text?: string }) => { + if (item.type === 'error') errors.push(item.text ?? 'error'); + }) as unknown as CommandContext['ui']['addItem'], + clear: () => {}, + setDebugMessage: () => {}, + pendingItem: null, + setPendingItem: () => {}, + loadHistory: () => {}, + toggleCorgiMode: () => {}, + toggleDebugProfiler: () => {}, + toggleVimEnabled: () => Promise.resolve(true), + setLlxprtMdFileCount: () => {}, + updateHistoryTokenCount: () => {}, + reloadCommands: () => {}, + extensionsUpdateState: + new Map() as CommandContext['ui']['extensionsUpdateState'], + dispatchExtensionStateUpdate: () => {}, + addConfirmUpdateExtensionRequest: () => {}, + }, + session: { + stats: {} as CommandContext['session']['stats'], + sessionShellAllowlist: new Set(), + }, + todoContext: { + todos: live.todos, + updateTodos: live.updateTodos, + // Production (useAppInput.ts) passes a no-op refreshTodos to the command + // context; no list subcommand invokes it. + refreshTodos: () => {}, + }, + }; +} + +async function runSubcommand( + live: TodoContextValue, + name: string, + args: string, +): Promise { + const sub = todoCommand.subCommands?.find((c) => c.name === name); + const action = sub?.action; + if (!action) throw new Error(`task subcommand "${name}" has no action`); + const errors: string[] = []; + await act(async () => { + // The action may be sync (most task operations) or async; normalize so a + // sync return is awaited without an await-thenable lint violation. + await Promise.resolve(action(commandContextFrom(live, errors), args)); + }); + return errors; +} + +/** + * Seed the session's store on disk, mount the provider, and settle its + * initial read so the provider is at the seed before assertions run. + */ +async function seedDiskAndMount( + sessionId: string, + seed: Todo[], + agentId?: string, +): Promise { + await storeFor(sessionId, agentId).writeTodos(seed); + const mounted = mountProvider(sessionId, agentId); + await act(async () => { + await settleRefresh(mounted.result.current.refreshTodos); + }); + expect(mounted.result.current.todos).toEqual(seed); + return mounted; +} + +/** Seed, observe, and run one subcommand; return the observed events. */ +async function seedObserveRun( + sessionId: string, + seed: Todo[], + name: string, + args: string, +): Promise { + const mounted = await seedDiskAndMount(sessionId, seed); + const events = observeTodoChannel(); + const errors = await runSubcommand(mounted.result.current, name, args); + expect(errors).toEqual([]); + return events; +} + +function todo( + id: string, + content: string, + status: Todo['status'] = 'pending', + subtasks?: NonNullable, +): Todo { + return subtasks ? { id, content, status, subtasks } : { id, content, status }; +} + +describe('TodoProvider observation (issue #3052)', () => { + describe('/todo clear on a non-empty list', () => { + it('publishes an empty replacement, clears provider state, and clears disk', async () => { + const sessionId = 'obs-clear'; + const mounted = await seedDiskAndMount(sessionId, [ + todo('c1', 'Write plan'), + todo('c2', 'Ship fix'), + ]); + expect(mounted.result.current.todos).toHaveLength(2); + const events = observeTodoChannel(); + + const errors = await runSubcommand(mounted.result.current, 'clear', ''); + expect(errors).toEqual([]); + + expect(events).toHaveLength(1); + expect(events[0].todos).toEqual([]); + expect(mounted.result.current.todos).toEqual([]); + await waitFor(() => { + expect(readDiskTodos(sessionId)).toEqual([]); + }); + }); + }); + + describe('every mutation subcommand publishes to the observer', () => { + it('/todo set marks the task in_progress and publishes', async () => { + const events = await seedObserveRun( + 'obs-set', + [todo('s1', 'Task one')], + 'set', + '1', + ); + expect(events).toHaveLength(1); + expect(events[0].todos).toEqual([ + { id: 's1', content: 'Task one', status: 'in_progress' }, + ]); + }); + + it('/todo unset returns the task to pending and publishes', async () => { + const events = await seedObserveRun( + 'obs-unset', + [todo('u1', 'Task one', 'in_progress')], + 'unset', + '1', + ); + expect(events).toHaveLength(1); + expect(events[0].todos).toEqual([ + { id: 'u1', content: 'Task one', status: 'pending' }, + ]); + }); + + it('/todo add inserts a task and publishes the grown list', async () => { + const events = await seedObserveRun( + 'obs-add', + [todo('a1', 'First')], + 'add', + '2 Second', + ); + expect(events).toHaveLength(1); + expect(events[0].todos).toHaveLength(2); + expect(events[0].todos[1].content).toBe('Second'); + }); + + it('/todo add 1.2 inserts a subtask and publishes', async () => { + const events = await seedObserveRun( + 'obs-add-subtask', + [ + todo('p1', 'Parent', 'pending', [ + { id: 'p1.1', content: 'First sub' }, + ]), + ], + 'add', + '1.2 Second sub', + ); + expect(events).toHaveLength(1); + const published = events[0].todos[0]; + expect(published.subtasks?.length).toBe(2); + expect(published.subtasks?.[1]?.content).toBe('Second sub'); + }); + + it('/todo remove drops the task and publishes', async () => { + const events = await seedObserveRun( + 'obs-remove-n', + [todo('r1', 'Keep'), todo('r2', 'Drop')], + 'remove', + '2', + ); + expect(events).toHaveLength(1); + expect(events[0].todos).toEqual([todo('r1', 'Keep')]); + }); + + it('/todo remove 1.2 drops the subtask and publishes', async () => { + const events = await seedObserveRun( + 'obs-remove-subtask', + [ + todo('p1', 'Parent', 'pending', [ + { id: 'p1.1', content: 'Keep' }, + { id: 'p1.2', content: 'Drop' }, + ]), + ], + 'remove', + '1.2', + ); + expect(events).toHaveLength(1); + const published = events[0].todos[0]; + expect(published.subtasks?.length).toBe(1); + expect(published.subtasks?.[0]?.content).toBe('Keep'); + }); + + it('/todo remove all empties the list and publishes an empty replacement', async () => { + const events = await seedObserveRun( + 'obs-remove-all', + [todo('ra1', 'One'), todo('ra2', 'Two')], + 'remove', + 'all', + ); + expect(events).toHaveLength(1); + expect(events[0].todos).toEqual([]); + }); + + it('/todo remove 2-4 drops the range and publishes', async () => { + const events = await seedObserveRun( + 'obs-remove-range', + [ + todo('rr1', 'One'), + todo('rr2', 'Two'), + todo('rr3', 'Three'), + todo('rr4', 'Four'), + todo('rr5', 'Five'), + ], + 'remove', + '2-4', + ); + expect(events).toHaveLength(1); + expect(events[0].todos).toEqual([ + todo('rr1', 'One'), + todo('rr5', 'Five'), + ]); + }); + + it('/todo undo resets status to pending and publishes', async () => { + const events = await seedObserveRun( + 'obs-undo', + [todo('z1', 'In flight', 'in_progress')], + 'undo', + '1', + ); + expect(events).toHaveLength(1); + expect(events[0].todos).toEqual([ + { id: 'z1', content: 'In flight', status: 'pending' }, + ]); + }); + + it('/todo undo 2-4 resets the range to pending and publishes', async () => { + const events = await seedObserveRun( + 'obs-undo-range', + [ + todo('ur1', 'One'), + todo('ur2', 'Two', 'in_progress'), + todo('ur3', 'Three', 'completed'), + todo('ur4', 'Four', 'in_progress'), + todo('ur5', 'Five'), + ], + 'undo', + '2-4', + ); + expect(events).toHaveLength(1); + expect(events[0].todos).toEqual([ + todo('ur1', 'One'), + todo('ur2', 'Two'), + todo('ur3', 'Three'), + todo('ur4', 'Four'), + todo('ur5', 'Five'), + ]); + }); + + it('/todo undo all resets every task to pending and publishes', async () => { + const events = await seedObserveRun( + 'obs-undo-all', + [todo('ua1', 'One', 'in_progress'), todo('ua2', 'Two', 'completed')], + 'undo', + 'all', + ); + expect(events).toHaveLength(1); + expect(events[0].todos).toEqual([todo('ua1', 'One'), todo('ua2', 'Two')]); + }); + + it('/todo load publishes the loaded session', async () => { + const sessionId = 'obs-load'; + // Write a saved session file distinct from the active session file and + // pin its mtime to the future so getTodoSessionFiles lists it first + // (i.e. `load 1` resolves to it) regardless of other tests' files. + fs.mkdirSync(todosDir(), { recursive: true }); + const archivePath = path.join(todosDir(), 'todo-obs-load-archive.json'); + const archived: Todo[] = [todo('L1', 'Loaded task')]; + fs.writeFileSync( + archivePath, + JSON.stringify({ todos: archived, paused: false }), + ); + const future = new Date('2099-01-01T00:00:00Z'); + fs.utimesSync(archivePath, future, future); + cleanups.push(() => fs.rmSync(archivePath, { force: true })); + const mounted = await seedDiskAndMount(sessionId, [ + todo('pre', 'Before load'), + ]); + const events = observeTodoChannel(); + const errors = await runSubcommand(mounted.result.current, 'load', '1'); + expect(errors).toEqual([]); + + expect(events).toHaveLength(1); + expect(events[0].todos).toEqual(archived); + expect(mounted.result.current.todos).toEqual(archived); + }); + }); + + // The JSP observation seam (createTodoObservationSubscription) intentionally + // strips sessionId, so these assert agent/todos only. Session-id identity is + // asserted directly against the canonical event later. + describe('published payload carries provider agent scoping (JSP seam)', () => { + it('publishes the explicit agentId prop on the event', async () => { + const agentId = 'agent-7'; + const mounted = await seedDiskAndMount( + 'obs-scope-explicit', + [todo('e1', 'Scoped')], + agentId, + ); + const events = observeTodoChannel(); + await runSubcommand(mounted.result.current, 'clear', ''); + expect(events).toHaveLength(1); + expect(events[0].agentId).toBe(agentId); + }); + + it('publishes undefined agentId under the default (no agentId prop)', async () => { + const mounted = await seedDiskAndMount('obs-scope-default', [ + todo('d1', 'Default'), + ]); + const events = observeTodoChannel(); + await runSubcommand(mounted.result.current, 'clear', ''); + expect(events).toHaveLength(1); + expect(events[0].agentId).toBeUndefined(); + }); + }); + + describe('a single mutation applies once and publishes once', () => { + it('applies /todo add exactly once to provider state and emits one publication', async () => { + const sessionId = 'obs-once'; + const mounted = await seedDiskAndMount(sessionId, [todo('o1', 'Seed')]); + const events = observeTodoChannel(); + const rendersBefore = mounted.result.all.length; + + await runSubcommand(mounted.result.current, 'add', '2 Added'); + + // updateTodos applies local state exactly once (one committed render) + // and publishes exactly once. The origin's own echo is suppressed by the + // per-provider flag, so no second setTodos fires. + expect(mounted.result.all.length).toBe(rendersBefore + 1); + expect(events).toHaveLength(1); + expect(events[0].todos).toHaveLength(2); + }); + }); + + describe('external todoEvents emits still reach the provider (regression)', () => { + it('applies a TodoWrite-shaped emit for the same session/agent', async () => { + const sessionId = 'obs-ext-same'; + const agentId = 'agent-9'; + const mounted = await seedDiskAndMount( + sessionId, + [todo('x1', 'Original')], + agentId, + ); + const external: Todo[] = [todo('x2', 'From tool', 'in_progress')]; + act(() => { + todoEvents.emitTodoUpdated({ + sessionId, + agentId, + todos: external, + timestamp: new Date(), + }); + }); + expect(mounted.result.current.todos).toEqual(external); + }); + + it('ignores an emit for a different session', async () => { + const sessionId = 'obs-ext-diff'; + const mounted = await seedDiskAndMount(sessionId, [ + todo('k1', 'Keep me'), + ]); + const before = mounted.result.current.todos; + act(() => { + todoEvents.emitTodoUpdated({ + sessionId: 'some-other-session', + agentId: undefined, + todos: [todo('intruder', 'Should not apply')], + timestamp: new Date(), + }); + }); + expect(mounted.result.current.todos).toEqual(before); + }); + + it('ignores an emit for a different agent in the same session', async () => { + const sessionId = 'obs-ext-diff-agent'; + const agentId = 'agent-owner'; + const mounted = await seedDiskAndMount( + sessionId, + [todo('m1', 'Owner task')], + agentId, + ); + const before = mounted.result.current.todos; + act(() => { + todoEvents.emitTodoUpdated({ + sessionId, + agentId: 'agent-intruder', + todos: [todo('spy', 'Other agent')], + timestamp: new Date(), + }); + }); + expect(mounted.result.current.todos).toEqual(before); + }); + }); + + describe('shouldClearTodos + provider updateTodos choke point (auto-clear)', () => { + it('publishes an empty replacement when shouldClearTodos triggers updateTodos([])', async () => { + const sessionId = 'obs-autoclear'; + const mounted = await seedDiskAndMount(sessionId, [ + todo('ac1', 'Done one', 'completed'), + todo('ac2', 'Done two', 'completed'), + ]); + const events = observeTodoChannel(); + + expect(shouldClearTodos(mounted.result.current.todos)).toBe(true); + act(() => { + mounted.result.current.updateTodos([]); + }); + + expect(events).toHaveLength(1); + expect(events[0].todos).toEqual([]); + expect(mounted.result.current.todos).toEqual([]); + }); + }); + + // The JSP seam strips sessionId, so session-scoping is asserted directly + // against the canonical todoEvents payload here. + describe('raw canonical event carries provider session/agent identity', () => { + it('carries the explicit sessionId and agentId', async () => { + const sessionId = 'obs-raw-explicit'; + const agentId = 'agent-raw'; + const mounted = await seedDiskAndMount( + sessionId, + [todo('r1', 'Seed')], + agentId, + ); + + const rawEvents: TodoUpdateEvent[] = []; + const listener = (event: TodoUpdateEvent): void => { + rawEvents.push(event); + }; + todoEvents.onTodoUpdated(listener); + cleanups.push(() => todoEvents.offTodoUpdated(listener)); + + act(() => { + mounted.result.current.updateTodos([todo('r2', 'New')]); + }); + + expect(rawEvents.length).toBeGreaterThanOrEqual(1); + const last = rawEvents[rawEvents.length - 1]; + expect(last.sessionId).toBe(sessionId); + expect(last.agentId).toBe(agentId); + }); + + it('carries the sessionId with undefined agentId under the default', async () => { + const sessionId = 'obs-raw-default'; + const mounted = await seedDiskAndMount(sessionId, [todo('d1', 'Seed')]); + + const rawEvents: TodoUpdateEvent[] = []; + const listener = (event: TodoUpdateEvent): void => { + rawEvents.push(event); + }; + todoEvents.onTodoUpdated(listener); + cleanups.push(() => todoEvents.offTodoUpdated(listener)); + + act(() => { + mounted.result.current.updateTodos([todo('d2', 'New')]); + }); + + const last = rawEvents[rawEvents.length - 1]; + expect(last.sessionId).toBe(sessionId); + expect(last.agentId).toBeUndefined(); + }); + }); + + // The origin suppresses only its own exact synchronous event object. Because + // the echo carries the same array reference as the local state updateTodos + // just applied, suppression is not independently observable through a render + // count; these tests instead pin peer delivery and prove that later or + // synchronously nested external event objects remain authoritative. + describe('origin echo suppression (issue #3052)', () => { + it('delivers a provider-originated publication to a matching peer provider', async () => { + const sessionId = 'peer-deliver'; + const agentId = 'agent-peer'; + const seed = [todo('p-seed', 'Seed')]; + await storeFor(sessionId, agentId).writeTodos(seed); + + // Two independent provider instances share the same session/agent scope + // and the singleton todoEvents channel. + const origin = mountProvider(sessionId, agentId); + const peer = mountProvider(sessionId, agentId); + await act(async () => { + await settleRefresh(origin.result.current.refreshTodos); + await settleRefresh(peer.result.current.refreshTodos); + }); + expect(origin.result.current.todos).toEqual(seed); + expect(peer.result.current.todos).toEqual(seed); + + const events = observeTodoChannel(); + const published: Todo[] = [todo('p1', 'From origin', 'in_progress')]; + act(() => { + origin.result.current.updateTodos(published); + }); + + // The origin skips its exact publication object; the peer's listener still + // applies the replacement exactly once, and the observer receives exactly + // one publication. + expect(origin.result.current.todos).toEqual(published); + expect(peer.result.current.todos).toEqual(published); + expect(events).toHaveLength(1); + expect(events[0].todos).toEqual(published); + }); + + it('a provider-originated publication does not suppress a later external event', async () => { + const sessionId = 'echo-s'; + const agentId = 'agent-echo'; + const mounted = await seedDiskAndMount( + sessionId, + [todo('e-seed', 'Seed')], + agentId, + ); + + act(() => { + mounted.result.current.updateTodos([todo('e1', 'Origin update')]); + }); + + const external: Todo[] = [todo('e2', 'External', 'completed')]; + act(() => { + todoEvents.emitTodoUpdated({ + sessionId, + agentId, + todos: external, + timestamp: new Date(), + }); + }); + expect(mounted.result.current.todos).toEqual(external); + }); + + it('does not mistake a synchronously nested external event for the origin publication', async () => { + const sessionId = 'echo-nested'; + const agentId = 'agent-nested'; + const mounted = await seedDiskAndMount( + sessionId, + [todo('n-seed', 'Seed')], + agentId, + ); + const nested: Todo[] = [todo('n-external', 'Nested external')]; + let nestedPublished = false; + const publishNested = (): void => { + if (nestedPublished) return; + nestedPublished = true; + todoEvents.emitTodoUpdated({ + sessionId, + agentId, + todos: nested, + timestamp: new Date(), + }); + }; + todoEvents.prependListener(TodoEvent.TODO_UPDATED, publishNested); + cleanups.push(() => + todoEvents.removeListener(TodoEvent.TODO_UPDATED, publishNested), + ); + + act(() => { + mounted.result.current.updateTodos([todo('n-origin', 'Origin')]); + }); + + expect(mounted.result.current.todos).toEqual(nested); + }); + + it('preserves a synchronously nested provider update as the final list', async () => { + const sessionId = 'echo-nested-provider'; + const agentId = 'agent-nested-provider'; + const mounted = await seedDiskAndMount( + sessionId, + [todo('np-seed', 'Seed')], + agentId, + ); + + // Spy ONLY on the TodoStore write boundary so the outer write can be held, + // making persistence ordering deterministic rather than dependent on the + // filesystem scheduler. The provider, React, the event emitter, and the + // observation seam stay real; only the storage write is intercepted, and + // the real captured write is invoked on release (no mock theater). + const realWriteTodos = TodoStore.prototype.writeTodos; + let outerCapture: { store: TodoStore; todos: Todo[] } | null = null; + let writeTodosCalls = 0; + let releaseOuterWrite: () => void = () => {}; + const outerWriteHeld = new Promise((resolve) => { + releaseOuterWrite = resolve; + }); + TodoStore.prototype.writeTodos = function ( + this: TodoStore, + todos: Todo[], + ): Promise { + writeTodosCalls++; + if (outerCapture === null) { + outerCapture = { store: this, todos }; + return outerWriteHeld; + } + return realWriteTodos.call(this, todos); + }; + cleanups.push(() => { + TodoStore.prototype.writeTodos = realWriteTodos; + }); + + const nested: Todo[] = [todo('np-nested', 'Nested provider update')]; + let nestedPublished = false; + const publishNested = (): void => { + if (nestedPublished) return; + nestedPublished = true; + mounted.result.current.updateTodos(nested); + }; + todoEvents.prependListener(TodoEvent.TODO_UPDATED, publishNested); + cleanups.push(() => + todoEvents.removeListener(TodoEvent.TODO_UPDATED, publishNested), + ); + + act(() => { + mounted.result.current.updateTodos([todo('np-origin', 'Origin')]); + }); + + expect(mounted.result.current.todos).toEqual(nested); + + // The nested write is chained behind the held outer write, so it has not + // started: only the outer write was invoked, and disk still carries the + // seed. This proves the nested write is queued, not racing. + expect(writeTodosCalls).toBe(1); + expect(readDiskTodos(sessionId, agentId)).toEqual([ + todo('np-seed', 'Seed'), + ]); + + // Release: invoke the real captured outer write (persisting Origin), then + // unblock the chain so the queued nested write runs and wins on disk. + const capture = unwrapOuterCapture(outerCapture); + await realWriteTodos.call(capture.store, capture.todos); + expect(readDiskTodos(sessionId, agentId)).toEqual([ + todo('np-origin', 'Origin'), + ]); + releaseOuterWrite(); + await waitFor(() => { + expect(readDiskTodos(sessionId, agentId)).toEqual(nested); + }); + }); + }); +}); diff --git a/project-plans/issue3052/plan.md b/project-plans/issue3052/plan.md new file mode 100644 index 0000000000..d8c7468bd5 --- /dev/null +++ b/project-plans/issue3052/plan.md @@ -0,0 +1,238 @@ +# Issue #3052 — Publish provider-originated todo edits to observers + +## Problem + +`TodoWrite` (the tool) and `TodoProvider.updateTodos` (the CLI) are two +independent write paths to the task list. Only the tool path published +`todoEvents.emitTodoUpdated`: + +- `packages/tools/src/tools/todo-write.ts` writes the store, then emits + unconditionally — including for an empty array. +- `TodoProvider.updateTodos` did `setTodos(newTodos)` + `store.writeTodos(...)` + and never emitted. + +Everything that reaches an external observer is downstream of the event: + +- `packages/cli/src/observation/jspWiring.ts` `createTodoObservationSubscription` + subscribes to `todoEvents` and forwards to `observeTodosReplaced`, which calls + `JspProducer.observeTodosReplaced` and publishes a `todos.replaced` transition. +- `packages/cli/src/zed-integration/zedIntegration.ts` subscribes to the same + event. + +Because `updateTodos` never emitted, every CLI-side mutation was invisible to +those consumers, and the last tool-written list was retained indefinitely. + +## Invariant (bounded scope) + +Every provider-originated mutation that changes the externally observed current +state publishes exactly once on the canonical `todoEvents` channel. This mirrors +the TodoWrite tool's canonical event **shape and channel**, not its call +ordering. The originating provider's own `todoEvents` listener must not re-enter +on its own echo. + +## Affected call sites (all funnel through `updateTodos`) + +- `packages/cli/src/ui/commands/todoCommand.ts` — `clear`, `remove all`, `load` +- `packages/cli/src/ui/commands/todoOperations.ts` — `applyStatusChange`, + `addSubtaskAtPosition`, `addTaskAtPosition`, `removeSubtaskAtPosition`, + `removeTaskAtPosition`, `removeRangeOfTasks`, `undoAllTodos`, + `undoRangeOfTodos`, `undoSingleTodo` +- `packages/cli/src/ui/containers/AppContainer/hooks/useAppInput.ts` — the + auto-clear of a fully-completed list on the next user submit + +Patching individual commands is wrong. The fix belongs at the choke point: +`updateTodos` publishes after applying state. `refreshTodos` does **not** +publish (see "Out of scope"). + +## Chosen fix + +A shared module-level `publishTodos` helper constructs the canonical event and +the single write path (`updateTodos`) calls it exactly once. + +``` +updateTodos(newTodos) + -> setTodos(newTodos) (optimistic UI) + -> persistOrdered(newTodos) (disk; fire-and-forget, but ordered — see below; + failure -> error) + -> publishTodos(...) (records exact event in origin ref, + then synchronously emits to observer + peer) +``` + +### Synchronous, optimistic publication semantics (deliberate) + +`updateTodos` keeps its existing synchronous API and its established optimistic +UI + fire-and-forget persistence semantics. Publication happens **synchronously +after** local state is applied and persistence is initiated: the observation +contract is "current accepted UI state", and emitting immediately preserves the +fail-fast behavior of the synchronous event emitter (a throwing observer +propagates to the caller). This issue does **not** broaden into converting every +`updateTodos` caller to async, nor does it change persistence ordering relative +to the tool path. We claim identical canonical event shape/channel only — not +identical ordering to TodoWrite. + +### Per-provider ordered persistence (required by synchronous nesting) + +`persistOrdered` keeps persistence fire-and-forget (no caller becomes async), +but it orders writes **per provider** so they reach disk in update call order. +The hook holds an `inFlightWriteRef` (`Promise | null`). The first write +(ref null) starts immediately — before publication — preserving optimistic +semantics; each subsequent write chains behind the in-flight one via +`previous.then(write, write)` (the rejection branch runs the write anyway, so a +failed write reports the save error but does not poison later writes). When the +tail settles and is still current, the ref clears to null so the next write is a +fresh immediate start. + +This is required, not optional: the synchronous, fail-fast publication means a +prepended `todoEvents` listener can invoke a nested `updateTodos` while the +outer publish is still on the stack. Without ordering, the outer and nested +fire-and-forget writes race on the same store file and can complete out of +order, leaving the stale outer data on disk (a real failure CI caught that +local focused runs masked). Ordering does not await persistence before +publishing and does not convert any caller to async; it only serializes the +per-provider write chain. + +### Per-provider echo suppression (issue #3052) + +A provider-originated publication would otherwise re-enter the provider's own +`todoEvents` listener (the listener that mirrors external TodoWrite events into +local state). Each `TodoProvider` instance holds an `originPublicationRef`. `publishTodos` +constructs the canonical event object, records that exact object in the ref, +emits synchronously, and clears the ref in `finally`. The origin's listener skips +only an event whose object identity matches the ref. The ref is per-instance, +so: + +- the origin's own listener does not re-enter on its echo; +- external observers (JSP/jefe, Zed) receive the event exactly once; +- any matching peer provider (same session/agent) receives the event exactly + once — its ref does not contain the event; +- external or synchronously nested events use different event objects and remain + authoritative. + +Because the echo carries the same array reference `updateTodos` just applied, +suppression is not independently observable through a render count. Behavioral +coverage therefore verifies peer delivery, later external-event authority, and +a synchronously nested external event that must not be mistaken for the origin's +publication. + +## Out of scope (with rationale) + +### Mount/session refresh publication — OUT OF SCOPE + +The mount read and session-change read do **not** publish. `TodoStore.readTodos` +maps a parse or I/O failure to an empty list (`[]`), so a refresh cannot +distinguish an authoritative empty list from a load failure. Publishing the +refresh would risk advertising a stale or failed state as current, which is +outside the mutation bug this issue fixes. External TodoWrite events remain the +authoritative source for external observers. + +### generationRef / concurrency / stale-read coordination — OUT OF SCOPE + +The `generationRef` counter, the in-flight-refresh invalidation, and the +deferred-read race guard existed solely to make refresh publication safe (a +`/todo clear` while a mount read is resolving could otherwise clobber newer +accepted state). With refresh no longer publishing, that coordination has no +purpose and was removed; refresh behavior is restored close to HEAD (a plain +try/catch read with no generation gating). Reactive runtime session handoff is +also out of scope. + +### Async API conversion (await persistence before publishing) — OUT OF SCOPE + +A recommendation to make the provider await persistence before publishing +(equalizing ordering with the tool path) is intentionally **rejected**. It would +be a major API and call-graph expansion (every `updateTodos` caller would become +async) far beyond the mutation-visibility bug, and it would change established +optimistic semantics. The fix keeps the existing synchronous, optimistic, +fire-and-forget persistence and makes documentation honest about the difference. + +### Per-provider ordered persistence — IN SCOPE (required) + +What **is** in scope — and required — is ordered fire-and-forget persistence +*within one provider*. The synchronous, fail-fast publication means a prepended +`todoEvents` listener can invoke a nested `updateTodos` while the outer publish +is still on the stack. Two concurrent fire-and-forget `TodoStore` writes to the +same file would then race and could complete out of order, leaving the stale +outer data on disk — a real race CI caught that local focused runs masked. The +fix (see "Per-provider ordered persistence" above) chains writes per provider so +they reach disk in update call order, while keeping `updateTodos` synchronous and +fire-and-forget. Async API conversion (awaiting persistence before publishing) +remains out of scope; only the per-provider write chain is serialized. + +## `/todo delete` is deliberately untouched + +`/todo delete` (`deleteAllSessions` / `deleteSessionRange` / +`deleteSingleSession` in `todoOperations.ts`) operates on saved session files +via `fs.unlinkSync`, never calls `updateTodos`, and is therefore untouched. + +## Tests (behavioral, Bun, no mock theater) + +File: `packages/cli/src/ui/contexts/__tests__/todoProvider.observation.bun.tsx`. + +These are provider-to-canonical-observation-seam integration tests. The wiring +under test is real: + +- Render the real `TodoProvider` and capture its real context value. +- Subscribe with the real `createTodoObservationSubscription` — the exact seam + `JspProducer` subscribes through — so a passing assertion proves the + observation channel is reached. +- Drive the real `todoCommand` subcommands with a `CommandContext` whose + `todoContext` is the LIVE provider context. + +The provider, React, the event emitter, and the observation seam stay real. +Storage is isolated to a per-process temp dir by the manifest preloads; each +test uses a unique sessionId. A peer provider is mounted in its own React root +to assert peer delivery. The only place storage is intercepted is the +nested-provider persistence test, which spies on `TodoStore`'s write boundary +(`TodoStore.prototype.writeTodos`) solely to hold the outer write and prove the +nested write is queued behind it, then invokes the real captured write — the +provider, React, the event emitter, and the observation seam are not mocked. +This makes that regression deterministic rather than dependent on the filesystem +scheduler (CI caught a race local focused runs masked). + +Coverage: + +1. `/todo clear` publishes an empty replacement, clears provider state and disk. +2. Every mutation subcommand publishes: `set`, `unset`, `add`, `add 1.2`, + `remove `, `remove 1.2`, `remove all`, `remove 2-4`, `undo`, `undo 2-4`, + `undo all`, `load`. +3. Published payload carries the provider `agentId` (explicit and default) on the + JSP seam. +4. A single mutation applies to provider state exactly once (render count) and + publishes once. +5. External `todoEvents` emits still reach the provider (same session/agent), + and are ignored for a different session or agent. +6. The `shouldClearTodos` + `updateTodos([])` auto-clear choke point publishes. +7. The raw canonical event carries `sessionId`/`agentId` identity. +8. Origin echo suppression: a provider-originated publication still reaches a + matching peer provider (per-instance flag), and a later authoritative + external event is still applied (the flag is one-shot and does not leak). +9. A synchronously nested provider update is the final list on disk. The test + holds the outer write via the `TodoStore` write-boundary spy, proves the + nested write is queued (only the outer write invoked, disk still seeded), + then releases the real captured outer write and asserts the nested write wins + on disk — deterministic, not filesystem-scheduler dependent. + +## Manifest registration + +The file lives in its own manifest entry in `scripts/bun-test-manifest.ts` +(`workspace: 'cli'`) with both preloads: + +- `test-setup-storage-isolation.ts` — redirects `Storage` roots to a per-process + temp dir, so the real `TodoStore` disk I/O is sandboxed. +- `bun-test-setup.ts` — the React/Ink/JSDOM setup. + +The `*.bun.tsx` suffix keeps it out of Vitest's `**/*.{test,spec}.*` include, +so no Vitest exclusion entry is needed. + +The focused invocation (the @ast-grep `napi-darwin-arm64` native optional +binding must be installed; `bun install` restores it without changing +`bun.lock`): + +``` +bun scripts/run_bun_tests.ts todoProvider.observation.bun.tsx +``` + +## Verification + +`npm run test`, `npm run lint`, `npm run typecheck`, `npm run format`, +`npm run build`, and +`bun scripts/start.ts --profile-load stepfun-37 "write me a haiku and nothing else"`. diff --git a/scripts/bun-test-manifest.ts b/scripts/bun-test-manifest.ts index c5eaa7b393..8e92f158ab 100644 --- a/scripts/bun-test-manifest.ts +++ b/scripts/bun-test-manifest.ts @@ -219,6 +219,16 @@ export const BUN_NATIVE_TEST_MANIFEST: readonly BunTestWorkspaceEntry[] = [ 'src/ui/hooks/agentStream/__tests__/useSubmitQuery.terminalError.bun.tsx', ], }, + { + // Issue #3052: TodoProvider must publish slash-command mutations on the + // todoEvents observation channel. Uses the real provider + real + // createTodoObservationSubscription seam (no mocks on that seam). Drives + // the REAL TodoStore against disk, so it isolates storage roots via the + // shared preload in addition to the React/Ink setup. + workspace: 'cli', + preload: ['test-setup-storage-isolation.ts', 'bun-test-setup.ts'], + files: ['src/ui/contexts/__tests__/todoProvider.observation.bun.tsx'], + }, { workspace: 'core', files: [