Skip to content

Commit a42f832

Browse files
committed
Fix the resume strip/apply mismatch and make onTasksChange required
Resumed transcripts stripped a manage_tasks call's raw rows only when it had a successful tool_result, but hydrateTasksFromTurns already applies the call unconditionally on the tool_call itself — a call with an errored or missing result kept its raw rows next to the aggregated task block instead of being replaced by it. The strip now matches the apply: manage_tasks rows come out regardless of the result's outcome, because the tool_call is what the underlying tool's side-effect-free handler makes authoritative, not whatever result eventually shows up. onTasksChange moves from optional to required on ChatDirectorOptions, same motivation as CL-5709: an omitted required field is a visible gap in a caller's diff, not an invisible one.
1 parent a796ab5 commit a42f832

9 files changed

Lines changed: 117 additions & 57 deletions

File tree

‎src/agent/director.test.ts‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ describe("ChatDirector tool-only loop protection", () => {
100100
const providerlessPolicy = { providerName: "test-provider" };
101101

102102
test("nudges once at the family threshold, after pending tools execute", async () => {
103-
const director = createChatDirector("system", [], { provider: providerlessPolicy });
103+
const director = createChatDirector("system", [], { onTasksChange: () => {}, provider: providerlessPolicy });
104104
const capabilities = makeCapabilities();
105105

106106
// Default family nudges at 12 consecutive tool-only turns.
@@ -111,7 +111,7 @@ describe("ChatDirector tool-only loop protection", () => {
111111
});
112112

113113
test("the nudge is one-shot — it does not repeat on the next tool-only turn", async () => {
114-
const director = createChatDirector("system", [], { provider: providerlessPolicy });
114+
const director = createChatDirector("system", [], { onTasksChange: () => {}, provider: providerlessPolicy });
115115
const capabilities = makeCapabilities();
116116

117117
await runToolOnlyStreak(director, capabilities, 12);
@@ -122,7 +122,7 @@ describe("ChatDirector tool-only loop protection", () => {
122122
});
123123

124124
test("pauses and stops issuing infers at the family pause threshold", async () => {
125-
const director = createChatDirector("system", [], { provider: providerlessPolicy });
125+
const director = createChatDirector("system", [], { onTasksChange: () => {}, provider: providerlessPolicy });
126126
const capabilities = makeCapabilities();
127127

128128
// Default family pauses at 20 consecutive tool-only turns.
@@ -136,7 +136,7 @@ describe("ChatDirector tool-only loop protection", () => {
136136
});
137137

138138
test("resumes after the operator sends a new message", async () => {
139-
const director = createChatDirector("system", [], { provider: providerlessPolicy });
139+
const director = createChatDirector("system", [], { onTasksChange: () => {}, provider: providerlessPolicy });
140140
const capabilities = makeCapabilities();
141141

142142
await runToolOnlyStreak(director, capabilities, 20);
@@ -147,7 +147,7 @@ describe("ChatDirector tool-only loop protection", () => {
147147
});
148148

149149
test("a dismissed ask_operator counts toward the streak like any other tool-only turn", async () => {
150-
const director = createChatDirector("system", [], { provider: providerlessPolicy });
150+
const director = createChatDirector("system", [], { onTasksChange: () => {}, provider: providerlessPolicy });
151151
const capabilities = makeCapabilities();
152152

153153
// 11 ordinary tool-only turns, then a turn whose only tool call is a
@@ -197,7 +197,7 @@ describe("ChatDirector tool-only loop protection", () => {
197197
});
198198

199199
test("a busy-but-progressing session (text interleaved with tools) never trips", async () => {
200-
const director = createChatDirector("system", [], { provider: providerlessPolicy });
200+
const director = createChatDirector("system", [], { onTasksChange: () => {}, provider: providerlessPolicy });
201201
const capabilities = makeCapabilities();
202202

203203
let lastActions: ReactorAction[] = [];
@@ -212,7 +212,7 @@ describe("ChatDirector tool-only loop protection", () => {
212212
});
213213

214214
test("grok's tightened thresholds fire earlier than the default family", async () => {
215-
const director = createChatDirector("system", [], { provider: { providerName: "xai/default", model: "grok-4.5" } });
215+
const director = createChatDirector("system", [], { onTasksChange: () => {}, provider: { providerName: "xai/default", model: "grok-4.5" } });
216216
const capabilities = makeCapabilities();
217217

218218
// Grok nudges at 6, well below the default family's 12.

‎src/agent/director.ts‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -305,10 +305,15 @@ function isCodeFile(path: string): boolean {
305305

306306
// Single implementation of "what does a manage_tasks tool call do to the
307307
// task list", shared by the live decide() loop below and hydrateTasksFromTurns.
308+
// Task state is owned by the director, not by the tool: manage_tasks's
309+
// handler (src/agent/tools.ts) performs no side effect of its own — it
310+
// parses the same arguments and returns a fixed "Tasks updated." string. The
311+
// tool_call is therefore the authoritative event, and applying it here does
312+
// not need to wait on a tool_result the handler never varies.
308313
// Returns null when the call is not manage_tasks or its arguments don't
309314
// parse, so callers can distinguish "no valid manage_tasks call here" from
310315
// "a valid call that happened to be a no-op" — the latter still counts as an
311-
// update for onTasksChange purposes, matching prior behavior.
316+
// update for onTasksChange purposes.
312317
function applyManageTasksToolCall(tasks: Task[], block: { name: string; arguments: unknown }): Task[] | null {
313318
if (block.name !== "manage_tasks") return null;
314319
const taskArgs = parseManageTasksArgs(block.arguments);
@@ -321,7 +326,7 @@ export type ChatDirectorOptions = {
321326
inactivityTimeoutMs?: number | undefined;
322327
totalTimeoutMs?: number | undefined;
323328
workflowCoordinator?: WorkflowCoordinator | undefined;
324-
onTasksChange?: ((tasks: Task[]) => void) | undefined;
329+
onTasksChange: (tasks: Task[]) => void;
325330
requestContinuation?: (() => void) | undefined;
326331
provider?: { providerName: string; model?: string } | undefined;
327332
};
@@ -370,7 +375,7 @@ class ChatDirectorImpl extends DefaultDirector {
370375
private pendingToolOnlyNudge = false;
371376
private pausedForToolOnly = false;
372377

373-
constructor(systemPrompt: string, toolDefinitions: ToolDefinition[], options: ChatDirectorImplOptions = {}) {
378+
constructor(systemPrompt: string, toolDefinitions: ToolDefinition[], options: ChatDirectorImplOptions) {
374379
super(systemPrompt, toolDefinitions, {});
375380
this._systemPrompt = systemPrompt;
376381
this._toolDefinitions = toolDefinitions;
@@ -802,7 +807,7 @@ class ChatDirectorImpl extends DefaultDirector {
802807
export function createChatDirector(
803808
systemPrompt: string,
804809
toolDefinitions: ToolDefinition[],
805-
options: ChatDirectorOptions = {},
810+
options: ChatDirectorOptions,
806811
): ChatDirector {
807812
const { provider, ...rest } = options;
808813
return new ChatDirectorImpl(systemPrompt, toolDefinitions, {
@@ -813,11 +818,11 @@ export function createChatDirector(
813818
});
814819
}
815820

816-
// Task state on hydrate is derived with the same manage_tasks-handling logic
817-
// live sessions use (applyManageTasksToolCall), applied unconditionally on
818-
// each tool_call regardless of whether its tool_result later errors — a
819-
// resumed transcript's task list matches what a live session would have
820-
// held at that point, rather than a looser hydrate-only interpretation.
821+
// Uses the same applyManageTasksToolCall a live session's decide() loop uses,
822+
// so hydrate necessarily reaches the same task state live decide() would
823+
// have produced from this transcript: the tool_call is the authoritative
824+
// event (see applyManageTasksToolCall), and there is only the one function
825+
// that knows how to turn a manage_tasks call into a task list.
821826
export function hydrateTasksFromTurns(turns: ConversationTurn[]): Task[] {
822827
let tasks: Task[] = [];
823828
for (const turn of turns) {

‎src/director.test.ts‎

Lines changed: 25 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ describe("operator declined tool calls", () => {
7979
// reactor and break further sends, and it does not re-infer off a bare
8080
// decline.
8181
test("chat director surfaces the decline and waits, keeping the reactor alive", async () => {
82-
const director = createChatDirector("", []);
82+
const director = createChatDirector("", [], { onTasksChange: () => {} });
8383
const actions = actionsArray(await director.decide(makeToolErrorEvent("c", declined), mockState, mockCapabilities));
8484
expect(hasCheckpoint(actions)).toBe(true);
8585
expect(hasDeclineReply(actions)).toBe(true);
@@ -109,7 +109,7 @@ describe("open-task termination guard", () => {
109109
const hasReply = (a: ReactorAction[]): boolean => a.some((x) => x.type === "reply");
110110

111111
test("re-infers instead of ending the turn while a task is still open", async () => {
112-
const director = createChatDirector("base", []);
112+
const director = createChatDirector("base", [], { onTasksChange: () => {} });
113113
await director.decide(manageTasksEvent("doing"), mockState, mockCapabilities);
114114

115115
const actions = actionsArray(await director.decide(textTurn(), mockState, mockCapabilities));
@@ -118,7 +118,7 @@ describe("open-task termination guard", () => {
118118
});
119119

120120
test("ends the turn normally once every task is terminal", async () => {
121-
const director = createChatDirector("base", []);
121+
const director = createChatDirector("base", [], { onTasksChange: () => {} });
122122
await director.decide(manageTasksEvent("done"), mockState, mockCapabilities);
123123

124124
const actions = actionsArray(await director.decide(textTurn(), mockState, mockCapabilities));
@@ -127,7 +127,7 @@ describe("open-task termination guard", () => {
127127
});
128128

129129
test("stops nudging and lets the turn end after the cap of content-free attempts", async () => {
130-
const director = createChatDirector("base", []);
130+
const director = createChatDirector("base", [], { onTasksChange: () => {} });
131131
await director.decide(manageTasksEvent("doing"), mockState, mockCapabilities);
132132

133133
for (let i = 0; i < 3; i++) {
@@ -142,7 +142,7 @@ describe("open-task termination guard", () => {
142142
test("empty model turn settles with an empty reply before wait", async () => {
143143
// DefaultDirector ends empty responses with bare wait; without a reply,
144144
// agent.send hangs and the TUI Working spinner sticks forever.
145-
const director = createChatDirector("base", []);
145+
const director = createChatDirector("base", [], { onTasksChange: () => {} });
146146
const emptyTurn = {
147147
type: "inference.done",
148148
turn: { role: "assistant", model: "test", timestamp: 0, content: [] },
@@ -158,7 +158,7 @@ describe("open-task termination guard", () => {
158158
});
159159

160160
test("a declined tool with open tasks re-infers, then terminates after its cap", async () => {
161-
const director = createChatDirector("base", []);
161+
const director = createChatDirector("base", [], { onTasksChange: () => {} });
162162
await director.decide(manageTasksEvent("doing"), mockState, mockCapabilities);
163163

164164
for (let i = 0; i < 2; i++) {
@@ -177,7 +177,7 @@ describe("open-task termination guard", () => {
177177
// single user turn — the budget is monotonic per inbound message, not per
178178
// tool call, so it does not matter whether a tool call happens at all.
179179
test("a no-op tool call between nudges does not reset the idle budget", async () => {
180-
const director = createChatDirector("base", []);
180+
const director = createChatDirector("base", [], { onTasksChange: () => {} });
181181
await director.decide(manageTasksEvent("doing"), mockState, mockCapabilities);
182182

183183
// Two content-free terminations spend two of the three nudges.
@@ -201,7 +201,7 @@ describe("open-task termination guard", () => {
201201
});
202202

203203
test("a new user message resets the idle budget for the next turn", async () => {
204-
const director = createChatDirector("base", []);
204+
const director = createChatDirector("base", [], { onTasksChange: () => {} });
205205
await director.decide(manageTasksEvent("doing"), mockState, mockCapabilities);
206206

207207
for (let i = 0; i < 3; i++) {
@@ -221,7 +221,7 @@ describe("open-task termination guard", () => {
221221
});
222222

223223
test("a successful tool call between declines does not reset the declined budget", async () => {
224-
const director = createChatDirector("base", []);
224+
const director = createChatDirector("base", [], { onTasksChange: () => {} });
225225
await director.decide(manageTasksEvent("doing"), mockState, mockCapabilities);
226226

227227
// Spend both of the declined-path nudges, with a successful tool result
@@ -242,7 +242,7 @@ describe("open-task termination guard", () => {
242242
});
243243

244244
test("a declined tool with no open tasks surfaces the decline immediately", async () => {
245-
const director = createChatDirector("base", []);
245+
const director = createChatDirector("base", [], { onTasksChange: () => {} });
246246
const actions = actionsArray(await director.decide(makeToolErrorEvent("c", declined), mockState, mockCapabilities));
247247
expect(actions.some((a) => a.type === "reply" && "content" in a && a.content === "Tool call rejected by operator.")).toBe(true);
248248
expect(actions.some((a) => a.type === "infer")).toBe(false);
@@ -274,6 +274,7 @@ describe("chatDirector compaction", () => {
274274
test("schedules idle compaction after an over-threshold text-only reply", async () => {
275275
let continuations = 0;
276276
const director = createChatDirector("", [], {
277+
onTasksChange: () => {},
277278
requestContinuation: () => {
278279
continuations++;
279280
},
@@ -328,7 +329,7 @@ describe("chatDirector compaction", () => {
328329
}
329330

330331
function chatDirectorWithContinuation(onContinuation?: () => void) {
331-
return createChatDirector("", [], { requestContinuation: onContinuation ?? (() => {}) });
332+
return createChatDirector("", [], { onTasksChange: () => {}, requestContinuation: onContinuation ?? (() => {}) });
332333
}
333334

334335
test("compacts at the tool.done pause once over threshold", async () => {
@@ -439,31 +440,31 @@ describe("chatDirector compaction", () => {
439440
describe("chatDirector LSP auto-activation", () => {
440441
test("reading a code file activates the lsp tool on success", async () => {
441442
const activated: string[][] = [];
442-
const director = createChatDirector("", [], { onActivateTools: (names: string[]) => activated.push(names) });
443+
const director = createChatDirector("", [], { onTasksChange: () => {}, onActivateTools: (names: string[]) => activated.push(names) });
443444
await director.decide(makeInferenceDoneEvent([{ id: "c", name: "read_file", args: { path: "src/foo.ts" } }]), mockState, mockCapabilities);
444445
await director.decide(makeToolDoneEvent("c"), mockState, mockCapabilities);
445446
expect(activated).toEqual([["lsp"]]);
446447
});
447448

448449
test("editing a code file activates lsp", async () => {
449450
const activated: string[][] = [];
450-
const director = createChatDirector("", [], { onActivateTools: (names: string[]) => activated.push(names) });
451+
const director = createChatDirector("", [], { onTasksChange: () => {}, onActivateTools: (names: string[]) => activated.push(names) });
451452
await director.decide(makeInferenceDoneEvent([{ id: "c", name: "edit_file", args: { path: "lib/bar.rs" } }]), mockState, mockCapabilities);
452453
await director.decide(makeToolDoneEvent("c"), mockState, mockCapabilities);
453454
expect(activated).toEqual([["lsp"]]);
454455
});
455456

456457
test("a non-code file does not activate lsp", async () => {
457458
const activated: string[][] = [];
458-
const director = createChatDirector("", [], { onActivateTools: (names: string[]) => activated.push(names) });
459+
const director = createChatDirector("", [], { onTasksChange: () => {}, onActivateTools: (names: string[]) => activated.push(names) });
459460
await director.decide(makeInferenceDoneEvent([{ id: "c", name: "read_file", args: { path: "README.md" } }]), mockState, mockCapabilities);
460461
await director.decide(makeToolDoneEvent("c"), mockState, mockCapabilities);
461462
expect(activated).toEqual([]);
462463
});
463464

464465
test("a failed read does not activate lsp", async () => {
465466
const activated: string[][] = [];
466-
const director = createChatDirector("", [], { onActivateTools: (names: string[]) => activated.push(names) });
467+
const director = createChatDirector("", [], { onTasksChange: () => {}, onActivateTools: (names: string[]) => activated.push(names) });
467468
await director.decide(makeInferenceDoneEvent([{ id: "c", name: "read_file", args: { path: "src/foo.ts" } }]), mockState, mockCapabilities);
468469
await director.decide(makeToolErrorEvent("c", "Error: not found"), mockState, mockCapabilities);
469470
expect(activated).toEqual([]);
@@ -495,7 +496,7 @@ describe("updateToolDefinitions rewrites infer tools", () => {
495496
};
496497

497498
test("a tool registered after construction is advertised on the next inference", async () => {
498-
const director = createChatDirector("base-prompt", []);
499+
const director = createChatDirector("base-prompt", [], { onTasksChange: () => {} });
499500
director.updateToolDefinitions([lateTool]);
500501

501502
const result = await director.decide(makeMessageReceivedEvent("hello"), mockState, capabilitiesWithInferArgs);
@@ -508,7 +509,7 @@ describe("updateToolDefinitions rewrites infer tools", () => {
508509
// The provider cache is a prefix cache keyed on the tools array; a tool_search
509510
// between turns must not reshape it.
510511
test("wire tools are byte-identical across a turn that ran tool_search", async () => {
511-
const director = createChatDirector("base-prompt", [lateTool]);
512+
const director = createChatDirector("base-prompt", [lateTool], { onTasksChange: () => {} });
512513

513514
const before = await firstInferTools(director, makeMessageReceivedEvent("do work"));
514515

@@ -529,7 +530,7 @@ describe("updateToolDefinitions rewrites infer tools", () => {
529530
// advance_workflow is always on the wire so a workflow going active never grows
530531
// the array and busts the provider cache prefix.
531532
test("advance_workflow is advertised even with no active workflow", async () => {
532-
const director = createChatDirector("base-prompt", []);
533+
const director = createChatDirector("base-prompt", [], { onTasksChange: () => {} });
533534
director.updateToolDefinitions([lateTool]);
534535

535536
const result = await director.decide(makeMessageReceivedEvent("hello"), mockState, capabilitiesWithInferArgs);
@@ -564,6 +565,7 @@ describe("updateToolDefinitions rewrites infer tools", () => {
564565
const director = createChatDirector(
565566
"base-prompt",
566567
computeAdvertised(toolset.dynamicRunner.currentDefinitions()),
568+
{ onTasksChange: () => {} },
567569
);
568570

569571
// Before discovery: the MCP tool is registered (dispatchable) but not wired.
@@ -600,7 +602,7 @@ describe("updateToolDefinitions rewrites infer tools", () => {
600602

601603
test("the new-task path also carries the current tools", async () => {
602604
const classifier = async (_msg: string, _meta: SessionMetadata) => ({ kind: "new_task" as const, reason: "pivot" } as TaskBoundary);
603-
const director = createChatDirector("base-prompt", [], { taskClassifier: classifier });
605+
const director = createChatDirector("base-prompt", [], { onTasksChange: () => {}, taskClassifier: classifier });
604606
director.updateToolDefinitions([lateTool]);
605607

606608
const result = await director.decide(makeMessageReceivedEvent("new thing"), mockState, capabilitiesWithInferArgs);
@@ -656,7 +658,7 @@ describe("transient nudges", () => {
656658
}) as unknown as ReactorInboundEvent;
657659

658660
test("open-task nudge uses ephemeralTurns, not systemPrompt", async () => {
659-
const director = createChatDirector("stable-base", []);
661+
const director = createChatDirector("stable-base", [], { onTasksChange: () => {} });
660662
await director.decide(manageTasksEvent("doing"), mockState, mockCapabilities);
661663
const actions = actionsArray(await director.decide(textTurn(), mockState, mockCapabilities));
662664
const infer = actions.find((a) => a.type === "infer");
@@ -704,7 +706,7 @@ describe("goal continue-rule", () => {
704706

705707
test("active not-met goal rewrites a clean yield into re-infer", async () => {
706708
const { createGoalGovernor } = await import("./agent/goal.js");
707-
const director = createChatDirector("base", []);
709+
const director = createChatDirector("base", [], { onTasksChange: () => {} });
708710
const g = createGoalGovernor({
709711
evaluate: async () => ({ met: false, reason: "tests still red" }),
710712
});
@@ -724,7 +726,7 @@ describe("goal continue-rule", () => {
724726

725727
test("met goal leaves terminal reply and marks achieved", async () => {
726728
const { createGoalGovernor } = await import("./agent/goal.js");
727-
const director = createChatDirector("base", []);
729+
const director = createChatDirector("base", [], { onTasksChange: () => {} });
728730
const g = createGoalGovernor({
729731
evaluate: async () => ({ met: true, reason: "green" }),
730732
});
@@ -744,7 +746,7 @@ describe("goal continue-rule", () => {
744746
test("open-task nudge still wins over goal when tasks are open", async () => {
745747
const { createGoalGovernor } = await import("./agent/goal.js");
746748
let evals = 0;
747-
const director = createChatDirector("base", []);
749+
const director = createChatDirector("base", [], { onTasksChange: () => {} });
748750
const g = createGoalGovernor({
749751
evaluate: async () => {
750752
evals++;

0 commit comments

Comments
 (0)