From 0f3cf73a854ced718fd2fac27e5337c2bc3c8e9d Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 9 Jul 2026 19:12:48 +0000 Subject: [PATCH 1/3] fix(agent): stopped terminal yield wake loops - Aborted the active agent loop synchronously when a terminal yield tool result finishes, so IRC-wake turns stop before another provider call. - Added a regression covering idle IRC wake handling after a terminal yield. Fixes #4963 --- packages/agent/CHANGELOG.md | 4 + packages/agent/src/agent-loop.ts | 6 ++ packages/coding-agent/CHANGELOG.md | 4 + .../src/prompts/system/workflow-notice.md | 10 +- .../coding-agent/src/session/agent-session.ts | 33 +++++- .../agent-session-advisor-suppression.test.ts | 100 +++++++++++++++++- 6 files changed, 148 insertions(+), 9 deletions(-) diff --git a/packages/agent/CHANGELOG.md b/packages/agent/CHANGELOG.md index 04b655eeb..a93ed021e 100644 --- a/packages/agent/CHANGELOG.md +++ b/packages/agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed aborted tool-result hooks from continuing into another provider call before the abort settled. ([#4963](https://github.com/can1357/oh-my-pi/issues/4963)) + ## [16.3.12] - 2026-07-08 ### Added diff --git a/packages/agent/src/agent-loop.ts b/packages/agent/src/agent-loop.ts index c8b1b82e5..9d7f33230 100644 --- a/packages/agent/src/agent-loop.ts +++ b/packages/agent/src/agent-loop.ts @@ -1084,6 +1084,12 @@ async function runLoopBody( } } + // A tool hook may abort to mark the tool result as terminal (e.g. subagent yield). + // Stop before the next provider call; event listeners observe the result too late. + if (signal?.aborted) { + hasMoreToolCalls = false; + } + if (toolCalls.length > 0) { pausedTurnContinuations = 0; } else if ( diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d1ad895e4..bac444188 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed kept-alive task subagents entering a repeated provider-call loop after an IRC wake and terminal `yield`. ([#4963](https://github.com/can1357/oh-my-pi/issues/4963)) + ## [16.3.13] - 2026-07-09 ### Fixed diff --git a/packages/coding-agent/src/prompts/system/workflow-notice.md b/packages/coding-agent/src/prompts/system/workflow-notice.md index 74eddee21..3e62ca5b0 100644 --- a/packages/coding-agent/src/prompts/system/workflow-notice.md +++ b/packages/coding-agent/src/prompts/system/workflow-notice.md @@ -51,20 +51,20 @@ Decompose first, then {{#if taskBatch}}batch the independent leaves{{else}}issue {{#if taskBatch}} task( - context: "# Goal\nReview the auth diff...\n# Constraints\nRead-only...\n# Contract\nReturn findings as severity/file/line/fix...", + context: "# Goal\nReview the auth diff…\n# Constraints\nRead-only…\n# Contract\nReturn findings as severity/file/line/fix…", tasks: [ - { id: "AuthOwner", role: "Auth Storage Reviewer", assignment: "# Target\npackages/ai/src/auth-storage.ts\n# Change\nTrace credential selection...\n# Acceptance\nReturn confirmed findings only..." }, - { id: "PromptOwner", role: "Prompt Contract Reviewer", assignment: "# Target\npackages/coding-agent/src/prompts/**\n# Change\nCheck active-tool guidance...\n# Acceptance\nReturn mismatches and exact prompt lines..." }, + { id: "AuthOwner", role: "Auth Storage Reviewer", assignment: "# Target\npackages/ai/src/auth-storage.ts\n# Change\nTrace credential selection…\n# Acceptance\nReturn confirmed findings only…" }, + { id: "PromptOwner", role: "Prompt Contract Reviewer", assignment: "# Target\npackages/coding-agent/src/prompts/**\n# Change\nCheck active-tool guidance…\n# Acceptance\nReturn mismatches and exact prompt lines…" }, ] ) {{else}} task( role: "Auth Storage Reviewer", - assignment: "# Target\npackages/ai/src/auth-storage.ts\n# Change\nReview the auth diff. Shared contract: read-only; return findings as severity/file/line/fix.\n# Acceptance\nReturn confirmed findings only..." + assignment: "# Target\npackages/ai/src/auth-storage.ts\n# Change\nReview the auth diff. Shared contract: read-only; return findings as severity/file/line/fix.\n# Acceptance\nReturn confirmed findings only…" ) task( role: "Prompt Contract Reviewer", - assignment: "# Target\npackages/coding-agent/src/prompts/**\n# Change\nCheck active-tool guidance. Shared contract: read-only; return mismatches and exact prompt lines.\n# Acceptance\nReturn confirmed findings only..." + assignment: "# Target\npackages/coding-agent/src/prompts/**\n# Change\nCheck active-tool guidance. Shared contract: read-only; return mismatches and exact prompt lines.\n# Acceptance\nReturn confirmed findings only…" ) {{/if}} diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 3b4274e5f..9fbe9f3a0 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -2222,8 +2222,8 @@ export class AgentSession { this.#maybeAbortStreamingEdit(event); this.#maybeInterruptGeminiHeaderRunaway(message, assistantMessageEvent); }); - // Per-tool TTSR reminders are folded into the matched tool's result via this hook. - this.agent.afterToolCall = ctx => this.#ttsrAfterToolCall(ctx); + // Tool-result hook owns synchronous post-tool actions that must affect the current loop. + this.agent.afterToolCall = ctx => this.#afterToolCall(ctx); this.agent.providerSessionState = this.#providerSessionState; this.#syncAgentSessionId(); this.#syncTodoPhasesFromBranch(); @@ -3671,9 +3671,10 @@ export class AgentSession { this.#planModeReminderAwaitingProgress = false; } } - if (event.type === "tool_execution_end" && event.toolName === "yield" && !event.isError) { + if (event.type === "tool_execution_end" && this.#isTerminalYieldToolResult(event)) { this.#lastSuccessfulYieldToolCallId = event.toolCallId; this.#yieldTerminationPending = true; + this.agent.abort(); } // TTSR: Check for pattern matches on assistant text/thinking and tool argument deltas @@ -4418,6 +4419,19 @@ export class AgentSession { } } + #afterToolCall(ctx: AfterToolCallContext): AfterToolCallResult | undefined { + if ( + this.#isTerminalYieldToolResult({ + toolName: ctx.toolCall.name, + isError: ctx.isError, + result: ctx.result, + }) + ) { + this.agent.abort(); + } + return this.#ttsrAfterToolCall(ctx); + } + /** `afterToolCall` hook: fold any per-tool TTSR reminders into the result. */ #ttsrAfterToolCall(ctx: AfterToolCallContext): AfterToolCallResult | undefined { const rules = this.#perToolTtsrInjections.get(ctx.toolCall.id); @@ -10589,6 +10603,19 @@ export class AgentSession { } return COMPACTION_CHECK_NONE; } + #isTerminalYieldToolResult(event: { toolName: string; isError?: boolean; result?: { details?: unknown } }): boolean { + if (event.toolName !== "yield" || event.isError) return false; + const details = event.result?.details; + if (!details || typeof details !== "object") return true; + const record = details as Record; + return !( + record.status === "success" && + Array.isArray(record.type) && + record.type.length > 0 && + record.type.every(item => typeof item === "string") + ); + } + #assistantMessageHasSuccessfulYieldToolCall(assistantMessage: AssistantMessage, toolCallId: string): boolean { const lastToolCall = assistantMessage.content .slice() diff --git a/packages/coding-agent/test/agent-session-advisor-suppression.test.ts b/packages/coding-agent/test/agent-session-advisor-suppression.test.ts index 2ac1780ed..363232740 100644 --- a/packages/coding-agent/test/agent-session-advisor-suppression.test.ts +++ b/packages/coding-agent/test/agent-session-advisor-suppression.test.ts @@ -18,7 +18,8 @@ * follow-up stays queued for the next explicit resume rather than auto-running. */ import { afterEach, beforeEach, describe, expect, it } from "bun:test"; -import { Agent, type AgentMessage } from "@oh-my-pi/pi-agent-core"; +import { Agent, type AgentMessage, type AgentTool } from "@oh-my-pi/pi-agent-core"; +import type { ToolCall } from "@oh-my-pi/pi-ai"; import { createMockModel, type MockModel, type MockResponse } from "@oh-my-pi/pi-ai/providers/mock"; import { getBundledModel } from "@oh-my-pi/pi-catalog/models"; import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; @@ -29,6 +30,18 @@ import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage"; import { USER_INTERRUPT_LABEL } from "@oh-my-pi/pi-coding-agent/session/messages"; import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; import { Snowflake, TempDir } from "@oh-my-pi/pi-utils"; +import { type } from "arktype"; + +interface MockYieldDetails { + status: "success"; + data?: unknown; + type?: string | string[]; +} + +const mockYieldParameters = type({ + result: "unknown", + "type?": "unknown", +}); const ADVISOR_TYPE = "advisor"; @@ -94,6 +107,56 @@ describe("AgentSession advisor auto-resume suppression", () => { return { session, sessionManager, mock, streamStarted: started.promise }; } + function readYieldResultData(result: unknown): unknown { + if (!result || typeof result !== "object" || !("data" in result)) return undefined; + return result.data; + } + + function isYieldType(value: unknown): value is string | string[] { + return ( + typeof value === "string" || + (Array.isArray(value) && value.length > 0 && value.every(item => typeof item === "string")) + ); + } + + function createMockYieldTool(): AgentTool { + return { + name: "yield", + label: "Yield", + description: "Mock yield tool", + parameters: mockYieldParameters, + execute: async (_toolCallId, params) => { + const details: MockYieldDetails = { status: "success", data: readYieldResultData(params.result) }; + if (isYieldType(params.type)) details.type = params.type; + return { + content: [{ type: "text", text: "Result submitted." }], + details, + }; + }, + }; + } + + function createYieldMockResponse(args: { result: { data: unknown }; type?: string | string[] }): MockResponse { + const toolCall: ToolCall = { + type: "toolCall", + id: `call_yield_${Snowflake.next()}`, + name: "yield", + arguments: args, + }; + return { + content: [toolCall], + stopReason: "toolUse", + usage: { + input: 1, + output: 1, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 2, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + }; + } + function advisorCard(content: string) { return { customType: ADVISOR_TYPE, @@ -325,6 +388,41 @@ describe("AgentSession advisor auto-resume suppression", () => { expect(mock.calls.length).toBe(2); }); + it("stops an idle IRC wake after a terminal yield", async () => { + const model = getBundledModel("anthropic", "claude-sonnet-4-5"); + if (!model) throw new Error("Expected claude-sonnet-4-5 model to exist"); + let providerCalls = 0; + const mock = createMockModel({ + handler: () => { + providerCalls++; + if (providerCalls > 1) { + throw new Error("terminal yield must not start a second provider call"); + } + return createYieldMockResponse({ result: { data: { ok: true } } }); + }, + }); + const agent = new Agent({ + getApiKey: () => "test-key", + initialState: { model, systemPrompt: ["Test"], tools: [createMockYieldTool()] }, + streamFn: mock.stream, + }); + const sessionManager = SessionManager.inMemory(); + const settings = Settings.isolated({ "compaction.enabled": false }); + const authStorage = await AuthStorage.create(tempDir.join(`auth-${Snowflake.next()}.db`)); + authStorages.push(authStorage); + authStorage.setRuntimeApiKey("anthropic", "test-key"); + const modelRegistry = new ModelRegistry(authStorage, tempDir.join("models.yml")); + session = new AgentSession({ agent, sessionManager, settings, modelRegistry }); + const msg: IrcMessage = { id: "m-yield", from: "peer", to: "me", body: "status?", ts: Date.now() }; + + const outcome = await session.deliverIrcMessage(msg); + await session.waitForIdle(); + + expect(outcome).toBe("woken"); + expect(providerCalls).toBe(1); + expect(mock.calls.length).toBe(1); + }); + it("flushes an accepted IRC aside on dispose instead of dropping it", async () => { const { session, streamStarted } = await createParkedSession(); const running = session.prompt("do the thing"); From 92307e11d87b55787694190685d3678307408969 Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 9 Jul 2026 19:24:39 +0000 Subject: [PATCH 2/3] test(agent): updated terminal yield expectations - Updated stale yield-empty-stop regressions for the new terminal-yield contract. - Kept coverage for clearing yield termination before the next prompt and IRC wake. Fixes #4963 --- ...ssion-yield-empty-stop-suppression.test.ts | 40 ++++++++----------- 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/packages/coding-agent/test/agent-session-yield-empty-stop-suppression.test.ts b/packages/coding-agent/test/agent-session-yield-empty-stop-suppression.test.ts index f414402b4..7d8abc77d 100644 --- a/packages/coding-agent/test/agent-session-yield-empty-stop-suppression.test.ts +++ b/packages/coding-agent/test/agent-session-yield-empty-stop-suppression.test.ts @@ -1,11 +1,11 @@ /** - * Regression: a trailing empty assistant `stop` arriving after a successful - * `yield` must NOT trigger empty-stop retry or any other auto-continuation. + * Regression: a terminal `yield` must stop the current prompt loop before a + * provider continuation can produce a trailing empty assistant `stop`. * * The session's executor treats a successful yield as the terminal result for - * a scripted subagent run; if the empty-stop recovery path then schedules - * `agent.continue()`, the already-yielded child resumes and produces post-yield - * tool calls (see issue #3389). + * a scripted subagent run; if the loop continues after that tool result, the + * already-yielded child resumes and can enter post-yield retries or tool calls + * (see issues #3389 and #4963). */ import { afterEach, describe, expect, it, vi } from "bun:test"; import * as path from "node:path"; @@ -147,20 +147,17 @@ afterEach(async () => { }); describe("AgentSession yield empty-stop suppression", () => { - it("does not retry a trailing empty assistant stop after a successful yield", async () => { - const { session, mock } = await createHarness([yieldCall("done", "call-yield-done"), emptyStop()]); + it("does not continue to a trailing empty assistant stop after a successful yield", async () => { + const { session, mock } = await createHarness([yieldCall("done", "call-yield-done")]); await session.prompt("do work then yield"); await session.waitForIdle(); - // Two model calls: the yield turn and the trailing empty stop. Without the - // fix, the empty stop would schedule a `continue()` and either drive a - // third call or throw "no response configured" from the mock. - expect(mock.calls).toHaveLength(2); + expect(mock.calls).toHaveLength(1); expect(reminderMessages(session.agent.state.messages)).toHaveLength(0); }); - it("suppresses multiple trailing empty stops within the same yield-terminated run", async () => { + it("stops at the terminal yield instead of consuming scripted trailing empty stops", async () => { const { session, mock } = await createHarness([ yieldCall("done", "call-yield-multi"), emptyStop(), @@ -171,18 +168,14 @@ describe("AgentSession yield empty-stop suppression", () => { await session.prompt("yield then maybe trail"); await session.waitForIdle(); - // Without suppression, empty-stop retries would consume extra mock entries - // and append at least one reminder. With the fix, the loop ends at the - // first trailing empty stop. - expect(mock.calls).toHaveLength(2); + expect(mock.calls).toHaveLength(1); expect(reminderMessages(session.agent.state.messages)).toHaveLength(0); }); it("clears yield-termination on the next prompt so empty stops retry normally", async () => { const { session, mock } = await createHarness([ - // Run 1: yield then trailing empty stop. Suppression applies. + // Run 1: terminal yield stops without consuming a trailing provider response. yieldCall("first", "call-yield-first"), - emptyStop(), // Run 2: empty stop should retry as usual now that the flag has cleared. recordCall("alpha", "call-record-alpha"), emptyStop(), @@ -191,7 +184,7 @@ describe("AgentSession yield empty-stop suppression", () => { await session.prompt("yield first"); await session.waitForIdle(); - expect(mock.calls).toHaveLength(2); + expect(mock.calls).toHaveLength(1); expect(reminderMessages(session.agent.state.messages)).toHaveLength(0); await session.prompt("now record"); @@ -199,15 +192,14 @@ describe("AgentSession yield empty-stop suppression", () => { // Three additional calls (record, emptyStop, finished). Exactly one // empty-stop reminder injected on the second run. - expect(mock.calls).toHaveLength(5); + expect(mock.calls).toHaveLength(4); expect(reminderMessages(session.agent.state.messages)).toHaveLength(1); }); it("treats an idle IRC wake after a yielded run as a fresh turn for empty-stop retry", async () => { const { session, mock } = await createHarness([ - // Run 1: yield then trailing empty stop. Suppression applies only to this yielded run. + // Run 1: terminal yield stops without consuming a trailing provider response. yieldCall("first", "call-yield-before-irc"), - emptyStop(), // Run 2: an idle IRC wake is a fresh turn, so its empty stop should retry normally. emptyStop(), { content: ["recovered after IRC retry"], stopReason: "stop" }, @@ -215,7 +207,7 @@ describe("AgentSession yield empty-stop suppression", () => { await session.prompt("yield first"); await session.waitForIdle(); - expect(mock.calls).toHaveLength(2); + expect(mock.calls).toHaveLength(1); expect(reminderMessages(session.agent.state.messages)).toHaveLength(0); const outcome = await session.deliverIrcMessage({ @@ -228,7 +220,7 @@ describe("AgentSession yield empty-stop suppression", () => { expect(outcome).toBe("woken"); await session.waitForIdle(); - expect(mock.calls).toHaveLength(4); + expect(mock.calls).toHaveLength(3); expect(reminderMessages(session.agent.state.messages)).toHaveLength(1); expect(assistantText(session.agent.state.messages)).toContain("recovered after IRC retry"); }); From 9af5a28c64efb6c7ea3d7e39ef71e6d15995d75e Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 10 Jul 2026 03:37:26 +0000 Subject: [PATCH 3/3] fix(agent): marked terminal yield before abort - Marked terminal yield state in the synchronous tool-result hook before aborting the loop. - Ignored the later tool_execution_end event for synchronously terminated yield calls so stale events cannot suppress the next prompt. Fixes #4963 --- .../coding-agent/src/session/agent-session.ts | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 9fbe9f3a0..d2bd9310d 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -1846,6 +1846,7 @@ export class AgentSession { * Cleared before every new prompt turn so the next turn evaluates cleanly. */ #yieldTerminationPending = false; + #synchronouslyTerminatedYieldToolCallIds = new Set(); #providerSessionState = new Map(); #hindsightSessionState: HindsightSessionState | undefined = undefined; readonly rawSseDebugBuffer: RawSseDebugBuffer; @@ -3672,9 +3673,11 @@ export class AgentSession { } } if (event.type === "tool_execution_end" && this.#isTerminalYieldToolResult(event)) { - this.#lastSuccessfulYieldToolCallId = event.toolCallId; - this.#yieldTerminationPending = true; - this.agent.abort(); + const alreadyTerminated = this.#synchronouslyTerminatedYieldToolCallIds.delete(event.toolCallId); + if (!alreadyTerminated) { + this.#markTerminalYieldToolCall(event.toolCallId); + this.agent.abort(); + } } // TTSR: Check for pattern matches on assistant text/thinking and tool argument deltas @@ -4427,6 +4430,8 @@ export class AgentSession { result: ctx.result, }) ) { + this.#markTerminalYieldToolCall(ctx.toolCall.id); + this.#synchronouslyTerminatedYieldToolCallIds.add(ctx.toolCall.id); this.agent.abort(); } return this.#ttsrAfterToolCall(ctx); @@ -10616,6 +10621,11 @@ export class AgentSession { ); } + #markTerminalYieldToolCall(toolCallId: string): void { + this.#lastSuccessfulYieldToolCallId = toolCallId; + this.#yieldTerminationPending = true; + } + #assistantMessageHasSuccessfulYieldToolCall(assistantMessage: AssistantMessage, toolCallId: string): boolean { const lastToolCall = assistantMessage.content .slice()