fix: gate SILENT_ABORT_MARKER at three unguarded render sites
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
This commit is contained in:
@@ -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: {
|
||||
|
||||
@@ -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", "▶") : " ";
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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: [] });
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
@@ -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> = {}): 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<typeof vi.spyOn>;
|
||||
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);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user