diff --git a/packages/agent/CHANGELOG.md b/packages/agent/CHANGELOG.md index f26ff2f64..f90c9cddb 100644 --- a/packages/agent/CHANGELOG.md +++ b/packages/agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed compaction summarizer throws losing the provider's HTTP status. `generateSummary`, `generateHandoff`, `generateShortSummary`, and `generateTurnPrefixSummary` now route their `stopReason === "error"` throws through a `createSummarizationError` helper that copies `AssistantMessage.errorStatus` onto the thrown `Error` as `.status`, letting downstream consumers (e.g. `AgentSession.#isCompactionAuthFailure` in `@oh-my-pi/pi-coding-agent`) branch on real provider 401/403s without regex-scraping the message body. + ## [15.5.0] - 2026-05-26 ### Added diff --git a/packages/agent/src/compaction/compaction.ts b/packages/agent/src/compaction/compaction.ts index 380525d66..4e6d7271a 100644 --- a/packages/agent/src/compaction/compaction.ts +++ b/packages/agent/src/compaction/compaction.ts @@ -550,6 +550,23 @@ function resolveCompactionEffort(model: Model, level: ThinkingLevel | undefined) return clampThinkingLevelForModel(model, requested); } +/** + * Build the error thrown when an LLM summarization call ends with + * `stopReason === "error"`. Carries the provider's HTTP `errorStatus` + * onto a top-level `.status` field so callers (notably + * `AgentSession.#isCompactionAuthFailure`) can branch on 401/403 without + * regex-scraping `error.message`. The `auth_unavailable` synthetic + * (pi-native gateway) does not populate `errorStatus`, hence the legacy + * message-based check is still required upstream — see issue #986. + */ +function createSummarizationError(prefix: string, response: AssistantMessage): Error { + const error: Error & { status?: number } = new Error(`${prefix}: ${response.errorMessage || "Unknown error"}`); + if (response.errorStatus !== undefined) { + error.status = response.errorStatus; + } + return error; +} + /** * Generate a summary of the conversation using the LLM. * If previousSummary is provided, uses the update prompt to merge. @@ -649,7 +666,7 @@ export async function generateSummary( ); if (response.stopReason === "error") { - throw new Error(`Summarization failed: ${response.errorMessage || "Unknown error"}`); + throw createSummarizationError("Summarization failed", response); } const textContent = response.content @@ -731,7 +748,7 @@ export async function generateHandoff( ); if (response.stopReason === "error") { - throw new Error(`Handoff generation failed: ${response.errorMessage || "Unknown error"}`); + throw createSummarizationError("Handoff generation failed", response); } return response.content @@ -790,7 +807,7 @@ async function generateShortSummary( ); if (response.stopReason === "error") { - throw new Error(`Short summary failed: ${response.errorMessage || "Unknown error"}`); + throw createSummarizationError("Short summary failed", response); } return response.content @@ -1128,7 +1145,7 @@ async function generateTurnPrefixSummary( ); if (response.stopReason === "error") { - throw new Error(`Turn prefix summarization failed: ${response.errorMessage || "Unknown error"}`); + throw createSummarizationError("Turn prefix summarization failed", response); } return response.content diff --git a/packages/agent/test/compaction-error-status.test.ts b/packages/agent/test/compaction-error-status.test.ts new file mode 100644 index 000000000..b9144b67b --- /dev/null +++ b/packages/agent/test/compaction-error-status.test.ts @@ -0,0 +1,161 @@ +import { afterEach, describe, expect, test, vi } from "bun:test"; +import type { AgentMessage } from "@oh-my-pi/pi-agent-core"; +import { + type CompactionPreparation, + compact, + createFileOps, + DEFAULT_COMPACTION_SETTINGS, + generateHandoff, +} from "@oh-my-pi/pi-agent-core/compaction"; +import type { AssistantMessage, Model } from "@oh-my-pi/pi-ai"; +import * as ai from "@oh-my-pi/pi-ai"; +import { getBundledModel } from "@oh-my-pi/pi-ai/models"; + +// Pins the fix for the "raw 401 surfaced as Compaction failed:" bug. +// +// When a real provider returns HTTP 401/403 from a summarization call, +// `instrumentedCompleteSimple` resolves with `stopReason: "error"` and +// `errorStatus` populated by the provider's catch block (e.g. +// `packages/ai/src/providers/anthropic.ts`). The compaction layer must +// surface that status on the thrown Error so +// `AgentSession.#isCompactionAuthFailure` can route to the authenticated +// fallback model instead of dumping the raw ` ` string +// into the UI. +// +// `generateHandoff` is the cheapest vehicle (one LLM call). The same +// `createSummarizationError` helper backs all four summarizer throw +// sites in `packages/agent/src/compaction/compaction.ts`, so verifying +// one site is sufficient to lock the contract. + +function makeAssistantStop(content: AssistantMessage["content"]): AssistantMessage { + return { + role: "assistant", + content, + timestamp: Date.now(), + provider: "mock", + model: "mock", + api: "mock", + usage: { + input: 0, + output: 0, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 0, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + stopReason: "stop", + }; +} + +function makeAssistantError(errorStatus: number | undefined, errorMessage: string): AssistantMessage { + return { + role: "assistant", + content: [], + timestamp: Date.now(), + provider: "mock", + model: "mock", + api: "mock", + usage: { + input: 0, + output: 0, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 0, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + stopReason: "error", + errorMessage, + ...(errorStatus !== undefined ? { errorStatus } : {}), + }; +} + +function getAnthropicModel(): Model { + const model = getBundledModel("anthropic", "claude-sonnet-4-5"); + if (!model) throw new Error("Expected built-in anthropic/claude-sonnet-4-5 to exist"); + return model; +} + +const handoffMessages: AgentMessage[] = [ + { role: "user", content: "begin", timestamp: 1 }, + makeAssistantStop([{ type: "text", text: "ok" }]), +]; + +function makeUserMessage(text: string, timestamp = Date.now()): AgentMessage { + return { role: "user", content: text, timestamp }; +} + +function makePreparation(overrides: Partial = {}): CompactionPreparation { + return { + firstKeptEntryId: "kept-1", + messagesToSummarize: [ + makeUserMessage("history msg"), + makeAssistantStop([{ type: "text", text: "history reply" }]), + ], + turnPrefixMessages: [makeUserMessage("turn prefix msg")], + recentMessages: [makeUserMessage("recent msg")], + isSplitTurn: true, + tokensBefore: 12_345, + fileOps: createFileOps(), + settings: { ...DEFAULT_COMPACTION_SETTINGS, remoteEnabled: false }, + ...overrides, + }; +} + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("compaction error-status propagation", () => { + test("generateHandoff throws Error with .status === 401 when provider returns 401", async () => { + vi.spyOn(ai, "completeSimple").mockResolvedValue( + makeAssistantError( + 401, + '401 {"type":"error","error":{"type":"authentication_error","message":"Invalid authentication credentials"}}', + ), + ); + + const error = await generateHandoff(handoffMessages, getAnthropicModel(), "stale-key", { + systemPrompt: ["sp"], + tools: [], + }).catch(err => err); + + expect(error).toBeInstanceOf(Error); + const e = error as Error & { status?: number }; + expect(e.status).toBe(401); + // Message still carries the upstream body so logs and telemetry + // retain the raw provider envelope. + expect(e.message).toContain("Handoff generation failed"); + expect(e.message).toContain("authentication_error"); + }); + + test("compact() fan-out throws Error with .status === 403 when provider returns 403", async () => { + vi.spyOn(ai, "completeSimple").mockResolvedValue( + makeAssistantError(403, '403 {"error":{"type":"forbidden","message":"Access denied"}}'), + ); + + const error = await compact(makePreparation(), getAnthropicModel(), "stale-key").catch(err => err); + + expect(error).toBeInstanceOf(Error); + const e = error as Error & { status?: number }; + expect(e.status).toBe(403); + expect(e.message).toMatch(/Summarization failed|Short summary failed|Turn prefix summarization failed/); + }); + + test("missing errorStatus does not attach .status (preserves auth_unavailable regex path)", async () => { + vi.spyOn(ai, "completeSimple").mockResolvedValue( + makeAssistantError(undefined, "503 auth_unavailable: no auth available (providers=codex, model=gpt-5.4-mini)"), + ); + + const error = await generateHandoff(handoffMessages, getAnthropicModel(), "stale-key", { + systemPrompt: ["sp"], + tools: [], + }).catch(err => err); + + expect(error).toBeInstanceOf(Error); + const e = error as Error & { status?: number }; + // Synthetic pi-native gateway errors don't carry HTTP status; the + // upstream regex on `auth_unavailable` is the load-bearing detector. + expect(e.status).toBeUndefined(); + expect(e.message).toContain("auth_unavailable"); + }); +}); diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 6f54f39fe..b1001cf59 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed compaction surfacing raw HTTP 401/403 envelopes (e.g. `Compaction failed: 401 {"type":"error","error":{"type":"authentication_error",…}}`) instead of routing to an authenticated fallback model. The compaction layer now attaches the provider-reported HTTP status onto the thrown error, and `AgentSession`'s auth-failure detector branches on `error.status === 401 || 403` in addition to the existing `auth_unavailable` regex. When a fallback model role (e.g. `modelRoles.smol`) is configured, compaction retries it transparently; otherwise the user sees the actionable "Compaction requires usable credentials for …" hint instead of the raw provider envelope. + ## [15.5.8] - 2026-05-28 ### Breaking Changes diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 2edeb541c..3c8a43a93 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -6379,6 +6379,14 @@ export class AgentSession { } #isCompactionAuthFailure(error: unknown): boolean { if (!(error instanceof Error)) return false; + // Real provider 401/403 — surfaced as `.status` by the compaction layer + // (see `createSummarizationError` in packages/agent/src/compaction/compaction.ts). + // Without this branch, an expired/revoked Anthropic key would bypass the + // authenticated-fallback path and dump the raw HTTP body into the UI. + const status = (error as Error & { status?: number }).status; + if (status === 401 || status === 403) return true; + // pi-native gateway synthetic for "no credential configured" (issue #986). + // Carries no HTTP status, so the legacy message regex stays. return /auth_unavailable|no auth available/i.test(error.message); } diff --git a/packages/coding-agent/test/issue-986-compaction-auth-fallback.test.ts b/packages/coding-agent/test/issue-986-compaction-auth-fallback.test.ts index 2722e5aeb..dc0b30556 100644 --- a/packages/coding-agent/test/issue-986-compaction-auth-fallback.test.ts +++ b/packages/coding-agent/test/issue-986-compaction-auth-fallback.test.ts @@ -138,4 +138,77 @@ describe("issue #986 compaction auth fallback", () => { ); expect((error as Error).message).not.toMatch(/auth_unavailable/i); }); + + it("falls back when the current provider returns a real HTTP 401 from the compaction call", async () => { + // Companion to the auth_unavailable test above: that case exercises the + // pi-native gateway synthetic ("no credential configured"), this one + // exercises a configured-but-rejected credential (rotated/revoked + // Anthropic key, expired OAuth token, wrong workspace). Before the + // status-aware detector landed, only the synthetic was caught — a real + // 401 from the provider bypassed the fallback and dumped the raw HTTP + // body into the UI as "Compaction failed: 401 {...}". + const { currentModel, fallbackModel } = await createSession({ fallbackModelRole: "smol" }); + const compactSpy = vi.spyOn(compactionModule, "compact").mockImplementation(async (preparation, model) => { + if (model.provider === currentModel.provider && model.id === currentModel.id) { + throw Object.assign( + new Error( + 'Turn prefix summarization failed: 401 {"type":"error","error":{"type":"authentication_error","message":"Invalid authentication credentials"}}', + ), + { status: 401 }, + ); + } + if (model.provider !== fallbackModel.provider || model.id !== fallbackModel.id) { + throw new Error(`Unexpected compaction model ${model.provider}/${model.id}`); + } + return { + summary: "fallback summary", + shortSummary: "fallback short summary", + firstKeptEntryId: preparation.firstKeptEntryId, + tokensBefore: 42, + details: { provider: model.provider }, + }; + }); + vi.spyOn(modelRegistry, "getApiKey").mockImplementation(async model => { + if (model.provider === currentModel.provider && model.id === currentModel.id) return "stale-codex-token"; + if (model.provider === fallbackModel.provider && model.id === fallbackModel.id) return "anthropic-token"; + return undefined; + }); + + const result = await session.compact(); + + expect(result.summary).toBe("fallback summary"); + expect(compactSpy).toHaveBeenCalledTimes(2); + expect(compactSpy.mock.calls.map(([, model]) => `${model.provider}/${model.id}`)).toEqual([ + `${currentModel.provider}/${currentModel.id}`, + `${fallbackModel.provider}/${fallbackModel.id}`, + ]); + }); + + it("fails fast with the configured-credentials hint when a 401 has no authenticated fallback", async () => { + const { currentModel } = await createSession({ configureFallbackAuth: false }); + vi.spyOn(compactionModule, "compact").mockImplementation(async (_preparation, model) => { + if (model.provider === currentModel.provider && model.id === currentModel.id) { + throw Object.assign( + new Error( + 'Summarization failed: 401 {"type":"error","error":{"type":"authentication_error","message":"Invalid authentication credentials"}}', + ), + { status: 401 }, + ); + } + throw new Error(`Unexpected compaction model ${model.provider}/${model.id}`); + }); + vi.spyOn(modelRegistry, "getApiKey").mockImplementation(async model => { + if (model.provider === currentModel.provider && model.id === currentModel.id) return "stale-codex-token"; + return undefined; + }); + + const error = await session.compact().catch(err => err); + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toContain( + `Compaction requires usable credentials for ${currentModel.provider}/${currentModel.id}`, + ); + // The raw provider envelope must not leak into the actionable error. + expect((error as Error).message).not.toContain("authentication_error"); + expect((error as Error).message).not.toMatch(/\b401\b/); + }); });