fix(models, oauth): placeholder-key suppression, subscription badge, 401 hint, OAuth flow timeout

apps/server/src/bridge/in-process.ts

@@ -5,6 +5,7 @@ import {
 	settings as ompSettings,
 	type AgentSession,
 } from "@oh-my-pi/pi-coding-agent";
+import { getEnvApiKey } from "@oh-my-pi/pi-ai";
 import { runExtensionCompact, runExtensionSetModel } from "@oh-my-pi/pi-coding-agent/extensibility/extensions/compact-handler";
 import { getSessionSlashCommands } from "@oh-my-pi/pi-coding-agent/extensibility/extensions/get-commands-handler";
 // `Model` is owned by `@oh-my-pi/pi-ai`, a transitive dep we don't bring in
@@ -33,7 +34,9 @@ import type {
 
 import { logger } from "../log.ts";
 import { getDeckModelRegistry } from "../auth-singleton.ts";
+import { looksLikePlaceholderKey } from "../credential-quality.ts";
 import { getEffectivePrelude } from "../orientation-store.ts";
+import { notificationService } from "../notifications/index.ts";
 import { ExtensionUIBridge } from "./ext-ui-bridge.ts";
 import { PlanModeBridge } from "./plan-mode-bridge.ts";
 import type {
@@ -458,6 +461,21 @@ export class InProcessAgentBridge implements AgentBridge {
 					}
 				}
 			}
+			// Issue #4 recovery hint: when the SDK surfaces an auth-shaped error
+			// (401 / "Incorrect API key") on a request to an API-key provider
+			// AND a subscription (OAuth) variant of the same model name exists
+			// AND is actually authenticated, fire a deck notification telling
+			// the operator to switch. Without this, the chat shows the raw 401
+			// inline and the operator has no idea why a fresh ChatGPT-Plus
+			// install rejected their first prompt. See issue #4.
+			if (type === "notice") {
+				const n = event as { level?: string; message?: string };
+				if (n.level === "error" && typeof n.message === "string" && looksLikeAuthError(n.message)) {
+					this.maybeSuggestSubscriptionFallback(session, n.message).catch((err) =>
+						log.warn("subscription-fallback hint failed", err),
+					);
+				}
+			}
 		});
 
 		this.active.set(sessionId, {
@@ -537,6 +555,46 @@ export class InProcessAgentBridge implements AgentBridge {
 		this.bumpActivity(sessionId);
 		return entry.planBridge.respond(proposalId, response);
 	}
+
+	/**
+	 * Issue #4: emit a deck notification when an inline auth error on the
+	 * current model has a known recovery path (subscription provider with
+	 * the same model id is authenticated). Idempotent in the failure case —
+	 * if any precondition is missing we just bail silently. The notification
+	 * lands in the standard dropdown + optional OS toast so the operator
+	 * sees it even if the chat is scrolled past the inline error.
+	 */
+	private async maybeSuggestSubscriptionFallback(
+		session: AgentSession,
+		errorMessage: string,
+	): Promise<void> {
+		const snap = (session as unknown as { snapshot?: () => { model?: { provider?: string; id?: string } } }).snapshot?.();
+		const current = snap?.model;
+		if (!current?.provider || !current.id) return;
+		// Already on a subscription provider — nothing to suggest.
+		if (getSubscriptionProviders().has(current.provider)) return;
+		const registry = await this.ensureModelRegistry();
+		// Look for any subscription provider carrying the same model id that's
+		// authenticated (auth.db has OAuth credential).
+		const alternative = registry
+			.getAll()
+			.map((m) => m as unknown as SdkModel)
+			.find((m) => {
+				if (m.id !== current.id) return false;
+				const provider = String(m.provider);
+				if (!getSubscriptionProviders().has(provider)) return false;
+				const sdkModel = m as unknown as Parameters<ModelRegistry["isUsingOAuth"]>[0];
+				return registry.isUsingOAuth(sdkModel);
+			});
+		if (!alternative) return;
+		const altProvider = String(alternative.provider);
+		await notificationService.notify({
+			level: "warn",
+			title: `Authentication failed for ${current.provider}/${current.id}`,
+			body: `You appear to be authenticated for the same model under \`${altProvider}\` (subscription). Switch in the model picker to use your subscription instead.\n\nOriginal error: ${errorMessage.slice(0, 240)}`,
+			source: `bridge:auth-fallback`,
+		});
+	}
 }
 
 export class InProcessSessionHandle implements SessionHandle {
@@ -1108,17 +1166,102 @@ function summarize(raw: any): SessionSummary {
 	};
 }
 
+/**
+ * Provider IDs that represent a true consumer subscription — the user
+ * paid a monthly fee (Claude Pro/Max, ChatGPT Plus/Pro, Copilot, Cursor)
+ * or a coding plan (Z.AI GLM, Alibaba, MiniMax, Kimi). The picker badges
+ * these so users can tell subscription variants apart from API-key
+ * variants of the same model name (the actual bug from issue #4).
+ *
+ * Intentionally an explicit allowlist, not `getOAuthProviders()` from the
+ * SDK. The SDK's "OAuth providers" is a broader category that also
+ * includes local runtimes (Ollama, LM Studio, vLLM), gateway services
+ * (LiteLLM, Kilo, Cloudflare AI Gateway), and pure-API-tier providers
+ * (Cerebras, Fireworks, Together, HuggingFace) — none of which are
+ * "subscriptions" in the user-facing sense. Calling Ollama a
+ * "subscription" in the model picker is actively misleading.
+ *
+ * Used for two purposes by `modelInfoFromSdk` and the issue-#4 hint:
+ *   - Tag rows with `isSubscription: true` so the picker can badge them.
+ *   - Pick recovery targets for the 401-fallback notification.
+ *
+ * When the SDK adds a new subscription-style provider, add it here.
+ * False negatives (missing a real subscription) are graceful — the user
+ * just doesn't get the badge. False positives (claiming Ollama is a
+ * subscription) are confusing and that's what we're fixing here.
+ */
+const SUBSCRIPTION_PROVIDER_IDS: ReadonlySet<string> = new Set([
+	"anthropic", // Claude Pro/Max — competes with anthropic API key for Claude models
+	"openai-codex", // ChatGPT Plus/Pro — competes with openai API key for gpt-5/etc.
+	"github-copilot", // Copilot subscription
+	"cursor", // Cursor IDE subscription — surfaces Claude/GPT models
+	"perplexity", // Perplexity Pro/Max — competes with perplexity API key
+	"alibaba-coding-plan", // Alibaba Coding Plan
+	"zai", // Z.AI GLM Coding Plan
+	"minimax-code", // MiniMax Coding Plan (International)
+	"minimax-code-cn", // MiniMax Coding Plan (China)
+	"kimi-code", // Kimi Code
+	"google-antigravity", // Google Antigravity (preview)
+]);
+function getSubscriptionProviders(): ReadonlySet<string> {
+	return SUBSCRIPTION_PROVIDER_IDS;
+}
+
+/**
+ * Heuristic match for "this error is an auth failure on the API call we
+ * just made". Used to gate the issue-#4 subscription-fallback hint. Kept
+ * narrow on purpose: false positives mean we suggest a switch when none is
+ * needed, which is annoying; the worst case is silence on a less-common
+ * error shape, which is the existing behavior.
+ */
+function looksLikeAuthError(message: string): boolean {
+	const m = message.toLowerCase();
+	if (m.includes("401")) return true;
+	if (m.includes("incorrect api key")) return true;
+	if (m.includes("invalid api key")) return true;
+	if (m.includes("invalid_api_key")) return true;
+	if (m.includes("unauthorized")) return true;
+	if (m.includes("authentication failed")) return true;
+	if (m.includes("api key is required")) return true;
+	return false;
+}
+
 function modelInfoFromSdk(
 	model: SdkModel,
 	registry: ModelRegistry,
 	current: { provider: string; id: string } | undefined,
 ): ModelInfo {
+	const provider = String(model.provider);
+	const sdkModel = model as unknown as Parameters<ModelRegistry["hasConfiguredAuth"]>[0];
+	const hasAuth = registry.hasConfiguredAuth(sdkModel);
+	const usingOAuth = registry.isUsingOAuth(sdkModel);
+	const isSubscription = getSubscriptionProviders().has(provider);
+	// `isAvailable` semantics: would a call routed to this provider succeed?
+	//   - SDK reports no configured auth at all → false (keyless paths are
+	//     also flagged via hasConfiguredAuth, so this also covers them).
+	//   - SDK has an OAuth credential in auth.db (`isUsingOAuth`) → true,
+	//     regardless of what's in process.env.
+	//   - Otherwise an env-var API key is the credential source. Validate
+	//     that the value isn't a known placeholder (`sk-your-…here`, etc.)
+	//     — see credential-quality.ts and issue #4.
+	let isAvailable = hasAuth;
+	if (isAvailable && !usingOAuth) {
+		const envValue = getEnvApiKey(provider);
+		// Only suppress when the env-var IS the credential. An empty env var
+		// with `hasConfiguredAuth=true` means auth came from somewhere else
+		// (auth.db non-OAuth entry, keyless provider, foundry, etc.) — trust
+		// the SDK in that case.
+		if (envValue && looksLikePlaceholderKey(envValue)) {
+			isAvailable = false;
+		}
+	}
 	const info: ModelInfo = {
-		provider: String(model.provider),
+		provider,
 		id: model.id,
 		label: model.name || model.id,
-		isAvailable: registry.hasConfiguredAuth(model as unknown as Parameters<ModelRegistry["hasConfiguredAuth"]>[0]),
+		isAvailable,
 	};
+	if (isSubscription) info.isSubscription = true;
 	if (typeof model.contextWindow === "number" && model.contextWindow > 0) {
 		info.contextWindow = model.contextWindow;
 	}

apps/server/src/credential-quality.test.ts

@@ -0,0 +1,93 @@
+import { describe, expect, test } from "bun:test";
+import { looksLikePlaceholderKey } from "./credential-quality.ts";
+
+describe("looksLikePlaceholderKey", () => {
+	test("treats undefined / null / empty as placeholder", () => {
+		expect(looksLikePlaceholderKey(undefined)).toBe(true);
+		expect(looksLikePlaceholderKey(null)).toBe(true);
+		expect(looksLikePlaceholderKey("")).toBe(true);
+		expect(looksLikePlaceholderKey("   ")).toBe(true);
+		expect(looksLikePlaceholderKey("\n\t")).toBe(true);
+	});
+
+	test("rejects the exact value from issue #4", () => {
+		// The literal string OpenAI echoed back in the 401 from the bug report.
+		expect(looksLikePlaceholderKey("sk-your-XXXXXXXXXXXXXXXXhere")).toBe(true);
+		expect(looksLikePlaceholderKey("sk-your-api-key-here")).toBe(true);
+	});
+
+	test("rejects common .env.example placeholders", () => {
+		const cases = [
+			"sk-XXXXXXXXXXXXXXXX",
+			"sk-XXX",
+			"your-api-key",
+			"your_api_key_here",
+			"YOUR-OPENAI-KEY",
+			"<your-anthropic-key>",
+			"<api-key>",
+			"changeme",
+			"change-me",
+			"change_me",
+			"CHANGEME",
+			"example",
+			"example-key",
+			"example_value",
+			"placeholder",
+			"placeholder-token",
+			"dummy",
+			"dummy-key",
+			"test-key",
+			"testkey",
+			"${OPENAI_API_KEY}",
+			"$OPENAI_API_KEY",
+			"xxx",
+			"XXXXXX",
+			"?????",
+			"00000000000000000000",
+		];
+		for (const c of cases) {
+			expect(looksLikePlaceholderKey(c)).toBe(true);
+		}
+	});
+
+	test("rejects keys that are too short to be real for their family", () => {
+		expect(looksLikePlaceholderKey("sk-short")).toBe(true); // OpenAI floor 40
+		expect(looksLikePlaceholderKey("sk-ant-short")).toBe(true); // Anthropic floor 40
+		expect(looksLikePlaceholderKey("AIza-short")).toBe(true); // Google floor 30
+		expect(looksLikePlaceholderKey("xai-short")).toBe(true); // xAI floor 40
+		expect(looksLikePlaceholderKey("gsk_short")).toBe(true); // Groq floor 40
+	});
+
+	test("accepts realistic-looking keys", () => {
+		// Realistic-shape values (random, not real). We accept anything that's
+		// the right shape and length for its prefix family.
+		expect(
+			looksLikePlaceholderKey(
+				"sk-proj-Ab9cD3fGh2jKl4mNo5pQr6sTu7vWx8yZ0a1bC2dE3fG4hI5jK6lM7nO8pQ9r",
+			),
+		).toBe(false);
+		expect(
+			looksLikePlaceholderKey(
+				"sk-ant-api03-aB1cD2eF3gH4iJ5kL6mN7oP8qR9sT0uV1wX2yZ3aB4cD5eF6gH7iJ8kL9mN0o-AbCdEf",
+			),
+		).toBe(false);
+		expect(looksLikePlaceholderKey("AIzaSyA1bC2dE3fG4hI5jK6lM7nO8pQ9rS0tUv")).toBe(false);
+		expect(
+			looksLikePlaceholderKey("gsk_AbCdEfGhIjKlMnOpQrStUvWxYz0123456789AbCdEfGhIj"),
+		).toBe(false);
+		// Generic 32+ char hex token (no recognizable prefix)
+		expect(
+			looksLikePlaceholderKey("e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"),
+		).toBe(false);
+	});
+
+	test("accepts OAuth bearer tokens with the standard JWT-ish shape", () => {
+		// OAuth tokens stored in env (e.g. OPENAI_CODEX_OAUTH_TOKEN) are
+		// typically long base64-ish blobs. Make sure we don't false-positive.
+		expect(
+			looksLikePlaceholderKey(
+				"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4ifQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
+			),
+		).toBe(false);
+	});
+});

apps/server/src/credential-quality.ts

@@ -0,0 +1,83 @@
+/**
+ * Heuristics for detecting placeholder / invalid provider credential values.
+ *
+ * Motivating bug (issue #4): a user demoed the deck and a viewer clicked
+ * `gpt-5`, expecting their ChatGPT Plus subscription to drive the request.
+ * Instead OpenAI returned `401 Incorrect API key provided: sk-your-****here`.
+ * The literal placeholder `sk-your-...here` had been sitting in their
+ * environment (common .env.example value); the SDK saw a non-empty
+ * `OPENAI_API_KEY`, marked the openai-provider variant of `gpt-5` as
+ * available, and that beat the openai-codex (subscription) variant of the
+ * same model name in the picker.
+ *
+ * `looksLikePlaceholderKey` is what the deck's model-availability layer
+ * uses to suppress these false-positive credentials so the picker only
+ * surfaces models the user can actually call. It's intentionally
+ * conservative — we'd rather hide a few real-but-weird keys than ship a
+ * model that 401s on click.
+ */
+
+/**
+ * Patterns that match values commonly seen in tutorial `.env.example` files
+ * or "I'll fill this in later" stubs. Matched case-insensitively against the
+ * trimmed value.
+ */
+const PLACEHOLDER_PATTERNS: readonly RegExp[] = [
+	// `sk-your-XXXXhere`, `sk-your-api-key-here`, ...
+	/^sk-your[-_]/i,
+	// `sk-XXXXXX`, `sk-XXX...`
+	/^sk-x{3,}/i,
+	// `your-api-key`, `your_api_key_here`, `YOUR-OPENAI-KEY`, ...
+	/^your[-_ ]?(api|openai|anthropic|google|key)/i,
+	// `<your-key>`, `<api-key>`, ...
+	/^<.*>$/,
+	// `changeme`, `change-me`, `change_me`
+	/^change[-_ ]?me/i,
+	// `example`, `example-key`, `example_value`
+	/^example([-_ ]|$)/i,
+	// `placeholder`, `placeholder-key`, ...
+	/^placeholder/i,
+	// `dummy`, `dummy-key`, ...
+	/^dummy([-_ ]|$)/i,
+	// `test-key`, `testkey`, `test_key` — too short to be a real secret
+	/^test[-_]?key/i,
+	// Unsubstituted shell variable references: `${OPENAI_API_KEY}`, `$VAR`
+	/^\$\{/,
+	/^\$[A-Z_][A-Z0-9_]*$/,
+	// All-X or all-? values
+	/^x+$/i,
+	/^\?+$/,
+	// All-zero placeholder
+	/^0+$/,
+];
+
+/**
+ * Minimum length for a value that doesn't otherwise look like a placeholder,
+ * keyed by recognizable prefix. Real provider keys are well over these
+ * bounds; anything shorter is almost certainly a stub.
+ */
+function minimumReasonableLength(value: string): number {
+	if (value.startsWith("sk-ant-")) return 40; // Anthropic keys ~108 chars
+	if (value.startsWith("sk-or-")) return 40; // OpenRouter
+	if (value.startsWith("sk-")) return 40; // OpenAI ~51 chars
+	if (value.startsWith("AIza")) return 30; // Google API keys 39 chars
+	if (value.startsWith("xai-")) return 40; // xAI
+	if (value.startsWith("gsk_")) return 40; // Groq
+	return 16; // Generic floor — narrower would catch real values
+}
+
+/**
+ * Returns true when `value` is empty or matches a known placeholder pattern.
+ * Empty/whitespace counts as a placeholder because the caller treats `true`
+ * as "do not advertise this credential as usable."
+ */
+export function looksLikePlaceholderKey(value: string | undefined | null): boolean {
+	if (value === undefined || value === null) return true;
+	const v = value.trim();
+	if (v.length === 0) return true;
+	for (const re of PLACEHOLDER_PATTERNS) {
+		if (re.test(v)) return true;
+	}
+	if (v.length < minimumReasonableLength(v)) return true;
+	return false;
+}