From bcb48c4e4355d59ba43c4add3a2bcf05b0c388fb Mon Sep 17 00:00:00 2001 From: can1357 Date: Sat, 6 Jun 2026 15:41:25 +0200 Subject: [PATCH] fix(coding-agent/modes): enabled eager foreground rendering and suppressed duplicate error lines - Updated EventController to keep foreground render mode active from agent_start through agent_end so live-region rebuild remains enabled during a turn. - Pinned stop-reason assistant errors to the editor banner and suppressed the transcript's inline Error line while pinned, then restored it on the next agent_start. - Added tests covering error-banner suppression/restoration and eager native scrollback rebuild ordering before loading animation. --- packages/coding-agent/CHANGELOG.md | 2 + .../src/modes/components/assistant-message.ts | 23 ++++++- .../src/modes/controllers/event-controller.ts | 36 +++++++--- .../event-controller-error-banner.test.ts | 57 +++++++++++++++- .../test/interactive-mode-shutdown.test.ts | 67 ------------------- .../event-controller-tool-render-mode.test.ts | 16 ++++- packages/hashline/src/input.ts | 4 +- 7 files changed, 120 insertions(+), 85 deletions(-) delete mode 100644 packages/coding-agent/test/interactive-mode-shutdown.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index b3fc3d319..0acd15da1 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -16,6 +16,7 @@ ### Fixed +- Fixed the idle `Working...` loader freezing on ED3-risk terminals with unobservable native scrollback by keeping foreground live-region rendering enabled from `agent_start` until `agent_end`, before the first assistant or tool event arrives. - Fixed framed tool output blocks rendering one column inset inside tool boxes; modern bordered blocks now span the same width as legacy background-filled tool boxes. - Fixed potential `TimeoutError` aborts for short `timeout` eval cells during long bridged `agent()`/`llm()` work where no progress events are emitted until completion - Fixed retry recovery to allow automatic retries without switching models when `retry.modelFallback` is disabled. @@ -25,6 +26,7 @@ - Fixed `sdk-async-job-manager-singleton` tests flaking under the full parallel suite. The four `createAgentSession`-based cases ran on the default 5000ms per-test timeout, which two real session startups can exceed when `test:ts` saturates the machine across packages; on timeout the still-running test body and `afterEach` reset raced, surfacing a spurious "Unhandled error between tests" on the `AsyncJobManager.instance()` assertion. They now carry an explicit 60000ms timeout, matching the convention used by the other session-creating tests in this suite. - Fixed streaming `eval`, `bash`, `ssh`, and `task` call previews overflowing the live transcript viewport and cutting off their top while pending. A volatile tool block taller than the viewport could strand its scrolled-off head out of native scrollback on ED3-risk terminals (committed nowhere, repainted nowhere) until the result landed. The pending `eval` source preview now follows the streaming edge in a bounded 12-line tail window (newest lines pinned to the bottom, "… N earlier lines" on top) so you can watch the code being written without the box overflowing; `bash`/`ssh` commands and `task` context use a bounded head+tail window. `Ctrl+O` still lifts the cap for a full view. - Fixed the streaming `write` call preview ignoring `Ctrl+O` so the expand toggle was a no-op while a file was being written. Unlike the `eval`/`bash`/`ssh`/`task` streaming previews, `formatStreamingContent` never received the `expanded` flag, leaving the preview pinned to a bounded 12-line tail window even after pressing `Ctrl+O` — so on a large write you could not widen past the streaming edge until the tool result landed. The preview now lifts the cap to the full file (head through tail) when expanded, matching the documented streaming-preview behavior of the other tools. +- Fixed turn-ending provider errors rendering twice — once as the transcript's inline `Error: …` line and again in the pinned banner above the editor (added in 15.9.5). The inline line is now suppressed while the same error is mirrored in the banner and restored to the transcript when the banner clears at the next turn, so the error stays in history without the duplicate render at the error moment. ### Removed diff --git a/packages/coding-agent/src/modes/components/assistant-message.ts b/packages/coding-agent/src/modes/components/assistant-message.ts index 17e9ab0b8..d61ae5c04 100644 --- a/packages/coding-agent/src/modes/components/assistant-message.ts +++ b/packages/coding-agent/src/modes/components/assistant-message.ts @@ -18,6 +18,15 @@ export class AssistantMessageComponent extends Container { #convertedKittyImages = new Map(); #kittyConversionsInFlight = new Set(); #transcriptBlockFinalized: boolean; + /** + * When true, the turn-ending `Error: …` line for `stopReason === "error"` is + * suppressed because the same error is currently shown in the pinned banner + * above the editor (see `EventController` + `ErrorBannerComponent`). Avoids + * rendering the identical error twice (inline + banner) at the error moment. + * Restored to `false` when the banner is cleared at the next turn so the + * transcript keeps the error in history. + */ + #errorPinned = false; constructor( message?: AssistantMessage, @@ -49,6 +58,18 @@ export class AssistantMessageComponent extends Container { this.hideThinkingBlock = hide; } + /** + * Toggle suppression of the inline `Error: …` line while the same error is + * pinned in the banner above the editor. Re-renders so the change is visible. + */ + setErrorPinned(pinned: boolean): void { + if (this.#errorPinned === pinned) return; + this.#errorPinned = pinned; + if (this.#lastMessage) { + this.updateContent(this.#lastMessage); + } + } + isTranscriptBlockFinalized(): boolean { return this.#transcriptBlockFinalized; } @@ -246,7 +267,7 @@ export class AssistantMessageComponent extends Container { this.#contentContainer.addChild(new Spacer(1)); } this.#contentContainer.addChild(new Text(theme.fg("error", abortMessage), 1, 0)); - } else if (message.stopReason === "error") { + } else if (message.stopReason === "error" && !this.#errorPinned) { const errorMsg = message.errorMessage || "Unknown error"; this.#contentContainer.addChild(new Spacer(1)); this.#contentContainer.addChild(new Text(theme.fg("error", `Error: ${errorMsg}`), 1, 0)); diff --git a/packages/coding-agent/src/modes/controllers/event-controller.ts b/packages/coding-agent/src/modes/controllers/event-controller.ts index 2bc4734a1..7b9fb1ba7 100644 --- a/packages/coding-agent/src/modes/controllers/event-controller.ts +++ b/packages/coding-agent/src/modes/controllers/event-controller.ts @@ -49,9 +49,15 @@ export class EventController { #lastIntent: string | undefined = undefined; #backgroundToolCallIds = new Set(); #assistantMessageStreaming = false; + #agentTurnActive = false; #readToolCallArgs = new Map>(); #readToolCallAssistantComponents = new Map(); #lastAssistantComponent: AssistantMessageComponent | undefined = undefined; + // Assistant component whose turn-ending error is currently mirrored in the + // pinned banner. Its inline `Error: …` line is suppressed while pinned and + // restored when the banner clears at the next `agent_start` (see + // #handleMessageEnd / #handleAgentStart). + #pinnedErrorComponent: AssistantMessageComponent | undefined = undefined; #idleCompactionTimer?: NodeJS.Timeout; #ircExpiryTimers = new Map(); #handlers: AgentSessionEventHandlers; @@ -172,21 +178,21 @@ export class EventController { const run = this.#handlers[event.type] as (e: AgentSessionEvent) => Promise; await run(event); - // While assistant text or a foreground tool is streaming, rows above the - // viewport can re-layout after they have already entered native scrollback - // (Markdown fences, wrapping, previews). Let the TUI rebuild history on - // those offscreen edits instead of deferring, which otherwise leaves stale - // tail rows duplicated above the live viewport. - // Background-running tools are excluded so late async updates outside the - // active foreground stream keep the no-yank deferral; agent_start resets - // the mode at every turn boundary. + // While an assistant turn is active, visible status chrome and foreground + // transcript blocks can re-render after rows have entered native scrollback + // (idle Working loader, Markdown fences, wrapping, tool previews). Let the + // TUI use its foreground live-region path instead of idle deferral, which + // can otherwise leave the loader/status frame frozen until the next input. + // Background-running tools after the turn ends are excluded so late async + // updates keep the no-yank deferral; agent_start/agent_end bracket the + // foreground turn. if (STREAM_RENDER_MODE_EVENTS[event.type]) { this.#refreshToolRenderMode(); } } #refreshToolRenderMode(): void { - let foregroundToolActive = this.#assistantMessageStreaming; + let foregroundToolActive = this.#agentTurnActive || this.#assistantMessageStreaming; if (!foregroundToolActive) { for (const toolCallId of this.ctx.pendingTools.keys()) { if (!this.#backgroundToolCallIds.has(toolCallId)) { @@ -199,11 +205,16 @@ export class EventController { } async #handleAgentStart(_event: Extract): Promise { + this.#agentTurnActive = true; this.#lastIntent = undefined; this.#readToolCallArgs.clear(); this.#readToolCallAssistantComponents.clear(); this.#assistantMessageStreaming = false; this.#lastAssistantComponent = undefined; + // Restore the previous turn's inline error in the transcript before dropping + // the banner, so the error stays in history once the banner is gone. + this.#pinnedErrorComponent?.setErrorPinned(false); + this.#pinnedErrorComponent = undefined; this.ctx.clearPinnedError(); if (this.ctx.retryEscapeHandler) { this.ctx.editor.onEscape = this.ctx.retryEscapeHandler; @@ -215,6 +226,7 @@ export class EventController { this.ctx.statusContainer.clear(); } this.#cancelIdleCompaction(); + this.#refreshToolRenderMode(); this.ctx.ensureLoadingAnimation(); this.ctx.ui.requestRender(); } @@ -493,12 +505,15 @@ export class EventController { this.ctx.streamingMessage = undefined; // Pin a turn-ending provider error (e.g. Anthropic content-filter block) // above the editor so it survives transcript scroll. Cleared at the next - // turn's agent_start. + // turn's agent_start. Suppress the transcript's inline `Error: …` line for + // the same message while pinned so the error isn't rendered twice. if ( event.message.stopReason === "error" && event.message.errorMessage && !isSilentAbort(event.message.errorMessage) ) { + this.#lastAssistantComponent?.setErrorPinned(true); + this.#pinnedErrorComponent = this.#lastAssistantComponent; this.ctx.showPinnedError(event.message.errorMessage); } this.ctx.statusLine.invalidate(); @@ -646,6 +661,7 @@ export class EventController { } } async #handleAgentEnd(_event: Extract): Promise { + this.#agentTurnActive = false; this.#assistantMessageStreaming = false; if (this.ctx.loadingAnimation) { this.ctx.loadingAnimation.stop(); diff --git a/packages/coding-agent/test/event-controller-error-banner.test.ts b/packages/coding-agent/test/event-controller-error-banner.test.ts index 4fa39bcee..7400343cb 100644 --- a/packages/coding-agent/test/event-controller-error-banner.test.ts +++ b/packages/coding-agent/test/event-controller-error-banner.test.ts @@ -7,8 +7,10 @@ * `agent_start` via `ctx.clearPinnedError`. Aborts and normal stops must NOT * pin a banner. */ -import { beforeAll, describe, expect, it, vi } from "bun:test"; +import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "bun:test"; import type { AssistantMessage } from "@oh-my-pi/pi-ai"; +import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { AssistantMessageComponent } from "@oh-my-pi/pi-coding-agent/modes/components/assistant-message"; import { ErrorBannerComponent } from "@oh-my-pi/pi-coding-agent/modes/components/error-banner"; import { EventController } from "@oh-my-pi/pi-coding-agent/modes/controllers/event-controller"; import { initTheme } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; @@ -40,12 +42,22 @@ beforeAll(async () => { await initTheme(false); }); +beforeEach(async () => { + resetSettingsForTest(); + await Settings.init({ inMemory: true }); +}); + +afterEach(() => { + resetSettingsForTest(); +}); + function createFixture(streamingMessage?: AssistantMessage) { const streamingComponent = { updateContent: vi.fn(), setUsageInfo: vi.fn(), setComplete: vi.fn(), markTranscriptBlockFinalized: vi.fn(), + setErrorPinned: vi.fn(), }; const showPinnedError = vi.fn(); const clearPinnedError = vi.fn(); @@ -67,14 +79,14 @@ function createFixture(streamingMessage?: AssistantMessage) { } as unknown as InteractiveModeContext; const controller = new EventController(ctx); - return { controller, ctx, showPinnedError, clearPinnedError }; + return { controller, ctx, showPinnedError, clearPinnedError, streamingComponent }; } describe("EventController error banner", () => { it("pins the provider error above the editor when an assistant turn ends on stopReason error", async () => { const errorMessage = "Output blocked by content filtering policy"; const message = makeAssistantMessage({ stopReason: "error", errorMessage }); - const { controller, showPinnedError } = createFixture(message); + const { controller, showPinnedError, streamingComponent } = createFixture(message); await controller.handleEvent({ type: "message_end", message } as Extract< AgentSessionEvent, @@ -83,6 +95,26 @@ describe("EventController error banner", () => { expect(showPinnedError).toHaveBeenCalledTimes(1); expect(showPinnedError).toHaveBeenCalledWith(errorMessage); + // The same error is mirrored in the banner, so the transcript's inline + // `Error: …` line is suppressed to avoid a duplicate render. + expect(streamingComponent.setErrorPinned).toHaveBeenCalledWith(true); + }); + + it("restores the transcript inline error when the next turn starts", async () => { + const errorMessage = "Output blocked by content filtering policy"; + const message = makeAssistantMessage({ stopReason: "error", errorMessage }); + const { controller, clearPinnedError, streamingComponent } = createFixture(message); + + await controller.handleEvent({ type: "message_end", message } as Extract< + AgentSessionEvent, + { type: "message_end" } + >); + streamingComponent.setErrorPinned.mockClear(); + + await controller.handleEvent({ type: "agent_start" } as Extract); + + expect(clearPinnedError).toHaveBeenCalledTimes(1); + expect(streamingComponent.setErrorPinned).toHaveBeenCalledWith(false); }); it("does not pin a banner for a normal assistant stop", async () => { @@ -135,3 +167,22 @@ describe("ErrorBannerComponent", () => { expect(detailLines.length).toBeGreaterThan(0); }); }); + +describe("AssistantMessageComponent error pinning", () => { + it("hides the inline error while pinned and restores it afterwards", () => { + const message = makeAssistantMessage({ + content: [], + stopReason: "error", + errorMessage: "400 invalid reasoning value", + }); + const component = new AssistantMessageComponent(message); + + expect(Bun.stripANSI(component.render(120).join("\n"))).toContain("Error: 400 invalid reasoning value"); + + component.setErrorPinned(true); + expect(Bun.stripANSI(component.render(120).join("\n"))).not.toContain("Error: 400 invalid reasoning value"); + + component.setErrorPinned(false); + expect(Bun.stripANSI(component.render(120).join("\n"))).toContain("Error: 400 invalid reasoning value"); + }); +}); diff --git a/packages/coding-agent/test/interactive-mode-shutdown.test.ts b/packages/coding-agent/test/interactive-mode-shutdown.test.ts deleted file mode 100644 index 3d010d68d..000000000 --- a/packages/coding-agent/test/interactive-mode-shutdown.test.ts +++ /dev/null @@ -1,67 +0,0 @@ -import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "bun:test"; -import * as path from "node:path"; -import { Agent } from "@oh-my-pi/pi-agent-core"; -import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; -import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; -import { InteractiveMode } from "@oh-my-pi/pi-coding-agent/modes/interactive-mode"; -import { initTheme } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; -import { AgentSession } from "@oh-my-pi/pi-coding-agent/session/agent-session"; -import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage"; -import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; -import { postmortem, TempDir } from "@oh-my-pi/pi-utils"; - -describe("InteractiveMode shutdown", () => { - let authStorage: AuthStorage; - let mode: InteractiveMode; - let session: AgentSession; - let tempDir: TempDir; - - beforeAll(() => { - initTheme(); - }); - - beforeEach(async () => { - resetSettingsForTest(); - tempDir = TempDir.createSync("@pi-shutdown-"); - await Settings.init({ inMemory: true, cwd: tempDir.path() }); - authStorage = await AuthStorage.create(path.join(tempDir.path(), "testauth.db")); - const modelRegistry = new ModelRegistry(authStorage); - const model = modelRegistry.find("anthropic", "claude-sonnet-4-5"); - if (!model) throw new Error("Expected claude-sonnet-4-5 test model"); - - session = new AgentSession({ - agent: new Agent({ initialState: { model, systemPrompt: ["Test"], tools: [], messages: [] } }), - sessionManager: SessionManager.create(tempDir.path(), tempDir.path()), - settings: Settings.isolated(), - modelRegistry, - }); - mode = new InteractiveMode(session, "test"); - }); - - afterEach(async () => { - mode?.stop(); - vi.restoreAllMocks(); - await session?.dispose(); - authStorage?.close(); - tempDir?.removeSync(); - resetSettingsForTest(); - }); - - it("stops from the last committed TUI frame without forcing a teardown repaint", async () => { - const requestRenderSpy = vi.spyOn(mode.ui, "requestRender").mockImplementation(() => {}); - const stopSpy = vi.spyOn(mode.ui, "stop").mockImplementation(() => {}); - const drainSpy = vi.spyOn(mode.ui.terminal, "drainInput").mockResolvedValue(undefined); - const disposeSpy = vi.spyOn(session, "dispose").mockResolvedValue(undefined); - const quitSpy = vi.spyOn(postmortem, "quit").mockResolvedValue(undefined); - vi.spyOn(session.sessionManager, "getSessionId").mockReturnValue(""); - mode.isInitialized = true; - - await mode.shutdown(); - - expect(disposeSpy).toHaveBeenCalled(); - expect(requestRenderSpy.mock.calls.some(call => call[0] === true)).toBe(false); - expect(drainSpy).toHaveBeenCalledWith(1000); - expect(stopSpy).toHaveBeenCalled(); - expect(quitSpy).toHaveBeenCalledWith(0); - }); -}); diff --git a/packages/coding-agent/test/modes/controllers/event-controller-tool-render-mode.test.ts b/packages/coding-agent/test/modes/controllers/event-controller-tool-render-mode.test.ts index f0bb4d15c..34a320451 100644 --- a/packages/coding-agent/test/modes/controllers/event-controller-tool-render-mode.test.ts +++ b/packages/coding-agent/test/modes/controllers/event-controller-tool-render-mode.test.ts @@ -6,6 +6,7 @@ import type { AgentSessionEvent } from "@oh-my-pi/pi-coding-agent/session/agent- function createContext() { const setEagerNativeScrollbackRebuild = vi.fn(); + const ensureLoadingAnimation = vi.fn(); const pendingTools = new Map(); const chatContainer = { addChild: vi.fn(), removeChild: vi.fn() }; const ctx = { @@ -25,8 +26,10 @@ function createContext() { retryAttempt: 0, }, ui: { setEagerNativeScrollbackRebuild, requestRender: vi.fn() }, + clearPinnedError: vi.fn(), + ensureLoadingAnimation, } as unknown as InteractiveModeContext; - return { ctx, pendingTools, setEagerNativeScrollbackRebuild }; + return { ctx, pendingTools, setEagerNativeScrollbackRebuild, ensureLoadingAnimation }; } // A tool_execution_update for an id that is not pending is a no-op in its handler, @@ -67,6 +70,17 @@ describe("EventController tool render mode", () => { vi.restoreAllMocks(); }); + it("enables eager native scrollback rebuild before starting the idle Working loader", async () => { + const { ctx, ensureLoadingAnimation, setEagerNativeScrollbackRebuild } = createContext(); + const controller = new EventController(ctx); + + await controller.handleEvent({ type: "agent_start" } as unknown as AgentSessionEvent); + + expect(setEagerNativeScrollbackRebuild).toHaveBeenCalledWith(true); + expect(setEagerNativeScrollbackRebuild.mock.invocationCallOrder[0]!).toBeLessThan( + ensureLoadingAnimation.mock.invocationCallOrder[0]!, + ); + }); it("enables eager native scrollback rebuild while a foreground tool is pending", async () => { const { ctx, pendingTools, setEagerNativeScrollbackRebuild } = createContext(); const controller = new EventController(ctx); diff --git a/packages/hashline/src/input.ts b/packages/hashline/src/input.ts index 0dc597cc1..3513d1aea 100644 --- a/packages/hashline/src/input.ts +++ b/packages/hashline/src/input.ts @@ -55,9 +55,7 @@ function stripApplyPatchPathNoise(pathText: string): string { */ function tryParseRecoveryHeader(line: string, cwd?: string): RawSection | null { if (!line.startsWith(HL_FILE_PREFIX) || !line.endsWith(HL_FILE_SUFFIX)) return null; - const body = stripApplyPatchPathNoise( - line.slice(HL_FILE_PREFIX.length, line.length - HL_FILE_SUFFIX.length).trim(), - ); + const body = stripApplyPatchPathNoise(line.slice(HL_FILE_PREFIX.length, line.length - HL_FILE_SUFFIX.length).trim()); if (body.length === 0) return null; // Trailing `#XXXX` is the tag; everything before it is the path. The