Skip to content

Commit 49a2e84

Browse files
committed
refactor(chat): share reactor event-subscriber helper across sinks
Extract the duplicated tasks-changed/tools-activate subscriber blocks into handleChatDirectorEvent, log invalid payloads at debug level instead of dropping them silently, and clear queued task-change notifications when a turn throws.
1 parent b7b8954 commit 49a2e84

6 files changed

Lines changed: 299 additions & 46 deletions

File tree

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
import { describe, expect, test } from "bun:test";
2+
import {
3+
CHAT_TASKS_CHANGED_EVENT,
4+
CHAT_TOOLS_ACTIVATE_EVENT,
5+
} from "./director.js";
6+
import { handleChatDirectorEvent } from "./chat-event-subscribers.js";
7+
import type { Task } from "./tasks.js";
8+
9+
function makeLog() {
10+
const calls: { message: string; fields?: Record<string, unknown> }[] = [];
11+
return {
12+
calls,
13+
log: (message: string, fields?: Record<string, unknown>): void => {
14+
calls.push(fields !== undefined ? { message, fields } : { message });
15+
},
16+
};
17+
}
18+
19+
describe("handleChatDirectorEvent", () => {
20+
test("dispatches a valid tasks-changed payload without logging", () => {
21+
const seen: Task[][] = [];
22+
const log = makeLog();
23+
const handled = handleChatDirectorEvent(
24+
{
25+
type: CHAT_TASKS_CHANGED_EVENT,
26+
data: { tasks: [{ id: "t1", title: "work", status: "doing" }] },
27+
},
28+
{
29+
onTasksChanged: (tasks) => seen.push(tasks),
30+
onToolsActivate: () => {
31+
throw new Error("unexpected tools-activate dispatch");
32+
},
33+
},
34+
log.log,
35+
);
36+
expect(handled).toBe(true);
37+
expect(seen).toEqual([[{ id: "t1", title: "work", status: "doing" }]]);
38+
expect(log.calls).toEqual([]);
39+
});
40+
41+
test("dispatches a valid tools-activate payload without logging", () => {
42+
const seen: string[][] = [];
43+
const log = makeLog();
44+
const handled = handleChatDirectorEvent(
45+
{ type: CHAT_TOOLS_ACTIVATE_EVENT, data: { names: ["lsp"] } },
46+
{
47+
onTasksChanged: () => {
48+
throw new Error("unexpected tasks-changed dispatch");
49+
},
50+
onToolsActivate: (names) => seen.push([...names]),
51+
},
52+
log.log,
53+
);
54+
expect(handled).toBe(true);
55+
expect(seen).toEqual([["lsp"]]);
56+
expect(log.calls).toEqual([]);
57+
});
58+
59+
test("drops an invalid tasks payload with a debug log naming the failure", () => {
60+
let dispatched = false;
61+
const log = makeLog();
62+
const handled = handleChatDirectorEvent(
63+
{ type: CHAT_TASKS_CHANGED_EVENT, data: { tasks: "not-a-list" } },
64+
{
65+
onTasksChanged: () => {
66+
dispatched = true;
67+
},
68+
onToolsActivate: () => {
69+
dispatched = true;
70+
},
71+
},
72+
log.log,
73+
);
74+
expect(handled).toBe(true);
75+
expect(dispatched).toBe(false);
76+
expect(log.calls).toHaveLength(1);
77+
expect(log.calls[0]?.message).toMatch(/tasks-changed/);
78+
expect(typeof log.calls[0]?.fields?.["error"]).toBe("string");
79+
});
80+
81+
test("drops an invalid tools payload with a debug log naming the failure", () => {
82+
let dispatched = false;
83+
const log = makeLog();
84+
const handled = handleChatDirectorEvent(
85+
{ type: CHAT_TOOLS_ACTIVATE_EVENT, data: { names: [42] } },
86+
{
87+
onTasksChanged: () => {
88+
dispatched = true;
89+
},
90+
onToolsActivate: () => {
91+
dispatched = true;
92+
},
93+
},
94+
log.log,
95+
);
96+
expect(handled).toBe(true);
97+
expect(dispatched).toBe(false);
98+
expect(log.calls).toHaveLength(1);
99+
expect(log.calls[0]?.message).toMatch(/tools-activate/);
100+
expect(typeof log.calls[0]?.fields?.["error"]).toBe("string");
101+
});
102+
103+
test("ignores unrelated events without logging or dispatching", () => {
104+
let dispatched = false;
105+
const log = makeLog();
106+
const handled = handleChatDirectorEvent(
107+
{ type: "inference.done", data: {} },
108+
{
109+
onTasksChanged: () => {
110+
dispatched = true;
111+
},
112+
onToolsActivate: () => {
113+
dispatched = true;
114+
},
115+
},
116+
log.log,
117+
);
118+
expect(handled).toBe(false);
119+
expect(dispatched).toBe(false);
120+
expect(log.calls).toEqual([]);
121+
});
122+
});
Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
/**
2+
* Shared subscriber for the chat-director reactor events. The TUI and exec
3+
* stream sinks both listen for the task-list and tool-activation events the
4+
* chat director emits in place of the former host closures; the parse,
5+
* validation, and invalid-payload handling live here so the two sinks cannot
6+
* drift apart.
7+
*/
8+
9+
import { type } from "arktype";
10+
import {
11+
CHAT_TASKS_CHANGED_EVENT,
12+
CHAT_TOOLS_ACTIVATE_EVENT,
13+
ChatTasksChangedDataSchema,
14+
ChatToolsActivateDataSchema,
15+
} from "./director.js";
16+
import type { Task } from "./tasks.js";
17+
18+
export interface ChatDirectorEventHandlers {
19+
onTasksChanged: (tasks: Task[]) => void;
20+
onToolsActivate: (names: string[]) => void;
21+
}
22+
23+
export type ChatDirectorEventDebugLog = (
24+
message: string,
25+
fields?: Record<string, unknown>,
26+
) => void;
27+
28+
/**
29+
* Dispatch one stream event to the chat-director handlers. Returns true when
30+
* the event is a chat-director event (valid or not) so sinks can fall through
31+
* to their own handling otherwise. Invalid payloads are dropped after a
32+
* debug-level log naming the failure — never silently.
33+
*/
34+
export function handleChatDirectorEvent(
35+
event: { type: string; data: unknown },
36+
handlers: ChatDirectorEventHandlers,
37+
logDebug: ChatDirectorEventDebugLog,
38+
): boolean {
39+
if (event.type === CHAT_TASKS_CHANGED_EVENT) {
40+
const parsed = ChatTasksChangedDataSchema(event.data);
41+
if (parsed instanceof type.errors) {
42+
logDebug("chat tasks-changed event dropped invalid payload: {error}", {
43+
error: parsed.summary,
44+
});
45+
return true;
46+
}
47+
handlers.onTasksChanged(parsed.tasks);
48+
return true;
49+
}
50+
if (event.type === CHAT_TOOLS_ACTIVATE_EVENT) {
51+
const parsed = ChatToolsActivateDataSchema(event.data);
52+
if (parsed instanceof type.errors) {
53+
logDebug("chat tools-activate event dropped invalid payload: {error}", {
54+
error: parsed.summary,
55+
});
56+
return true;
57+
}
58+
handlers.onToolsActivate(parsed.names);
59+
return true;
60+
}
61+
return false;
62+
}

‎src/agent/director.test.ts‎

Lines changed: 72 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,12 @@ import type {
55
ReactorInboundEvent,
66
ReactorState,
77
} from "@intx/types/runtime";
8-
import { createChatDirector, toolSetDigest } from "./director.js";
8+
import {
9+
CHAT_TASKS_CHANGED_EVENT,
10+
createChatDirector,
11+
toolSetDigest,
12+
} from "./director.js";
13+
import type { WorkflowCoordinator } from "../workflows/coordinator.js";
914

1015
const mockState: ReactorState = { turns: [] } as unknown as ReactorState;
1116

@@ -356,4 +361,70 @@ describe("ChatDirector inference-error recovery (CL-6910)", () => {
356361
"unrecoverable inference error",
357362
);
358363
});
364+
365+
// A turn that throws after queueing task-change notifications must drop the
366+
// queue instead of flushing it stale on the next turn.
367+
test("a throwing turn drops queued task-change notifications", async () => {
368+
const throwingCoordinator = {
369+
isActive: () => true,
370+
currentStepIsGate: () => true,
371+
currentStepId: () => null,
372+
directive: () => {
373+
throw new Error("tool-listing exploded");
374+
},
375+
handleToolDone: () => false,
376+
} as unknown as WorkflowCoordinator;
377+
const director = createChatDirector("system", [], {
378+
workflowCoordinator: throwingCoordinator,
379+
});
380+
const capabilities = makeCapabilities();
381+
382+
const manageTasksTurn = {
383+
type: "inference.done",
384+
turn: {
385+
role: "assistant",
386+
model: "test",
387+
timestamp: 0,
388+
content: [
389+
{
390+
type: "tool_call",
391+
id: "manage-tasks",
392+
name: "manage_tasks",
393+
arguments: {
394+
action: "create",
395+
tasks: [{ id: "t1", title: "work", status: "doing" }],
396+
},
397+
},
398+
],
399+
},
400+
usage: { input: 0, output: 0 },
401+
source: "test",
402+
} as unknown as ReactorInboundEvent;
403+
404+
await expect(
405+
director.decide(manageTasksTurn, mockState, capabilities),
406+
).rejects.toThrow("tool-listing exploded");
407+
408+
director.setWorkflowCoordinator(undefined);
409+
const textTurn = {
410+
type: "inference.done",
411+
turn: {
412+
role: "assistant",
413+
model: "test",
414+
timestamp: 0,
415+
content: [{ type: "text", text: "all set" }],
416+
},
417+
usage: { input: 0, output: 0 },
418+
source: "test",
419+
} as unknown as ReactorInboundEvent;
420+
const actions = actionsArray(
421+
await director.decide(textTurn, mockState, capabilities),
422+
);
423+
const stale = actions.filter(
424+
(a) =>
425+
a.type === "emit" &&
426+
(a as { eventType?: string }).eventType === CHAT_TASKS_CHANGED_EVENT,
427+
);
428+
expect(stale).toEqual([]);
429+
});
359430
});

‎src/agent/director.ts‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -682,15 +682,26 @@ class ChatDirectorImpl extends DefaultDirector {
682682
state: ReactorState,
683683
capabilities: ReactorCapabilities,
684684
): Promise<ReactorAction | ReactorAction[]> {
685-
const settled = ensureCycleSettlesWithReply(
686-
await this.decideInner(event, state, capabilities),
687-
capabilities,
688-
);
689-
const withTools = this.withCurrentTools(settled);
690-
if (this.pendingEmits.length === 0) return withTools;
691-
const emits = this.pendingEmits;
692-
this.pendingEmits = [];
693-
return [...(Array.isArray(withTools) ? withTools : [withTools]), ...emits];
685+
try {
686+
const settled = ensureCycleSettlesWithReply(
687+
await this.decideInner(event, state, capabilities),
688+
capabilities,
689+
);
690+
const withTools = this.withCurrentTools(settled);
691+
if (this.pendingEmits.length === 0) return withTools;
692+
const emits = this.pendingEmits;
693+
this.pendingEmits = [];
694+
return [
695+
...(Array.isArray(withTools) ? withTools : [withTools]),
696+
...emits,
697+
];
698+
} catch (err) {
699+
// A failed turn must not leak its queued task/tool notifications into
700+
// the next turn — drop them so the next turn starts clean instead of
701+
// flushing stale updates.
702+
this.pendingEmits = [];
703+
throw err;
704+
}
694705
}
695706

696707
private async decideInner(

‎src/exec/runner.ts‎

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -21,14 +21,8 @@ import {
2121
import { formatDirectorSystemPrompt } from "../agent/directors/identity.js";
2222
import { DIRECTOR_REGISTRY } from "../agent/directors/registry.js";
2323
import type { DirectorId, DirectorPackage } from "../agent/directors/types.js";
24-
import {
25-
CHAT_TASKS_CHANGED_EVENT,
26-
CHAT_TOOLS_ACTIVATE_EVENT,
27-
ChatTasksChangedDataSchema,
28-
ChatToolsActivateDataSchema,
29-
submitOutputDefinition,
30-
} from "../agent/director.js";
31-
import { type } from "arktype";
24+
import { submitOutputDefinition } from "../agent/director.js";
25+
import { handleChatDirectorEvent } from "../agent/chat-event-subscribers.js";
3226
import {
3327
shellDefinition,
3428
updatePlanDefinition,
@@ -976,18 +970,18 @@ export async function runExec(config: Config): Promise<ExecResult> {
976970
// stdout output today (unlike the TUI's chrome zone) — debug logging
977971
// is the closest match to how this mode already surfaces other
978972
// in-session state changes.
979-
if (event.type === CHAT_TASKS_CHANGED_EVENT) {
980-
const parsed = ChatTasksChangedDataSchema(event.data);
981-
if (!(parsed instanceof type.errors)) {
982-
logger.debug("tasks updated: {tasks}", {
983-
tasks: parsed.tasks.map((t) => `${t.status}:${t.title}`).join(", "),
984-
});
985-
}
986-
} else if (event.type === CHAT_TOOLS_ACTIVATE_EVENT) {
987-
const parsed = ChatToolsActivateDataSchema(event.data);
988-
if (!(parsed instanceof type.errors))
989-
activatedToolNames.activate(parsed.names);
990-
}
973+
handleChatDirectorEvent(
974+
event,
975+
{
976+
onTasksChanged: (tasks) => {
977+
logger.debug("tasks updated: {tasks}", {
978+
tasks: tasks.map((t) => `${t.status}:${t.title}`).join(", "),
979+
});
980+
},
981+
onToolsActivate: (names) => activatedToolNames.activate(names),
982+
},
983+
(message, fields) => logger.debug(message, fields),
984+
);
991985
if (event.type === "inference.start" || event.type === "inference.done") {
992986
providerFailureObserved = false;
993987
providerError = undefined;

‎src/tui/runner/exit.ts‎

Lines changed: 9 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -58,13 +58,7 @@ import {
5858
type SnapshotKind,
5959
type SnapshotStatus,
6060
} from "./state.js";
61-
import { type } from "arktype";
62-
import {
63-
CHAT_TASKS_CHANGED_EVENT,
64-
CHAT_TOOLS_ACTIVATE_EVENT,
65-
ChatTasksChangedDataSchema,
66-
ChatToolsActivateDataSchema,
67-
} from "../../agent/director.js";
61+
import { handleChatDirectorEvent } from "../../agent/chat-event-subscribers.js";
6862

6963
const tuiLogger = getLogger([LOG_NAMESPACE_ROOT, "tui"]);
7064

@@ -309,15 +303,14 @@ export async function createRunLifecycle(
309303
// Chat-director reactor events (replacing the former onTasksChange /
310304
// onActivateTools closures): task-list changes repaint the chrome panel,
311305
// tool activation opens the call gate for the named tools.
312-
if (event.type === CHAT_TASKS_CHANGED_EVENT) {
313-
const parsed = ChatTasksChangedDataSchema(event.data);
314-
if (!(parsed instanceof type.errors))
315-
services.emitter.emit("tasks", parsed.tasks);
316-
} else if (event.type === CHAT_TOOLS_ACTIVATE_EVENT) {
317-
const parsed = ChatToolsActivateDataSchema(event.data);
318-
if (!(parsed instanceof type.errors))
319-
services.activatedToolNames.activate(parsed.names);
320-
}
306+
handleChatDirectorEvent(
307+
event,
308+
{
309+
onTasksChanged: (tasks) => services.emitter.emit("tasks", tasks),
310+
onToolsActivate: (names) => services.activatedToolNames.activate(names),
311+
},
312+
(message, fields) => tuiLogger.debug(message, fields),
313+
);
321314
services.runSink.sink(eventForSink);
322315
services.cycleRecorder.handleEvent(event);
323316
if (onTurnBoundary(event)) {

0 commit comments

Comments
 (0)