Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/CodexAcpServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,15 +131,26 @@ export interface SessionState {
sessionTitle: string | null;
sessionTitleSource: "unset" | "fallback" | "explicit" | "unknown";
sessionFailure?: SessionFailure;
/**
* Warning-severity advisories, tracked separately from {@link SessionState.sessionFailure} so a
* non-fatal notice never overwrites the revision bookkeeping of an in-flight terminal failure.
*/
sessionNotice?: SessionFailure;
}

export type SessionFailureCategory =
| "transport_lost" | "auth_required" | "rate_limited" | "quota_exhausted" | "overloaded"
| "context_exhausted" | "budget_exhausted" | "policy_denied" | "bad_request"
| "provider_error" | "internal_error";
| "provider_error" | "internal_error" | "advisory";

export type SessionFailureAction = "retry" | "reconnect" | "login" | "new_turn" | "new_session";

/**
* How loudly the client should render the record. Absent on the wire means `error`, so an AIR build
* that predates warning support keeps treating every record it receives as a failure.
*/
export type SessionFailureSeverity = "error" | "warning";

export interface SessionFailure {
id: string;
revision: number;
Expand All @@ -150,6 +161,7 @@ export interface SessionFailure {
retryable: boolean;
actions: SessionFailureAction[];
turnId?: string;
severity?: SessionFailureSeverity;
}

const CODEX_PROCESS_EXITED_ERROR_CODE = 1001;
Expand Down
70 changes: 66 additions & 4 deletions src/CodexEventHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type {
ServerNotification
} from "./app-server";
import type {
SessionFailure,
SessionFailureAction,
SessionFailureCategory,
SessionState,
Expand All @@ -16,6 +17,7 @@ import type {
CodexErrorInfo,
CommandExecutionOutputDeltaNotification,
ConfigWarningNotification,
DeprecationNoticeNotification,
ErrorNotification,
ItemGuardianApprovalReviewCompletedNotification,
ItemGuardianApprovalReviewStartedNotification,
Expand Down Expand Up @@ -103,8 +105,23 @@ const SESSION_FAILURE_PRESENTATION: Record<SessionFailureCategory, {
bad_request: {message: "Codex could not process this request.", retryable: false, actions: ["new_turn"]},
provider_error: {message: "The model provider reported an error.", retryable: true, actions: ["retry"]},
internal_error: {message: "Codex encountered an internal error.", retryable: true, actions: ["retry"]},
// Warning-severity advisories carry the app-server's own wording, so this message is only the
// fallback for a notice recorded without one.
advisory: {message: "Codex reported a warning.", retryable: false, actions: ["new_session"]},
};

/** Both `configWarning` and `deprecationNotice` carry a summary plus optional elaboration. */
function joinSummaryAndDetails(summary: string, details: string | null): string {
return details ? `${summary}\n\n${details}` : summary;
}

/**
* Records sharing an id form one logical banner whose revisions must increase; a new id restarts at 1.
*/
function nextSessionFailureRevision(previous: SessionFailure | undefined, id: string): number {
return previous?.id === id ? previous.revision + 1 : 1;
}

type StringCodexErrorInfo = Extract<CodexErrorInfo, string>;
type StructuredCodexErrorInfo = Exclude<CodexErrorInfo, string>;
type KeysOfUnion<T> = T extends unknown ? keyof T : never;
Expand Down Expand Up @@ -403,6 +420,8 @@ export class CodexEventHandler {
return this.createWarningEvent(notification.params);
case "guardianWarning":
return null;
case "deprecationNotice":
return this.createDeprecationNoticeEvent(notification.params);
case "item/autoApprovalReview/started":
return this.handleGuardianApprovalReviewStarted(notification.params);
case "item/autoApprovalReview/completed":
Expand Down Expand Up @@ -456,7 +475,6 @@ export class CodexEventHandler {
case "windowsSandbox/setupCompleted":
case "account/login/completed":
case "skills/changed":
case "deprecationNotice":
case "mcpServer/oauthLogin/completed":
case "externalAgentConfig/import/completed":
case "rawResponseItem/completed":
Expand Down Expand Up @@ -487,11 +505,29 @@ export class CodexEventHandler {
}

private async createConfigWarningEvent(event: ConfigWarningNotification): Promise<UpdateSessionEvent> {
const detailsText = event.details ? `\n\n${event.details}` : "";
return createAgentTextMessageChunk(`Config warning: ${event.summary}${detailsText}\n\n`);
const text = joinSummaryAndDetails(event.summary, event.details);
if (this.supportsTypedSessionFailures) {
return this.createSessionFailureUpdate(this.recordSessionNotice(text));
}
return createAgentTextMessageChunk(`Config warning: ${text}\n\n`);
}

/**
* Unlike `warning` and `configWarning`, this notification was dropped outright, so there is no
* legacy rendering to preserve. It is surfaced only to clients that negotiated typed records;
* every other client keeps seeing exactly what it sees today, which is nothing.
*/
private createDeprecationNoticeEvent(event: DeprecationNoticeNotification): UpdateSessionEvent | null {
if (!this.supportsTypedSessionFailures) return null;
return this.createSessionFailureUpdate(
this.recordSessionNotice(joinSummaryAndDetails(event.summary, event.details)),
);
}

private createWarningEvent(event: WarningNotification): UpdateSessionEvent {
if (this.supportsTypedSessionFailures) {
return this.createSessionFailureUpdate(this.recordSessionNotice(event.message));
}
return createAgentTextMessageChunk(`Warning: ${event.message}\n\n`);
}

Expand Down Expand Up @@ -975,7 +1011,7 @@ export class CodexEventHandler {
: `${turnId}:error`;
const failure: NonNullable<SessionState["sessionFailure"]> = {
id,
revision: previous?.id === id ? previous.revision + 1 : 1,
revision: nextSessionFailureRevision(previous, id),
phase: "active" as const,
category,
source: "codex",
Expand All @@ -988,6 +1024,32 @@ export class CodexEventHandler {
return failure;
}

/**
* Records a warning-severity advisory in its own slot and under its own id namespace, so it
* shares the wire contract with terminal failures without competing for their revision counter.
*
* The advisory replaces whichever advisory preceded it: the app-server sends these as standalone
* hints, so only the newest one is worth a banner.
*/
private recordSessionNotice(safeMessage: string): NonNullable<SessionState["sessionNotice"]> {
const presentation = SESSION_FAILURE_PRESENTATION.advisory;
const previous = this.sessionState.sessionNotice;
const id = `${this.sessionState.sessionId}:notice:${this.sessionFailureEpoch}`;
const notice: NonNullable<SessionState["sessionNotice"]> = {
id,
revision: nextSessionFailureRevision(previous, id),
phase: "active" as const,
category: "advisory",
source: "codex",
safeMessage,
retryable: presentation.retryable,
actions: presentation.actions,
severity: "warning",
};
this.sessionState.sessionNotice = notice;
return notice;
}

private createSessionFailureMeta(
failure: NonNullable<SessionState["sessionFailure"]>,
): Record<string, unknown> {
Expand Down
178 changes: 178 additions & 0 deletions src/__tests__/CodexACPAgent/typed-session-failure-wire.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,186 @@ describe("typed session failures over ACP transport", () => {
expect(JSON.stringify(fixture.updates)).not.toContain("raw idle provider detail");
expect(JSON.stringify(fixture.updates)).not.toContain("secret idle detail");
});

it("delivers an app-server warning as a typed advisory instead of assistant text", async () => {
const fixture = await createIdleFixture("wire-warning");

fixture.sendServerNotification({
method: "warning",
params: {
threadId: fixture.sessionId,
message: "Heads up: Long threads and multiple compactions can cause the model to be less accurate.",
},
});
await fixture.codexClient.waitForSessionNotifications(fixture.sessionId);
await vi.waitFor(() => expect(fixture.updates).toHaveLength(1));

expect(fixture.updates[0]).toMatchObject({
update: {
sessionUpdate: "session_info_update",
_meta: {
jetbrains: {
air: {
sessionFailure: {
id: expect.stringMatching(/^wire-warning:notice:[0-9a-f-]+$/),
category: "advisory",
severity: "warning",
phase: "active",
revision: 1,
retryable: false,
actions: ["new_session"],
safeMessage:
"Heads up: Long threads and multiple compactions can cause the model to be less accurate.",
},
},
},
},
},
});
// The whole point of the change: it must not arrive as an agent message chunk.
expect(JSON.stringify(fixture.updates)).not.toContain("agent_message_chunk");
expect(JSON.stringify(fixture.updates)).not.toContain("Warning: ");
});

it("folds a config warning's details into the advisory message", async () => {
const fixture = await createIdleFixture("wire-config-warning");

fixture.sendServerNotification({
method: "configWarning",
params: {summary: "Unknown key `foo`", details: "in ~/.codex/config.toml"},
});
await fixture.codexClient.waitForSessionNotifications(fixture.sessionId);
await vi.waitFor(() => expect(fixture.updates).toHaveLength(1));

expect(fixture.updates[0]).toMatchObject({
update: {
_meta: {
jetbrains: {
air: {
sessionFailure: {
category: "advisory",
severity: "warning",
safeMessage: "Unknown key `foo`\n\nin ~/.codex/config.toml",
},
},
},
},
},
});
});

it("surfaces a deprecation notice that used to be dropped outright", async () => {
const fixture = await createIdleFixture("wire-deprecation");

fixture.sendServerNotification({
method: "deprecationNotice",
params: {summary: "`--legacy-flag` is deprecated", details: "Use `--flag` instead."},
});
await fixture.codexClient.waitForSessionNotifications(fixture.sessionId);
await vi.waitFor(() => expect(fixture.updates).toHaveLength(1));

expect(fixture.updates[0]).toMatchObject({
update: {
sessionUpdate: "session_info_update",
_meta: {
jetbrains: {
air: {
sessionFailure: {
category: "advisory",
severity: "warning",
safeMessage: "`--legacy-flag` is deprecated\n\nUse `--flag` instead.",
},
},
},
},
},
});
});

it("still drops a deprecation notice when the capability is absent", async () => {
const fixture = await createIdleFixture("wire-legacy-deprecation", {});

fixture.sendServerNotification({
method: "deprecationNotice",
params: {summary: "`--legacy-flag` is deprecated", details: null},
});
await fixture.codexClient.waitForSessionNotifications(fixture.sessionId);

// This notification produced nothing before typed records existed; a client that did not
// negotiate them must not suddenly start seeing it.
expect(fixture.updates).toEqual([]);
});

it("keeps warnings as assistant text when the capability is absent", async () => {
const fixture = await createIdleFixture("wire-legacy-warning", {});

fixture.sendServerNotification({
method: "warning",
params: {threadId: fixture.sessionId, message: "legacy advisory"},
});
await fixture.codexClient.waitForSessionNotifications(fixture.sessionId);
await vi.waitFor(() => expect(fixture.updates).toHaveLength(1));

expect(fixture.updates[0]!.update).toMatchObject({
sessionUpdate: "agent_message_chunk",
content: {type: "text", text: "Warning: legacy advisory\n\n"},
});
});

it("keeps an advisory in its own id namespace so it never bumps an active failure's revision", async () => {
const fixture = await createIdleFixture("wire-mixed");

fixture.sendServerNotification({
method: "error",
params: {
threadId: fixture.sessionId,
turnId: "turn-id",
willRetry: false,
error: {message: "provider blew up", codexErrorInfo: "serverOverloaded", additionalDetails: null},
},
});
fixture.sendServerNotification({
method: "warning",
params: {threadId: fixture.sessionId, message: "unrelated advisory"},
});
await fixture.codexClient.waitForSessionNotifications(fixture.sessionId);
await vi.waitFor(() => expect(fixture.updates).toHaveLength(2));

const records = fixture.updates.map(update => (update.update._meta as {
jetbrains: {air: {sessionFailure: {id: string; revision: number; severity?: string}}};
}).jetbrains.air.sessionFailure);
// Distinct ids, each starting its own revision sequence at 1.
expect(records[0]).toMatchObject({revision: 1, category: "overloaded"});
expect(records[1]).toMatchObject({revision: 1, category: "advisory", severity: "warning"});
expect(records[0]!.id).not.toEqual(records[1]!.id);
expect(records[0]).not.toHaveProperty("severity");
});
});

/** A fixture whose session already completed a turn, so notifications route to a live event handler. */
async function createIdleFixture(
sessionId: string,
clientCapabilities: acp.ClientCapabilities = typedFailureCapabilities,
) {
const fixture = createWireFixture();
await fixture.initialize(clientCapabilities);
const sessionState = createTestSessionState({sessionId, account: {type: "apiKey"}});
vi.spyOn(fixture.server, "getSessionState").mockReturnValue(sessionState);
vi.spyOn(fixture.appServer, "turnStart").mockResolvedValue({turn: createTurn("inProgress")});
vi.spyOn(fixture.appServer, "awaitTurnCompleted").mockResolvedValue({
threadId: sessionId,
turn: createTurn("completed"),
});

await fixture.client.prompt({
sessionId,
prompt: [{type: "text", text: "settle the session"}],
});
fixture.updates.splice(0);

return {...fixture, sessionId};
}

function createWireFixture(options: {exitCode?: number | null; stderr?: string} = {}) {
const mockConnections = createMockConnections();
const appServer = new CodexAppServerClient(mockConnections.mockCodexConnection);
Expand Down