diff --git a/.codeyam/state/step-highwater.json b/.codeyam/state/step-highwater.json deleted file mode 100644 index f4a3f36..0000000 --- a/.codeyam/state/step-highwater.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "feature": "first-run-clarity-guided-tour-and-starter-questions", - "maxStep": 1, - "version": 22, - "updatedAt": "2026-08-03T18:02:44.689480+00:00" -} \ No newline at end of file diff --git a/Sources/AppCore/AskCoachView.swift b/Sources/AppCore/AskCoachView.swift index 21a2cbe..24d5901 100644 --- a/Sources/AppCore/AskCoachView.swift +++ b/Sources/AppCore/AskCoachView.swift @@ -150,6 +150,17 @@ public struct AskCoachView: View { reply = CoachReply(intent: .general, text: "Your AI coach key was rejected. Reconnect it in Settings, then ask again.", mood: .concerned) + } catch CoachError.rateLimited { + // Previously fell through to the connection copy, which sent the + // user to check their wifi over a provider-side throttle. + reply = CoachReply(intent: .general, + text: "Your AI provider is rate limiting requests right now. Give it a minute and ask again.", + mood: .concerned) + } catch CoachError.upstream(let message) { + // A provider-side configuration fault. Saying "check your + // connection" here sent the user chasing a network problem that + // did not exist, so state what actually happened. + reply = CoachReply(intent: .general, text: message, mood: .concerned) } catch { reply = CoachReply(intent: .general, text: "I couldn't reach Buddy just now. Check your connection and try again.", diff --git a/Sources/AppCore/Coach/RemoteCoach.swift b/Sources/AppCore/Coach/RemoteCoach.swift index 273f256..15467a5 100644 --- a/Sources/AppCore/Coach/RemoteCoach.swift +++ b/Sources/AppCore/Coach/RemoteCoach.swift @@ -213,6 +213,12 @@ public enum CoachError: Error, Equatable { case rateLimited case server case network + /// The backend reached the provider and the provider refused for a reason the + /// user can act on (an unavailable model, an exhausted token budget). Carries + /// the backend's own message: collapsing these into `.server` is what made a + /// misconfigured provider read as "check your connection" in the chat, which + /// pointed the user at the one thing that was not wrong. + case upstream(String) } /// Calls the backend coach proxy. Stateless; safe to construct per request. @@ -245,6 +251,31 @@ public struct RemoteCoach { let safetyFlag: Bool } + /// The backend's error envelope: `{ "error": "", "message": "" }`. + private struct ErrorBody: Decodable { + let error: String + let message: String? + } + + /// The actionable reason from a non-2xx body, when the backend supplied one. + /// Only codes the user can actually do something about are surfaced; a bare + /// `upstream_error` stays a generic failure so a real outage still reads as one. + static func errorMessage(from data: Data) -> String? { + guard let body = try? JSONDecoder().decode(ErrorBody.self, from: data) else { return nil } + let actionable: Set = [ + "model_unavailable", + "token_budget_exhausted", + // An exhausted balance is the single most likely real-world failure + // and the one the user can actually fix, so it must never be + // flattened into a retry or connection message. + "insufficient_quota", + ] + guard actionable.contains(body.error), let message = body.message, !message.isEmpty else { + return nil + } + return message + } + /// The context actually put on the wire. The journal is projected down to its /// bounded recent slice here — at the single point where the payload leaves /// the device — so an unbounded diary can never push `loadHistory` out of the @@ -291,7 +322,14 @@ public struct RemoteCoach { case 200: break case 401: throw CoachError.invalidKey // only a rejected key surfaces to the user; a 400 (bad request) is our bug, not theirs case 429: throw CoachError.rateLimited - default: throw CoachError.server + default: + // The backend distinguishes a provider misconfiguration (unavailable + // model, exhausted token budget) from an outage. Carry that reason + // through rather than flattening it into a generic failure. + if let message = Self.errorMessage(from: data) { + throw CoachError.upstream(message) + } + throw CoachError.server } guard let decoded = try? JSONDecoder().decode(ResponseBody.self, from: data) else { diff --git a/Tests/AppCoreTests/CoachErrorSurfacingTests.swift b/Tests/AppCoreTests/CoachErrorSurfacingTests.swift new file mode 100644 index 0000000..ce90b77 --- /dev/null +++ b/Tests/AppCoreTests/CoachErrorSurfacingTests.swift @@ -0,0 +1,65 @@ +import XCTest +@testable import AppCore + +// How a failed coach call reaches the user. +// +// Regression: a working OpenAI key produced "I couldn't reach Buddy just now. +// Check your connection and try again." The key was valid and the network was +// fine — the backend had rejected the request for a provider-side reason — but +// every non-401 collapsed into `.server`, so the chat pointed the user at the +// one thing that was not wrong. These pin the reason surviving the trip. +final class CoachErrorSurfacingTests: XCTestCase { + + private func body(_ json: String) -> Data { Data(json.utf8) } + + func testActionableUpstreamReasonSurvives() { + let data = body(""" + {"error":"model_unavailable","message":"OpenAI rejected the request: model \\"gpt-5\\" may be unavailable to this key."} + """) + XCTAssertEqual(RemoteCoach.errorMessage(from: data), + "OpenAI rejected the request: model \"gpt-5\" may be unavailable to this key.") + } + + /// The real cause of the reported outage: an OpenAI account with no credits. + /// The user can fix this in a minute, but only if they are told what it is. + func testOutOfCreditsIsSurfaced() { + let data = body(""" + {"error":"insufficient_quota","message":"Your OpenAI account is out of credits, so it declined the request. Add credits to your OpenAI account and ask again."} + """) + let message = RemoteCoach.errorMessage(from: data) + XCTAssertNotNil(message) + XCTAssertTrue(message!.contains("out of credits")) + // The two messages this used to be mistaken for. + XCTAssertFalse(message!.contains("connection")) + XCTAssertFalse(message!.contains("try again shortly")) + } + + func testExhaustedBudgetIsSurfaced() { + let data = body(""" + {"error":"token_budget_exhausted","message":"OpenAI used its entire token budget before answering."} + """) + XCTAssertEqual(RemoteCoach.errorMessage(from: data), + "OpenAI used its entire token budget before answering.") + } + + /// A genuine outage stays generic — the caller falls back to the offline + /// coach rather than showing the user backend jargon they cannot act on. + func testGenericUpstreamErrorIsNotSurfaced() { + XCTAssertNil(RemoteCoach.errorMessage(from: body(#"{"error":"upstream_error"}"#))) + XCTAssertNil(RemoteCoach.errorMessage(from: body(#"{"error":"no_text"}"#))) + } + + func testMalformedOrEmptyBodiesAreIgnored() { + XCTAssertNil(RemoteCoach.errorMessage(from: body("not json"))) + XCTAssertNil(RemoteCoach.errorMessage(from: Data())) + XCTAssertNil(RemoteCoach.errorMessage(from: body(#"{"error":"model_unavailable"}"#))) + XCTAssertNil(RemoteCoach.errorMessage(from: body(#"{"error":"model_unavailable","message":""}"#))) + } + + /// The error the chat renders must not be the connection copy. + func testUpstreamErrorIsDistinctFromNetworkError() { + XCTAssertNotEqual(CoachError.upstream("model unavailable"), CoachError.server) + XCTAssertNotEqual(CoachError.upstream("model unavailable"), CoachError.network) + XCTAssertEqual(CoachError.upstream("same"), CoachError.upstream("same")) + } +} diff --git a/api/_lib/llm.ts b/api/_lib/llm.ts index c54d909..3648ff2 100644 --- a/api/_lib/llm.ts +++ b/api/_lib/llm.ts @@ -150,16 +150,90 @@ function parseJsonReply(text: string): LlmResult { } } -/** Map any provider's HTTP status onto the status the app already handles. */ -function throwForStatus(status: number | undefined, provider: LlmProvider): never { +/** + * Map any provider's HTTP status onto the status the app already handles. + * + * `detail` carries the provider's own error text when we have it. Without it a + * misconfigured model id and a genuine outage both surfaced as "could not be + * reached", which is what made a broken OpenAI key impossible to diagnose from + * the app: the user saw a network-sounding message for a config bug. + */ +function throwForStatus( + status: number | undefined, + provider: LlmProvider, + detail?: ProviderError, +): never { const name = provider === "anthropic" ? "Anthropic" : provider === "openai" ? "OpenAI" : "Gemini"; + const suffix = detail?.message ? ` ${detail.message}` : ""; if (status === 401 || status === 403) { throw new LlmError(401, "invalid_key", `That API key was rejected by ${name}.`); } if (status === 429) { + // An empty balance is NOT a transient limit and never clears on its own. + // 402 (not 429) so the app parses the body instead of routing it to the + // retry path — "try again shortly" is unactionable when the account is dry. + if (isQuotaExhausted(detail)) { + throw new LlmError( + 402, + "insufficient_quota", + `Your ${name} account is out of credits, so it declined the request. Add credits to your ${name} account and ask again.`, + ); + } throw new LlmError(429, "rate_limited", `Your ${name} account is rate limited. Try again shortly.`); } - throw new LlmError(502, "upstream_error", `${name} could not be reached.`); + // 400/404 are OUR bug (unknown model, malformed request), not an outage, and + // not something the user can retry past. Say so, and pass the provider's + // reason through so the cause is visible without reproducing locally. + if (status === 400 || status === 404) { + throw new LlmError( + 502, + "model_unavailable", + `${name} rejected the request: model "${modelFor(provider)}" may be unavailable to this key.${suffix}`, + ); + } + throw new LlmError(502, "upstream_error", `${name} could not be reached.${suffix}`); +} + +/** A provider's error body, normalized to the bits we route on. */ +interface ProviderError { + message?: string; + /** Provider's own classifier, e.g. "insufficient_quota". */ + type?: string; + /** Provider's own code, e.g. "credit_balance_exhausted". */ + code?: string; +} + +/** Best-effort extraction of a provider's error from its response body. */ +async function errorDetail(response: Response): Promise { + const body = (await response.json().catch(() => null)) as + | { error?: { message?: string; type?: string; code?: string } | string } + | null; + if (!body) return undefined; + const err = body.error; + if (typeof err === "string") return { message: err.slice(0, 300) }; + if (!err) return undefined; + return { + message: typeof err.message === "string" ? err.message.slice(0, 300) : undefined, + type: typeof err.type === "string" ? err.type : undefined, + code: typeof err.code === "string" ? err.code : undefined, + }; +} + +/** + * Whether a 429 is an exhausted balance rather than a burst limit. + * + * These are opposite problems wearing the same status code: a burst limit clears + * on its own in seconds, an empty balance never does. Telling someone with no + * credits to "try again shortly" is advice that cannot work, so they get routed + * apart here. + */ +function isQuotaExhausted(detail?: ProviderError): boolean { + const needles = ["insufficient_quota", "credit_balance_exhausted", "billing_hard_limit_reached"]; + const haystack = `${detail?.type ?? ""} ${detail?.code ?? ""}`.toLowerCase(); + if (needles.some((n) => haystack.includes(n))) return true; + // Gemini/others don't use OpenAI's type/code vocabulary; fall back to prose. + const message = (detail?.message ?? "").toLowerCase(); + return message.includes("no credits remaining") || message.includes("exceeded your current quota"); } // MARK: - Anthropic @@ -182,7 +256,10 @@ async function completeAnthropic(apiKey: string, request: LlmRequest): Promise { let response: Response; try { @@ -203,7 +293,7 @@ async function completeOpenAI(apiKey: string, request: LlmRequest): Promise ({ role: t.role, content: t.content })), @@ -218,15 +308,30 @@ async function completeOpenAI(apiKey: string, request: LlmRequest): Promise null)) as { - choices?: Array<{ message?: { content?: string | null; refusal?: string | null } }>; + choices?: Array<{ + finish_reason?: string; + message?: { content?: string | null; refusal?: string | null }; + }>; } | null; - const choice = body?.choices?.[0]?.message; + const first = body?.choices?.[0]; + const choice = first?.message; // A strict-schema refusal comes back in its own field, with content null. if (choice?.refusal) return { kind: "refusal" }; - if (typeof choice?.content !== "string" || !choice.content) return { kind: "empty" }; + if (typeof choice?.content !== "string" || !choice.content) { + // Empty output because the budget ran out is a fixable configuration fault, + // not an upstream outage — name it so it can't hide behind "no text" again. + if (first?.finish_reason === "length") { + throw new LlmError( + 502, + "token_budget_exhausted", + `OpenAI used its entire token budget before answering (model "${MODELS.openai}"). Raise max_completion_tokens.`, + ); + } + return { kind: "empty" }; + } return parseJsonReply(choice.content); } @@ -274,7 +379,7 @@ async function completeGemini(apiKey: string, request: LlmRequest): Promise null)) as { candidates?: Array<{ finishReason?: string; content?: { parts?: Array<{ text?: string }> } }>; diff --git a/test/api/llm.test.ts b/test/api/llm.test.ts index 9bd7639..fb701ac 100644 --- a/test/api/llm.test.ts +++ b/test/api/llm.test.ts @@ -201,6 +201,114 @@ describe("complete — OpenAI", () => { })); await expect(complete({ provider: "openai", apiKey: "sk-x" }, request())).rejects.toBeInstanceOf(LlmError); }); + + // Regression: a connected OpenAI key produced "I couldn't reach Buddy just + // now. Check your connection and try again." The key was fine and the network + // was fine — the reply budget was being consumed by reasoning tokens, and the + // resulting empty completion was indistinguishable from an outage. + + it("gives the reply real headroom beyond the reasoning budget", async () => { + const fetchMock = stubFetch({ choices: [{ message: { content: '{"text":"Rest day."}' } }] }); + await complete({ provider: "openai", apiKey: "sk-openai-1" }, request()); + const sent = JSON.parse(fetchMock.mock.calls[0][1].body as string); + // `max_completion_tokens` covers reasoning AND visible output, so a budget + // sized for a 2-4 sentence answer can be spent before any text is emitted. + expect(sent.max_completion_tokens).toBeGreaterThanOrEqual(16000); + expect(sent.max_tokens).toBeUndefined(); + }); + + it("reports an exhausted token budget as its own fault, not an outage", async () => { + stubFetch({ choices: [{ finish_reason: "length", message: { content: "" } }] }); + await expect( + complete({ provider: "openai", apiKey: "sk-x" }, request()), + ).rejects.toMatchObject({ status: 502, code: "token_budget_exhausted" }); + }); + + it("still reports a plain empty completion as empty", async () => { + stubFetch({ choices: [{ finish_reason: "stop", message: { content: "" } }] }); + expect(await complete({ provider: "openai", apiKey: "sk-x" }, request())).toEqual({ + kind: "empty", + }); + }); + + it("names an unavailable model instead of claiming the provider is unreachable", async () => { + stubFetch( + { error: { message: "The model `gpt-5` does not exist or you do not have access to it." } }, + { ok: false, status: 404 }, + ); + const err = await complete({ provider: "openai", apiKey: "sk-x" }, request()).catch((e) => e); + expect(err).toMatchObject({ status: 502, code: "model_unavailable" }); + // The provider's own reason must survive, or this is undiagnosable from the app. + expect(err.message).toMatch(/does not exist or you do not have access/); + expect(err.message).not.toMatch(/could not be reached/); + }); + + it("passes the provider's reason through on a 400", async () => { + stubFetch( + { error: { message: "Unsupported parameter: 'max_tokens'." } }, + { ok: false, status: 400 }, + ); + await expect( + complete({ provider: "openai", apiKey: "sk-x" }, request()), + ).rejects.toThrow(/Unsupported parameter/); + }); + + // A genuine outage must still read as one, so the new codes stay meaningful. + it("still reports a 500 as an unreachable provider", async () => { + stubFetch({}, { ok: false, status: 500 }); + await expect( + complete({ provider: "openai", apiKey: "sk-x" }, request()), + ).rejects.toMatchObject({ code: "upstream_error" }); + }); + + // Regression: the actual cause of the reported outage. An account with no + // credits returns 429 — the same status as a burst limit — and was reported as + // "rate limited, try again shortly". That advice can never succeed: the balance + // does not refill on its own. The app then degraded it further into "check your + // connection", so three layers each pointed further from the real problem. + // + // This payload is verbatim what OpenAI returned for the reported failure. + const NO_CREDITS = { + error: { + message: + "You have no credits remaining. Add credits to continue using the API at https://platform.openai.com/settings/organization/billing/.", + type: "insufficient_quota", + param: null, + code: "credit_balance_exhausted", + }, + }; + + it("tells an exhausted balance apart from a burst rate limit", async () => { + stubFetch(NO_CREDITS, { ok: false, status: 429 }); + const err = await complete({ provider: "openai", apiKey: "sk-x" }, request()).catch((e) => e); + + expect(err).toMatchObject({ status: 402, code: "insufficient_quota" }); + // 402, not 429: the app routes 429 to a retry path, and retrying is exactly + // what cannot help here. + expect(err.status).not.toBe(429); + expect(err.message).toMatch(/out of credits/i); + expect(err.message).not.toMatch(/try again shortly/i); + }); + + it("still reports a genuine burst limit as retryable", async () => { + stubFetch( + { error: { message: "Rate limit reached for requests", type: "requests", code: "rate_limit_exceeded" } }, + { ok: false, status: 429 }, + ); + await expect( + complete({ provider: "openai", apiKey: "sk-x" }, request()), + ).rejects.toMatchObject({ status: 429, code: "rate_limited" }); + }); + + it("recognizes an exhausted balance from prose when there is no type or code", async () => { + stubFetch( + { error: { message: "You exceeded your current quota, please check your plan and billing details." } }, + { ok: false, status: 429 }, + ); + await expect( + complete({ provider: "openai", apiKey: "sk-x" }, request()), + ).rejects.toMatchObject({ status: 402, code: "insufficient_quota" }); + }); }); describe("complete — Gemini", () => {