Skip to content

Commit b220ab5

Browse files
committed
fix(coding-agent,ai): scope auth-stale to credential failures; clear lockouts only on successful explicit selection
Fresh-eyes review fixes: - New "permission" failure kind: 403s and permission/forbidden/access-denied error types (Anthropic permission_error, SDK PermissionDeniedError, AWS AccessDeniedException) are entitlement or policy denials, not bad credentials. They are permanent (no retry) but never mark auth stale, so a model/org/region-scoped 403 cannot lock out the whole provider. - An auth verdict now needs structured evidence (401 status or an explicit authentication error type); free-form message text alone no longer launders into a stale-marking "structured" auth diagnostic. - AgentSession.setModel is the single owner of the stale-auth clear and commits it only when staleness is the sole blocker of an explicit selection; the in-process and daemon set_model lookups consult the full catalog for stale-auth providers instead of mutating stale state before validation, so a mistyped model id or failed refresh no longer unlocks a provider that was proven bad.
1 parent 066fc18 commit b220ab5

12 files changed

Lines changed: 130 additions & 33 deletions
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
11
- Changed failure classification to stop treating bare 403 responses as authentication failures; an explicit authentication/permission error type or a 401 status is required.
2+
- Added a `permission` failure kind: 403/permission-type errors classify as entitlement denials (permanent, no retry) instead of authentication failures, and auth verdicts require structured evidence (401 or an explicit authentication error type).

‎packages/ai/src/utils/stream-failure.ts‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ export type StreamFailureKind =
1515
| "rate_limit"
1616
| "server_error"
1717
| "auth"
18+
| "permission"
1819
| "invalid_request"
1920
| "malformed_response"
2021
| "unknown";
@@ -48,6 +49,7 @@ const KIND_MESSAGES: Record<StreamFailureKind, string> = {
4849
rate_limit: "Provider rate limit exceeded",
4950
server_error: "Provider server error",
5051
auth: "Provider authentication failed",
52+
permission: "Provider denied access to the requested resource",
5153
invalid_request: "Provider rejected the request",
5254
malformed_response: "Provider returned a malformed response",
5355
unknown: "Provider stream failed",
@@ -77,10 +79,11 @@ export function classifyStreamFailure(providerErrorType?: string, status?: numbe
7779
if (/rate_limit|usage_limit|usage_not_included|throttl/.test(type) || status === 429) {
7880
return "rate_limit";
7981
}
80-
// 403 alone is NOT auth: providers return it for region blocks, org
81-
// policy, and model-access denials. Require an explicit auth/permission
82-
// error type unless the status is 401.
83-
if (/authentication|permission|unauthorized/.test(type) || status === 401) return "auth";
82+
// Only bad credentials (401 / explicit authentication types) are auth.
83+
// Permission/403 shapes are entitlement or policy denials (model access,
84+
// org policy, region) and must not trigger a stale-auth lockout.
85+
if (/authentication|unauthorized/.test(type) || status === 401) return "auth";
86+
if (/permission|forbidden|access.?denied/.test(type) || status === 403) return "permission";
8487
if (type.includes("invalid_request") || type.includes("not_found_error") || status === 400 || status === 404) {
8588
return "invalid_request";
8689
}
@@ -166,9 +169,16 @@ function extractStreamFailureParts(error: unknown): { info: StreamFailureInfo; d
166169
const retryAfterMs =
167170
typeof err.retryAfterMs === "number" && err.retryAfterMs >= 0 ? err.retryAfterMs : parseRetryAfterMs(headers);
168171

172+
let kind = classifyStreamFailure(providerErrorType ?? error.message, status);
173+
// An auth verdict drives the provider-wide stale-auth lockout; free-form
174+
// message text without a status is too weak to justify it.
175+
if (kind === "auth" && providerErrorType === undefined && status === undefined) {
176+
kind = "unknown";
177+
}
178+
169179
return {
170180
info: {
171-
kind: classifyStreamFailure(providerErrorType ?? error.message, status),
181+
kind,
172182
providerErrorType,
173183
status,
174184
requestId,

‎packages/ai/test/stream-failure.test.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,9 @@ describe("classifyStreamFailure", () => {
4949
["guardrail_intervened", undefined, "safety"],
5050
["authentication_error", undefined, "auth"],
5151
[undefined, 401, "auth"],
52-
["permission_error", 403, "auth"],
53-
[undefined, 403, "unknown"],
52+
["permission_error", 403, "permission"],
53+
["PermissionDeniedError", 403, "permission"],
54+
[undefined, 403, "permission"],
5455
["invalid_request_error", undefined, "invalid_request"],
5556
["api_error", undefined, "server_error"],
5657
[undefined, 503, "server_error"],
@@ -139,6 +140,14 @@ describe("extractStreamFailureInfo", () => {
139140
expect(extractStreamFailureInfo(new Error("provider overloaded, retry later")).kind).toBe("overloaded");
140141
expect(extractStreamFailureInfo("not an error").kind).toBe("unknown");
141142
});
143+
144+
test("never classifies auth from message text alone", () => {
145+
// No structured type and no status: an auth verdict would lock the whole
146+
// provider, so free-form text must not produce one.
147+
expect(extractStreamFailureInfo(new Error("Unauthorized: authentication failed")).kind).toBe("unknown");
148+
const with401 = Object.assign(new Error("Unauthorized"), { status: 401 });
149+
expect(extractStreamFailureInfo(with401).kind).toBe("auth");
150+
});
142151
});
143152

144153
describe("formatStreamFailureMessage", () => {
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,3 @@
11
- Removed the 401/403 message-text sniffing that could mark a whole provider auth-stale from non-auth errors (e.g. region-block 403s); auth-stale now requires a structured provider auth failure.
22
- Changed explicit model selection to clear a provider's stale-auth lockout so the request runs again instead of failing with "Model not found"; a structured auth failure re-marks it.
3+
- Changed the stale-auth clear to commit only when an explicit model selection is blocked solely by staleness; failed lookups and validations no longer unlock a provider.

‎packages/coding-agent/src/core/agent-session.ts‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7126,11 +7126,17 @@ export class AgentSession {
71267126
}
71277127

71287128
async setModel(model: Model<any>, options: ModelSelectOptions = {}): Promise<void> {
7129-
// Explicit selection overrides a stale-auth lockout; a structured auth
7130-
// failure on the next request re-marks the provider.
7131-
this._modelRegistry.clearProviderAuthStale(model.provider);
71327129
if (!this._modelRegistry.hasConfiguredAuth(model)) {
7133-
throw new Error(`No API key for ${model.provider}/${model.id}`);
7130+
// Explicit selection is the recovery path from a stale-auth lockout:
7131+
// clear (single owner of the clear) only when staleness is the sole
7132+
// blocker; a structured auth failure on the next request re-marks it.
7133+
if (this._modelRegistry.getProviderAuthStatus(model.provider).source !== "stale") {
7134+
throw new Error(`No API key for ${model.provider}/${model.id}`);
7135+
}
7136+
this._modelRegistry.clearProviderAuthStale(model.provider);
7137+
if (!this._modelRegistry.hasConfiguredAuth(model)) {
7138+
throw new Error(`No API key for ${model.provider}/${model.id}`);
7139+
}
71347140
}
71357141
if (!(await this._modelRegistry.canUseModel(model))) {
71367142
throw new Error(`Model "${model.provider}/${model.id}" is not available for the current Prime team.`);

‎packages/coding-agent/src/core/provider-retry.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ export function providerStreamFailureRetryAfterMs(message: AssistantMessage): nu
5959
* retry so a transient auth hiccup does not immediately mark auth stale.
6060
*/
6161
export function isPermanentProviderFailureKind(kind: string | undefined, retriesPerformed: number): boolean {
62-
if (kind === "invalid_request" || kind === "refusal") {
62+
if (kind === "invalid_request" || kind === "refusal" || kind === "permission") {
6363
return true;
6464
}
6565
return retriesPerformed > 0 && kind === "auth";

‎packages/coding-agent/src/modes/agent-connection/in-process-agent-connection.ts‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -448,13 +448,14 @@ export class InProcessAgentConnection implements AgentConnection {
448448
}
449449

450450
async setModel(provider: string, modelId: string): Promise<AgentConnectionModel> {
451-
// Clear before the lookup: a stale-auth provider's models are excluded
452-
// from the available list, which would misreport them as not found.
453-
this.session.modelRegistry.clearProviderAuthStale(provider);
454-
const availableModels = await this.session.modelRegistry.refreshAvailableModels();
455-
const model = availableModels.find((candidate) => {
456-
return candidate.provider === provider && candidate.id === modelId;
457-
});
451+
const registry = this.session.modelRegistry;
452+
const availableModels = await registry.refreshAvailableModels();
453+
const model =
454+
availableModels.find((candidate) => candidate.provider === provider && candidate.id === modelId) ??
455+
// A stale-auth provider's models are excluded from the available list;
456+
// explicit selection may still target them. The lookup never mutates
457+
// stale state: session.setModel owns the clear on a successful switch.
458+
(registry.getProviderAuthStatus(provider).source === "stale" ? registry.find(provider, modelId) : undefined);
458459
if (!model) {
459460
throw new Error(`Model not found: ${provider}/${modelId}`);
460461
}

‎packages/coding-agent/src/modes/daemon/daemon-mode.ts‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4791,13 +4791,17 @@ export class AgentDaemon {
47914791
case "set_model": {
47924792
const state = this.getSessionState(command.activeSessionId);
47934793
const session = state.runtime.session;
4794-
// Clear before the lookup: a stale-auth provider's models are excluded
4795-
// from the available list, which would misreport them as not found.
4796-
session.modelRegistry.clearProviderAuthStale(command.provider);
47974794
const availableModels = await session.modelRegistry.refreshAvailableModels();
4798-
const model = availableModels.find((candidate) => {
4799-
return candidate.provider === command.provider && candidate.id === command.modelId;
4800-
});
4795+
const model =
4796+
availableModels.find(
4797+
(candidate) => candidate.provider === command.provider && candidate.id === command.modelId,
4798+
) ??
4799+
// A stale-auth provider's models are excluded from the available
4800+
// list; explicit selection may still target them. The lookup never
4801+
// mutates stale state: session.setModel owns the clear on success.
4802+
(session.modelRegistry.getProviderAuthStatus(command.provider).source === "stale"
4803+
? session.modelRegistry.find(command.provider, command.modelId)
4804+
: undefined);
48014805
if (!model) {
48024806
throw new Error(`Model not found: ${command.provider}/${command.modelId}`);
48034807
}

‎packages/coding-agent/test/daemon-agent-roster.test.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -468,7 +468,6 @@ describe("worker roster reporter", () => {
468468
// set_model has no session-event carrier either; its explicit flush publishes the new model.
469469
const session = state.runtime.session as unknown as Record<string, unknown>;
470470
session.modelRegistry = {
471-
clearProviderAuthStale: () => {},
472471
refreshAvailableModels: async () => [{ provider: "prov", id: "m2" }],
473472
};
474473
session.setModel = async (model: unknown) => {

‎packages/coding-agent/test/daemon-mode.test.ts‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8877,7 +8877,6 @@ describe("daemon mode helpers", () => {
88778877
runtime: ActiveSessionState["runtime"] & {
88788878
session: {
88798879
modelRegistry: {
8880-
clearProviderAuthStale(provider: string): void;
88818880
refreshAvailableModels(): Promise<unknown[]>;
88828881
};
88838882
isStreaming: boolean;
@@ -8888,7 +8887,6 @@ describe("daemon mode helpers", () => {
88888887
};
88898888
state.runtime.session = {
88908889
modelRegistry: {
8891-
clearProviderAuthStale: vi.fn(),
88928890
refreshAvailableModels: vi.fn(async () => [model]),
88938891
},
88948892
isStreaming: true,
@@ -8936,7 +8934,6 @@ describe("daemon mode helpers", () => {
89368934
runtime: ActiveSessionState["runtime"] & {
89378935
session: {
89388936
modelRegistry: {
8939-
clearProviderAuthStale(provider: string): void;
89408937
refreshAvailableModels(): Promise<unknown[]>;
89418938
};
89428939
isStreaming: boolean;
@@ -8947,7 +8944,6 @@ describe("daemon mode helpers", () => {
89478944
};
89488945
state.runtime.session = {
89498946
modelRegistry: {
8950-
clearProviderAuthStale: vi.fn(),
89518947
refreshAvailableModels: vi.fn(async () => [model]),
89528948
},
89538949
isStreaming: false,

0 commit comments

Comments
 (0)