fix(agent): patched compaction 401/403 fallback to copy error status
- Centralized compaction stop-reason error throws via createSummarizationError(). - Set compaction thrown errors to copy response.errorStatus into Error.status. - Expanded compaction auth detection to treat HTTP 401/403 as auth failures with regex fallback preserved. - Added regression tests for 401/403 status propagation and compaction fallback auth behavior. - Documented both package fixes in Unreleased Fixed changelog entries.
This commit is contained in:
can1357
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 `<status> <body>` 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> = {}): 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");
|
||||
});
|
||||
});
|
||||
@@ -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
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
|
||||
@@ -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/);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user