From c118eacdaa6027980e85b4e9c86d7c20dd9865f6 Mon Sep 17 00:00:00 2001 From: cognitive <152830360+METAeuPHORIC@users.noreply.github.com> Date: Wed, 13 May 2026 23:48:51 +0000 Subject: [PATCH] fix: gate SILENT_ABORT_MARKER at three unguarded render sites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review flagged that the silent-abort sentinel ("__omp.silent_abort__") persists into AssistantMessage.errorMessage but three downstream consumers render errorMessage verbatim: - session-observer-overlay.ts: renders "✗ Error: __omp.silent_abort__" when content is empty (confirmed user-visible today) - print-mode.ts: writes marker to stderr and exits non-zero (latent; plan-mode→compact not reachable from print mode today, but unguarded) - acp-agent.ts: emits marker as agent_message_chunk text to ACP clients when message has no other notifications (latent) Add isSilentAbort() guard at each site. Extend the SILENT_ABORT_MARKER consumer list in messages.ts doc comment to include all six consumers. Add regression tests: overlay (2 tests), print-mode (2 tests), ACP replay (1 test). Op: correct Restores: spec:silent-abort-marker-never-surfaces --- .../coding-agent/src/modes/acp/acp-agent.ts | 4 +- .../components/session-observer-overlay.ts | 3 +- packages/coding-agent/src/modes/print-mode.ts | 8 +- packages/coding-agent/src/session/messages.ts | 6 +- packages/coding-agent/test/acp-agent.test.ts | 49 ++++++ .../test/silent-abort-overlay-render.test.ts | 166 ++++++++++++++++++ .../test/silent-abort-print-mode.test.ts | 108 ++++++++++++ 7 files changed, 337 insertions(+), 7 deletions(-) create mode 100644 packages/coding-agent/test/silent-abort-overlay-render.test.ts create mode 100644 packages/coding-agent/test/silent-abort-print-mode.test.ts diff --git a/packages/coding-agent/src/modes/acp/acp-agent.ts b/packages/coding-agent/src/modes/acp/acp-agent.ts index 2a3556c9b..42ba86995 100644 --- a/packages/coding-agent/src/modes/acp/acp-agent.ts +++ b/packages/coding-agent/src/modes/acp/acp-agent.ts @@ -53,7 +53,7 @@ import type { MCPServerConfig } from "../../mcp/types"; import { loadAllExtensions } from "../../modes/components/extensions/state-manager"; import { theme } from "../../modes/theme/theme"; import type { AgentSession, AgentSessionEvent } from "../../session/agent-session"; -import { SKILL_PROMPT_MESSAGE_TYPE } from "../../session/messages"; +import { isSilentAbort, SKILL_PROMPT_MESSAGE_TYPE } from "../../session/messages"; import { SessionManager, type SessionInfo as StoredSessionInfo, @@ -1393,7 +1393,7 @@ export class AcpAgent implements Agent { } } } - if (notifications.length === 0 && message.errorMessage) { + if (notifications.length === 0 && message.errorMessage && !isSilentAbort(message.errorMessage)) { notifications.push({ sessionId, update: { diff --git a/packages/coding-agent/src/modes/components/session-observer-overlay.ts b/packages/coding-agent/src/modes/components/session-observer-overlay.ts index cf37498d2..dbf6da018 100644 --- a/packages/coding-agent/src/modes/components/session-observer-overlay.ts +++ b/packages/coding-agent/src/modes/components/session-observer-overlay.ts @@ -18,6 +18,7 @@ import type { ToolResultMessage } from "@oh-my-pi/pi-ai"; import { Container, Markdown, type MarkdownTheme, matchesKey } from "@oh-my-pi/pi-tui"; import { formatDuration, formatNumber, logger } from "@oh-my-pi/pi-utils"; import type { KeyId } from "../../config/keybindings"; +import { isSilentAbort } from "../../session/messages"; import type { SessionMessageEntry } from "../../session/session-manager"; import { parseSessionEntries } from "../../session/session-manager"; import { PREVIEW_LIMITS, replaceTabs, TRUNCATE_LENGTHS, truncateToWidth } from "../../tools/render-utils"; @@ -285,7 +286,7 @@ export class SessionObserverOverlayComponent extends Container { if (msg.role === "assistant") { // Handle error messages with empty content - if (msg.content.length === 0 && msg.errorMessage) { + if (msg.content.length === 0 && msg.errorMessage && !isSilentAbort(msg.errorMessage)) { const startLine = lines.length; const isSelected = entryIndex === this.#selectedEntryIndex; const cursor = isSelected ? theme.fg("accent", "▶") : " "; diff --git a/packages/coding-agent/src/modes/print-mode.ts b/packages/coding-agent/src/modes/print-mode.ts index ce693216b..a232c4f83 100644 --- a/packages/coding-agent/src/modes/print-mode.ts +++ b/packages/coding-agent/src/modes/print-mode.ts @@ -9,6 +9,7 @@ import type { AssistantMessage, ImageContent } from "@oh-my-pi/pi-ai"; import { sanitizeText } from "@oh-my-pi/pi-natives"; import { runExtensionCompact, runExtensionSetModel } from "../extensibility/extensions/compact-handler"; import type { AgentSession } from "../session/agent-session"; +import { isSilentAbort } from "../session/messages"; /** * Options for print mode. @@ -150,8 +151,11 @@ export async function runPrintMode(session: AgentSession, options: PrintModeOpti if (lastMessage?.role === "assistant") { const assistantMsg = lastMessage as AssistantMessage; - // Check for error/aborted - if (assistantMsg.stopReason === "error" || assistantMsg.stopReason === "aborted") { + // Check for error/aborted — skip silent-abort (plan-mode compaction transition) + if ( + (assistantMsg.stopReason === "error" || assistantMsg.stopReason === "aborted") && + !isSilentAbort(assistantMsg.errorMessage) + ) { const errorLine = sanitizeText(assistantMsg.errorMessage || `Request ${assistantMsg.stopReason}`); const flushed = process.stderr.write(`${errorLine}\n`); if (flushed) { diff --git a/packages/coding-agent/src/session/messages.ts b/packages/coding-agent/src/session/messages.ts index d06ff3c63..bbf1aa24a 100644 --- a/packages/coding-agent/src/session/messages.ts +++ b/packages/coding-agent/src/session/messages.ts @@ -47,8 +47,10 @@ export interface SkillPromptDetails { * branches identically. * * Consumers: `AgentSession.#handleAgentEvent` (stamper) writes this value; - * `EventController.#handleMessageEnd`, `AssistantMessageComponent`, and - * `ui-helpers.addMessageToChat` (renderers) read it via `isSilentAbort`. */ + * `EventController.#handleMessageEnd`, `AssistantMessageComponent`, + * `ui-helpers.addMessageToChat` (renderers), `SessionObserverOverlay + * #buildTranscriptLines`, `runPrintMode`, and `AcpAgent#replayAssistantMessage` + * (fallback error emission) read it via `isSilentAbort`. */ export const SILENT_ABORT_MARKER = "__omp.silent_abort__"; /** Type-guard for `SILENT_ABORT_MARKER`. Renderers MUST branch on this rather diff --git a/packages/coding-agent/test/acp-agent.test.ts b/packages/coding-agent/test/acp-agent.test.ts index ee169311a..1091bf6e6 100644 --- a/packages/coding-agent/test/acp-agent.test.ts +++ b/packages/coding-agent/test/acp-agent.test.ts @@ -16,6 +16,7 @@ import { _resetSettingsForTest, Settings } from "../src/config/settings"; import { AcpAgent } from "../src/modes/acp/acp-agent"; import type { PlanModeState } from "../src/plan-mode/state"; import type { AgentSession, AgentSessionEvent } from "../src/session/agent-session"; +import { SILENT_ABORT_MARKER } from "../src/session/messages"; import { SessionManager } from "../src/session/session-manager"; import { expectAcpStructure } from "./helpers/acp-schema"; @@ -557,6 +558,54 @@ describe("ACP agent", () => { await Bun.sleep(0); }); + it("does not replay silent-abort marker as agent_message_chunk to ACP clients", async () => { + const harness = await createHarness(); + const stored = new FakeAgentSession(harness.cwdA); + harness.sessions.push(stored); + stored.sessionManager.appendMessage({ role: "user", content: "start", timestamp: Date.now() }); + // Simulate a silent-abort assistant message: empty content, errorMessage = marker + stored.sessionManager.appendMessage({ + role: "assistant", + content: [], + api: "anthropic-messages", + provider: "anthropic", + model: TEST_MODELS[0].id, + usage: { + input: 0, + output: 0, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 0, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + stopReason: "aborted", + errorMessage: SILENT_ABORT_MARKER, + timestamp: Date.now(), + }); + await stored.sessionManager.ensureOnDisk(); + await stored.sessionManager.flush(); + + await harness.agent.loadSession({ + sessionId: stored.sessionId, + cwd: harness.cwdA, + mcpServers: [], + }); + const replayChunks = harness.updates.filter( + update => update.sessionId === stored.sessionId && update.update.sessionUpdate === "agent_message_chunk", + ); + // The silent-abort marker MUST NOT surface as a replayed message chunk + const markerChunks = replayChunks.filter( + update => + update.update.sessionUpdate === "agent_message_chunk" && + update.update.content.type === "text" && + update.update.content.text === SILENT_ABORT_MARKER, + ); + expect(markerChunks).toHaveLength(0); + + harness.abortController.abort(); + await Bun.sleep(0); + }); + it("advertises ACP-safe builtins and skill commands", async () => { const harness = await createHarness(); const created = await harness.agent.newSession({ cwd: harness.cwdA, mcpServers: [] }); diff --git a/packages/coding-agent/test/silent-abort-overlay-render.test.ts b/packages/coding-agent/test/silent-abort-overlay-render.test.ts new file mode 100644 index 000000000..ae2015028 --- /dev/null +++ b/packages/coding-agent/test/silent-abort-overlay-render.test.ts @@ -0,0 +1,166 @@ +/** + * Regression: observer overlay must not render SILENT_ABORT_MARKER verbatim. + * + * Codex review flagged that `session-observer-overlay.ts` renders `errorMessage` + * without filtering the silent-abort sentinel. This test exercises the full + * `#buildTranscriptLines` path through a real JSONL session file and mock registry. + */ +import { afterEach, beforeAll, beforeEach, describe, expect, it } from "bun:test"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { SessionObserverOverlayComponent } from "../src/modes/components/session-observer-overlay"; +import type { ObservableSession } from "../src/modes/session-observer-registry"; +import { initTheme } from "../src/modes/theme/theme"; +import { SILENT_ABORT_MARKER } from "../src/session/messages"; + +const SESSION_ID = "test-session-1"; + +function makeJsonlSessionFile(dirPath: string, entries: object[]): string { + const filePath = path.join(dirPath, "session.jsonl"); + const lines = entries.map(e => JSON.stringify(e)); + fs.writeFileSync(filePath, `${lines.join("\n")}\n`, "utf-8"); + return filePath; +} + +function makeSubagentRegistry(sessions: ObservableSession[]) { + return { + getSessions: () => sessions, + onChange: () => () => {}, + setMainSession: () => {}, + getActiveSubagentCount: () => sessions.filter(s => s.status === "active").length, + } as unknown as import("../src/modes/session-observer-registry").SessionObserverRegistry; +} + +describe("Observer overlay silent-abort regression", () => { + let tmpDir: string; + + beforeAll(() => { + initTheme(); + }); + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "omp-overlay-test-")); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it("does not render ✗ Error: for silent-abort assistant messages with empty content", () => { + const sessionFile = makeJsonlSessionFile(tmpDir, [ + { type: "session", version: 3, id: SESSION_ID, timestamp: new Date().toISOString() }, + { + type: "message", + id: "msg-user-1", + parentId: null, + timestamp: new Date().toISOString(), + message: { role: "user", content: "hello", timestamp: Date.now() }, + }, + { + type: "message", + id: "msg-assistant-1", + parentId: "msg-user-1", + timestamp: new Date().toISOString(), + message: { + role: "assistant", + content: [], + api: "anthropic-messages", + provider: "anthropic", + model: "claude-sonnet-4-5", + stopReason: "aborted", + errorMessage: SILENT_ABORT_MARKER, + usage: { + input: 0, + output: 0, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 0, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + timestamp: Date.now(), + }, + }, + ]); + + const registry = makeSubagentRegistry([ + { + id: SESSION_ID, + kind: "subagent", + label: "Test Subagent", + status: "active", + sessionFile, + lastUpdate: Date.now(), + }, + ]); + + const overlay = new SessionObserverOverlayComponent(registry, () => {}, ["Ctrl+S"]); + + // Render with a reasonable width — the overlay reads the session file + // and calls #buildTranscriptLines internally. + const rendered = overlay.render(120); + const renderedText = rendered.join("\n"); + + // The sentinel MUST NOT appear verbatim in any rendered line + expect(renderedText).not.toContain(SILENT_ABORT_MARKER); + // The error prefix MUST NOT appear for a silent-abort message + expect(renderedText).not.toContain("✗ Error:"); + }); + + it("renders normal error messages with ✗ Error: prefix", () => { + const sessionFile = makeJsonlSessionFile(tmpDir, [ + { type: "session", version: 3, id: SESSION_ID, timestamp: new Date().toISOString() }, + { + type: "message", + id: "msg-user-2", + parentId: null, + timestamp: new Date().toISOString(), + message: { role: "user", content: "hello", timestamp: Date.now() }, + }, + { + type: "message", + id: "msg-assistant-2", + parentId: "msg-user-2", + timestamp: new Date().toISOString(), + message: { + role: "assistant", + content: [], + api: "anthropic-messages", + provider: "anthropic", + model: "claude-sonnet-4-5", + stopReason: "error", + errorMessage: "Connection timed out", + usage: { + input: 0, + output: 0, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 0, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + timestamp: Date.now(), + }, + }, + ]); + + const registry = makeSubagentRegistry([ + { + id: SESSION_ID, + kind: "subagent", + label: "Test Subagent", + status: "failed", + sessionFile, + lastUpdate: Date.now(), + }, + ]); + + const overlay = new SessionObserverOverlayComponent(registry, () => {}, ["Ctrl+S"]); + + const rendered = overlay.render(120); + const renderedText = rendered.join("\n"); + + // A real error message SHOULD be rendered with the ✗ Error: prefix + expect(renderedText).toContain("✗ Error:"); + expect(renderedText).toContain("Connection timed out"); + }); +}); diff --git a/packages/coding-agent/test/silent-abort-print-mode.test.ts b/packages/coding-agent/test/silent-abort-print-mode.test.ts new file mode 100644 index 000000000..7d2749433 --- /dev/null +++ b/packages/coding-agent/test/silent-abort-print-mode.test.ts @@ -0,0 +1,108 @@ +/** + * Regression: print-mode must not write SILENT_ABORT_MARKER to stderr. + * + * Codex review flagged that `print-mode.ts` renders `errorMessage` verbatim + * when stopReason is "aborted", which would surface the sentinel to stderr + * (and exit with code 1). This test verifies the guard skips silent-abort. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; +import type { AssistantMessage } from "@oh-my-pi/pi-ai"; +import type { AgentSession } from "../src/session/agent-session"; +import { SILENT_ABORT_MARKER } from "../src/session/messages"; + +function makeAssistantMessage(overrides: Partial = {}): AssistantMessage { + return { + role: "assistant", + content: [{ type: "text", text: "draft" }], + api: "anthropic-messages", + provider: "anthropic", + model: "claude-sonnet-4-5", + stopReason: "stop", + usage: { + input: 0, + output: 0, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 0, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + timestamp: Date.now(), + ...overrides, + }; +} + +/** Minimal mock of AgentSession for print-mode text output path */ +function createMockSession(messages: AssistantMessage[]): AgentSession { + return { + state: { messages }, + sessionManager: { + getHeader: () => undefined, + }, + extensionRunner: undefined, + subscribe: () => () => {}, + prompt: async () => {}, + dispose: async () => {}, + } as unknown as AgentSession; +} + +describe("Print-mode silent-abort regression", () => { + let exitSpy: ReturnType; + let stderrOutput: string[]; + + beforeEach(() => { + stderrOutput = []; + vi.spyOn(process.stderr, "write").mockImplementation((chunk: unknown) => { + stderrOutput.push(String(chunk)); + return true; + }); + exitSpy = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); + vi.spyOn(process.stdout, "write").mockImplementation((...args: unknown[]) => { + // Invoke callback if present (runPrintMode flushes stdout before returning) + const last = args[args.length - 1]; + if (typeof last === "function") last(); + return true; + }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("does not write silent-abort marker to stderr or exit non-zero", async () => { + const { runPrintMode } = await import("../src/modes/print-mode"); + + const silentAbortMsg = makeAssistantMessage({ + stopReason: "aborted", + errorMessage: SILENT_ABORT_MARKER, + content: [], + }); + + const session = createMockSession([silentAbortMsg]); + await runPrintMode(session, { mode: "text" }); + + // The silent-abort marker MUST NOT appear in stderr + const stderrText = stderrOutput.join(""); + expect(stderrText).not.toContain(SILENT_ABORT_MARKER); + // process.exit MUST NOT have been called (clean termination) + expect(exitSpy).not.toHaveBeenCalled(); + }); + + it("writes real error messages to stderr and exits non-zero", async () => { + const { runPrintMode } = await import("../src/modes/print-mode"); + + const errorMsg = makeAssistantMessage({ + stopReason: "error", + errorMessage: "Rate limit exceeded", + content: [], + }); + + const session = createMockSession([errorMsg]); + await runPrintMode(session, { mode: "text" }); + + // A real error SHOULD be written to stderr + const stderrText = stderrOutput.join(""); + expect(stderrText).toContain("Rate limit exceeded"); + // process.exit(1) SHOULD have been called + expect(exitSpy).toHaveBeenCalledWith(1); + }); +});