From d8d1b00e44bfc6854b42a22dc3f30b3b257b4567 Mon Sep 17 00:00:00 2001 From: mikemikimike <13286568797@163.com> Date: Fri, 28 Aug 2026 17:58:57 +0800 Subject: [PATCH 1/2] fix(agent): reserve final step for response --- src/agent/agent-loop.test.ts | 51 ++++++++++++++++++++++++++++++++++++ src/agent/agent-loop.ts | 30 ++++++++++++++++++--- src/agent/step-executor.ts | 43 ++++++++++++++++++++++++++++-- 3 files changed, 119 insertions(+), 5 deletions(-) diff --git a/src/agent/agent-loop.test.ts b/src/agent/agent-loop.test.ts index 86d68c9a..65b85682 100644 --- a/src/agent/agent-loop.test.ts +++ b/src/agent/agent-loop.test.ts @@ -338,6 +338,57 @@ describe("AgentLoop end-to-end with mock LLM", () => { expect(result.session.lastError).toMatch(/max_steps_reached: 2 steps/); }); + it("reserves the final step for a terminal reply", async () => { + const registry = buildDefaultToolRegistry(); + registry.register({ + name: "noop", + description: "no-op", + readonly: true, + async run() { + return { + tool: "noop", + status: "ok", + summary: "verified", + details: {}, + truncated: false, + }; + }, + }); + let calls = 0; + const prompts: string[] = []; + const loop = new AgentLoop({ + registry, + slotManager: new SlotManager(2), + grammar: 'root ::= "ok"', + llmComplete: async (params) => { + calls += 1; + prompts.push(params.prompt); + return makeCompletion( + calls === 1 + ? JSON.stringify({ tool: "noop", args: {} }) + : JSON.stringify({ tool: "reply", args: { text: "verified" } }), + ); + }, + toolDescriptors: TOOLS, + capabilities: CAPS, + skillCatalog: SKILLS, + }); + const result = await loop.runTurn( + createEmptySessionState({ id: "chat-finalize", workingDir }), + { userMessage: "verify", maxSteps: 2, signal: new AbortController().signal }, + ); + + expect(calls).toBe(2); + expect(prompts[1]).toContain("final allowed step"); + expect(result.reason).toBe("reply"); + expect(result.stepCount).toBe(2); + expect(result.session.status).toBe("pending"); + expect(result.session.turns.at(-1)).toMatchObject({ + kind: "assistant_reply", + text: "verified", + }); + }); + it("injects a transient notice into the next prompt when a no-progress loop is detected", async () => { const registry = buildDefaultToolRegistry(); registry.register({ diff --git a/src/agent/agent-loop.ts b/src/agent/agent-loop.ts index b8036043..0fc46995 100644 --- a/src/agent/agent-loop.ts +++ b/src/agent/agent-loop.ts @@ -565,6 +565,10 @@ export class AgentLoop { } const noticeForThisStep = pendingNotice; pendingNotice = undefined; + const finalizationStep = i === options.maxSteps - 1; + const finalizationNotice = + "This is the final allowed step. Do not call any non-terminal tool; " + + "summarize the completed work with reply, or end the session with finish."; try { const profileFacts = this.deps.profileFactsProvider?.(); const activeProfile = @@ -576,14 +580,26 @@ export class AgentLoop { const outcome = await executeStep( { session: state, - toolDescriptors: this.deps.toolDescriptors, + toolDescriptors: finalizationStep + ? this.deps.toolDescriptors.filter( + ({ name }) => name === "reply" || name === "finish", + ) + : this.deps.toolDescriptors, capabilities: this.deps.capabilities, skillCatalog: this.deps.skillCatalog, stepIndex: i, signal: options.signal, - ...(noticeForThisStep !== undefined - ? { transientNotice: noticeForThisStep } + ...(finalizationStep || noticeForThisStep !== undefined + ? { + transientNotice: [ + noticeForThisStep, + ...(finalizationStep ? [finalizationNotice] : []), + ] + .filter((notice): notice is string => notice !== undefined) + .join("\n\n"), + } : {}), + ...(finalizationStep ? { terminalOnly: true } : {}), ...(profileFacts !== undefined ? { profileFacts } : {}), ...(options.userMessage !== undefined ? { userMessage: options.userMessage } @@ -796,6 +812,14 @@ export class AgentLoop { recordSurfacedLessons(state); recordSurfacedProcedures(state); } catch (err) { + if (finalizationStep) { + // A failed finalization must not execute more work or turn a + // bounded run into an unbounded retry. Preserve the established + // explicit max-steps/stalled outcome instead. + stepsTaken += 1; + reason = "max_steps"; + break; + } runError = err instanceof Error ? err : new Error(String(err)); const category = classifyFailure(err); this.deps.logger?.error("agent loop failed", { diff --git a/src/agent/step-executor.ts b/src/agent/step-executor.ts index 98636f3d..b3d307a9 100644 --- a/src/agent/step-executor.ts +++ b/src/agent/step-executor.ts @@ -216,6 +216,8 @@ export interface StepContext { * continuation) — contextual facts stay suppressed. */ userMessage?: string | null; + /** Restrict this step to the terminal reply/finish tools. */ + terminalOnly?: boolean; } /** @@ -329,9 +331,14 @@ async function executeStepInner( ctx: StepContext, deps: StepDependencies, ): Promise { + const stepToolDescriptors = ctx.terminalOnly + ? ctx.toolDescriptors.filter( + ({ name }) => name === "reply" || name === "finish", + ) + : ctx.toolDescriptors; const prompt = buildPrompt({ session: ctx.session, - toolDescriptors: ctx.toolDescriptors, + toolDescriptors: stepToolDescriptors, capabilities: ctx.capabilities, skillCatalog: ctx.skillCatalog, currentDate: formatCurrentDate(new Date()), @@ -393,7 +400,7 @@ async function executeStepInner( deps, slotId: slot.slotId, sessionId: ctx.session.id, - toolDescriptors: ctx.toolDescriptors, + toolDescriptors: stepToolDescriptors, signal: ctx.signal, }); @@ -599,6 +606,22 @@ async function executeStepInner( deps.profile, parseDepsFor(completion, deps), ); + if (ctx.terminalOnly && parsed.ok) { + const nonTerminal = parsed.batch.calls.find( + ({ tool }) => tool !== "reply" && tool !== "finish", + ); + if (nonTerminal) { + parsed = { + ok: false, + error: new BatchValidationError( + "finalization step only accepts reply or finish", + [ + `non-terminal tool is not allowed at the step budget: ${nonTerminal.tool}`, + ], + ), + }; + } + } if (parsed.ok) { const validation = validateBatch(parsed.batch, deps.registry); if (!validation.ok) { @@ -725,6 +748,22 @@ async function executeStepInner( deps.profile, parseDepsFor(completion, deps), ); + if (ctx.terminalOnly && parsed.ok) { + const nonTerminal = parsed.batch.calls.find( + ({ tool }) => tool !== "reply" && tool !== "finish", + ); + if (nonTerminal) { + parsed = { + ok: false, + error: new BatchValidationError( + "finalization step only accepts reply or finish", + [ + `non-terminal tool is not allowed at the step budget: ${nonTerminal.tool}`, + ], + ), + }; + } + } if (parsed.ok) { const validation = validateBatch(parsed.batch, deps.registry); if (!validation.ok) { From 55e4e5cce2d52ee0f2578f3b90e3059d1595cca5 Mon Sep 17 00:00:00 2001 From: plombeer31 Date: Fri, 28 Aug 2026 19:26:38 +0300 Subject: [PATCH 2/2] fix(agent): keep cancellation semantics on the finalization step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reserved-final-step guard in the agent loop's catch block preserved the explicit max-steps/stalled outcome for ANY error thrown during finalization — including an AbortError from the user pressing Esc. Issue #107's acceptance criteria require cancellation semantics to remain unchanged, so classify the failure BEFORE the guard and let a user abort keep its `cancelled` reason/status; only provider and parse failures fall back to the preserved max-steps outcome (now with a structured warn log instead of a silent break). Also pins the surrounding behavior with three regressions: - a user abort during the reserved final inference still ends the turn as `cancelled`, never `stalled`/`max_steps` (fails without this fix); - a model that insists on a non-terminal tool at the budget edge gets exactly one repair round-trip, executes nothing, and the turn keeps the explicit stalled outcome; - maxSteps=1 deliberately makes the only step terminal — the tool call is rejected before execution and the repair pass must produce the final reply. Co-Authored-By: Claude Fable 5 --- src/agent/agent-loop.test.ts | 162 +++++++++++++++++++++++++++++++++++ src/agent/agent-loop.ts | 32 ++++--- 2 files changed, 184 insertions(+), 10 deletions(-) diff --git a/src/agent/agent-loop.test.ts b/src/agent/agent-loop.test.ts index 65b85682..798f6d5a 100644 --- a/src/agent/agent-loop.test.ts +++ b/src/agent/agent-loop.test.ts @@ -389,6 +389,168 @@ describe("AgentLoop end-to-end with mock LLM", () => { }); }); + it("keeps the cancelled outcome when the user aborts during the finalization step", async () => { + const registry = buildDefaultToolRegistry(); + registry.register({ + name: "noop", + description: "no-op", + readonly: true, + async run() { + return { + tool: "noop", + status: "ok", + summary: "noop", + details: {}, + truncated: false, + }; + }, + }); + let calls = 0; + const controller = new AbortController(); + const loop = new AgentLoop({ + registry, + slotManager: new SlotManager(2), + grammar: 'root ::= "ok"', + llmComplete: async () => { + calls += 1; + if (calls === 1) { + return makeCompletion(JSON.stringify({ tool: "noop", args: {} })); + } + // The user presses Esc while the reserved final inference is in + // flight — the provider surfaces it as an abort. + controller.abort(); + const err = new Error("The operation was aborted"); + err.name = "AbortError"; + throw err; + }, + toolDescriptors: TOOLS, + capabilities: CAPS, + skillCatalog: SKILLS, + }); + const result = await loop.runTurn( + createEmptySessionState({ id: "chat-finalize-cancel", workingDir }), + { userMessage: "verify", maxSteps: 2, signal: controller.signal }, + ); + + expect(calls).toBe(2); + expect(result.reason).toBe("cancelled"); + expect(result.session.status).toBe("cancelled"); + expect(result.session.lastError ?? "").not.toMatch(/max_steps/); + }); + + it("gives the finalization step one repair attempt, then preserves the stalled outcome", async () => { + const registry = buildDefaultToolRegistry(); + let noopRuns = 0; + registry.register({ + name: "noop", + description: "no-op", + readonly: true, + async run() { + noopRuns += 1; + return { + tool: "noop", + status: "ok", + summary: "noop", + details: {}, + truncated: false, + }; + }, + }); + let calls = 0; + const stepEventTypes: string[] = []; + const loop = new AgentLoop({ + registry, + slotManager: new SlotManager(2), + grammar: 'root ::= "ok"', + // The model insists on a non-terminal tool even on the reserved + // final step and its repair attempt. + llmComplete: async () => { + calls += 1; + return makeCompletion(JSON.stringify({ tool: "noop", args: {} })); + }, + toolDescriptors: TOOLS, + capabilities: CAPS, + skillCatalog: SKILLS, + onEvent: (event) => { + if (event.type === "llm_event") stepEventTypes.push(event.event.type); + }, + }); + const result = await loop.runTurn( + createEmptySessionState({ id: "chat-finalize-stubborn", workingDir }), + { userMessage: "verify", maxSteps: 2, signal: new AbortController().signal }, + ); + + // Step 0 executes the tool; the finalization step burns its first + // completion plus exactly one repair round-trip, and neither may + // execute the non-terminal call. + expect(calls).toBe(3); + expect(noopRuns).toBe(1); + expect(stepEventTypes.filter((t) => t === "parse_retry")).toHaveLength(1); + expect(result.reason).toBe("max_steps"); + expect(result.session.status).toBe("stalled"); + expect(result.session.lastError).toMatch(/max_steps_reached: 2 steps/); + expect(result.session.turns.at(-1)).toMatchObject({ + kind: "assistant_reply", + text: expect.stringContaining("max_steps"), + }); + }); + + it("treats the only step of a maxSteps=1 turn as terminal — no tool can ever run", async () => { + const registry = buildDefaultToolRegistry(); + let noopRuns = 0; + registry.register({ + name: "noop", + description: "no-op", + readonly: true, + async run() { + noopRuns += 1; + return { + tool: "noop", + status: "ok", + summary: "noop", + details: {}, + truncated: false, + }; + }, + }); + let calls = 0; + const prompts: string[] = []; + const loop = new AgentLoop({ + registry, + slotManager: new SlotManager(2), + grammar: 'root ::= "ok"', + llmComplete: async (params) => { + calls += 1; + prompts.push(params.prompt); + return makeCompletion( + calls === 1 + ? JSON.stringify({ tool: "noop", args: {} }) + : JSON.stringify({ tool: "reply", args: { text: "summary only" } }), + ); + }, + toolDescriptors: TOOLS, + capabilities: CAPS, + skillCatalog: SKILLS, + }); + const result = await loop.runTurn( + createEmptySessionState({ id: "chat-one-step", workingDir }), + { userMessage: "hi", maxSteps: 1, signal: new AbortController().signal }, + ); + + // With a budget of one, the single step IS the finalization step: + // the tool call is rejected before execution and the repair pass + // must produce the terminal reply. + expect(prompts[0]).toContain("final allowed step"); + expect(calls).toBe(2); + expect(noopRuns).toBe(0); + expect(result.reason).toBe("reply"); + expect(result.session.status).toBe("pending"); + expect(result.session.turns.at(-1)).toMatchObject({ + kind: "assistant_reply", + text: "summary only", + }); + }); + it("injects a transient notice into the next prompt when a no-progress loop is detected", async () => { const registry = buildDefaultToolRegistry(); registry.register({ diff --git a/src/agent/agent-loop.ts b/src/agent/agent-loop.ts index 0fc46995..7c87091b 100644 --- a/src/agent/agent-loop.ts +++ b/src/agent/agent-loop.ts @@ -812,16 +812,35 @@ export class AgentLoop { recordSurfacedLessons(state); recordSurfacedProcedures(state); } catch (err) { - if (finalizationStep) { + runError = err instanceof Error ? err : new Error(String(err)); + const category = classifyFailure(err); + // `cancelled` is user-initiated and should close the turn + // cleanly without marking the session as failed. Classified + // BEFORE the finalization guard below: a user abort during the + // reserved final step must keep its `cancelled` outcome + // (issue #107 — cancellation semantics remain unchanged), not + // be relabelled `max_steps`. + const cancelled = + err instanceof CancelledError || + (err instanceof LlmFailure && err.category === "cancelled") || + category === "cancelled"; + if (finalizationStep && !cancelled) { // A failed finalization must not execute more work or turn a // bounded run into an unbounded retry. Preserve the established // explicit max-steps/stalled outcome instead. + this.deps.logger?.warn( + "finalization step failed; preserving max-steps outcome", + { + sessionId: state.id, + stepIndex: i, + error: runError.message, + category, + }, + ); stepsTaken += 1; reason = "max_steps"; break; } - runError = err instanceof Error ? err : new Error(String(err)); - const category = classifyFailure(err); this.deps.logger?.error("agent loop failed", { sessionId: state.id, stepIndex: i, @@ -837,13 +856,6 @@ export class AgentLoop { sessionId: state.id, category, }); - // `cancelled` is user-initiated and should close the turn - // cleanly without marking the session as failed. Everything - // else keeps the existing failed-terminal contract. - const cancelled = - err instanceof CancelledError || - (err instanceof LlmFailure && err.category === "cancelled") || - category === "cancelled"; if (cancelled) { state = { ...state, status: "cancelled" }; this.deps.onEvent?.({ type: "loop_completed", reason: "cancelled" });