mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-12 22:26:13 +00:00
fix(coach): say so when the model answered with reasoning alone
When every provider in the chain answered a Coach round with reasoning and no reply, even after being asked once more without reasoning, the turn ended with "could not reach a provider, please try again in a moment". The provider was reached, and trying again in a moment changes nothing for a model that cannot stop reasoning. Such a chain now ends with its own code, coach.provider.reasoning_only, and the drawer says the model only reasoned and never wrote an answer, and suggests another model. The choice between credential_expired, rate_limited, reasoning_only and unavailable moved into one function beside the other turn error rules, where it is tested on its own. An un-wrapped reasoning-only error maps the same way. Clients that do not know the code keep showing the generic provider message. Refs #1276
This commit is contained in:
1 parent
c9673883b1
commit
31e107b96a
14 files changed
+144
-28
No files matched your search
@@ -817,6 +817,14 @@ describe("errorCodeToI18nKey", () => {
|
||||
);
|
||||
});
|
||||
|
||||
// #1276 — a model that only reasoned was reached; "try again in a
|
||||
// moment" is the wrong advice for it.
|
||||
it("maps coach.provider.reasoning_only to its own copy", () => {
|
||||
expect(errorCodeToI18nKey("coach.provider.reasoning_only")).toBe(
|
||||
"insights.coach.errorReasoningOnly",
|
||||
);
|
||||
});
|
||||
|
||||
// v1.39 — a refused turn names its reason. The capability refusals carry
|
||||
// their code in `meta.errorCode`; each maps to copy that says what is off
|
||||
// and who can change it, never the generic "provider unavailable".
|
||||
|
||||
@@ -87,6 +87,11 @@ export function errorCodeToI18nKey(code: string): string {
|
||||
// the user to guided setup with a one-line explainer + a Settings
|
||||
// link rendered alongside the bubble copy.
|
||||
return "insights.coach.errorNoProvider";
|
||||
case "coach.provider.reasoning_only":
|
||||
// #1276 — the model was reached and answered with its reasoning
|
||||
// only, even after being asked once more. "Try again in a moment"
|
||||
// would be the wrong advice; the copy names what happened.
|
||||
return "insights.coach.errorReasoningOnly";
|
||||
case "coach.provider.unavailable":
|
||||
case "coach.provider.empty":
|
||||
case "coach.stream":
|
||||
|
||||
@@ -0,0 +1,76 @@
|
||||
/**
|
||||
* The `coach.provider.*` code a failed provider chain surfaces. #1276 — a
|
||||
* chain whose every hop answered with reasoning and no reply gets its own
|
||||
* code: "could not reach a provider, try again in a moment" is wrong for it,
|
||||
* because the provider was reached and answered, just never with an answer.
|
||||
*/
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
import type { FallbackHop } from "@/lib/ai/provider-runner";
|
||||
|
||||
import { chainFailureCode, classifyBubblingProviderError } from "../errors";
|
||||
|
||||
function hop(over: Partial<FallbackHop>): FallbackHop {
|
||||
return {
|
||||
providerType: "openai-compatible",
|
||||
attempt: 1,
|
||||
failureReason: "OpenAI-compatible gateway returned empty content",
|
||||
httpStatus: 0,
|
||||
...over,
|
||||
};
|
||||
}
|
||||
|
||||
describe("chainFailureCode", () => {
|
||||
it("names a chain that only ever reasoned", () => {
|
||||
expect(
|
||||
chainFailureCode({
|
||||
attempts: [hop({ failureKind: "reasoning_only" })],
|
||||
primaryCredentialExpired: false,
|
||||
}),
|
||||
).toBe("coach.provider.reasoning_only");
|
||||
});
|
||||
|
||||
it("stays generic when another hop failed for another reason", () => {
|
||||
expect(
|
||||
chainFailureCode({
|
||||
attempts: [
|
||||
hop({ failureKind: "reasoning_only" }),
|
||||
hop({ attempt: 2, httpStatus: 503, failureKind: "http" }),
|
||||
],
|
||||
primaryCredentialExpired: false,
|
||||
}),
|
||||
).toBe("coach.provider.unavailable");
|
||||
});
|
||||
|
||||
it("keeps the existing codes", () => {
|
||||
expect(
|
||||
chainFailureCode({
|
||||
attempts: [hop({ httpStatus: 401 })],
|
||||
primaryCredentialExpired: true,
|
||||
}),
|
||||
).toBe("coach.provider.credential_expired");
|
||||
expect(
|
||||
chainFailureCode({
|
||||
attempts: [hop({ httpStatus: 429 }), hop({ httpStatus: 429 })],
|
||||
primaryCredentialExpired: false,
|
||||
}),
|
||||
).toBe("coach.provider.rate_limited");
|
||||
expect(
|
||||
chainFailureCode({ attempts: [], primaryCredentialExpired: false }),
|
||||
).toBe("coach.provider.unavailable");
|
||||
});
|
||||
});
|
||||
|
||||
describe("classifyBubblingProviderError", () => {
|
||||
it("names an un-wrapped reasoning-only reply", () => {
|
||||
const err = Object.assign(new Error("returned empty content"), {
|
||||
httpStatus: 0,
|
||||
kind: "reasoning_only",
|
||||
upstream: "openai-compatible",
|
||||
});
|
||||
expect(classifyBubblingProviderError(err)).toEqual({
|
||||
code: "reasoning_only",
|
||||
httpStatus: 0,
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -8,9 +8,41 @@
|
||||
*/
|
||||
import { annotate, getEvent } from "@/lib/logging/context";
|
||||
import { redactOptional, redactSecrets } from "@/lib/logging/redact";
|
||||
import type { AllProvidersFailedError } from "@/lib/ai/provider-runner";
|
||||
|
||||
import type { TurnEmitter } from "./types";
|
||||
|
||||
/**
|
||||
* The `coach.provider.*` code for a provider chain where every hop failed.
|
||||
*
|
||||
* - v1.11.0 — the person's PRIMARY provider failed auth-class (401/403):
|
||||
* the credential is dead, not the service, so the drawer deep-links to
|
||||
* reconnect (`credential_expired`).
|
||||
* - v1.4.25 — every hop landed on 429: transient, on the provider's side
|
||||
* (`rate_limited`).
|
||||
* - #1276 — every hop answered with reasoning and no reply, after the
|
||||
* client already asked once more (`reasoning_only`). The provider was
|
||||
* reached; "try again in a moment" is the wrong advice, and the thought
|
||||
* itself is never shown as the answer.
|
||||
* - Anything else: `unavailable`.
|
||||
*/
|
||||
export function chainFailureCode(
|
||||
err: Pick<AllProvidersFailedError, "attempts" | "primaryCredentialExpired">,
|
||||
): string {
|
||||
if (err.primaryCredentialExpired) return "coach.provider.credential_expired";
|
||||
const { attempts } = err;
|
||||
if (attempts.length > 0 && attempts.every((a) => a.httpStatus === 429)) {
|
||||
return "coach.provider.rate_limited";
|
||||
}
|
||||
if (
|
||||
attempts.length > 0 &&
|
||||
attempts.every((a) => a.failureKind === "reasoning_only")
|
||||
) {
|
||||
return "coach.provider.reasoning_only";
|
||||
}
|
||||
return "coach.provider.unavailable";
|
||||
}
|
||||
|
||||
/**
|
||||
* v1.21.3 — classify a provider error that bubbled out of the chain runner
|
||||
* un-wrapped (i.e. not an `AllProvidersFailedError`). The provider clients tag
|
||||
@@ -18,15 +50,17 @@ import type { TurnEmitter } from "./types";
|
||||
* provider failure that must surface a graceful `coach.provider.*` frame rather
|
||||
* than rethrow into an HTTP 500. Returns `null` for anything that is NOT a
|
||||
* recognisable provider error (a real server bug), so those keep the 500 +
|
||||
* GlitchTip path. Status mapping mirrors `AllProvidersFailedError`: 401/403 →
|
||||
* credential_expired, 429 → rate_limited, everything else → unavailable.
|
||||
* GlitchTip path. Status mapping mirrors `chainFailureCode`: 401/403 →
|
||||
* credential_expired, 429 → rate_limited, a reasoning-only reply →
|
||||
* reasoning_only, everything else → unavailable.
|
||||
*/
|
||||
export function classifyBubblingProviderError(err: unknown): {
|
||||
code: "credential_expired" | "rate_limited" | "unavailable";
|
||||
code:
|
||||
"credential_expired" | "rate_limited" | "reasoning_only" | "unavailable";
|
||||
httpStatus: number | null;
|
||||
} | null {
|
||||
if (err === null || typeof err !== "object") return null;
|
||||
const e = err as { upstream?: unknown; httpStatus?: unknown };
|
||||
const e = err as { upstream?: unknown; httpStatus?: unknown; kind?: unknown };
|
||||
const hasUpstreamTag = typeof e.upstream === "string";
|
||||
const status = typeof e.httpStatus === "number" ? e.httpStatus : null;
|
||||
// Require the wire tag — a bare `{ httpStatus }` from unrelated code must not
|
||||
@@ -36,6 +70,10 @@ export function classifyBubblingProviderError(err: unknown): {
|
||||
return { code: "credential_expired", httpStatus: status };
|
||||
}
|
||||
if (status === 429) return { code: "rate_limited", httpStatus: status };
|
||||
// #1276 — the empty-reply sentinel of a reply that was reasoning only.
|
||||
if (e.kind === "reasoning_only") {
|
||||
return { code: "reasoning_only", httpStatus: status };
|
||||
}
|
||||
return { code: "unavailable", httpStatus: status };
|
||||
}
|
||||
|
||||
|
||||
@@ -85,7 +85,7 @@ import type { TurnLedger } from "./budget";
|
||||
import type { TurnReasoning } from "./reasoning";
|
||||
import type { TurnChain } from "./chain";
|
||||
import type { TurnContext } from "./context";
|
||||
import { classifyBubblingProviderError } from "./errors";
|
||||
import { chainFailureCode, classifyBubblingProviderError } from "./errors";
|
||||
import { snapshotStep, toStep } from "./steps";
|
||||
import type { TurnEmitter } from "./types";
|
||||
|
||||
@@ -769,29 +769,9 @@ export async function runTurnModel(args: {
|
||||
credentialExpired: err.primaryCredentialExpired,
|
||||
},
|
||||
});
|
||||
// v1.11.0 — when the user's PRIMARY provider failed with an
|
||||
// auth-class status (401/403), the credential is dead, not the
|
||||
// service. Surface a distinct `credential_expired` frame so the
|
||||
// drawer can deep-link the user to reconnect rather than telling
|
||||
// them to "try again later" — the gap that let an expired codex
|
||||
// token silently kill all generation.
|
||||
if (err.primaryCredentialExpired) {
|
||||
return { ok: false, code: "coach.provider.credential_expired" };
|
||||
}
|
||||
// v1.4.25 — distinguish provider rate-limit (every attempt
|
||||
// landed on 429) from generic unavailability. The drawer's
|
||||
// error-decoder surfaces the rate-limit copy with a warning
|
||||
// toast instead of the generic provider-down message, so the
|
||||
// user understands the limit is transient.
|
||||
const allRateLimited =
|
||||
err.attempts.length > 0 &&
|
||||
err.attempts.every((a) => a.httpStatus === 429);
|
||||
return {
|
||||
ok: false,
|
||||
code: allRateLimited
|
||||
? "coach.provider.rate_limited"
|
||||
: "coach.provider.unavailable",
|
||||
};
|
||||
// Credential expired, rate limited, reasoning without an answer
|
||||
// (#1276), or unavailable: `chainFailureCode` says which and why.
|
||||
return { ok: false, code: chainFailureCode(err) };
|
||||
}
|
||||
// v1.21.3 — defence in depth. The chain runner wraps every hard provider
|
||||
// failure in `AllProvidersFailedError`, but a provider client can still
|
||||
|
||||
Reference in new issue
Block a user