diff --git a/docs/extensions.md b/docs/extensions.md index 63062deb9..e589f9c88 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -251,7 +251,7 @@ Cancelable pre-events: - `agent_start` / `agent_end` — agent loop lifecycle notification; `agent_end` remains notification-only - `session_stop` — main-session stop hook, awaited before settle; may continue with `{ continue: true, additionalContext }` or `{ decision: "block", reason }`; capped at 8 consecutive continuations and never fires for task/subagent sessions - `turn_start` / `turn_end` -- `message_start` / `message_update` / `message_end` +- `message_start` / `message_update` / `message_end` — lifecycle notifications; `message_end` receives a detached message snapshot, so use `tool_result` or `context` when an extension needs to change provider context ### Tool lifecycle diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 94864acb6..765066463 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -42,6 +42,9 @@ - Fixed agent-facing prompts mentioning tools that may be absent from the session catalog: `todo`/`grep` workflow guidance in the system prompt, `glob`/`read`/`edit` drill-in hints in the project prompt, `ask` directives in plan mode, and the orchestrate notice's `task`/`edit`/`write`/`lsp`/`bash`/`todo` budget are now gated on tool availability. - Fixed a `/skill:` token embedded in a `/plan` or `/vibe` inline prompt being sent to the agent as literal text instead of loading the skill; mode-command inline prompts now dispatch skill invocations through the same custom-message path as the editor submit flow ([#8137](https://github.com/can1357/oh-my-pi/issues/8137)). - Fixed Codex reset fireworks triggering on ordinary weekly-usage decreases when the provider had not advanced the quota reset deadline. +### Fixed + +- Fixed below-threshold tool turns waiting for asynchronous session persistence when no mid-run compaction will run, while preserving journal writes when a `message_end` listener fails and isolating notification-only handler payloads from late context mutations ([#8283](https://github.com/can1357/oh-my-pi/pull/8283) by [@ethancawse](https://github.com/ethancawse)). ## [17.2.15] - 2026-08-12 diff --git a/packages/coding-agent/src/extensibility/extensions/types.ts b/packages/coding-agent/src/extensibility/extensions/types.ts index 964d422ec..766519f7a 100644 --- a/packages/coding-agent/src/extensibility/extensions/types.ts +++ b/packages/coding-agent/src/extensibility/extensions/types.ts @@ -733,7 +733,10 @@ export interface MessageUpdateEvent { assistantMessageEvent: AssistantMessageEvent; } -/** Fired when a message ends */ +/** + * Fired when a message ends. Notification-only: the message is a detached + * snapshot, so in-place changes do not rewrite agent or provider context. + */ export interface MessageEndEvent { type: "message_end"; message: AgentMessage; diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 61f851008..ea3fcf31f 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -423,6 +423,35 @@ type SetSessionNameWithTrigger = ( const kPersistedSessionEntryId = Symbol("persistedSessionEntryId"); type PersistedAssistantMessage = AssistantMessage & { [kPersistedSessionEntryId]?: string }; +/** + * Clone one top-level notification field without ever returning an object owned + * by the live session. Most values take the lossless structured-clone path. If + * a third-party metadata object contains functions or other unsupported values, + * JSON sanitization drops those values; a cyclic/non-JSON value finally degrades + * to a descriptive string rather than retaining a shared mutable reference. + */ +function cloneMessageEndNotificationField(value: unknown): unknown { + try { + return structuredClone(value); + } catch {} + try { + const json = JSON.stringify(value); + if (json !== undefined) return JSON.parse(json) as unknown; + } catch {} + return String(value); +} + +/** Build a detached, notification-only snapshot of an AgentMessage. */ +function cloneMessageEndNotification(message: AgentMessage): AgentMessage { + const snapshot: Record = {}; + for (const key of Reflect.ownKeys(message)) { + const descriptor = Object.getOwnPropertyDescriptor(message, key); + if (!descriptor?.enumerable) continue; + snapshot[key] = cloneMessageEndNotificationField(Reflect.get(message, key)); + } + return snapshot as unknown as AgentMessage; +} + export class AgentSession { readonly agent: Agent; readonly sessionManager: SessionManager; @@ -2297,6 +2326,27 @@ export class AgentSession { } } + #persistMessageEnd(message: AgentMessage): void { + if (message.role === "hookMessage" || message.role === "custom") { + // Prewalk's plan nudge is a one-run steering instruction. Persisting it would + // resurrect the consumed prompt on resume, fork, or any context rebuild. + if (!isPrewalkPlanNudge(message)) { + this.sessionManager.appendCustomMessageEntry( + message.customType, + message.content, + message.display, + message.details, + message.attribution ?? "agent", + ); + } + if (message.role === "custom" && message.customType === "ttsr-injection") { + this.#ttsr.markInjectedFromDetails(message.details); + } + return; + } + this.#persistSessionMessageIfMissing(message); + } + /** * On a user-interrupted (`Esc`) abort, copy the trailing thinking run into a * hidden `display: false` continuity message for the next turn WITHOUT @@ -2469,7 +2519,17 @@ export class AgentSession { try { await this.#emitSessionEvent(displayEvent); } catch (error) { - messageEndPersistence?.release(); + if (event.type === "message_end") { + const persistMessageEnd = () => this.#persistMessageEnd(event.message); + try { + if (messageEndPersistence) await messageEndPersistence.persist(persistMessageEnd); + else persistMessageEnd(); + } catch (persistenceError) { + logger.warn("Failed to persist message after session event emission failed", { + error: String(persistenceError), + }); + } + } throw error; } } @@ -2527,27 +2587,7 @@ export class AgentSession { // Handle session persistence if (event.type === "message_end") { - const persistMessageEnd = () => { - // Check if this is a hook/custom message - if (event.message.role === "hookMessage" || event.message.role === "custom") { - // Prewalk's plan nudge is a one-run steering instruction. Persisting it would - // resurrect the consumed prompt on resume, fork, or any context rebuild. - if (!isPrewalkPlanNudge(event.message)) { - this.sessionManager.appendCustomMessageEntry( - event.message.customType, - event.message.content, - event.message.display, - event.message.details, - event.message.attribution ?? "agent", - ); - } - if (event.message.role === "custom" && event.message.customType === "ttsr-injection") { - this.#ttsr.markInjectedFromDetails(event.message.details); - } - } else { - this.#persistSessionMessageIfMissing(event.message); - } - }; + const persistMessageEnd = () => this.#persistMessageEnd(event.message); if (messageEndPersistence) { await messageEndPersistence.persist(persistMessageEnd); } else { @@ -3426,9 +3466,16 @@ export class AgentSession { }; await this.#extensionRunner.emit(extensionEvent); } else if (event.type === "message_end") { + // `message_end` is a notification, not a context-rewrite hook. Detach its + // payload from agent-owned history so an async observer that mutates the + // event after an `await` cannot race mid-run maintenance and enlarge (or + // otherwise rewrite) the next provider request after its threshold check. + // Explicit `tool_result` / `context` hooks remain the supported mutation + // surfaces. Third-party metadata that is not structured-cloneable is + // sanitized field-by-field without retaining nested live references. const extensionEvent: MessageEndEvent = { type: "message_end", - message: event.message, + message: cloneMessageEndNotification(event.message), }; await this.#extensionRunner.emit(extensionEvent); } else if (event.type === "tool_execution_start") { diff --git a/packages/coding-agent/src/session/session-maintenance.ts b/packages/coding-agent/src/session/session-maintenance.ts index ac451716a..4a62968fb 100644 --- a/packages/coding-agent/src/session/session-maintenance.ts +++ b/packages/coding-agent/src/session/session-maintenance.ts @@ -1097,6 +1097,16 @@ export class SessionMaintenance { .find((message): message is AssistantMessage => message.role === "assistant"); if (!lastAssistant || lastAssistant.stopReason === "aborted" || lastAssistant.stopReason === "error") return; + // Decide from the live agent context before waiting for the asynchronous + // session journal. The persistence barrier is required only when maintenance + // will actually rewrite history; awaiting it on every ordinary tool turn lets + // a slow message_end listener leave the TUI "generating" with no provider + // request or tool running. + const billedContextTokens = calculateContextTokens(lastAssistant.usage); + const storedContextTokens = this.#estimateStoredContextTokens(); + const contextTokens = compactionContextTokens(billedContextTokens, storedContextTokens); + if (!shouldCompact(contextTokens, contextWindow, compactionSettings)) return; + if (!(await this.#host.persistTurnMessagesForMidRunCompaction(context))) return; if (this.#midTurnCompactionDeadEnds.has(activeMessages)) { // A prior boundary already ran the dead-end rescue and could not reduce @@ -1118,11 +1128,6 @@ export class SessionMaintenance { this.#midTurnDeadEndPendingPrePrompt = false; } - const billedContextTokens = calculateContextTokens(lastAssistant.usage); - const storedContextTokens = this.#estimateStoredContextTokens(); - const contextTokens = compactionContextTokens(billedContextTokens, storedContextTokens); - if (!shouldCompact(contextTokens, contextWindow, compactionSettings)) return; - // Promote to a larger-context sibling before compacting, mirroring the // pre-prompt (runPrePromptCompactionIfNeeded) and post-turn threshold // (checkCompaction) paths. Without this, a long mid-turn tool loop that diff --git a/packages/coding-agent/test/agent-session-goal-midrun-compaction.test.ts b/packages/coding-agent/test/agent-session-goal-midrun-compaction.test.ts index 25101f333..5c253f909 100644 --- a/packages/coding-agent/test/agent-session-goal-midrun-compaction.test.ts +++ b/packages/coding-agent/test/agent-session-goal-midrun-compaction.test.ts @@ -78,7 +78,12 @@ describe("AgentSession mid-run threshold compaction", () => { async function createHarness( settingsOverride: Record = {}, - options: { extensionRunner?: ExtensionRunner } = {}, + options: { + extensionRunner?: ExtensionRunner; + onProviderCall?: (index: number) => void; + configureAgent?: (agent: Agent) => void; + toolResultDetails?: unknown; + } = {}, ): Promise<{ session: AgentSession; observedContexts: string[][]; @@ -108,7 +113,10 @@ describe("AgentSession mid-run threshold compaction", () => { label: "Bash", description: "Mock bash tool", parameters: type({}), - execute: async () => ({ content: [{ type: "text" as const, text: "tool output" }] }), + execute: async () => ({ + content: [{ type: "text" as const, text: "tool output" }], + ...(options.toolResultDetails === undefined ? {} : { details: options.toolResultDetails }), + }), }; let call = 0; @@ -118,6 +126,7 @@ describe("AgentSession mid-run threshold compaction", () => { convertToLlm, streamFn: (_model, context) => { const index = call++; + options.onProviderCall?.(index); observedContexts.push(context.messages.map(message => JSON.stringify(message))); const stream = new AssistantMessageEventStream(); const isToolTurn = index === 0; @@ -151,6 +160,7 @@ describe("AgentSession mid-run threshold compaction", () => { return stream; }, }); + options.configureAgent?.(agent); const session = new AgentSession({ agent, @@ -212,6 +222,162 @@ describe("AgentSession mid-run threshold compaction", () => { expect(observedContexts[1].join("\n")).toContain("HANDOFF-MID-RUN-COMPACTED-IN-PLACE"); }); + it("does not wait for message persistence below the mid-run threshold", async () => { + const releaseMessageEnd = Promise.withResolvers(); + const messageEndEntered = Promise.withResolvers(); + const nextProviderCall = Promise.withResolvers(); + const extensionRunner = { + hasHandlers: vi.fn((eventType: string) => eventType === "message_end"), + emitBeforeAgentStart: vi.fn(async () => undefined), + emit: vi.fn(async (event: { type: string; message?: AgentMessage }) => { + if ( + event.type === "message_end" && + event.message?.role === "assistant" && + event.message.stopReason === "toolUse" + ) { + messageEndEntered.resolve(); + await releaseMessageEnd.promise; + } + }), + } as unknown as ExtensionRunner; + const { session } = await createHarness( + { "compaction.thresholdTokens": 100_000 }, + { + extensionRunner, + onProviderCall: index => { + if (index === 1) nextProviderCall.resolve(); + }, + }, + ); + const compactSpy = mockCompaction("SHOULD-NOT-RUN"); + + const prompt = session.prompt("work below the maintenance threshold"); + const messageEndOutcome = await Promise.race([ + messageEndEntered.promise.then(() => "entered" as const), + Bun.sleep(2_000).then(() => "blocked" as const), + ]); + const providerOutcome = + messageEndOutcome === "entered" + ? await Promise.race([ + nextProviderCall.promise.then(() => "dispatched" as const), + Bun.sleep(2_000).then(() => "blocked" as const), + ]) + : "blocked"; + releaseMessageEnd.resolve(); + const promptOutcome = await Promise.race([ + prompt.then(() => "settled" as const), + Bun.sleep(2_000).then(() => "blocked" as const), + ]); + + expect(messageEndOutcome).toBe("entered"); + expect(providerOutcome).toBe("dispatched"); + expect(promptOutcome).toBe("settled"); + expect(compactSpy).not.toHaveBeenCalled(); + }); + + it("persists a tool result when its message_end listener rejects below the mid-run threshold", async () => { + let rejected = false; + const extensionRunner = { + hasHandlers: vi.fn((eventType: string) => eventType === "message_end"), + emitBeforeAgentStart: vi.fn(async () => undefined), + emit: vi.fn(async (event: { type: string; message?: AgentMessage }) => { + if (!rejected && event.type === "message_end" && event.message?.role === "toolResult") { + rejected = true; + throw new Error("intentional message_end failure"); + } + }), + } as unknown as ExtensionRunner; + const { session, sessionManager } = await createHarness( + { "compaction.thresholdTokens": 100_000 }, + { extensionRunner }, + ); + + await session.prompt("work below the maintenance threshold"); + + const persistedToolResults = sessionManager + .getBranch() + .filter(entry => entry.type === "message" && entry.message.role === "toolResult"); + expect(rejected).toBe(true); + expect(persistedToolResults).toHaveLength(1); + }); + + it("isolates late message_end mutations from the next provider request", async () => { + const releaseMutation = Promise.withResolvers(); + const mutationApplied = Promise.withResolvers(); + const toolResultHookEntered = Promise.withResolvers(); + const secondModelCallEntered = Promise.withResolvers(); + const releaseSecondModelCall = Promise.withResolvers(); + const mutationMarker = `LATE-MESSAGE-END-MUTATION-${"x".repeat(500_000)}`; + const liveDetails = { + nested: { state: "original" }, + nonCloneable: () => "third-party callback", + }; + let interceptedToolResult = false; + const extensionRunner = { + hasHandlers: vi.fn((eventType: string) => eventType === "message_end"), + emitBeforeAgentStart: vi.fn(async () => undefined), + emit: vi.fn(async (event: { type: string; message?: AgentMessage }) => { + if (interceptedToolResult || event.type !== "message_end" || event.message?.role !== "toolResult") return; + interceptedToolResult = true; + toolResultHookEntered.resolve(); + await releaseMutation.promise; + event.message.content = [{ type: "text", text: mutationMarker }]; + (event.message.details as { nested: { state: string } }).nested.state = "mutated"; + mutationApplied.resolve(); + }), + } as unknown as ExtensionRunner; + let modelCall = 0; + const { session, observedContexts } = await createHarness( + { "compaction.thresholdTokens": 100_000 }, + { + extensionRunner, + toolResultDetails: liveDetails, + configureAgent: agent => { + agent.addBeforeModelCallHook(async () => { + if (modelCall++ !== 1) return; + secondModelCallEntered.resolve(); + await releaseSecondModelCall.promise; + }); + }, + }, + ); + + const prompt = session.prompt("keep notification mutations out of live context"); + const toolResultHookOutcome = await Promise.race([ + toolResultHookEntered.promise.then(() => "entered" as const), + Bun.sleep(2_000).then(() => "blocked" as const), + ]); + const secondModelCallOutcome = + toolResultHookOutcome === "entered" + ? await Promise.race([ + secondModelCallEntered.promise.then(() => "dispatched" as const), + Bun.sleep(2_000).then(() => "blocked" as const), + ]) + : "blocked"; + releaseMutation.resolve(); + const mutationOutcome = await Promise.race([ + mutationApplied.promise.then(() => "applied" as const), + Bun.sleep(2_000).then(() => "blocked" as const), + ]); + releaseSecondModelCall.resolve(); + const promptOutcome = await Promise.race([ + prompt.then(() => "settled" as const), + Bun.sleep(2_000).then(() => "blocked" as const), + ]); + + expect(toolResultHookOutcome).toBe("entered"); + expect(secondModelCallOutcome).toBe("dispatched"); + expect(mutationOutcome).toBe("applied"); + expect(promptOutcome).toBe("settled"); + expect(observedContexts).toHaveLength(2); + expect(observedContexts[1].join("\n")).not.toContain("LATE-MESSAGE-END-MUTATION"); + expect(JSON.stringify(session.messages)).not.toContain("LATE-MESSAGE-END-MUTATION"); + expect(liveDetails.nested.state).toBe("original"); + const storedToolResult = session.messages.find(message => message.role === "toolResult"); + if (!storedToolResult) throw new Error("Expected a stored tool result"); + expect((storedToolResult.details as { nested: { state: string } }).nested.state).toBe("original"); + }); + it("preserves the just-finished tool turn when message_end hooks are still pending", async () => { const releaseMessageEnd = Promise.withResolvers(); const messageEndEntered = Promise.withResolvers(); @@ -277,7 +443,7 @@ describe("AgentSession mid-run threshold compaction", () => { expect(persistedToolTurnRoles).toEqual(["assistant", "toolResult"]); }); - it("treats same-key assistant content variants as persisted before mid-run compaction", async () => { + it("keeps synchronous message_end mutations notification-local during mid-run compaction", async () => { const extensionRuntime = new ExtensionRuntime(); const extension = await loadExtensionFromFactory( pi => { @@ -308,6 +474,7 @@ describe("AgentSession mid-run threshold compaction", () => { expect(compactSpy).toHaveBeenCalledTimes(1); expect(observedContexts.length).toBeGreaterThanOrEqual(2); expect(observedContexts[1].join("\n")).toContain("MID-RUN-COMPACTED-WITH-CONTENT-VARIANT"); + expect(JSON.stringify(session.messages)).not.toContain("display-variant"); }); it("does not compact mid-run outside goal mode when disabled", async () => {