diff --git a/docs/docs/configure/providers.md b/docs/docs/configure/providers.md index 67fd570822..47d2f8bd77 100644 --- a/docs/docs/configure/providers.md +++ b/docs/docs/configure/providers.md @@ -68,6 +68,7 @@ registration request; **No** is selected by default. After registration, the mod explicit model is selected. Big Pickle is retired as a new selection — it no longer appears in the picker or the full model catalog for users choosing a model for the first time. Users already on Big Pickle are still detected on launch and offered Altimate Base through the same consent gate. +If you decline the default switch, `declinedManagedBaseDefault: true` in the state directory's `model.json` keeps public Zen ahead of registered Base for headless and ACP defaults, with Base used only as a last resort; accepting migration or explicitly selecting Base clears the flag. Official release binaries embed the current gateway endpoint at build time. Operators and local development can override it without changing code: diff --git a/docs/docs/reference/telemetry.md b/docs/docs/reference/telemetry.md index df5c491e35..5f1dad6e69 100644 --- a/docs/docs/reference/telemetry.md +++ b/docs/docs/reference/telemetry.md @@ -57,8 +57,8 @@ We collect the following categories of events: | `onboarding_started` | The first-run setup gate opened (fresh launch with no usable model). | | `model_picker_shown` | The provider picker was displayed. `trigger` distinguishes the first run from `/connect`, from declining Altimate Base, and from the prompt gate. | | `provider_selected` | A provider row was chosen — `altimate_gateway`, `altimate_base`, `anthropic`, `openai`, `google`, `search_all`, or `other` for anything outside the curated five. `provider_id` carries the raw id only for publicly-known providers, so a provider you named yourself in config is reported as `other` with no name attached. `via_search` marks a pick made inside the full catalogue after choosing "Search all providers…". **Choosing search emits this event twice for one user** — once as `search_all`, then again with the provider actually chosen — so count distinct users or filter on `via_search`, not raw event count. Recorded at the moment of choice, so a sign-in that is then cancelled still counts. | -| `altimate_base_confirm_shown` / `altimate_base_choice` | The Altimate Base disclosure was shown (`welcome` or `model` origin), and what the user decided (`accept`/`cancel`). | -| `altimate_base_register_result` | The consented registration outcome: `success`, `rate_limited`, `unavailable`, `network`, or `error`. No credential or gateway response body is included. | +| `altimate_base_confirm_shown` / `altimate_base_choice` | The Altimate Base disclosure was shown, and what the user decided (`accept`/`cancel`). `origin` is `welcome`, `model`, or `migration` (returning free-default users offered Altimate Base on launch); required for the disclosure event and optional for the choice event. | +| `altimate_base_register_result` | The consented registration outcome: `success`, `rate_limited`, `unavailable`, `network`, or `error`. Optional `origin` is `welcome`, `model`, or `migration` (returning free-default users offered Altimate Base on launch). No credential or gateway response body is included. | | `gateway_device_code_issued` | The Altimate Gateway authorize URL was built and the browser open attempted. **Name note:** the flow is a browser loopback OAuth — there is no device code. The name follows the original event spec. | | `gateway_auth_completed` / `gateway_auth_failed` | Gateway sign-in outcome. `reason` is `timeout`, `denied`, or `error` — never the underlying message, which can contain the instance name. An unrecognised callback state does not reject the pending attempt, so a CSRF mismatch surfaces as `timeout`. | | `instance_connected` | Credentials received and saved. `time_to_connect_ms` runs from the start of the authorize call, so it includes the browser launch. No instance or tenant name is sent. | diff --git a/packages/core/src/global.ts b/packages/core/src/global.ts index 6f620fa0fb..f9ef256da1 100644 --- a/packages/core/src/global.ts +++ b/packages/core/src/global.ts @@ -30,13 +30,34 @@ const paths = { repos: path.join(data, "repos"), cache, config, - state, + // altimate_change start — cubic review (3986917361): unlike `home` above, `state` was a plain + // const with no test-isolation override, so any consumer reading `Global.Path.state` — or + // `Flock`'s lock directory, which is derived from it (see the `Flock.setGlobal` call below) — + // silently touched the REAL, current developer's state directory in tests. Mirror `home`'s + // pattern: a getter honoring `OPENCODE_TEST_STATE_HOME`, read fresh on every access. + get state() { + return process.env.OPENCODE_TEST_STATE_HOME ?? state + }, + // altimate_change end tmp, } export const Path = paths -Flock.setGlobal({ state }) +// altimate_change start — cubic review (3986917361): `Flock.setGlobal` used to be given the +// frozen `state` const directly, snapshotted once at this module's import time — even after +// adding the `OPENCODE_TEST_STATE_HOME` override to `Path.state` above, `Flock`'s own internal +// lock-directory resolution would still have kept using whatever `state` was BEFORE any test set +// that env var (module imports happen once, before a test's own `beforeAll`/mount code runs). A +// getter-backed property here means `Flock`'s `root()` — which just reads `global.state` as a +// plain property — re-evaluates `Path.state` fresh on every lock acquisition instead, so setting +// `OPENCODE_TEST_STATE_HOME` redirects BOTH `Global.Path.state` reads and `Flock`'s lock root. +Flock.setGlobal({ + get state() { + return Path.state + }, +}) +// altimate_change end await Promise.all([ fs.mkdir(Path.data, { recursive: true }), diff --git a/packages/opencode/src/acp/directory.ts b/packages/opencode/src/acp/directory.ts index 1be274f198..d69b99a0ef 100644 --- a/packages/opencode/src/acp/directory.ts +++ b/packages/opencode/src/acp/directory.ts @@ -34,6 +34,14 @@ export type Snapshot = { readonly defaultModeID: string readonly availableCommands: readonly Command.Info[] readonly defaultModel?: DefaultModel + // altimate_change start — cache the project config that drives default-model selection, so the + // mutable model.json state (recents, decline flag) can be re-read at each selection instead of + // being frozen into the snapshot + readonly defaultModelConfig?: { + readonly model?: string + readonly provider?: Record + } + // altimate_change end } export interface LoaderInterface { @@ -61,6 +69,9 @@ export const build = (input: { readonly defaultModeID: string readonly commands: readonly Command.Info[] readonly defaultModel?: DefaultModel + // altimate_change start — see `Snapshot.defaultModelConfig` + readonly defaultModelConfig?: Snapshot["defaultModelConfig"] + // altimate_change end }): Snapshot => { const modelOptions = Provider.sort( Object.values(input.providers).flatMap((provider) => @@ -110,6 +121,9 @@ export const build = (input: { : (input.modes[0]?.id ?? input.defaultModeID), availableCommands: input.commands, ...(input.defaultModel ? { defaultModel: input.defaultModel } : {}), + // altimate_change start — see `Snapshot.defaultModelConfig` + ...(input.defaultModelConfig ? { defaultModelConfig: input.defaultModelConfig } : {}), + // altimate_change end } } diff --git a/packages/opencode/src/acp/service.ts b/packages/opencode/src/acp/service.ts index b69c30eeac..74b41a1b99 100644 --- a/packages/opencode/src/acp/service.ts +++ b/packages/opencode/src/acp/service.ts @@ -805,19 +805,6 @@ async function loadDirectorySnapshot(sdk: OpencodeClient, directory: string) { // provider either, so it fails closed the same way an explicit allowlist without it does. const snapshotProviders = configLoaded && !hasProviderAllowlist ? providers : withoutManagedBase() // altimate_change end - const defaultModelStarted = performance.now() - // altimate_change start — resolve the default against the SAME filtered snapshot advertised to - // the client. Resolving against the unfiltered `providers` map let a project that sets - // `model: "altimate-free/altimate-base"` alongside any `provider` allowlist end up with a - // `defaultModel` pointing at a provider this snapshot had just excluded — ACP would still - // select and route the managed model even though it was hidden from `modelOptions`. - const defaultModel = defaultModelFromConfig( - config?.model, - snapshotProviders, - config?.provider as Record | undefined, - ) - // altimate_change end - ACPProfile.duration("acp.directory.defaultModel.resolve", defaultModelStarted, { configured: !!defaultModel }) const modes = agents .filter((agent) => agent.mode !== "subagent" && agent.hidden !== true) .map((agent) => ({ @@ -846,7 +833,10 @@ async function loadDirectorySnapshot(sdk: OpencodeClient, directory: string) { modes, defaultModeID: agents.find((agent) => agent.mode === "primary" && agent.hidden !== true)?.name ?? "build", commands: commands.toSorted((a, b) => a.name.localeCompare(b.name)), - ...(defaultModel ? { defaultModel } : {}), + // altimate_change start — cache project config, but resolve mutable model.json state at each + // default selection + defaultModelConfig: { model: config?.model, provider: config?.provider }, + // altimate_change end }) }) } @@ -856,6 +846,10 @@ export function defaultModelFromConfig( configuredModel: string | undefined, providers: Record, providerFilter?: Record, + // altimate_change start — persisted recents and default-switch consent, normalized by the shared state reader + declinedManagedBaseDefault = false, + recent: Awaited>["recent"] = [], + // altimate_change end ): Directory.DefaultModel | undefined { // altimate_change start — fork Provider ids are branded ProviderID/ModelID; re-brand to core ProviderV2.ID/ModelV2.ID (identity at runtime) const configured = configuredModel @@ -869,6 +863,16 @@ export function defaultModelFromConfig( const configuredProviderEntries = Object.keys(providerFilter ?? {}) const hasProviderAllowlist = configuredProviderEntries.length > 0 + for (const entry of recent) { + const providerID = ProviderV2.ID.make(entry.providerID) + const modelID = ModelV2.ID.make(entry.modelID) + if (!Object.hasOwn(providers, providerID)) continue + if (!Object.hasOwn(providers[providerID].models, modelID)) continue + // Match Provider.defaultModel(): only managed Base recents are restricted by an allowlist. + if (entry.providerID === "altimate-free" && hasProviderAllowlist) continue + return { providerID, modelID } + } + // Prefer altimate-backend/altimate-default when the fork's backend is available and the user // hasn't pinned a model — restores dropped fork behavior (the merge fell straight through to the // opencode provider, routing ACP clients away from altimate's backend). Honors an explicit @@ -882,11 +886,22 @@ export function defaultModelFromConfig( return { providerID: ProviderV2.ID.make("altimate-backend"), modelID: ModelV2.ID.make("altimate-default") } } - // First-session ACP startup must not scan historical sessions just to infer - // a default. Configured model, opencode provider, then sorted best model keep - // the protocol response deterministic without extra session/message reads. + // First-session ACP startup must not scan historical sessions just to infer a default. + // Recents above come from model.json, not session storage. After configured/recent choices + // and the backend preference, use the opencode provider, then the sorted best model, + // without extra session/message reads. + const baseProvider = providers[ProviderV2.ID.make("altimate-free")] + const registeredBaseAvailable = Boolean(baseProvider?.models[ModelV2.ID.make("altimate-base")]) && !hasProviderAllowlist const providerAllowed = (id: string) => - id !== "altimate-free" && (!hasProviderAllowlist || Object.prototype.hasOwnProperty.call(providerFilter, id)) + id !== "altimate-free" && + (!hasProviderAllowlist || Object.prototype.hasOwnProperty.call(providerFilter, id)) && + !( + registeredBaseAvailable && + !declinedManagedBaseDefault && + id === "opencode" && + providers[ProviderV2.ID.make(id)]?.options.apiKey === "public" && + !providers[ProviderV2.ID.make(id)]?.key + ) const opencodeProvider = providerAllowed("opencode") ? providers[ProviderV2.ID.make("opencode")] : undefined const opencodeModel = opencodeProvider ? Provider.sort(Object.values(opencodeProvider.models)).find((model) => model.id !== "big-pickle") @@ -901,21 +916,41 @@ export function defaultModelFromConfig( ).find((model) => !(model.providerID === "opencode" && model.id === "big-pickle")) if (best) return { providerID: ProviderV2.ID.make(best.providerID), modelID: ModelV2.ID.make(best.id) } - // Altimate Base replaces Big Pickle as the free fallback, but only as a LAST resort and only - // after the user consented and registered (which is why it is present in `providers`). Anything - // else connected outranks the request-logging tier. A project provider block cannot force the - // managed model; an explicit configured model above remains authoritative. - const baseProvider = providers[ProviderV2.ID.make("altimate-free")] - if (!hasProviderAllowlist && baseProvider?.models[ModelV2.ID.make("altimate-base")]) { + // Altimate Base replaces Big Pickle as the free fallback only after the user consented and + // registered (which is why it is present in `providers`). Anything the user actually connected + // outranks the request-logging tier, except the keyless public Zen tier, which ranks below + // registered Base unless the user declined the default switch in model.json. After a decline, + // public Zen stays in both scans and Base is only the last resort. A keyed Zen account still + // wins. A project provider block cannot force the managed model; an explicit configured model + // above remains authoritative. + if (registeredBaseAvailable) { return { providerID: ProviderV2.ID.make("altimate-free"), modelID: ModelV2.ID.make("altimate-base") } } return undefined // altimate_change end } -// altimate_change start — keep Big Pickle explicitly selectable but never choose it implicitly -export function selectDefaultModel(snapshot: Directory.Snapshot) { - if (snapshot.defaultModel) return snapshot.defaultModel +// altimate_change start — Big Pickle is never chosen by the implicit provider/model SCANS below +// (the `opencodeModel`/`best` fallbacks both exclude it) — but a persisted `recent` entry is the +// user's own past pick, so it is honored verbatim, including a legacy Big Pickle one (kilo review +// round 6, 3986171219: mirrors `Provider.defaultModel()`'s identical recents-loop rationale in +// provider.ts — the TUI owns the migration because it owns the disclosure, so rewriting it here +// would move a declining user to the request-logging tier with no prompt). +export async function selectDefaultModel(snapshot: Directory.Snapshot) { + if (snapshot.defaultModelConfig) { + const started = performance.now() + const { recent, declinedManagedBaseDefault } = await Provider.readDefaultModelState() + // Resolve against the filtered catalogue so an excluded managed provider cannot be selected. + const selected = defaultModelFromConfig( + snapshot.defaultModelConfig.model, + snapshot.providers, + snapshot.defaultModelConfig.provider, + declinedManagedBaseDefault, + recent, + ) + ACPProfile.duration("acp.directory.defaultModel.resolve", started, { configured: !!selected }) + if (selected) return selected + } else if (snapshot.defaultModel) return snapshot.defaultModel // Big Pickle remains explicitly selectable for existing users, but Altimate Base replaces it as // the free implicit choice. Do not silently route a new ACP session back to Big Pickle when it is // the first (or only) sorted catalogue entry and no usable default was resolved above. @@ -935,17 +970,14 @@ function availableModel(snapshot: Directory.Snapshot, model: Directory.DefaultMo : undefined } -function requireDefaultModel(snapshot: Directory.Snapshot) { - const selected = selectDefaultModel(snapshot) - return selected - ? Effect.succeed(selected) - : Effect.fail( - new ACPError.ServiceFailureError({ - safeMessage: "No supported model is configured. Register Altimate Base or configure another provider.", - service: "model", - }), - ) -} +const requireDefaultModel = Effect.fn("ACP.requireDefaultModel")(function* (snapshot: Directory.Snapshot) { + const selected = yield* request(() => selectDefaultModel(snapshot), "model") + if (selected) return selected + return yield* new ACPError.ServiceFailureError({ + safeMessage: "No supported model is configured. Register Altimate Base or configure another provider.", + service: "model", + }) +}) // altimate_change end function detectSlashCommand(parts: ReturnType) { diff --git a/packages/opencode/src/altimate/telemetry/index.ts b/packages/opencode/src/altimate/telemetry/index.ts index 2b9cc25c32..eccd2a3c49 100644 --- a/packages/opencode/src/altimate/telemetry/index.ts +++ b/packages/opencode/src/altimate/telemetry/index.ts @@ -1027,18 +1027,20 @@ export namespace Telemetry { type: "altimate_base_confirm_shown" timestamp: number session_id: string - origin: "welcome" | "model" + origin: "welcome" | "model" | "migration" } | { type: "altimate_base_choice" timestamp: number session_id: string + origin?: "welcome" | "model" | "migration" choice: "accept" | "cancel" } | { type: "altimate_base_register_result" timestamp: number session_id: string + origin?: "welcome" | "model" | "migration" result: "success" | "rate_limited" | "unavailable" | "network" | "error" } | { diff --git a/packages/opencode/src/global/index.ts b/packages/opencode/src/global/index.ts index 1885142786..5e0dc7609e 100644 --- a/packages/opencode/src/global/index.ts +++ b/packages/opencode/src/global/index.ts @@ -24,7 +24,19 @@ export namespace Global { log: path.join(data, "log"), cache, config, - state, + // altimate_change start — cubic review round 5, P2: unlike `home` above, `state` was a + // plain module-load-time const with no test-isolation override, so any test reading or + // writing through `Global.Path.state` (recent-model / migration-decline persistence in + // `model.json`) was silently touching the REAL, current developer's state directory — + // racing any other test file doing the same thing in parallel, and risking clobbering real + // state if a test run were killed mid-write. Mirror `home`'s pattern with its own getter and + // env var so `Global.Path.state` can be redirected to a throwaway temp dir per test (see + // `test/fixture/fixture.ts`'s `withTestStateHome`), without changing production behavior — + // the getter is evaluated fresh on every access, and the env var is unset outside tests. + get state() { + return process.env.OPENCODE_TEST_STATE_HOME || state + }, + // altimate_change end } } diff --git a/packages/opencode/src/provider/provider.ts b/packages/opencode/src/provider/provider.ts index 9aee4337d0..f05cd2ec18 100644 --- a/packages/opencode/src/provider/provider.ts +++ b/packages/opencode/src/provider/provider.ts @@ -2193,12 +2193,26 @@ export namespace Provider { ) } - // altimate_change start — discard malformed persisted model references before use + // altimate_change start — normalize persisted model references and default-switch consent function isModelReference(model: unknown): model is { providerID: ProviderID; modelID: ModelID } { if (!model || typeof model !== "object") return false const value = model as Record return typeof value.providerID === "string" && typeof value.modelID === "string" } + + // Share the TUI's persisted default-switch consent with headless and ACP selection. + // Missing, unreadable, or malformed state preserves the existing default behavior. + export async function readDefaultModelState() { + return Filesystem.readJson<{ + recent?: { providerID: ProviderID; modelID: ModelID }[] + declinedManagedBaseDefault?: boolean + }>(path.join(Global.Path.state, "model.json")) + .then((state) => ({ + recent: Array.isArray(state?.recent) ? state.recent.filter(isModelReference) : [], + declinedManagedBaseDefault: state?.declinedManagedBaseDefault === true, + })) + .catch(() => ({ recent: [], declinedManagedBaseDefault: false })) + } // altimate_change end export async function defaultModel() { @@ -2218,20 +2232,16 @@ export namespace Provider { const baseModelID = ModelID.make(FreeTier.MODEL_ID) const baseProvider = providers[baseProviderID] const registeredBaseAvailable = Boolean(baseProvider?.models[baseModelID]) && !hasProviderAllowlist - const recent = (await Filesystem.readJson<{ recent?: { providerID: ProviderID; modelID: ModelID }[] }>( - path.join(Global.Path.state, "model.json"), - ) - .then((x) => (Array.isArray(x.recent) ? x.recent.filter(isModelReference) : [])) - .catch(() => [])) as { providerID: ProviderID; modelID: ModelID }[] + const { recent, declinedManagedBaseDefault } = await readDefaultModelState() for (const entry of recent) { // A recent entry is the user's own last pick, so it is never rewritten here — not even a // legacy Big Pickle one. The TUI owns the migration because it owns the disclosure, and // `migrateLegacyDefault()` rewrites model.json on accept, so headless follows on the next // launch. Migrating here instead would move a declining user to the request-logging tier // with no prompt and no way to refuse. + if (!Object.hasOwn(providers, entry.providerID)) continue const provider = providers[entry.providerID] - if (!provider) continue - if (!provider.models[entry.modelID]) continue + if (!Object.hasOwn(provider.models, entry.modelID)) continue // Keep legacy recent-model behavior unchanged for every other provider; // only the consent-gated managed provider must not bypass this project. if (entry.providerID === FreeTier.PROVIDER_ID && !providerAllowed(String(entry.providerID))) continue @@ -2258,9 +2268,12 @@ export namespace Provider { // altimate_change end // altimate_change start — select registered Altimate Base and never select Big Pickle implicitly - // Altimate Base owns the free fallback role that used to belong to Big Pickle, but only as a - // LAST resort. Anything the user has actually connected outranks the request-logging tier, so - // adding a paid key never silently routes prompts to the free gateway. A project provider + // Altimate Base owns the free fallback role that used to belong to Big Pickle. Anything the + // user has actually connected outranks the request-logging tier; the keyless public Zen tier + // ranks below registered Base unless the user declined the default switch in model.json. + // After a decline, public Zen stays in the scan and Base is only the last resort. + // A keyed Zen account still wins, so adding a paid key never silently routes prompts to the + // free gateway. A project provider // block cannot force the managed model; an explicit `model` setting above remains // authoritative. // Base is excluded from the ordinary scan so it can only be reached by the last-resort branch @@ -2270,6 +2283,14 @@ export namespace Provider { ) if (candidates.length === 0 && !registeredBaseAvailable) throw new Error("no providers found") for (const provider of candidates) { + if ( + registeredBaseAvailable && + !declinedManagedBaseDefault && + provider.id === "opencode" && + provider.options.apiKey === "public" && + !provider.key + ) + continue const model = sort(Object.values(provider.models)).find( (candidate) => !(provider.id === "opencode" && candidate.id === "big-pickle"), ) diff --git a/packages/opencode/test/acp/default-model.test.ts b/packages/opencode/test/acp/default-model.test.ts index 701628f5ee..5cdfebfc1c 100644 --- a/packages/opencode/test/acp/default-model.test.ts +++ b/packages/opencode/test/acp/default-model.test.ts @@ -2,12 +2,16 @@ // rewrote defaultModelFromConfig and dropped the fork's "prefer altimate-backend/altimate-default" // behavior, routing ACP clients (Zed/editors) to the opencode provider instead of altimate's backend. import { describe, expect, test } from "bun:test" +import fs from "node:fs/promises" +import path from "node:path" +import { Global } from "@/global" import { ProviderV2 } from "@opencode-ai/core/provider" import { ModelV2 } from "@opencode-ai/core/model" import { Provider } from "@/provider/provider" import { ProviderSchema } from "@/provider/schema" import { ACPService } from "@/acp/service" import { Directory } from "@/acp/directory" +import { withTestStateHome } from "../fixture/fixture" const model = (providerID: ProviderSchema.ProviderID, id: string): Provider.Model => ({ id: ProviderSchema.ModelID.make(id), @@ -82,6 +86,166 @@ describe("ACP defaultModelFromConfig", () => { }) }) + test("registered Altimate Base outranks public Zen in both implicit scans", () => { + const zen = provider("opencode", ["big-pickle", "nemotron-3-super-free"]) + zen.options.apiKey = "public" + const result = ACPService.defaultModelFromConfig( + undefined, + providers(zen, provider("altimate-free", ["altimate-base"])), + ) + expect(result).toEqual({ + providerID: ProviderV2.ID.make("altimate-free"), + modelID: ModelV2.ID.make("altimate-base"), + }) + }) + + test.each([ + { + name: "public Zen recent outranks registered Base", + recent: ["opencode/nemotron-3-super-free"], + expected: "opencode/nemotron-3-super-free", + }, + { name: "unloaded provider recent is ignored", recent: ["missing/model"], expected: "altimate-free/altimate-base" }, + { name: "missing model recent is ignored", recent: ["opencode/missing"], expected: "altimate-free/altimate-base" }, + { name: "__proto__ provider is ignored", recent: ["__proto__/x"], expected: "altimate-free/altimate-base" }, + { name: "constructor provider is ignored", recent: ["constructor/x"], expected: "altimate-free/altimate-base" }, + { name: "__proto__ model is ignored", recent: ["opencode/__proto__"], expected: "altimate-free/altimate-base" }, + { name: "constructor model is ignored", recent: ["opencode/constructor"], expected: "altimate-free/altimate-base" }, + { + name: "first available recent wins", + recent: ["missing/model", "opencode/missing", "opencode/nemotron-3-super-free", "altimate-free/altimate-base"], + expected: "opencode/nemotron-3-super-free", + }, + { + name: "Base recent is skipped with an allowlist even when included", + recent: ["altimate-free/altimate-base", "opencode/nemotron-3-super-free"], + filter: { "altimate-free": {}, opencode: {} }, + expected: "opencode/nemotron-3-super-free", + }, + { + name: "non-managed recent retains precedence outside the allowlist", + recent: ["opencode/nemotron-3-super-free"], + filter: { "altimate-backend": {} }, + expected: "opencode/nemotron-3-super-free", + }, + { + name: "configured model outranks recents", + configured: "altimate-free/altimate-base", + recent: ["opencode/nemotron-3-super-free"], + expected: "altimate-free/altimate-base", + }, + { name: "no recents preserves the Base fallback", recent: [], expected: "altimate-free/altimate-base" }, + ])("$name", ({ recent, filter, configured, expected }) => { + const zen = provider("opencode", ["nemotron-3-super-free"]) + zen.options.apiKey = "public" + const expectedModel = Provider.parseModel(expected) + expect( + ACPService.defaultModelFromConfig( + configured, + providers(zen, provider("altimate-free", ["altimate-base"])), + filter, + false, + recent.map(Provider.parseModel), + ), + ).toEqual({ + providerID: ProviderV2.ID.make(expectedModel.providerID), + modelID: ModelV2.ID.make(expectedModel.modelID), + }) + }) + + test.each([ + { flag: true, providerID: "opencode", modelID: "nemotron-3-super-free" }, + { flag: false, providerID: "altimate-free", modelID: "altimate-base" }, + { flag: undefined, providerID: "altimate-free", modelID: "altimate-base" }, + { flag: "yes", providerID: "altimate-free", modelID: "altimate-base" }, + ])("honors persisted default-switch decline flag $flag", async ({ flag, providerID, modelID }) => { + // altimate_change — Cursor/cubic review round 5, P2/P3: `Global.Path.state` is not + // test-isolated on its own (unlike `Global.Path.home`), so writing `model.json` through it + // directly touched the real developer state directory and raced other tests doing the same. + // `withTestStateHome` redirects it to a throwaway temp dir (already `mkdir`'d) for the + // duration of this test; see its declaration in `test/fixture/fixture.ts`. + await withTestStateHome(async () => { + const stateFile = path.join(Global.Path.state, "model.json") + await fs.writeFile(stateFile, JSON.stringify({ recent: [], declinedManagedBaseDefault: flag })) + const zen = provider("opencode", ["big-pickle", "nemotron-3-super-free"]) + zen.options.apiKey = "public" + const state = await Provider.readDefaultModelState() + const result = ACPService.defaultModelFromConfig( + undefined, + providers(zen, provider("altimate-free", ["altimate-base"])), + undefined, + state.declinedManagedBaseDefault, + state.recent, + ) + expect(result).toEqual({ + providerID: ProviderV2.ID.make(providerID), + modelID: ModelV2.ID.make(modelID), + }) + }) + }) + + test("a decline still permits Base as the last resort and as an explicit choice", () => { + const available = providers(provider("altimate-free", ["altimate-base"]), provider("opencode", ["big-pickle"])) + for (const configured of [undefined, "altimate-free/altimate-base"]) { + expect(ACPService.defaultModelFromConfig(configured, available, undefined, true)).toEqual({ + providerID: ProviderV2.ID.make("altimate-free"), + modelID: ModelV2.ID.make("altimate-base"), + }) + } + }) + + test.each([{}, { apiKey: "public" }])( + "a keyed Zen account outranks registered Altimate Base with options %j", + (options) => { + const zen = provider("opencode", ["nemotron-3-super-free"]) + zen.key = "test-zen-key" + zen.options = options + const result = ACPService.defaultModelFromConfig( + undefined, + providers(zen, provider("altimate-free", ["altimate-base"])), + ) + expect(result?.providerID).toBe(ProviderV2.ID.make("opencode")) + }, + ) + + test("a self-hosted provider with zero-cost metadata still outranks registered Base", () => { + const local = provider("local-llm", ["llama-3"]) + local.options = { apiKey: "public", baseURL: "http://localhost:11434/v1" } + const zen = provider("opencode", ["nemotron-3-super-free"]) + zen.options.apiKey = "public" + const result = ACPService.defaultModelFromConfig( + undefined, + providers(zen, provider("altimate-free", ["altimate-base"]), local), + ) + expect(result?.providerID).toBe(ProviderV2.ID.make("local-llm")) + }) + + test("public Zen stays available without registered Base or when a provider allowlist excludes Base", () => { + const zen = provider("opencode", ["nemotron-3-super-free"]) + zen.options.apiKey = "public" + expect(ACPService.defaultModelFromConfig(undefined, providers(zen))?.providerID).toBe(ProviderV2.ID.make("opencode")) + expect( + ACPService.defaultModelFromConfig( + undefined, + providers(zen, provider("altimate-free", ["altimate-base"])), + { opencode: {} }, + )?.providerID, + ).toBe(ProviderV2.ID.make("opencode")) + }) + + test("an explicitly configured public Zen model still outranks registered Base", () => { + const zen = provider("opencode", ["nemotron-3-super-free"]) + zen.options.apiKey = "public" + const result = ACPService.defaultModelFromConfig( + "opencode/nemotron-3-super-free", + providers(zen, provider("altimate-free", ["altimate-base"])), + ) + expect(result).toEqual({ + providerID: ProviderV2.ID.make("opencode"), + modelID: ModelV2.ID.make("nemotron-3-super-free"), + }) + }) + test("never chooses Big Pickle implicitly", () => { expect( ACPService.defaultModelFromConfig(undefined, providers(provider("opencode", ["big-pickle"]))), @@ -94,7 +258,7 @@ describe("ACP defaultModelFromConfig", () => { ).toBeUndefined() }) - test("does not reintroduce Big Pickle through the ACP snapshot fallback", () => { + test("does not reintroduce Big Pickle through the ACP snapshot fallback", async () => { const snapshot = { directory: "/tmp/acp-default-model-test", providers: {}, @@ -118,7 +282,7 @@ describe("ACP defaultModelFromConfig", () => { availableCommands: [], } satisfies Directory.Snapshot - expect(ACPService.selectDefaultModel(snapshot)).toEqual({ + expect(await ACPService.selectDefaultModel(snapshot)).toEqual({ providerID: ProviderV2.ID.make("openai"), modelID: ModelV2.ID.make("gpt-5"), }) @@ -145,9 +309,9 @@ describe("ACP defaultModelFromConfig", () => { }) test("a connected paid provider outranks registered Altimate Base", () => { - // Base logs requests, so it must never win over something the user actually connected. ACP has - // no recent-model list, so without this ordering a registered user with an Anthropic key would - // silently route every new session to the free logging tier. + // Base logs requests, so absent a configured model or persisted recent, it must never win over + // something the user actually connected. Otherwise a registered user with an Anthropic key + // would silently route every new session to the free logging tier. const result = ACPService.defaultModelFromConfig( undefined, providers(provider("altimate-free", ["altimate-base"]), provider("anthropic", ["claude-sonnet-4"])), @@ -209,7 +373,7 @@ describe("ACP defaultModelFromConfig", () => { }) }) - test("returns no snapshot fallback when Big Pickle is the only option", () => { + test("returns no snapshot fallback when Big Pickle is the only option", async () => { const snapshot = { directory: "/tmp/acp-big-pickle-only", providers: {}, @@ -227,7 +391,7 @@ describe("ACP defaultModelFromConfig", () => { availableCommands: [], } satisfies Directory.Snapshot - expect(ACPService.selectDefaultModel(snapshot)).toBeUndefined() + expect(await ACPService.selectDefaultModel(snapshot)).toBeUndefined() }) }) // altimate_change end diff --git a/packages/opencode/test/acp/service-session.test.ts b/packages/opencode/test/acp/service-session.test.ts index da9de3ed9a..654d8c36d1 100644 --- a/packages/opencode/test/acp/service-session.test.ts +++ b/packages/opencode/test/acp/service-session.test.ts @@ -1,4 +1,7 @@ import { describe, expect, it } from "bun:test" +import fs from "node:fs/promises" +import path from "node:path" +import { Global } from "@/global" import type { AgentSideConnection, ForkSessionResponse, @@ -22,6 +25,7 @@ import * as ACPService from "@/acp/service" import * as ACPError from "@/acp/error" import { UsageService } from "@/acp/usage" import type { Provider } from "@/provider/provider" +import { withTestStateHome } from "../fixture/fixture" const providerID = ProviderV2.ID.make("test") const modelID = ModelV2.ID.make("test-model") @@ -344,6 +348,55 @@ describe("ACP service sessions", () => { expect(creates).toHaveLength(0) }) + it.each([ + { recent: [{ providerID: "opencode", modelID: "nemotron-3-super-free" }] }, + { recent: [], declinedManagedBaseDefault: true }, + ])("re-reads model state for subsequent sessions in a cached directory: %j", async (state) => { + const zen = { + ...provider, + id: ProviderID.make("opencode"), + options: { apiKey: "public" }, + models: { + [ModelID.make("nemotron-3-super-free")]: { + ...provider.models[modelID], + id: ModelID.make("nemotron-3-super-free"), + providerID: ProviderID.make("opencode"), + }, + }, + } satisfies Provider.Info + const base = { + ...provider, + id: ProviderID.make("altimate-free"), + models: { + [ModelID.make("altimate-base")]: { + ...provider.models[modelID], + id: ModelID.make("altimate-base"), + providerID: ProviderID.make("altimate-free"), + }, + }, + } satisfies Provider.Info + // altimate_change — Cursor/cubic review round 5, P2/P3: `Global.Path.state` is not + // test-isolated on its own (unlike `Global.Path.home`), so writing `model.json` through it + // directly touched the real developer state directory and raced other tests doing the same. + // `withTestStateHome` redirects it to a throwaway temp dir for the duration of this test; see + // its declaration in `test/fixture/fixture.ts`. + await withTestStateHome(async () => { + const stateFile = path.join(Global.Path.state, "model.json") + await fs.writeFile(stateFile, JSON.stringify({ recent: [] })) + const { service } = makeService([], { providers: [zen, base] }) + const first = await Effect.runPromise(service.newSession({ cwd: "/workspace", mcpServers: [] })) + expect(select(first, "model")?.currentValue).toBe("altimate-free/altimate-base") + + await fs.writeFile(stateFile, JSON.stringify(state)) + const second = await Effect.runPromise(service.newSession({ cwd: "/workspace", mcpServers: [] })) + expect(select(second, "model")?.currentValue).toBe("opencode/nemotron-3-super-free") + + await fs.writeFile(stateFile, JSON.stringify({ recent: [] })) + const third = await Effect.runPromise(service.newSession({ cwd: "/workspace", mcpServers: [] })) + expect(select(third, "model")?.currentValue).toBe("altimate-free/altimate-base") + }) + }) + it("fails before creating a session when the configured model is unavailable", async () => { const bigPickleProvider = { ...provider, diff --git a/packages/opencode/test/fixture/fixture.ts b/packages/opencode/test/fixture/fixture.ts index 0ed16587ef..85a058aed1 100644 --- a/packages/opencode/test/fixture/fixture.ts +++ b/packages/opencode/test/fixture/fixture.ts @@ -289,3 +289,30 @@ export function provideTmpdirServer( }) }) } + +/** + * Isolates `Global.Path.state` (where `model.json` — recent-model and migration-decline + * persistence — lives) to a throwaway temp directory for the duration of `fn`, via the + * `OPENCODE_TEST_STATE_HOME` env var `Global.Path.state` reads (see `src/global/index.ts`). + * + * Without this, a test reading/writing `model.json` through `Global.Path.state` directly + * touches the REAL, current developer's state directory: unlike `Global.Path.home`, `state` had + * no test-isolation override, so those tests raced every other test file doing the same thing in + * parallel and could clobber real state if a run was killed mid-write (between deleting the real + * file and restoring it from a saved snapshot). + */ +export async function withTestStateHome(fn: () => Promise): Promise { + const dirpath = sanitizePath( + path.join(os.tmpdir(), "opencode-test-state-" + Math.random().toString(36).slice(2)), + ) + await fs.mkdir(dirpath, { recursive: true }) + const original = process.env.OPENCODE_TEST_STATE_HOME + process.env.OPENCODE_TEST_STATE_HOME = dirpath + try { + return await fn() + } finally { + if (original === undefined) delete process.env.OPENCODE_TEST_STATE_HOME + else process.env.OPENCODE_TEST_STATE_HOME = original + await clean(dirpath).catch(() => undefined) + } +} diff --git a/packages/opencode/test/provider/provider.test.ts b/packages/opencode/test/provider/provider.test.ts index 96bc76b6f1..be1c93a731 100644 --- a/packages/opencode/test/provider/provider.test.ts +++ b/packages/opencode/test/provider/provider.test.ts @@ -3,7 +3,7 @@ import path from "path" import fs from "fs/promises" import { generateText } from "ai" -import { tmpdir } from "../fixture/fixture" +import { tmpdir, withTestStateHome } from "../fixture/fixture" import { Instance } from "../../src/project/instance" import { ProjectID } from "../../src/project/schema" import { Provider } from "../../src/provider/provider" @@ -189,7 +189,8 @@ test("an Altimate Base-only provider block cannot select an unrelated provider", } }) -test("a connected provider outranks registered Altimate Base as the implicit default", async () => { +// altimate_change start — registered Base outranks only public Zen, preserving connected and recent choices +test.each([false, true])("a keyed Zen provider outranks registered Base with public marker %s", async (publicMarker) => { const credentials = spyOn(FreeTier, "credentialsForLoad").mockResolvedValue({ apiKey: "sk-altimate-base", baseURL: ALTIMATE_BASE_GATEWAY_URL, @@ -199,9 +200,15 @@ test("a connected provider outranks registered Altimate Base as the implicit def await using tmp = await tmpdir({ config: { provider: {} } }) await provideProviderTestInstance({ directory: tmp.path, + init: async () => Env.set("OPENCODE_API_KEY", "test-zen-key"), fn: async () => { - // Altimate Base logs requests, so it is only ever the LAST resort. Anything the user has - // actually connected wins, and `provider: {}` still does not act as an allowlist. + // A keyed Zen account outranks Base, and `provider: {}` is not an allowlist. + const providers = await Provider.list() + expect(providers.opencode.key).toBe("test-zen-key") + expect(providers.opencode.options.apiKey).not.toBe("public") + expect(providers[FreeTier.PROVIDER_ID]).toBeDefined() + // A retained public marker must not override the key on the loaded provider. + if (publicMarker) providers.opencode.options.apiKey = "public" const model = await Provider.defaultModel() expect(model).not.toEqual({ providerID: ProviderID.make(FreeTier.PROVIDER_ID), @@ -215,56 +222,172 @@ test("a connected provider outranks registered Altimate Base as the implicit def } }) -test("a persisted Big Pickle default is not silently migrated headlessly", async () => { +test.each([ + { flag: true, providerID: "opencode", modelID: "gpt-5-nano" }, + { flag: false, providerID: FreeTier.PROVIDER_ID, modelID: FreeTier.MODEL_ID }, + { flag: undefined, providerID: FreeTier.PROVIDER_ID, modelID: FreeTier.MODEL_ID }, + { flag: "yes", providerID: FreeTier.PROVIDER_ID, modelID: FreeTier.MODEL_ID }, +])("public Zen versus registered Base with persisted decline flag $flag", async ({ flag, providerID, modelID }) => { const credentials = spyOn(FreeTier, "credentialsForLoad").mockResolvedValue({ apiKey: "sk-altimate-base", baseURL: ALTIMATE_BASE_GATEWAY_URL, installSecret: "install-secret", }) - const stateFile = path.join(Global.Path.state, "model.json") - const previous = await fs.readFile(stateFile, "utf8").catch(() => undefined) + // altimate_change — Cursor/cubic review round 5, P2/P3: `Global.Path.state` is not + // test-isolated on its own (unlike `Global.Path.home`), so writing `model.json` through it + // directly touched the real developer state directory and raced other tests doing the same. + // `withTestStateHome` redirects it to a throwaway temp dir (already `mkdir`'d) for the + // duration of this test; see its declaration in `test/fixture/fixture.ts`. try { - await fs.mkdir(Global.Path.state, { recursive: true }) - await fs.writeFile(stateFile, JSON.stringify({ recent: [{ providerID: "opencode", modelID: "big-pickle" }] })) - await using tmp = await tmpdir({ config: { provider: {} } }) - await provideProviderTestInstance({ - directory: tmp.path, - fn: async () => { - // The TUI owns the migration because it owns the disclosure; rewriting the recent pick - // here would move a user who declined onto the request-logging tier with no prompt. - expect(await Provider.defaultModel()).toEqual({ - providerID: ProviderID.make("opencode"), - modelID: ModelID.make("big-pickle"), + await withTestStateHome(async () => { + const stateFile = path.join(Global.Path.state, "model.json") + await fs.writeFile(stateFile, JSON.stringify({ recent: [], declinedManagedBaseDefault: flag })) + await using tmp = await tmpdir({ + config: { provider: {}, enabled_providers: ["opencode", FreeTier.PROVIDER_ID] }, + }) + await provideProviderTestInstance({ + directory: tmp.path, + init: async () => Env.remove("OPENCODE_API_KEY"), + fn: async () => { + const providers = await Provider.list() + expect(Object.keys(providers).sort()).toEqual([FreeTier.PROVIDER_ID, "opencode"]) + expect(providers.opencode.options.apiKey).toBe("public") + expect(providers.opencode.key).toBeUndefined() + expect(providers.opencode.models["nemotron-3-super-free"]).toBeDefined() + expect(await Provider.defaultModel()).toEqual({ + providerID: ProviderID.make(providerID), + modelID: ModelID.make(modelID), + }) + }, + }) + }) + } finally { + credentials.mockRestore() + } +}) + +test("a persisted public Zen recent outranks registered Altimate Base", async () => { + const credentials = spyOn(FreeTier, "credentialsForLoad").mockResolvedValue({ + apiKey: "sk-altimate-base", + baseURL: ALTIMATE_BASE_GATEWAY_URL, + installSecret: "install-secret", + }) + // altimate_change — Cursor/cubic review round 5, P2/P3: see `withTestStateHome`'s declaration + // in `test/fixture/fixture.ts` — isolates `Global.Path.state` to a throwaway temp dir. + try { + await withTestStateHome(async () => { + const stateFile = path.join(Global.Path.state, "model.json") + await fs.writeFile( + stateFile, + JSON.stringify({ recent: [{ providerID: "opencode", modelID: "nemotron-3-super-free" }] }), + ) + await using tmp = await tmpdir({ config: { provider: {} } }) + await provideProviderTestInstance({ + directory: tmp.path, + init: async () => Env.remove("OPENCODE_API_KEY"), + fn: async () => { + const providers = await Provider.list() + expect(providers.opencode.options.apiKey).toBe("public") + expect(providers.opencode.key).toBeUndefined() + expect(providers[FreeTier.PROVIDER_ID]).toBeDefined() + expect(await Provider.defaultModel()).toEqual({ + providerID: ProviderID.make("opencode"), + modelID: ModelID.make("nemotron-3-super-free"), + }) + }, + }) + }) + } finally { + credentials.mockRestore() + } +}) +// altimate_change end + +test.each(["__proto__/x", "constructor/x", "opencode/__proto__", "opencode/constructor"])( + "ignores prototype-name recent %s and resolves the registered Base fallback", + async (recent) => { + const credentials = spyOn(FreeTier, "credentialsForLoad").mockResolvedValue({ + apiKey: "sk-altimate-base", + baseURL: ALTIMATE_BASE_GATEWAY_URL, + installSecret: "install-secret", + }) + // altimate_change — Cursor/cubic review round 5, P2/P3: see `withTestStateHome`'s + // declaration in `test/fixture/fixture.ts` — isolates `Global.Path.state` to a throwaway + // temp dir. + try { + await withTestStateHome(async () => { + const stateFile = path.join(Global.Path.state, "model.json") + await fs.writeFile(stateFile, JSON.stringify({ recent: [Provider.parseModel(recent)] })) + await using tmp = await tmpdir({ + config: { provider: {}, enabled_providers: ["opencode", FreeTier.PROVIDER_ID] }, }) - }, + await provideProviderTestInstance({ + directory: tmp.path, + init: async () => Env.remove("OPENCODE_API_KEY"), + fn: async () => { + expect(await Provider.defaultModel()).toEqual({ + providerID: ProviderID.make(FreeTier.PROVIDER_ID), + modelID: ModelID.make(FreeTier.MODEL_ID), + }) + }, + }) + }) + } finally { + credentials.mockRestore() + } + }, +) + +test("a persisted Big Pickle default is not silently migrated headlessly", async () => { + const credentials = spyOn(FreeTier, "credentialsForLoad").mockResolvedValue({ + apiKey: "sk-altimate-base", + baseURL: ALTIMATE_BASE_GATEWAY_URL, + installSecret: "install-secret", + }) + // altimate_change — Cursor/cubic review round 5, P2/P3: see `withTestStateHome`'s declaration + // in `test/fixture/fixture.ts` — isolates `Global.Path.state` to a throwaway temp dir. + try { + await withTestStateHome(async () => { + const stateFile = path.join(Global.Path.state, "model.json") + await fs.writeFile(stateFile, JSON.stringify({ recent: [{ providerID: "opencode", modelID: "big-pickle" }] })) + await using tmp = await tmpdir({ config: { provider: {} } }) + await provideProviderTestInstance({ + directory: tmp.path, + fn: async () => { + // The TUI owns the migration because it owns the disclosure; rewriting the recent pick + // here would move a user who declined onto the request-logging tier with no prompt. + expect(await Provider.defaultModel()).toEqual({ + providerID: ProviderID.make("opencode"), + modelID: ModelID.make("big-pickle"), + }) + }, + }) }) } finally { - if (previous === undefined) await fs.rm(stateFile, { force: true }) - else await fs.writeFile(stateFile, previous) credentials.mockRestore() } }) test("a persisted Big Pickle default remains until Altimate Base consent exists", async () => { const credentials = spyOn(FreeTier, "credentialsForLoad").mockResolvedValue(undefined) - const stateFile = path.join(Global.Path.state, "model.json") - const previous = await fs.readFile(stateFile, "utf8").catch(() => undefined) + // altimate_change — Cursor/cubic review round 5, P2/P3: see `withTestStateHome`'s declaration + // in `test/fixture/fixture.ts` — isolates `Global.Path.state` to a throwaway temp dir. try { - await fs.mkdir(Global.Path.state, { recursive: true }) - await fs.writeFile(stateFile, JSON.stringify({ recent: [{ providerID: "opencode", modelID: "big-pickle" }] })) - await using tmp = await tmpdir({ config: { provider: {} } }) - await provideProviderTestInstance({ - directory: tmp.path, - fn: async () => { - expect(await Provider.defaultModel()).toEqual({ - providerID: ProviderID.make("opencode"), - modelID: ModelID.make("big-pickle"), - }) - }, + await withTestStateHome(async () => { + const stateFile = path.join(Global.Path.state, "model.json") + await fs.writeFile(stateFile, JSON.stringify({ recent: [{ providerID: "opencode", modelID: "big-pickle" }] })) + await using tmp = await tmpdir({ config: { provider: {} } }) + await provideProviderTestInstance({ + directory: tmp.path, + fn: async () => { + expect(await Provider.defaultModel()).toEqual({ + providerID: ProviderID.make("opencode"), + modelID: ModelID.make("big-pickle"), + }) + }, + }) }) } finally { - if (previous === undefined) await fs.rm(stateFile, { force: true }) - else await fs.writeFile(stateFile, previous) credentials.mockRestore() } }) @@ -288,27 +411,27 @@ test("a provider allowlist filters a persisted Altimate Base recent before impli baseURL: ALTIMATE_BASE_GATEWAY_URL, installSecret: "install-secret", }) - const stateFile = path.join(Global.Path.state, "model.json") - const previous = await fs.readFile(stateFile, "utf8").catch(() => undefined) + // altimate_change — Cursor/cubic review round 5, P2/P3: see `withTestStateHome`'s declaration + // in `test/fixture/fixture.ts` — isolates `Global.Path.state` to a throwaway temp dir. try { - await fs.mkdir(Global.Path.state, { recursive: true }) - await fs.writeFile( - stateFile, - JSON.stringify({ recent: [{ providerID: FreeTier.PROVIDER_ID, modelID: FreeTier.MODEL_ID }] }), - ) - await using tmp = await tmpdir({ config: { provider: { anthropic: {} } } }) - await provideProviderTestInstance({ - directory: tmp.path, - init: async () => Env.set("ANTHROPIC_API_KEY", "test-api-key"), - fn: async () => { - const model = await Provider.defaultModel() - expect(String(model.providerID)).toBe("anthropic") - expect(String(model.modelID)).not.toBe(FreeTier.MODEL_ID) - }, + await withTestStateHome(async () => { + const stateFile = path.join(Global.Path.state, "model.json") + await fs.writeFile( + stateFile, + JSON.stringify({ recent: [{ providerID: FreeTier.PROVIDER_ID, modelID: FreeTier.MODEL_ID }] }), + ) + await using tmp = await tmpdir({ config: { provider: { anthropic: {} } } }) + await provideProviderTestInstance({ + directory: tmp.path, + init: async () => Env.set("ANTHROPIC_API_KEY", "test-api-key"), + fn: async () => { + const model = await Provider.defaultModel() + expect(String(model.providerID)).toBe("anthropic") + expect(String(model.modelID)).not.toBe(FreeTier.MODEL_ID) + }, + }) }) } finally { - if (previous === undefined) await fs.rm(stateFile, { force: true }) - else await fs.writeFile(stateFile, previous) credentials.mockRestore() } }) diff --git a/packages/tui/src/app.tsx b/packages/tui/src/app.tsx index 3bb029b426..f445d0a926 100644 --- a/packages/tui/src/app.tsx +++ b/packages/tui/src/app.tsx @@ -45,6 +45,7 @@ import { useSetupComplete, markFirstRunActive, resetSetupComplete, + useFirstRunOpenedThisLaunch, } from "./component/altimate-onboarding" // altimate_change end // altimate_change — Part 2 scan gate (fires once when Part 1 first completes) @@ -58,7 +59,9 @@ import { SDKProvider, useSDK } from "./context/sdk" import { StartupLoading } from "./component/startup-loading" import { SyncProvider, useSync } from "./context/sync" import { DataProvider } from "./context/data" -import { LocalProvider, useLocal } from "./context/local" +// altimate_change — fixes #1301 (Codex review, P2): `ALTIMATE_BASE_MIGRATION_DECLINED_KEY` moved +// to local.tsx so `local.model.hasUsableFreeDefault()` can read the same kv key. +import { LocalProvider, useLocal, ALTIMATE_BASE_MIGRATION_DECLINED_KEY, shouldSkipOnboardingAtStartup, shouldFireFirstRunFunnelAtStartup } from "./context/local" import { DialogModel } from "./component/dialog-model" import { useConnected } from "./component/use-connected" import { DialogMcp } from "./component/dialog-mcp" @@ -74,7 +77,9 @@ import { DialogConsoleOrg } from "./component/dialog-console-org" import { ThemeProvider, useTheme } from "./context/theme" import { Home } from "./routes/home" import { Session } from "./routes/session" -import { PromptHistoryProvider } from "./component/prompt/history" +// altimate_change — fixes #1301: `usePromptHistory` also carries a "returning user" signal +// (`hadHistoryAtStartup`) that the startup migration decision below consults. +import { PromptHistoryProvider, usePromptHistory } from "./component/prompt/history" import { FrecencyProvider } from "./component/prompt/frecency" import { PromptStashProvider } from "./component/prompt/stash" import { DialogAlert } from "./ui/dialog-alert" @@ -115,10 +120,6 @@ import { cliErrorMessage, errorFormat } from "./util/error" import { detectModeFromCOLORFGBG } from "./terminal-detection" // altimate_change end -// altimate_change start — remember an explicit migration decline without suppressing later manual setup -const ALTIMATE_BASE_MIGRATION_DECLINED_KEY = "altimate_base_big_pickle_migration_declined_v1" -// altimate_change end - const appGlobalBindingCommands = [ "session.list", "session.new", @@ -436,6 +437,10 @@ function App(props: { onSnapshot?: () => Promise; pluginHost: TuiPlugi const exit = useExit() const promptRef = usePromptRef() const pluginRuntime = usePluginRuntime() + // altimate_change start — fixes #1301: "returning user" signal for the startup migration + // decision below; see prompt/history.tsx. + const promptHistory = usePromptHistory() + // altimate_change end const attention = createTuiAttention({ renderer, config: tuiConfig, kv }) const clipboard = useClipboard() @@ -603,116 +608,220 @@ function App(props: { onSnapshot?: () => Promise; pluginHost: TuiPlugi const onboardingReady = useReady() // altimate_change — setup completion alone (no `connected()` term); see the scan-gate effect const setupComplete = useSetupComplete() + // altimate_change — cubic review (3986532221): see `firstRunOpenedThisLaunch`'s declaration in + // altimate-onboarding.tsx + const firstRunOpenedThisLaunch = useFirstRunOpenedThisLaunch() // altimate_change — onboarding funnel tracker (no-op when the host injected none) const trackOnboarding = useOnboardingTelemetry() // altimate_change end - // altimate_change start — move the retired Big Pickle default to Altimate Base - // Already-registered users migrate immediately. Everyone else sees the existing logging - // disclosure first; an explicit No is remembered and leaves their model untouched. - let legacyModelMigrationHandled = false + // altimate_change start — fixes #1301: move the retired Big Pickle default — and more broadly + // any implicit free public Zen default — to Altimate Base. Already-registered users migrate + // immediately. Returning users who are not yet registered see the existing logging disclosure + // first; an explicit No is remembered and leaves their model untouched. A brand-new user (no + // history anywhere) falls straight through to the ordinary first-run picker below: the + // migration disclosure reads as "your existing default changed," which is meaningless on a + // first launch. + // + // SINGLE DECISION: this used to be two independent `createEffect`s — one deciding migration, + // one deciding the first-run picker — each guarded only by its own one-shot latch. That missed + // every path where migration exits WITHOUT showing a dialog (silent migration, a prior + // decline, no consent operation available), and separately a `dialog.replace()` that loses a + // race to another dialog and returns `false`: latching "handled" before checking the replace + // result would suppress the first-run picker without migration ever actually being shown. + // Merging both into one decision, made once, closes both gaps: there is exactly one launch-time + // verdict — migrate silently, show the migration disclosure, or fall through to today's + // first-run logic — and only ONE of those branches is allowed to latch "handled". + let startupDecisionHandled = false + // Armed only when THIS launch starts genuinely un-onboarded (so the Part 2 scan + // gate below fires after the user completes first-run setup — not for a returning + // user whose onboardingReady merely flips false→true once sync loads providers). + let armScanGate = false createEffect(() => { - if (legacyModelMigrationHandled) return - if (!ready() || sync.status !== "complete" || !local.model.ready) return - if (!local.model.usesLegacyDefault()) { - legacyModelMigrationHandled = true - return - } + if (startupDecisionHandled) return + // Decide only once the plugin host has started, sync has finished loading providers, the + // persisted model selection has loaded, AND prompt history has loaded. `ready()` alone is + // plugin-host startup, which can settle before sync populates `sync.data.provider` — + // deciding then would transiently see a returning (connected) user as un-onboarded and + // re-show the picker + scan gate (see the regression this effect guards against, above). + // `sync.status` is the provider-load signal (same one used for continue/fork above). + // `local.model.ready` guards a parallel race: `model.json`'s read is async, and if provider + // sync finishes first, the legacy/returning checks below would see an empty recent list and + // misclassify a returning user as fresh. `promptHistory.loaded()` guards the same race for + // the "returning user" signal immediately below. `kv.ready` guards the SAME race for the + // decline check immediately below (PR #1302 review, P1): without it, a launch where kv.json's + // read is still in flight sees an actually-declined user as un-declined and silently migrates + // to Base before kv hydration can ever re-run this effect. + if (!ready() || sync.status !== "complete" || !local.model.ready || !promptHistory.loaded() || !kv.ready) return - // A previous decline is checked FIRST, before registration state. Registering Altimate Base - // for one task is not consent to move a Big Pickle default that the user already refused to - // move; without this the decline is silently overridden on every later launch. - if (kv.get(ALTIMATE_BASE_MIGRATION_DECLINED_KEY, false)) { - legacyModelMigrationHandled = true - return - } + // altimate_change — fixes #1301: a user is "returning" if there is any sign of prior use + // anywhere this TUI persists it: prompt history (independent of the current project's + // session list, and not windowed to the last 30 days the way session sync is), the current + // project's own session list, or a picker-written recent model. `hadHistoryAtStartup()` is a + // one-time snapshot — a prompt sent during THIS launch must not retroactively make the launch + // look like a return visit. + const returning = + promptHistory.hadHistoryAtStartup() || sync.data.session.length > 0 || local.model.recent().length > 0 - const altimateBaseAvailable = sync.data.provider.some( - (provider) => provider.id === "altimate-free" && Boolean(provider.models?.["altimate-base"]), - ) - if (altimateBaseAvailable) { - legacyModelMigrationHandled = true - local.model.migrateLegacyDefault() - return - } + // ---- Migration ---- + // A previous decline is checked FIRST, before registration state or eligibility. Registering + // Altimate Base for one task is not consent to move a free default that the user already + // refused to move; without this the decline is silently overridden on every later launch. + // altimate_change — PR #1302 review, P1 (Cursor + cubic): also honor the model.json flag, not + // only the kv key. `hasUsableFreeDefault()` already ORs both; this gate must too, or a launch + // where only the model.json flag is set (kv unread, or written from a different code path) + // can migrate right past a refusal that's actually on record. + const previouslyDeclined = + kv.get(ALTIMATE_BASE_MIGRATION_DECLINED_KEY, false) || local.model.declinedManagedBaseDefault() + if (!previouslyDeclined && local.model.usesLegacyDefault()) { + const altimateBaseAvailable = sync.data.provider.some( + (provider) => provider.id === "altimate-free" && Boolean(provider.models?.["altimate-base"]), + ) + // altimate_change — fixes #1301 (Codex review round 2, P1): an older picker-written Zen + // recent (predating the `explicitDefault` marker) is still the user's own past pick, not a + // truly implicit default — silently sweeping it into Base when registered skips the + // disclosure entirely. Big Pickle keeps today's behavior (always silent when registered); + // see `hasOwnPickOfImplicitDefault`'s declaration in local.tsx. + if (altimateBaseAvailable && !local.model.hasOwnPickOfImplicitDefault()) { + startupDecisionHandled = true + local.model.migrateLegacyDefault() + return + } - // altimate_change — the registration operation lives in its own dedicated context now, not on - // `sdk`; see context/altimate-base-consent.tsx. - if (!altimateBaseConsent) { - legacyModelMigrationHandled = true - return + // altimate_change — the registration operation lives in its own dedicated context now, not + // on `sdk`; see context/altimate-base-consent.tsx. A brand-new (non-returning) user never + // sees this disclosure — see the block comment above. + if (altimateBaseConsent && returning) { + const shown = dialog.replace(() => ( + { + kv.set(ALTIMATE_BASE_MIGRATION_DECLINED_KEY, true) + // altimate_change — fixes #1301 (Codex review, P1): the kv key alone is invisible + // to headless/server default selection (`Provider.defaultModel()`, ACP). Persist + // the same refusal into `model.json`, which the server already reads, so a decline + // made in the TUI is honored there too. + local.model.declineManagedBaseDefault() + }} + /> + )) + if (shown) { + startupDecisionHandled = true + return + } + // `dialog.replace` lost a race to another dialog and returned false without opening + // anything — fall through to first-run logic below instead of latching "handled" on a + // dialog nobody actually saw. + } + // Not registered, no consent operation available, or a brand-new user: no migration + // dialog this launch. Fall through to the ordinary first-run logic below. } - legacyModelMigrationHandled = true - dialog.replace(() => ( - kv.set(ALTIMATE_BASE_MIGRATION_DECLINED_KEY, true)} - /> - )) - }) - // altimate_change end - - // altimate_change start — AI-7774: first-run onboarding gate. On a fresh launch - // with no usable model, open the curated provider picker as the entry point (chat - // input stays visible; submit is gated in the prompt until setup completes). Fire - // EXACTLY once, and only after startup has settled (`ready()` = plugin host + - // sync bootstrap done), so a returning user with valid credentials never sees it. - let firstRunPickerHandled = false - // Armed only when THIS launch starts genuinely un-onboarded (so the Part 2 scan - // gate below fires after the user completes first-run setup — not for a returning - // user whose onboardingReady merely flips false→true once sync loads providers). - let armScanGate = false - createEffect(() => { - if (firstRunPickerHandled) return - // Decide only once the plugin host has started, sync has finished loading providers, AND the - // persisted model selection has loaded. `ready()` alone is plugin-host startup, which can - // settle before sync populates `sync.data.provider` — deciding then would transiently see a - // returning (connected) user as un-onboarded and re-show the picker + scan gate (see the - // regression this effect guards against, above). `sync.status` is the provider-load signal - // (same one used for continue/fork above). `local.model.ready` guards the same race the - // migration effect above already does: `model.json`'s read is async, and if provider sync - // finishes first, `hasExistingLegacySelection` below would see an empty recent list and - // misclassify a returning Big Pickle user as fresh. - if (!ready() || sync.status !== "complete" || !local.model.ready) return - // A Big Pickle selection proves this is an existing user, even though that zero-cost - // provider does not satisfy useConnected(). The migration effect above owns any consent - // prompt; never overwrite it with the first-run picker. - if (local.model.hasExistingLegacySelection()) { - firstRunPickerHandled = true + // ---- First-run onboarding gate ---- + // On a fresh launch with no usable model, open the curated provider picker as the entry + // point (chat input stays visible; submit is gated in the prompt until setup completes). + // A Big Pickle (or other legacy implicit) selection proves this is an existing user, even + // though that zero-cost provider does not satisfy useConnected(). The migration branch above + // owns any consent prompt for that case; never overwrite it with the first-run picker. + // altimate_change — fixes #1301 (Codex review, P2): `hasUsableFreeDefault()` covers the + // broader case — a free Zen model the user explicitly picked, or already declined migrating + // away from — the same way `hasExistingLegacySelection()` always covered Big Pickle. It also + // folds in `hasOwnPickOfImplicitDefault()` (an older picker-written free-Zen recent with no + // `explicitDefault` marker and no consent operation available) directly now — Codex review + // round 2, P2/P3: that case needs to be recognized everywhere `hasUsableFreeDefault()` is + // (in particular `useReady()`/the prompt gate), not only here at startup, or the SAME user + // hits the picker again on their next submit and loses whatever they typed. See + // `hasUsableFreeDefault`'s declaration in local.tsx for where the fold now lives. + // altimate_change start — Kilo review round 6 (3986171188): `shouldSkipOnboardingAtStartup` + // — see its declaration in local.tsx — added a discriminator for "did first-run genuinely + // complete THIS launch". Without it, an impatient first-run user who submits before this + // effect settles — the prompt gate (component/prompt/index.tsx) opens the picker on its own, + // they pick a free Zen model, `set()` marks it explicit/recent and `markSetupComplete()` runs + // — made `hasUsableFreeDefault()` true by the time THIS effect finally runs, latching here + // and returning before the `onboardingReady()` branch below (which exists for exactly this + // impatient-user case, per its own comment) ever got a chance to fire the funnel telemetry + // and `openScanGate()`. + // + // cubic review (3986532221): a bare `setupComplete()` is NOT that discriminator — it is a + // global flag `markSetupComplete()` sets for ANY model selection, first-run or not, so a + // RETURNING user (`hasExistingLegacySelection()` or `hasUsableFreeDefault()` already true) + // who does an ordinary `/model` switch while this effect is still settling ALSO makes + // `setupComplete()` true — which used to fall through to the `onboardingReady()` branch and + // fire onboarding telemetry + open the scan gate for a routine model change, not a first run. + // `firstRunOpenedThisLaunch()` (see its declaration in altimate-onboarding.tsx) is the fix: a + // one-way latch set only when the first-run picker itself actually opened THIS launch (this + // effect's own fallthrough below, or the prompt gate's equivalent) — `setupComplete() && + // firstRunOpenedThisLaunch()` is true only for a GENUINE first-run completion. A returning + // user's routine switch never sets `firstRunOpenedThisLaunch()`, so it correctly reads false + // and this branch keeps skipping for them, exactly as before this whole discriminator existed. + // A paid pick is unaffected either way: `hasUsableFreeDefault()` requires a free model, so it + // was never true for one. + if ( + shouldSkipOnboardingAtStartup( + local.model.hasExistingLegacySelection(), + // `=== true`: `hasUsableFreeDefault()` can also return `"pending"` (see its declaration + // and `hasUsableFreeDefaultGated` in local.tsx) — but this whole effect already returned + // early above unless `kv.ready`, so it is always a plain boolean by the time it runs + // here; the explicit check just satisfies the union type without widening it elsewhere. + local.model.hasUsableFreeDefault() === true, + setupComplete() && firstRunOpenedThisLaunch(), + ) + ) { + startupDecisionHandled = true return } - firstRunPickerHandled = true + // altimate_change end if (onboardingReady()) { // Not necessarily a returning user. The prompt gate (component/prompt/index.tsx) opens the // same picker as soon as the user tries to submit, which can happen BEFORE sync finishes // hydrating — and completing setup there makes onboardingReady() true by the time this // effect finally runs. Bailing out then skipped the funnel and the scan gate entirely for - // exactly the impatient-user case. setupComplete() is the discriminator: it starts false - // every launch and is only set by a setup the user completed during THIS one, so a genuine - // returning user never trips this branch. - if (setupComplete()) { - // Deliberately NOT markFirstRunActive(): its only clear is markSetupComplete(), which has - // already run on this branch and will not run again, so setting it here would latch the - // flag true for the rest of the session and make every later /model switch emit funnel - // events. The three events below are emitted directly and do not consult it. The prompt - // gate arms it instead, at the point the picker actually opens. + // exactly the impatient-user case. + startupDecisionHandled = true + // altimate_change — Codex re-review round 9: `setupComplete()` alone is NOT a safe + // discriminator here either — same class of bug as the `shouldSkipOnboardingAtStartup` + // branch above (cubic 3986532221), just reached from a different starting condition. A + // RETURNING user with a configured legacy default (e.g. Big Pickle) whose OWN skip + // predicates (`hasExistingLegacySelection()`/`hasUsableFreeDefault()`) are both false — + // because they switched `/model` to a PAID model, which neither predicate covers — still + // reaches `onboardingReady() === true` via `connected()`. If that `/model` switch raced + // this same startup effect, `setupComplete()` is true too, even though no first-run picker + // ever opened this launch. `firstRunOpenedThisLaunch()` (its declaration in + // altimate-onboarding.tsx) is required alongside it, same as the branch above: true only + // when the first-run picker itself actually opened THIS launch (this effect's own + // fallthrough below, or the prompt gate's equivalent). + if (shouldFireFirstRunFunnelAtStartup(setupComplete(), firstRunOpenedThisLaunch())) { + // Deliberately NOT markFirstRunActive() again here: it was already latched at the point + // the picker opened (this branch only reaches telemetry when `firstRunOpenedThisLaunch()` + // is already true), and `markSetupComplete()` has already run on this branch and will not + // run again — re-marking here would keep `firstRunActive` (the OTHER, resettable signal) + // true for the rest of the session and make every later `/model` switch emit funnel + // events. The three events below are emitted directly and do not consult it. scanGateShown = true trackOnboarding({ name: "onboarding_started" }) trackOnboarding({ name: "onboarding_completed" }) trackOnboarding({ name: "scan_gate_shown" }) openScanGate() } + // altimate_change end return } + // altimate_change start — fixes #1301 (Codex review, P2): latch (and arm the scan gate) only + // AFTER a successful replacement, not before. `dialog.replace()` can lose a race to another + // dialog and return `false` without opening anything; latching first left the decision + // "handled" and the scan gate armed for a picker nobody ever saw. Telemetry and + // `markFirstRunActive()` move with it — emitting "the first-run flow started" for a picker + // that never opened would be equally wrong. + const shown = dialog.replace(() => ) + if (!shown) return armScanGate = true markFirstRunActive() // altimate_change — funnel: top of the first-run flow. Emitted only on the branch that // actually opens the gate, so returning users never enter the funnel. trackOnboarding({ name: "onboarding_started" }) - dialog.replace(() => ) + startupDecisionHandled = true + // altimate_change end }) - // altimate_change end // altimate_change start — Part 2 scan gate: fire EXACTLY once, when the user has actually // finished picking a model during a first run. diff --git a/packages/tui/src/component/altimate-onboarding.tsx b/packages/tui/src/component/altimate-onboarding.tsx index ca23d24a58..c7c055afd2 100644 --- a/packages/tui/src/component/altimate-onboarding.tsx +++ b/packages/tui/src/component/altimate-onboarding.tsx @@ -4,7 +4,7 @@ // Base disclosure. Imports back into dialog-model are runtime-only (used inside // callbacks/JSX), so the circular reference is safe. import { createEffect, createMemo, createSignal, For, Show, onMount, onCleanup } from "solid-js" -import { useLocal } from "../context/local" +import { useLocal, isLegacyBigPickleModel } from "../context/local" import { useDialog } from "../ui/dialog" import { useTheme, selectedForeground } from "../context/theme" import { TextAttributes, RGBA } from "@opentui/core" @@ -35,8 +35,33 @@ const [setupComplete, setSetupComplete] = createSignal(false) // model switching, so it consults this before emitting any funnel event — otherwise every model // change for the life of the product would look like an onboarding provider choice. const [firstRunActive, setFirstRunActive] = createSignal(false) +// altimate_change start — cubic review (3986532221): app.tsx's startup effect used +// `setupComplete()` alone to decide whether THIS launch's model selection is a genuine +// first-run/impatient-picker completion (worth firing onboarding telemetry + the scan gate for) +// versus a RETURNING user's ordinary `/model` switch that merely raced the startup effect — +// `markSetupComplete()` fires for BOTH cases identically. `firstRunActive` above cannot answer +// this either: `markSetupComplete()` deliberately CLEARS it (so a later routine switch doesn't +// look like onboarding), so by the time app.tsx's effect gets around to checking it, it has +// already been reset to `false` for both a genuine first-run AND the very completion that would +// prove it happened. `firstRunOpenedThisLaunch` is a separate, ONE-WAY latch: set whenever the +// first-run picker actually opens THIS launch — either app.tsx's own startup fallthrough, or the +// prompt gate's equivalent for an impatient submit before that effect settles (see +// `component/prompt/index.tsx`'s `markFirstRunActive()` call) — and never cleared by +// `markSetupComplete()`/`clearFirstRunActive()` (only by `resetSetupComplete()`, on `/logout`, +// which returns the user to a genuinely fresh state). `setupComplete() && +// firstRunOpenedThisLaunch()` is the correct "did first-run genuinely complete this launch" +// signal; a returning user's routine mid-race `/model` switch has `setupComplete() === true` but +// `firstRunOpenedThisLaunch() === false`, so it reads as `false` and no longer fires anything. +const [firstRunOpenedThisLaunch, setFirstRunOpenedThisLaunch] = createSignal(false) +export function useFirstRunOpenedThisLaunch() { + return firstRunOpenedThisLaunch +} +// altimate_change end export function markFirstRunActive() { setFirstRunActive(true) + // altimate_change — see `firstRunOpenedThisLaunch`'s declaration above + setFirstRunOpenedThisLaunch(true) + // altimate_change end } /** * Clear without marking setup complete. @@ -65,12 +90,49 @@ export function markSetupComplete() { export function resetSetupComplete() { setSetupComplete(false) setFirstRunActive(false) + // altimate_change — see `firstRunOpenedThisLaunch`'s declaration above: /logout returns the + // user to a genuinely fresh state, so a first run after it must be free to latch again. + setFirstRunOpenedThisLaunch(false) + // altimate_change end } export function useReady() { const connected = useConnected() - return createMemo(() => connected() || setupComplete()) + // altimate_change start — fixes #1301 (Codex review, P2): a free public Zen model the user + // either chose on purpose or already declined migrating away from is a legitimate way to use + // the product, not "un-onboarded." Without this term, a returning free-default user who is + // explicit or already said No gets treated as not-ready on every relaunch — the first-run + // welcome picker reopens (see the first-run effect in app.tsx) and prompt submission itself + // reopens the picker and discards whatever was typed (see `useReady()`'s callers in + // component/prompt/index.tsx). `LocalProvider` wraps the whole app above `DialogProvider` (see + // app.tsx), so `useLocal()` is always available to every caller of `useReady()`. + const local = useLocal() + // altimate_change — Codex HOLD finding 1: `hasUsableFreeDefault()` can now return `"pending"` + // (kv not hydrated yet, see its declaration in local.tsx) as well as a boolean. Every consumer + // of `useReady()` EXCEPT the prompt submit gate only needs a plain boolean (display text, + // whether a command is enabled, the first-run chat lock) — `"pending"` collapses to `false` for + // all of them, the same conservative default this code had before kv.ready-awareness existed. + // `useReadyPending()` below is the ONE seam the submit gate uses to see the pending state + // itself, so it can defer instead of discarding. + return createMemo(() => connected() || setupComplete() || local.model.hasUsableFreeDefault() === true) + // altimate_change end } +// altimate_change start — Codex HOLD finding 1: true only when overall readiness cannot be +// decided YET — none of `connected()`/`setupComplete()` are already true, and the free-default +// predicate is specifically `"pending"` (kv still hydrating), not a settled `false`. The prompt +// submit gate (component/prompt/index.tsx) is the one caller that needs this: `useReady()` alone +// cannot distinguish "genuinely not usable, show the picker" from "don't know yet, kv is still +// loading" — both read as `false` there by design (see `useReady()`'s comment above), which is +// the right default for every OTHER consumer (display text, command enablement) but wrong for a +// submit gate whose `false` branch discards the typed prompt. This predicate lets the submit +// gate keep the prompt and retry once kv resolves, instead of guessing either way. +export function useReadyPending() { + const connected = useConnected() + const local = useLocal() + return createMemo(() => !connected() && !setupComplete() && local.model.hasUsableFreeDefault() === "pending") +} +// altimate_change end + /** * Setup completion ONLY — deliberately without the `connected()` term. * @@ -404,32 +466,150 @@ export function DialogAltimateBaseConfirm(props: { const [error, setError] = createSignal() const trackOnboarding = useOnboardingTelemetry() const firstRunActive = useFirstRunActive() + // altimate_change — PR #1302 review (Cursor "Accept can skip default rewrite", medium, real): + // captured HERE, ONCE, before `yes()` can run any registration — `local.model.launchDefault()` + // is a live memo (`fallbackModel()`), and registration + `sync.bootstrap()` can make + // `altimate-free/altimate-base` the first live provider, moving `fallbackModel()` to Base + // itself by the time `yes()` would otherwise re-read it. Passed to `migrateLegacyDefault({ + // from })` below so eligibility is re-checked against what the launch default WAS, not what it + // has since become. + const launchDefault = local.model.launchDefault() + // altimate_change start — cubic review round 5, P2: same snapshot reasoning as + // `launchDefault` above, applied to its display name too. `launchDefaultDisplay()` is a LIVE + // memo over the same `fallbackModel()` — calling it from JSX (as the disclosure copy used to) + // re-reads it on every re-render, so once `yes()`'s registration makes Altimate Base the new + // `fallbackModel()`, the disclosure still on screen (`yes()` awaits registration before the + // dialog closes) could rename itself to "Altimate Base" mid-sentence in copy that is + // specifically explaining why the CURRENT default is being replaced. Snapshotting here, once, + // alongside `launchDefault`, keeps the copy naming the model that was actually true when the + // dialog opened. + const launchDefaultDisplay = local.model.launchDefaultDisplay() + // altimate_change end let decided = false let choiceRecorded = false let disposed = false - const releaseCloseGuard = dialog.guardClose(() => !busy()) + // altimate_change start — Cursor/CodeRabbit/cubic review round 5: `recordChoice`'s + // `lastCloseReason !== "programmatic" && lastCloseReason !== "interrupt"` check treated + // `lastCloseReason === undefined` as "record it" — but `undefined` is also what a genuine + // top-level quit (process exit, Ctrl+C at the top of the app disposing the whole Solid root) + // leaves behind, since that teardown runs `onCleanup` without the close guard ever being + // consulted. That silently counted app quits as declines in `altimate_base_choice` telemetry. + // `chosen` is the positive signal instead: it is set ONLY inside `no()`/`yes()`, i.e. only when + // the user (or the guard's `queueMicrotask(no)` for a genuine dismiss) actually reached a + // decision. `onCleanup`'s unconditional `recordChoice("cancel")` fallback now records nothing + // for migration unless a decision was actually made. + let chosen = false + // altimate_change end + // altimate_change start — fixes #1301: the migration origin never entered the first-run funnel + // at all (it was gated on `firstRunActive()`, which migration never sets), so the disclosure + // that matters most for measuring the fix was invisible to telemetry. Migration is still not + // FIRST-RUN onboarding, so it stays out of the `firstRunActive()`-gated events below, but it + // gets its own unconditional emission with `origin: "migration"` on every event. + // + // `lastCloseReason` remembers which kind of close the guard most recently PERMITTED (`"dismiss"` + // for Escape/the backdrop click — `dialog.tsx`'s `dismiss()`, wired to the backdrop + // specifically; `"programmatic"` for this dialog's own `clear()`/`replace()` or an unrelated + // feature's; `"interrupt"` for Ctrl+C — see `ui/dialog.tsx`) so the `onCleanup` fallback below + // can tell them apart too. + let lastCloseReason: "dismiss" | "interrupt" | "programmatic" | undefined + const releaseCloseGuard = dialog.guardClose((reason) => { + // altimate_change — Kilo review round 6 (3986171185): a dismiss attempted WHILE `busy()` + // (registration in flight) is VETOED below — the close does not happen, no decision is made, + // `no()` is deliberately not queued. Recording `lastCloseReason` before that veto check used + // to leave it set to `"dismiss"` anyway, as a side effect of an attempt that never actually + // went through. If the app was then torn down before the guard was consulted again (mid + // registration, then a hard quit — the exact guard-free teardown path `onCleanup`'s fallback + // below exists for), that stale `"dismiss"` made the fallback persist a decline nobody + // actually made. Bail out before recording anything whenever the close is going to be + // vetoed for being busy — `lastCloseReason` now only ever reflects a close the guard + // actually PERMITTED (or explicitly routed to `no()`, below). + if (busy()) return false + lastCloseReason = reason + // Escape closes through `DialogProvider`'s keymap binding (`closeTop("dismiss")`), which + // calls this guard BEFORE the dialog's own `useKeyboard` below ever sees the key — so + // intercepting in `useKeyboard` alone would be too late; the dialog would already be gone. + // The backdrop click reaches here the same way, via `dialog.tsx`'s `dismiss()` (fixes #1301, + // Codex review round 2, P2: it used to call `clear()`, i.e. "programmatic", so clicking + // outside the dialog silently skipped both the decline AND the picker that keyboard Escape + // gets). This dialog's own visible "esc" label calls `no()` directly instead of going through + // the guard at all — see its `onMouseUp` below. For a migration DISMISSAL from any of these, + // veto the close and run the same routing `no()` does (persist the decline, open the picker) + // on a microtask instead of a bare dismissal, which the retired Big Pickle model cannot + // silently fall back to. `no()` sets `decided = true` before its own `dialog.replace`, so + // that replace passes this same guard on its re-check (reason "programmatic", by then + // decided) and this queued call cannot double-fire. + // + // Ctrl+C closes through the same binding but with reason "interrupt" (PR review round 3): + // Ctrl+C is a "get me out" gesture (quitting the app, or backing out of whatever's on + // screen), not "I decline Altimate Base specifically" the way Escape on THIS dialog is. Before + // this distinction existed, quitting with Ctrl+C twice while the migration dialog was open + // queued `no()` on the FIRST Ctrl+C (persist + picker takeover) before the second one could + // quit — recording a refusal the user never made. "interrupt" is deliberately NOT matched + // below, so it falls through to the same handling as a PROGRAMMATIC close: the close + // succeeds, nothing is persisted, and the disclosure is simply offered again next launch. + // + // A PROGRAMMATIC close (this dialog's own `clear()`/`replace()`, or an unrelated feature — + // command palette, session list — replacing the dialog stack out from under this one) is left + // alone here too. Neither it nor an interrupt is the user declining Altimate Base, so forcing + // `no()` for them turned harmless UI navigation (or quitting) into a persisted refusal plus an + // unwanted picker takeover. The `onCleanup` fallback below only persists a decline for the + // reasons this guard could not itself resolve into a decision. + if (reason === "dismiss" && props.origin === "migration" && !decided) { + queueMicrotask(no) + return false + } + return true + }) + // altimate_change end function recordChoice(choice: "accept" | "cancel") { if (choiceRecorded) return choiceRecorded = true - if (firstRunActive() && props.origin !== "migration") { - trackOnboarding({ name: "altimate_base_choice", choice }) + // altimate_change — Cursor/CodeRabbit/cubic review round 5: see `chosen`'s declaration above. + // `lastCloseReason === "dismiss"` is kept alongside `chosen` defensively (a genuine dismiss + // always routes through `no()`, which sets `chosen` first, but this keeps the condition + // correct even if that ordering ever changes) — it is `undefined` (top-level quit) and + // `"programmatic"`/`"interrupt"` (unrelated close, Ctrl+C) that must NOT record a choice. + if (props.origin === "migration" ? chosen || lastCloseReason === "dismiss" : firstRunActive()) { + trackOnboarding({ name: "altimate_base_choice", choice, origin: props.origin }) } } onMount(() => { - // Migration is not first-run onboarding and must not enter that funnel. - if (firstRunActive() && props.origin !== "migration") { + // altimate_change — fixes #1301: see the block comment on `releaseCloseGuard` above + if (props.origin === "migration" || firstRunActive()) { trackOnboarding({ name: "altimate_base_confirm_shown", origin: props.origin }) } }) onCleanup(() => { releaseCloseGuard() disposed = true - // Escape and click-away are handled by DialogProvider and never reach no(), but they are just - // as much a refusal. Persisting the decline here too keeps a dismissed migration prompt from - // reappearing on every launch forever. - if (!decided && props.origin === "migration") props.onDecline?.() + // altimate_change start — PR #1302 review (CodeRabbit + cubic, both flagged this; Kilo review + // round 6, 3986171185, corrected further): a genuine user DISMISSAL — keyboard Escape or the + // backdrop click, which `dialog.tsx` reports as `dismiss()` (reason "dismiss") — is normally + // fully handled above via `queueMicrotask(no)`, which sets `decided` before this ever runs, + // same as this dialog's own visible "esc" label (see its `onMouseUp` above, which calls + // `no()` directly). Ctrl+C is a separate "interrupt" reason, never "dismiss" — see the guard + // above. So this branch does not double an ORDINARY dismissal. It is not purely + // documentation, though: it is the actual safety net for a dismiss attempted WHILE `busy()` + // was true (registration in flight) followed by teardown before the guard is consulted + // again — the guard above now bails out BEFORE recording anything in that case, so + // `lastCloseReason` stays whatever it was before the vetoed attempt (typically `undefined`, + // since a legitimate prior close would already have set `decided`), and this condition + // correctly stays false for it too. A true positive here (a real, unqueued dismiss reaching + // teardown) would be an ordering bug elsewhere; this remains a deliberate belt-and-suspenders + // check, not dead code. + // + // The bug this also fixes: renderer teardown (process exit, Ctrl+C-to-quit at the TOP level, + // not this dialog's own Ctrl+C binding) runs this cleanup WITHOUT the guard ever having been + // consulted, so `lastCloseReason` stays `undefined`. The previous `!== "programmatic"` check + // treated "no reason at all" the same as "dismissed", persisting a refusal the user never + // made just from quitting the app. Requiring the reason to be the observed, positive + // "dismiss" — not merely "not programmatic" — excludes both `undefined` and "programmatic" + // (this dialog's own `clear()`/`replace()`, or an unrelated feature replacing the dialog + // stack out from under this one — neither is the user declining Altimate Base either). + if (!decided && props.origin === "migration" && lastCloseReason === "dismiss") props.onDecline?.() + // altimate_change end decided = true recordChoice("cancel") }) @@ -437,6 +617,8 @@ export function DialogAltimateBaseConfirm(props: { function no() { if (decided || busy()) return decided = true + // altimate_change — Cursor/CodeRabbit/cubic review round 5: see `chosen`'s declaration above + chosen = true recordChoice("cancel") // altimate_change — a migration decline no longer just leaves the dialog cleared: Big Pickle // is retired, so "pick something else" must actually route somewhere. `onDecline` still @@ -453,15 +635,19 @@ export function DialogAltimateBaseConfirm(props: { async function yes() { if (decided || busy()) return + // altimate_change — Cursor/CodeRabbit/cubic review round 5: see `chosen`'s declaration above + chosen = true recordChoice("accept") setBusy(true) setError(undefined) const outcome = await registerAltimateBase(altimateBaseConsent) if (disposed) return - if (firstRunActive() && props.origin !== "migration") { + // altimate_change — fixes #1301: see the block comment on `releaseCloseGuard` above + if (props.origin === "migration" || firstRunActive()) { trackOnboarding({ name: "altimate_base_register_result", result: outcome.ok ? "success" : outcome.result, + origin: props.origin, }) } if (!outcome.ok) { @@ -491,8 +677,10 @@ export function DialogAltimateBaseConfirm(props: { if (props.origin === "migration") { // A migration also removes the retired implicit model from recents. Re-check eligibility // after registration so a project allowlist or explicit model change made while the dialog - // was open cannot be overwritten by the returning-user migration. - const migrated = local.model.migrateLegacyDefault() + // was open cannot be overwritten by the returning-user migration. `from: launchDefault` + // (captured on mount, before registration) — see its declaration above — keeps this + // re-check from being defeated by `fallbackModel()` itself having moved to Base by now. + const migrated = local.model.migrateLegacyDefault({ from: launchDefault }) if (!migrated) { // Registration succeeded, but migration is no longer eligible — the user is still on the // retired Big Pickle model. Route to the picker instead of marking setup complete for a @@ -555,15 +743,53 @@ export function DialogAltimateBaseConfirm(props: { Use Altimate Base? - !busy() && dialog.clear()}> + {/* altimate_change start — fixes #1301 (Codex review round 2, P2): this visible label is + a user dismissal too, exactly like the keyboard key and the backdrop click — for + migration it must route through `no()` (persist the decline, open the picker), not a + bare `dialog.clear()`, or clicking it silently leaves the next server launch free to + pick Base again after a partial registration. */} + { + if (busy()) return + if (props.origin === "migration") { + no() + return + } + dialog.clear() + }} + > esc + {/* altimate_change end */} + {/* altimate_change start — fixes #1301: migration now also covers implicit free public + Zen defaults besides the retired Big Pickle id, so the copy must name whichever model + is actually being moved rather than always naming Big Pickle specifically. + PR #1302 review (CodeRabbit + cubic, both flagged this): this must describe the LAUNCH + default (the captured `launchDefault`/`launchDefaultDisplay` snapshots above, = what + `fallbackModel()` resolved to when the dialog opened) — the model migration eligibility + and `migrateLegacyDefault()` actually reason about — not `local.model.current()`/ + `parsed()` (a session-restored model on `restoreSession`/`--continue`) NOR the live + `local.model.launchDefault()`/`launchDefaultDisplay()` memos themselves (cubic review + round 5: those can change mid-dialog once `yes()`'s registration makes Altimate Base + the new live fallback, renaming this copy out from under the user while it explains why + the OLD default is being replaced). */} - - Big Pickle has been retired. - + + {`Your default model, ${launchDefaultDisplay.model}, is a public free model. Altimate Base is the free model Altimate hosts for data work.`} + + } + > + + Big Pickle has been retired. + + + {/* altimate_change end */} {ALTIMATE_BASE_DISCLOSURE} diff --git a/packages/tui/src/component/prompt/index.tsx b/packages/tui/src/component/prompt/index.tsx index b94bf9e4fa..7c8c5594bf 100644 --- a/packages/tui/src/component/prompt/index.tsx +++ b/packages/tui/src/component/prompt/index.tsx @@ -48,11 +48,11 @@ import { useDialog } from "../../ui/dialog" import { DialogProvider as DialogProviderConnect, WARNLIST } from "../dialog-provider" // altimate_change — first-run submit gate: open the curated welcome picker instead // of erroring when no model is ready yet (see altimate-onboarding.tsx). -import { DialogModelWelcome, useReady } from "../altimate-onboarding" +import { DialogModelWelcome, markFirstRunActive, useReady, useReadyPending } from "../altimate-onboarding" import { DialogAlert } from "../../ui/dialog-alert" import { useToast } from "../../ui/toast" import { useKV } from "../../context/kv" -import { createFadeIn } from "../../util/signal" +import { createDeferredRetry, createFadeIn } from "../../util/signal" import { DialogSkill } from "../dialog-skill" import { DialogWorkspaceUnavailable } from "../dialog-workspace-unavailable" import { useArgs } from "../../context/args" @@ -268,6 +268,12 @@ export function Prompt(props: PromptProps) { // up; and flag known-bad tool-callers with a persistent "⚠ unreliable model" chip in // the prompt meta row (same WARNLIST the model picker warns with). const ready = useReady() + // altimate_change — Codex HOLD finding 1: `!ready()` alone cannot distinguish "genuinely not + // usable, show the picker" from "don't know yet, kv is still hydrating" — see `readyPending`'s + // consumer below (`submitInner`) for why that distinction matters for a submit gate + // specifically, and `useReadyPending`'s declaration in altimate-onboarding.tsx for why it's a + // separate accessor rather than folded into `ready` itself. + const readyPending = useReadyPending() const unreliableModel = createMemo(() => Boolean(WARNLIST[local.model.parsed().model])) // altimate_change end @@ -1027,6 +1033,32 @@ export function Prompt(props: PromptProps) { }) let submitting = false + // altimate_change start — Codex HOLD finding 1: a submit attempted while `readyPending()` is + // true (kv still hydrating, see its declaration above) defers instead of discarding — see + // `submitInner`'s `readyPending()` branch below, which calls `deferredSubmit.defer()` rather + // than clearing the prompt. `createDeferredRetry` (util/signal.ts) is the retry: once + // `readyPending()` flips false (kv resolved either way), it re-attempts the exact same + // `submit()` call automatically, so a submission made during that window is neither lost nor + // stuck waiting on the user to press Enter again. Codex re-review round 8: extracted into a + // standalone, shared primitive (rather than the flag + `createEffect` inlined here) + // specifically so test/context/ready-pending.test.tsx exercises the SAME production code this + // component runs, not a hand-rolled reimplementation that could drift from — or stop + // reflecting — a change made only here. + // + // Codex re-review round 9: `getRevision` snapshots the prompt (text + attachments) at the + // moment it defers, and `createDeferredRetry` compares that snapshot against the LIVE prompt + // right before retrying — if the user edited the box (without pressing Enter again) while the + // submission was deferred, the retry is silently canceled rather than firing. Without this, a + // deferred prompt A followed by an untouched-by-Enter edit to B would have B auto-submitted the + // instant readiness resolved — a send the user never asked for, not a resend of the one they + // did. (A genuinely unedited resubmit still re-reads `store.prompt.input`/`.parts` live inside + // `submitInner`, not this snapshot, so the two can never drift apart when nothing changed; + // `unwrap` matches the same store-to-plain-object pattern already used elsewhere in this file, + // e.g. the prompt stash below.) + const deferredSubmit = createDeferredRetry(readyPending, () => void submit(), { + getRevision: () => unwrap(store.prompt), + }) + // altimate_change end async function submit() { // Prevent overlapping invocations (e.g. a double-pressed Enter, or the // input's native onSubmit racing another dispatch). Without this guard, @@ -1068,12 +1100,42 @@ export function Prompt(props: PromptProps) { // message with no provider ready opens the welcome picker (the message is // discarded) with a friendly line, rather than erroring. if (!ready()) { - dialog.replace(() => ( + // altimate_change — Codex HOLD finding 1: `readyPending()` (see its declaration above) + // means readiness genuinely cannot be decided yet — kv is still hydrating, and none of + // `connected()`/`setupComplete()` are already true either. Discarding the prompt and + // opening the picker HERE, before kv even finishes loading, is exactly the bug: a decliner + // whose refusal lives only in kv would look un-declined for that brief window and get + // bounced into onboarding they already completed once, losing whatever they just typed. + // Defer instead — keep the prompt exactly as-is, do not open anything — and let the retry + // effect above resubmit once `readyPending()` settles. + if (readyPending()) { + deferredSubmit.defer() + return false + } + // altimate_change — cubic review (3986532221); cursor/cubic re-review round 8 + // (3986991408/3987011218); cursor/cubic re-review round 9 (3987148590/3987174889): this is + // the prompt-gate's own equivalent of app.tsx's first-run picker (an impatient submit + // before that startup effect settled), so it must latch the SAME "first-run opened this + // launch" signal — see `firstRunOpenedThisLaunch`'s declaration in altimate-onboarding.tsx + // for why app.tsx's startup effect needs this to tell a genuine first-run completion apart + // from a returning user's routine `/model` switch racing that same effect. `dialog.replace()` + // can lose a race to another dialog (e.g. a close-guarded one, which a deferred submit's + // automatic retry — see `deferredSubmit` above — can hit directly, bypassing any focus + // check a manual Enter press would go through) and return `false` without opening anything. + // Mirror app.tsx's own identical `shown` check in BOTH of the ways it matters: latch only on + // success (round 8's fix — a RETURNING user's next `/model` pick must not look like a + // first-run completion for a picker nobody ever saw), AND return immediately on failure, + // same as app.tsx's own `if (!shown) return`, BEFORE clearing anything (round 9's fix — a + // failed picker must not also silently discard the prompt the user just typed; they can + // just submit again once whatever's blocking the dialog clears). + const shown = dialog.replace(() => ( )) + if (!shown) return false + markFirstRunActive() input.clear() input.extmarks.clear() setStore("prompt", { input: "", parts: [] }) diff --git a/packages/tui/src/context/kv.tsx b/packages/tui/src/context/kv.tsx index 7b90c95f59..2b1e08cd37 100644 --- a/packages/tui/src/context/kv.tsx +++ b/packages/tui/src/context/kv.tsx @@ -60,6 +60,15 @@ export const { use: useKV, provider: KVProvider } = createSimpleContext({ console.error("Failed to write KV state", { error }) }) }, + // altimate_change start — PR #1302 review (CodeRabbit + cubic "Await the atomic writes + // before disposing the state directory"): expose the queued-write chain so a caller (the + // dialog test harness's cleanup, primarily) can wait for everything set so far to actually + // land, instead of guessing with a fixed delay before removing the directory the write + // targets. + flush() { + return write + }, + // altimate_change end } return result }, diff --git a/packages/tui/src/context/local.tsx b/packages/tui/src/context/local.tsx index f7eea6237a..d5da655f82 100644 --- a/packages/tui/src/context/local.tsx +++ b/packages/tui/src/context/local.tsx @@ -12,6 +12,12 @@ import { readJson, writeJsonAtomic } from "../util/persistence" import { useTheme } from "./theme" import { useToast } from "../ui/toast" import { useRoute } from "./route" +// altimate_change — reuse the same free-tier marker `isAnyProviderConnected` uses so the two +// checks cannot silently diverge; see `isFreeZenModel` below. +import type { ConnectedProviderShape } from "../util/connected" +// altimate_change — fixes #1301 (Codex review, P2): `hasUsableFreeDefault` below needs to see the +// same migration-decline kv key app.tsx writes. +import { useKV } from "./kv" export type LocalTheme = { secondary: RGBA @@ -44,6 +50,10 @@ export const ALTIMATE_BASE_MODEL = { modelID: "altimate-base", } as const satisfies ModelRef +// altimate_change — remember an explicit migration decline without suppressing later manual +// setup. Moved here (from app.tsx) so `hasUsableFreeDefault` below can read the same key. +export const ALTIMATE_BASE_MIGRATION_DECLINED_KEY = "altimate_base_big_pickle_migration_declined_v1" + export function isModelRef(model: unknown): model is ModelRef { if (!model || typeof model !== "object") return false const value = model as Record @@ -91,6 +101,175 @@ export function isConfirmedExplicitSelection(current: unknown, explicitDefault: } // altimate_change end +// altimate_change start — fixes #1301: offer Altimate Base to every user riding an implicit free +// public Zen default, not only the retired Big Pickle id. `shouldMigrateLegacyDefault` above +// required Big Pickle in `recent`, but only a picker-driven pick ever writes `recent` — the vast +// majority of implicit-default users never touched a picker, so they were never offered Base. +export function isFreeZenModel(model: ModelRef | undefined, providers: readonly ConnectedProviderShape[]): boolean { + if (!model || model.providerID !== "opencode") return false + const provider = providers.find((item) => item.id === model.providerID) + const info = provider?.models[model.modelID] + if (!info) return false + // Same free-tier marker `util/connected.ts`'s `isAnyProviderConnected` uses: a missing cost or + // an explicit zero on the built-in `opencode` provider both mean the public free tier. + const cost = info.cost?.input + return cost == null || cost === 0 +} + +export function shouldOfferManagedBaseDefault( + current: ModelRef | undefined, + explicit: boolean, + providerConfig: unknown, + isFree: (model: ModelRef) => boolean, +): boolean { + if (explicit || !allowsManagedBaseDefault(providerConfig)) return false + if (current == null) return false + return isFree(current) +} +// altimate_change end + +// altimate_change start — fixes #1301 (Codex review, P2): pure predicate for "is the CURRENT +// model a free public Zen model the user is fine staying on" — explicitly chosen, or already +// declined migrating away from. A free default the user picked on purpose or already said No to +// moving is a legitimate way to use the product, not "un-onboarded"; without this, a returning +// free-default user who is explicit or already declined gets treated as not-ready on every +// relaunch (see `hasUsableFreeDefault`'s call site for what that breaks). +export function isUsableFreeDefault( + current: ModelRef | undefined, + isValid: (model: ModelRef) => boolean, + isFree: (model: ModelRef) => boolean, + explicit: boolean, + declined: boolean, +): boolean { + if (!current || !isValid(current)) return false + if (!isFree(current)) return false + return explicit || declined +} +// altimate_change end + +// altimate_change start — Kilo review round 6 / Codex HOLD finding 1: `hasUsableFreeDefault()`'s +// call site reads `kv.get(ALTIMATE_BASE_MIGRATION_DECLINED_KEY, false)` — a default that means +// "not declined" as far as `isUsableFreeDefault` above can tell, whether that's the true +// persisted value or just kv hasn't hydrated yet. For a pre-0.11.x decliner whose refusal lives +// ONLY in kv (no `explicitDefault` marker, no picker-written recent, and big-pickle so +// `hasOwnPickOfImplicitDefault()` is also false — exactly the population this migration +// targets), reading that default as "not declined" before kv is ready makes the WHOLE predicate +// false. Kilo's original finding stopped there; Codex caught the first attempted fix (treat an +// unready kv as "assume usable", i.e. return `true`) going the WRONG direction: that makes +// `useReady()` true immediately, before onboarding/migration has had any chance to run, so +// `--prompt` (or a fast manual submit) sails straight through to whatever implicit default is +// currently selected — including the public Zen tier a migration disclosure should have offered +// to move off of. "Assume usable" trades a false negative (discarded input) for a false positive +// (skipped onboarding) — worse, not better. +// The correct third state is PENDING, not `true`: an unready kv means this predicate genuinely +// cannot answer yet, so it must say so explicitly rather than guessing either boolean. Callers +// that only need a boolean (headless call sites, `app.tsx`'s startup effect, which already waits +// on `kv.ready` before running at all) coerce `pending` to `false` — the same conservative +// default the code had before kv.ready-awareness existed. The ONE caller that must NOT collapse +// `pending` to `false` is the prompt submit gate (`component/prompt/index.tsx`): a `false` there +// means "discard the input and open the picker", which is exactly the data-loss bug this was +// supposed to fix. `useReadyPending()` (see `altimate-onboarding.tsx`) is the seam that lets the +// submit gate DEFER — keep the typed prompt, don't judge yet, retry once kv actually resolves — +// instead of discarding it over an answer that was never computed. +export function hasUsableFreeDefaultGated(kvReady: boolean, computeUsable: () => boolean): boolean | "pending" { + if (!kvReady) return "pending" + return computeUsable() +} +// altimate_change end + +// altimate_change start — Kilo review round 6 (3986171188): app.tsx's startup effect used to +// latch "this launch needs no onboarding" purely off `hasExistingLegacySelection() || +// hasUsableFreeDefault()`, which can go true from a setup the user JUST completed THIS launch +// (an impatient first-run user submits before this effect settles, the prompt gate opens the +// picker on its own, they pick a free Zen model — `set()` marks it explicit/recent and +// `markSetupComplete()` runs) just as easily as from a genuinely RETURNING user's persisted +// state. Latching on the former skipped the `onboardingReady()` branch below it — which exists +// specifically to catch that same-launch-setup case and fire the funnel telemetry +// (`onboarding_started`/`onboarding_completed`/`scan_gate_shown`) plus `openScanGate()` — before +// it ever ran. `setupCompleteThisLaunch` is the discriminator app.tsx already uses one branch +// below for the identical reason: it starts `false` every launch and is set only by a setup +// completed DURING this one, so a genuine returning user's value is always `false` here and this +// gate's behavior for them is unchanged. Extracted as a pure predicate so app.tsx's startup +// effect (a large, deeply-nested `createEffect` not otherwise unit-testable) has one small, +// directly-testable seam for this specific ordering bug. +export function shouldSkipOnboardingAtStartup( + hasExistingLegacySelection: boolean, + hasUsableFreeDefault: boolean, + setupCompleteThisLaunch: boolean, +): boolean { + return (hasExistingLegacySelection || hasUsableFreeDefault) && !setupCompleteThisLaunch +} + +// The onboardingReady() branch's own discriminator (app.tsx), extracted like its sibling above so +// the test exercises the SAME predicate app.tsx calls rather than re-deriving the expression +// (review of #1302, round 10). `setupComplete` alone is a GLOBAL flag set by any model pick; a +// returning user's routine `/model` switch racing the startup effect must not read as a first-run +// completion — only a first-run picker that actually opened THIS launch qualifies. +export function shouldFireFirstRunFunnelAtStartup(setupComplete: boolean, firstRunOpenedThisLaunch: boolean): boolean { + return setupComplete && firstRunOpenedThisLaunch +} +// altimate_change end + +// altimate_change start — fixes #1301 (Codex review round 2, P1): an older picker-written Zen +// recent that predates the `explicitDefault` marker (see that field's declaration comment) is +// still the user's OWN past pick, not a truly implicit default — `recentModels()` only ever adds +// an entry through a deliberate `/model` pick, session restore, or this migration itself. Silent +// migration (when Base is already registered) must not sweep that up without asking; the +// disclosure stays declinable for it. Big Pickle is deliberately excluded: recents written before +// this whole distinction existed were always silently migrated, and that stays unchanged. +export function isOwnPastPickOfFreeDefault(current: ModelRef | undefined, recent: readonly ModelRef[]): boolean { + if (!current) return false + // Destructured BEFORE the `isLegacyBigPickleModel` check, not after: it is itself a type + // predicate over `ModelRef`, and TS (still, even through a `const` alias — "control flow + // analysis of aliased conditions") narrows `current` on its false branch by subtracting that + // asserted type from `current`'s already-`ModelRef` type, which collapses straight to `never` + // and breaks any later property access on `current` (same hazard `migrateLegacyRecentModels` + // documents above). + const { providerID, modelID } = current + if (isLegacyBigPickleModel(current)) return false + return recent.some((item) => item.providerID === providerID && item.modelID === modelID) +} +// altimate_change end + +// altimate_change start — fixes #1301 (Codex review round 2, P1): migration is a decision about +// the DEFAULT, not about an already-open conversation. `current` is `currentModel()` (can be a +// session-restored model, `restoreSession`/`--continue`); `previous` is the `fallbackModel()` +// captured before migration mutates anything — the implicit default actually being migrated +// away from. Only move the active agent's model when it is STILL that default (or there simply +// is no current model to preserve); a restored conversation on some other model must be left +// alone — migrating the default must not silently rewrite an unrelated open thread onto Base. +export function shouldMoveAgentModelDuringMigration( + current: ModelRef | undefined, + previous: ModelRef | undefined, +): boolean { + if (!current) return true + if (!previous) return false + return current.providerID === previous.providerID && current.modelID === previous.modelID +} +// altimate_change end + +// altimate_change start — Codex review round 2, P2: `migrateLegacyDefault({ from })`'s captured +// `from` must not bypass free-model validation entirely — a provider refresh moving +// `fallbackModel()` off `from` onto some OTHER (in particular PAID) model while the dialog is +// open must not still let accept insert Base. Only the ONE transition this capture exists for is +// allowed: the launch default is either still exactly `from`, or registration itself already +// moved it to Base (the expected post-registration state `usesLegacyDefault()`'s own `isFree` +// check can no longer see, since Base is not a free model). +function sameModel(a: ModelRef | undefined, b: ModelRef): boolean { + return a !== undefined && a.providerID === b.providerID && a.modelID === b.modelID +} + +export function isMigrationStillEligibleAfterCapture( + current: ModelRef | undefined, + from: ModelRef, + explicit: boolean, + providerConfig: unknown, +): boolean { + if (explicit || !allowsManagedBaseDefault(providerConfig)) return false + return sameModel(current, from) || sameModel(current, ALTIMATE_BASE_MODEL) +} +// altimate_change end + export function recentModels( model: { providerID: string; modelID: string }, recent: { providerID: string; modelID: string }[], @@ -107,11 +286,22 @@ export function recentModels( .map((item) => ({ providerID: item.providerID, modelID: item.modelID })) } -// altimate_change start — remove Big Pickle from migrated recents without touching other models -export function migrateLegacyRecentModels(recent: readonly unknown[]) { +// altimate_change start — remove Big Pickle from migrated recents without touching other models. +// `previous` additionally drops the free Zen model just migrated away from (any implicit free +// default now, not only Big Pickle) so `cycle()` does not bounce straight back onto it. +export function migrateLegacyRecentModels(recent: readonly unknown[], previous?: ModelRef) { return recentModels( ALTIMATE_BASE_MODEL, - recent.filter((model): model is ModelRef => isModelRef(model) && !isLegacyBigPickleModel(model)), + recent.filter( + // `isLegacyBigPickleModel` is checked LAST: it is itself a type predicate over `ModelRef`, + // and TS narrows `model` on its false branch by subtracting that asserted type from + // `model`'s current (already-`ModelRef`) type — which collapses straight to `never` and + // breaks the `previous` field access below if that access comes after this call instead. + (model): model is ModelRef => + isModelRef(model) && + !(previous && model.providerID === previous.providerID && model.modelID === previous.modelID) && + !isLegacyBigPickleModel(model), + ), ) } // altimate_change end @@ -125,6 +315,11 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ const theme = useTheme().theme const route = useRoute() const paths = useTuiPaths() + // altimate_change start — fixes #1301 (Codex review, P2): `hasUsableFreeDefault` reads the + // migration-decline kv key here too. `KVProvider` wraps `LocalProvider` in app.tsx, so this + // is always available. + const kv = useKV() + // altimate_change end function isModelValid(model: { providerID: string; modelID: string }) { const provider = sync.data.provider.find((item) => item.id === model.providerID) @@ -225,6 +420,13 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ // silently overwrites it. See `hasExplicitModel` / `shouldMigrateLegacyDefault` below. explicitDefault: ModelRef | undefined // altimate_change end + // altimate_change start — fixes #1301 (Codex review, P1): a migration decline used to + // live ONLY in the TUI's kv store (app.tsx's `ALTIMATE_BASE_MIGRATION_DECLINED_KEY`), + // which headless/server default selection (`Provider.defaultModel()`, ACP) cannot see. + // Persisting it here too, alongside the rest of the model state the server already reads + // from `model.json`, lets the server-side consent gate honor the same refusal. + declinedManagedBaseDefault: boolean + // altimate_change end }>({ ready: false, model: {}, @@ -234,27 +436,63 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ // altimate_change start — see the `explicitDefault` field declaration above explicitDefault: undefined, // altimate_change end + // altimate_change start — see the `declinedManagedBaseDefault` field declaration above + declinedManagedBaseDefault: false, + // altimate_change end }) + // altimate_change start — Codex re-review round 8: `cycle()`'s stable-order snapshot + // (`cycleOrder`, declared near its own definition below) needs to know when `recent` has + // changed for a reason OTHER than cycle()'s own pick, so it can re-capture and pick up + // entries a picker selection just added — otherwise a `/model` pick that reorders `recent` + // out from under a stale `cycleOrder` permanently excludes the newly-recent-ed model from + // the cycle. `recentsVersion` increments on every write to `modelStore.recent`, routed + // through `setRecent` (never call `setModelStore("recent", ...)` directly) so it can never + // drift out of sync with reality. + let recentsVersion = 0 + function setRecent(value: { providerID: string; modelID: string }[]) { + recentsVersion++ + setModelStore("recent", value) + } + // altimate_change end + const filePath = path.join(paths.state, "model.json") const state = { pending: false, } + // altimate_change start — PR #1302 review (CodeRabbit + cubic "Await the atomic writes + // before disposing the state directory"; Codex review round 2, P2: a single `pendingWrite` + // reassigned on every `save()` only let a caller wait for the LATEST write — an earlier one + // still in flight (rapid consecutive `save()` calls, e.g. `declineManagedBaseDefault()` + // immediately followed by another mutation) was silently dropped from what `persisted()` + // waited for). Track every outstanding write in a Set instead, each removing itself once + // settled; `persisted()` below awaits all of them, not just the newest. + const pendingWrites = new Set>() + // altimate_change end function save() { if (!modelStore.ready) { state.pending = true return } + // altimate_change start — PR #1302 review (CodeRabbit + cubic "Await the atomic writes + // before disposing the state directory"; see `pendingWrites`' declaration above): + // `const write =` captures the promise (this used to be a bare `void + // writeJsonAtomic(...)`), tracked in `pendingWrites` below so `persisted()` can await + // every outstanding write, not just the latest. `.catch()` on `write` itself keeps it + // from ever being an unhandled rejection (a handler is attached directly to it); + // `persisted()`'s `Promise.allSettled` tolerates either outcome regardless. state.pending = false - void writeJsonAtomic(filePath, { + const write = writeJsonAtomic(filePath, { recent: modelStore.recent, favorite: modelStore.favorite, variant: modelStore.variant, - // altimate_change start — persist the last explicitly-picked model across launches - explicitDefault: modelStore.explicitDefault, - // altimate_change end + explicitDefault: modelStore.explicitDefault, // fixes #1301: persist the last explicit pick + declinedManagedBaseDefault: modelStore.declinedManagedBaseDefault, // fixes #1301 }) + pendingWrites.add(write) + write.catch(() => {}).finally(() => pendingWrites.delete(write)) + // altimate_change end } readJson(filePath) @@ -262,7 +500,7 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ if (!x || typeof x !== "object") return const value = x as Record // altimate_change start — discard malformed persisted model references before default migration - if (Array.isArray(value.recent)) setModelStore("recent", value.recent.filter(isModelRef)) + if (Array.isArray(value.recent)) setRecent(value.recent.filter(isModelRef)) // altimate_change end if (Array.isArray(value.favorite)) setModelStore("favorite", value.favorite) if (typeof value.variant === "object" && value.variant !== null) @@ -270,6 +508,10 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ // altimate_change start — restore the last explicitly-picked model if (isModelRef(value.explicitDefault)) setModelStore("explicitDefault", value.explicitDefault) // altimate_change end + // altimate_change start — restore a persisted Base-migration decline + if (typeof value.declinedManagedBaseDefault === "boolean") + setModelStore("declinedManagedBaseDefault", value.declinedManagedBaseDefault) + // altimate_change end }) .catch(() => {}) .finally(() => { @@ -292,6 +534,23 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ return isConfirmedExplicitSelection(currentModel(), modelStore.explicitDefault) } + // altimate_change start — fixes #1301 (Codex review, P1): `usesImplicitFreeDefault` below + // judges eligibility against `fallbackModel()` (the LAUNCH default), so explicitness must be + // judged against that SAME model — not `currentModel()`, which `hasExplicitModel` above + // uses and which can be a session-restored model (`restoreSession`, `--continue`) unrelated + // to what this launch would actually fall back to. Using `hasExplicitModel()` there let an + // explicit Nemotron pick read as "implicit" whenever a different conversation happened to be + // open, and `migrateLegacyDefault()` then overwrote that restored conversation's model. + // Older picker-written recents without an `explicitDefault` marker remain eligible here by + // design (see that field's declaration comment) — those users still see one declinable + // migration prompt rather than being silently exempted forever. + function hasExplicitDefault() { + if (args.model || sync.data.config.model) return true + if (agent.current()?.model) return true + return isConfirmedExplicitSelection(fallbackModel(), modelStore.explicitDefault) + } + // altimate_change end + function hasExplicitLegacyModel() { const configured = [args.model, sync.data.config.model] .filter((model): model is string => Boolean(model)) @@ -330,6 +589,18 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ const isManagedBaseModel = (model: ModelRef) => model.providerID === ALTIMATE_BASE_MODEL.providerID && model.modelID === ALTIMATE_BASE_MODEL.modelID + // altimate_change — round 6 review (cursor/cubic/kilo, all agreeing): a prior fix here + // made `fallbackModel()` prefer a persisted `explicitDefault` over `recent`'s order, so + // `cycle()`'s deliberate pick (which marks `explicitDefault` without reordering `recent`) + // would survive to the next TUI launch. That introduced a WORSE bug: headless/ACP default + // resolution (`Provider.readDefaultModelState()`/`defaultModelFromConfig()`) reads only + // `recent`, never `explicitDefault`, so the TUI and server could resolve two different + // defaults from the same `model.json` after a cycle — and a malformed `explicitDefault` + // (e.g. a prototype-name `modelID`) would have poisoned the TUI launch default ahead of + // the same validity checks `recent` already goes through. Reverted; see `cycle()` below, + // which now reorders `recent` instead (`{ explicit: true, recent: true }`) so `recent` + // stays the single source of truth for TUI, `Provider.defaultModel()`, and ACP alike. + // A recent entry is the user's own past pick, so — matching `Provider.defaultModel()`'s // comment on the same tradeoff — it stays honored for every provider except the // consent-gated managed one; a narrowed project allowlist does not retroactively invalidate @@ -387,30 +658,136 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ const a = agent.current() if (!a) return setModelStore("model", a.name, model) - if (options?.recent) setModelStore("recent", recentModels(model, modelStore.recent)) + if (options?.recent) setRecent(recentModels(model, modelStore.recent)) // A picker-driven selection, as opposed to session restore or programmatic migration — // see `hasExplicitModel` above for why this needs its own persisted marker. if (options?.explicit) setModelStore("explicitDefault", { providerID: model.providerID, modelID: model.modelID }) + // altimate_change start — fixes #1301 (Codex review round 2, P2): ANY deliberate, + // interactive explicit selection of Altimate Base clears an earlier migration decline — + // not only `/connect`'s `set()`. `cycleFavorite` below calls `selectModel` directly, so + // the clearing has to live HERE, in the one place every explicit selection funnels + // through, or favorite-cycling to Base left `declinedManagedBaseDefault` (and the + // mirrored kv key) stuck `true`, which a later headless/ACP launch still reads as a + // refusal even though the user just picked Base on purpose. Both flags are cleared + // together — see `declineManagedBaseDefault()` below for where both are SET together. + if ( + options?.explicit && + model.providerID === ALTIMATE_BASE_MODEL.providerID && + model.modelID === ALTIMATE_BASE_MODEL.modelID + ) { + setModelStore("declinedManagedBaseDefault", false) + kv.set(ALTIMATE_BASE_MIGRATION_DECLINED_KEY, false) + } + // altimate_change end if (options?.recent || options?.explicit) save() selected = true }) return selected } - function usesLegacyDefault() { - return shouldMigrateLegacyDefault( - currentModel(), - modelStore.recent, - hasExplicitModel(), + // fixes #1301: evaluated against `fallbackModel()` (the LAUNCH default), not + // `currentModel()`. `currentModel()` can resolve to a session-restored model + // (`restoreSession`, `--continue`), which was never a deliberate choice either way and must + // not be mistaken for "this launch's implicit default" — see `restoreSession` below. + function usesImplicitFreeDefault() { + return shouldOfferManagedBaseDefault( + fallbackModel(), + hasExplicitDefault(), sync.data.config.provider, + (candidate) => isLegacyBigPickleModel(candidate) || isFreeZenModel(candidate, sync.data.provider), ) } + // Alias kept so existing call sites (app.tsx's migration effect, `migrateLegacyDefault` + // below) do not need to change. + const usesLegacyDefault = usesImplicitFreeDefault + + // altimate_change — fixes #1301 (Codex review round 2, P1): see `isOwnPastPickOfFreeDefault` + // above. Evaluated against the same launch default (`fallbackModel()`) eligibility is + // judged on, and app.tsx's silent-migration branch (Base already registered) consults it + // to fall back to the declinable disclosure instead. + function hasOwnPickOfImplicitDefault() { + return isOwnPastPickOfFreeDefault(fallbackModel(), modelStore.recent) + } function hasExistingLegacySelection() { return isExistingBigPickleSelection(currentModel(), modelStore.recent, hasExplicitLegacyModel()) } // altimate_change end + // altimate_change start — fixes #1301 (Codex review, P2): a free public Zen model the user + // either chose on purpose or already said No to migrating away from is a legitimate way to + // use the product, not "un-onboarded." Without this, a returning Nemotron user who + // explicitly selected it (or already declined once) sees the first-run welcome picker on + // every relaunch, re-enters the first-run funnel, and has prompt submission itself reopen + // the picker and discard whatever they typed (see `useReady()`'s callers in + // component/prompt/index.tsx). + function hasUsableFreeDefault() { + // Codex review round 2, P1: usability is about the model + // ACTUALLY IN USE right now, so explicitness must be judged against `currentModel()` too + // — `hasExplicitModel()`, not `hasExplicitDefault()` (which judges against the LAUNCH + // default `fallbackModel()`, the right comparison for migration eligibility, but the + // wrong one here). Cycling from free model A (the launch default) to free model B writes + // `explicitDefault = B`; comparing that against A made this predicate go false right + // after a deliberate pick, flipping `useReady()` true→false and reopening the picker (and + // clearing the prompt) on the very next submit. + // + // altimate_change — Kilo review round 6 / Codex HOLD finding 1: gated through + // `hasUsableFreeDefaultGated` — see its declaration above — so an unready `kv` reads as + // `"pending"` (genuinely undecided), not a boolean guess either way. Callers that need a + // plain boolean coerce it (`=== true`); `useReadyPending()` in altimate-onboarding.tsx is + // the one caller (the prompt submit gate) that must see the `"pending"` state itself. + return hasUsableFreeDefaultGated(kv.ready, () => ( + isUsableFreeDefault( + currentModel(), + isModelValid, + (candidate) => isLegacyBigPickleModel(candidate) || isFreeZenModel(candidate, sync.data.provider), + hasExplicitModel(), + kv.get(ALTIMATE_BASE_MIGRATION_DECLINED_KEY, false) || modelStore.declinedManagedBaseDefault, + ) || + // Codex review round 2, P2: an older picker-written free recent with no explicit marker + // (`hasOwnPickOfImplicitDefault`, judged against the LAUNCH default) is exempted from + // the startup picker in app.tsx — folded in here too so the prompt gate (which reads + // `useReady()`, built on this predicate) agrees, instead of catching that same user on + // their next submit and discarding whatever they typed. + hasOwnPickOfImplicitDefault() + )) + } + // altimate_change end + + // altimate_change start — PR #1302 review (CodeRabbit): shared by `parsed` below and + // `launchDefaultDisplay` — the migration disclosure needs to resolve a display name for the + // LAUNCH default (`fallbackModel()`), not only the current selection, via the exact same + // provider/model lookup so the two can never drift. + function modelDisplayName(value: ModelRef | undefined) { + if (!value) { + return { + provider: "Connect a provider", + model: "No provider selected", + reasoning: false, + } + } + const provider = sync.data.provider.find((item) => item.id === value.providerID) + const info = provider?.models[value.modelID] + return { + provider: provider?.name ?? value.providerID, + model: info?.name ?? value.modelID, + reasoning: info?.capabilities?.reasoning ?? false, + } + } + // altimate_change end + + // altimate_change start — Codex HOLD finding 2: `cycle()`'s traversal order, captured + // lazily on first use and held stable for the rest of the cycling sequence — see + // `cycle()`'s own comment below for why a LIVE read of `modelStore.recent` (which `cycle()` + // itself reorders via `selectModel(val, { recent: true })`) breaks repeated presses. + let cycleOrder: readonly { providerID: string; modelID: string }[] | undefined + // altimate_change — Codex re-review round 8: the version `cycleOrder` was captured at (or + // last resynced to, after cycle()'s own write) — see `recentsVersion`'s declaration above. + // A mismatch against the LIVE `recentsVersion` means something OTHER than `cycle()` wrote + // to `recent` since, and `cycleOrder` must be re-captured to see it. + let cycleOrderVersion = -1 + // altimate_change end + return { current: currentModel, get ready() { @@ -422,38 +799,77 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ favorite() { return modelStore.favorite }, + // altimate_change start — PR #1302 review (CodeRabbit "Migration copy names the wrong + // model"): the migration disclosure and `migrateLegacyDefault()` both reason about the + // LAUNCH default, not whatever `currentModel()` happens to be (which can be a + // session-restored model on `restoreSession`/`--continue`). Expose it, and its resolved + // display name, directly rather than making every caller re-derive them. + launchDefault: fallbackModel, + launchDefaultDisplay: createMemo(() => modelDisplayName(fallbackModel())), + // altimate_change end + // altimate_change start — body factored into `modelDisplayName` so `launchDefaultDisplay` + // above resolves names identically parsed: createMemo(() => { - const value = currentModel() - if (!value) { - return { - provider: "Connect a provider", - model: "No provider selected", - reasoning: false, - } - } - const provider = sync.data.provider.find((item) => item.id === value.providerID) - const info = provider?.models[value.modelID] - return { - provider: provider?.name ?? value.providerID, - model: info?.name ?? value.modelID, - reasoning: info?.capabilities?.reasoning ?? false, - } + return modelDisplayName(currentModel()) }), + // altimate_change end + // altimate_change start — PR #1302 review, cubic P2 (round 6: also pass `recent: true`, + // like `cycleFavorite` below) / Codex HOLD finding 2 (round 7: stable traversal order). + // Two requirements that pull in opposite directions if both aimed at the SAME array: + // 1. Cycling must move the picked model to the front of PERSISTED `recent` — that's the + // ONLY state headless/ACP default resolution (`Provider.readDefaultModelState()`, + // `defaultModelFromConfig()`) reads; without it the TUI and server can resolve two + // different launch defaults from the same `model.json` after a cycle (they have no + // notion of the earlier `explicitDefault`-only marker this used to rely on instead). + // 2. Cycling must visit every model in a stable order across repeated presses — reading + // the INDEX to advance from directly off that same, just-reordered `modelStore.recent` + // breaks this: cycling forward from B in [A, B, C] persists [B, A, C], so the NEXT + // forward press finds B now at index 0 (not 1) and its "next" becomes A — landing + // B → A → B forever instead of visiting every model (Codex caught this by actually + // executing it: HEAD's behavior was B → A → B; the correct behavior, matching the + // order before any cycling started, is B → C → A). + // `cycleOrder` (declared above, alongside `modelStore`) resolves this: it is a SEPARATE, + // stable snapshot of `recent`'s order, captured lazily on first use and held fixed for + // the rest of the cycling sequence — `cycle()`'s own index math walks THIS frozen list, + // never the live, self-reordering `modelStore.recent`. `selectModel(..., { recent: true })` + // still updates the real persisted `recent` on every pick, satisfying requirement 1; it + // just no longer feeds back into what `cycle()` itself reads for requirement 2. + // + // altimate_change — Codex re-review round 8: "held fixed for the rest of the cycling + // sequence" must not mean "held fixed forever." Only invalidating on "the current model + // fell out of `cycleOrder`" (the original round-7 check) went stale the moment a PICKER + // selection reordered `recent` without also knocking the current model out of the old + // snapshot: e.g. `recent = [A, B, C]`, cycle once (B is now current, `recent = [B, A, + // C]`), then the user picks D and A via `/model` (`recent` ends up `[A, D, C, B]`) — A is + // still present in the STALE `cycleOrder` (`[A, B, C]`), so the old check never + // re-captured, and D stayed permanently unreachable by cycling. `cycleOrderVersion` (see + // its declaration above) closes this: it also re-captures whenever `recentsVersion` has + // moved since `cycleOrder` was last captured OR resynced — which happens for ANY write + // to `recent`, picker or otherwise — while still recognizing cycle()'s OWN write (via the + // resync at the end of this function) so repeated presses with nothing else interleaved + // keep reusing the same stable snapshot, unaffected. cycle(direction: 1 | -1) { const current = currentModel() if (!current) return - const recent = modelStore.recent - const index = recent.findIndex((x) => x.providerID === current.providerID && x.modelID === current.modelID) + const findCurrent = (order: readonly { providerID: string; modelID: string }[]) => + order.findIndex((x) => x.providerID === current.providerID && x.modelID === current.modelID) + if (!cycleOrder || cycleOrderVersion !== recentsVersion || findCurrent(cycleOrder) === -1) { + cycleOrder = modelStore.recent.slice() + cycleOrderVersion = recentsVersion + } + const index = findCurrent(cycleOrder) if (index === -1) return let next = index + direction - if (next < 0) next = recent.length - 1 - if (next >= recent.length) next = 0 - const val = recent[next] + if (next < 0) next = cycleOrder.length - 1 + if (next >= cycleOrder.length) next = 0 + const val = cycleOrder[next] if (!val) return - const a = agent.current() - if (!a) return - setModelStore("model", a.name, { ...val }) + selectModel(val, { explicit: true, recent: true }) + // Absorb our OWN write (selectModel above bumped `recentsVersion` via `setRecent`) so + // it does not look like an external change the NEXT time `cycle()` runs. + cycleOrderVersion = recentsVersion }, + // altimate_change end cycleFavorite(direction: 1 | -1) { const favorites = modelStore.favorite.filter((item) => isModelValid(item)) if (!favorites.length) { @@ -496,16 +912,88 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ // altimate_change start — migrate Big Pickle defaults after managed-model consent usesLegacyDefault, hasExistingLegacySelection, - migrateLegacyDefault() { - if (!usesLegacyDefault() || !isModelValid(ALTIMATE_BASE_MODEL)) return false + // altimate_change — fixes #1301 (Codex review, P2): see `hasUsableFreeDefault`'s + // declaration above + hasUsableFreeDefault, + // altimate_change — fixes #1301 (Codex review round 2, P1): see + // `hasOwnPickOfImplicitDefault`'s declaration above + hasOwnPickOfImplicitDefault, + // altimate_change — fixes #1301 (Codex review round 2, P2/D): read-only accessor so + // callers (and tests) can check the persisted decline state directly, rather than only + // its downstream effects. + declinedManagedBaseDefault() { + return modelStore.declinedManagedBaseDefault + }, + // altimate_change — PR #1302 review (CodeRabbit + cubic): see `pendingWrite`'s + // declaration above. Awaiting this settles once the most recent `save()` has landed. + persisted() { + // altimate_change — see `pendingWrites`' declaration above. `allSettled` (not `all`) + // so one write's rejection can't stop the caller from also waiting out the others. + return Promise.allSettled([...pendingWrites]).then(() => undefined) + }, + // altimate_change start — fixes #1301 (Codex review, P1): see the + // `declinedManagedBaseDefault` field declaration above. Called from app.tsx's migration + // `onDecline`, alongside (not instead of) the existing kv-key write. + declineManagedBaseDefault() { + batch(() => { + setModelStore("declinedManagedBaseDefault", true) + save() + }) + }, + // altimate_change end + // altimate_change start — PR #1302 review (Cursor "Accept can skip default rewrite", + // medium, real): after registration, `yes()` calls `sdk.client.instance.dispose()` then + // `sync.bootstrap()`, which can make `altimate-free/altimate-base` the FIRST live + // provider — so a fresh `fallbackModel()`/`usesLegacyDefault()` re-check here resolves to + // Base itself, its `isFree(fallback)` term goes false, and a real accept looks + // ineligible: recents are never rewritten and the user is bounced to the welcome picker. + // `options.from` lets the caller (the migration dialog's `yes()`) pass the LAUNCH default + // it captured on mount, BEFORE registration ran. With `from` given, eligibility only + // re-checks the parts registration cannot invalidate — still not an explicit choice, + // still allowed by the project's provider allowlist — and skips re-deriving (and losing) + // the free-default check against a `fallbackModel()` that has since moved. The silent + // (already-registered) path in app.tsx keeps calling this with no `from`, unchanged. + migrateLegacyDefault(options?: { from?: ModelRef }) { + const from = options?.from + // altimate_change start — Codex review round 2, P2: see `isMigrationStillEligibleAfterCapture`'s + // declaration above for why `from` cannot just bypass eligibility entirely. + const eligible = from + ? isMigrationStillEligibleAfterCapture(fallbackModel(), from, hasExplicitDefault(), sync.data.config.provider) + : usesLegacyDefault() + if (!eligible || !isModelValid(ALTIMATE_BASE_MODEL)) return false + // altimate_change end + // Capture the model being migrated away from BEFORE mutating: reading it after + // `setModelStore("model", ...)` below would see Base, not the free default being + // dropped, so `migrateLegacyRecentModels` could never actually remove it from `recent`. + // Use the LAUNCH default (`fallbackModel`, or the caller's captured `from` — see above), + // the same value eligibility was judged on: `currentModel()` can be a session-restored + // model, which must not be dropped from `recent` just because the implicit default moved. + const previous = from ?? fallbackModel() batch(() => { const a = agent.current() - if (a) setModelStore("model", a.name, { ...ALTIMATE_BASE_MODEL }) - setModelStore("recent", migrateLegacyRecentModels(modelStore.recent)) + // altimate_change start — fixes #1301 (Codex review round 2, P1): migration is a + // decision about the DEFAULT, not about an already-open conversation. A restored + // session (`restoreSession`, `--continue`) can be on a DIFFERENT model than the + // implicit default this migration is about — unconditionally reassigning the active + // agent's model overwrote that conversation with Base. `shouldMoveAgentModelDuringMigration` + // (a pure, directly-tested predicate — see its declaration) decides whether THIS + // conversation is still actually on the default being migrated away from. The recents + // rewrite and decline-clear below still always happen regardless — those are about + // the DEFAULT going forward, independent of what this one conversation is showing. + if (a && shouldMoveAgentModelDuringMigration(currentModel(), previous)) + setModelStore("model", a.name, { ...ALTIMATE_BASE_MODEL }) + // altimate_change end + setRecent(migrateLegacyRecentModels(modelStore.recent, previous)) + // altimate_change — fixes #1301 (Codex review round 2, P2): an explicit accept via + // migration clears any earlier decline the same way `selectModel` does for every + // other explicit Base selection (`/connect`, favorite-cycling) — both flags together. + setModelStore("declinedManagedBaseDefault", false) + kv.set(ALTIMATE_BASE_MIGRATION_DECLINED_KEY, false) save() }) return true }, + // altimate_change end // Opening an old session restores the model that session was recorded with, verbatim. // Migration is a decision about the DEFAULT model and is owned by the disclosure flow in // app.tsx; applying it here rewrote historical threads onto the request-logging tier with diff --git a/packages/tui/src/context/onboarding-telemetry.tsx b/packages/tui/src/context/onboarding-telemetry.tsx index e4b60eb25e..5c6f570e7d 100644 --- a/packages/tui/src/context/onboarding-telemetry.tsx +++ b/packages/tui/src/context/onboarding-telemetry.tsx @@ -34,11 +34,16 @@ export type OnboardingTelemetryEvent = /** Set when the pick came from the full catalogue, i.e. after `searchAll`. */ via_search?: boolean } - | { name: "altimate_base_confirm_shown"; origin: "welcome" | "model" } - | { name: "altimate_base_choice"; choice: "accept" | "cancel" } + // altimate_change — fixes #1301: "migration" covers a returning user whose implicit free + // default (not only the retired Big Pickle id) is offered Altimate Base on relaunch. It is + // emitted unconditionally, unlike "welcome"/"model" which stay gated behind `firstRunActive()` + // — see `component/altimate-onboarding.tsx`. + | { name: "altimate_base_confirm_shown"; origin: "welcome" | "model" | "migration" } + | { name: "altimate_base_choice"; choice: "accept" | "cancel"; origin?: "welcome" | "model" | "migration" } | { name: "altimate_base_register_result" result: "success" | "rate_limited" | "unavailable" | "network" | "error" + origin?: "welcome" | "model" | "migration" } | { name: "scan_gate_shown" } | { name: "scan_gate_choice"; choice: "scan" | "skip" | "dismissed" } diff --git a/packages/tui/src/prompt/history.tsx b/packages/tui/src/prompt/history.tsx index 2dc4b2c0e6..e6b3540f18 100644 --- a/packages/tui/src/prompt/history.tsx +++ b/packages/tui/src/prompt/history.tsx @@ -1,5 +1,5 @@ import path from "path" -import { onMount } from "solid-js" +import { createSignal, onMount } from "solid-js" import { createStore, produce, unwrap } from "solid-js/store" import type { AgentPart, FilePart, TextPart } from "@opencode-ai/sdk/v2" import { createSimpleContext } from "../context/helper" @@ -46,6 +46,30 @@ export function isDuplicateEntry(previous: PromptInfo | undefined, next: PromptI return JSON.stringify(previous) === JSON.stringify(next) } +// altimate_change start — cubic review: the startup merge of disk-read `lines` with whatever +// `append()` already pushed in-memory during the read (`prev`) must honor the same two +// invariants normal appends do — no consecutive duplicate entries, capped at +// MAX_HISTORY_ENTRIES — rather than a raw concatenation that could reintroduce a duplicate +// straddling the two halves or exceed the cap. +export function mergeStartupHistory(lines: readonly PromptInfo[], prev: readonly PromptInfo[]): PromptInfo[] { + const merged: PromptInfo[] = [] + for (const entry of [...lines, ...prev]) { + if (isDuplicateEntry(merged.at(-1), entry)) continue + merged.push(entry) + } + return merged.slice(-MAX_HISTORY_ENTRIES) +} +// altimate_change end + +// altimate_change start — round 6 review: both full-rewrite call sites (the startup flush and +// `append()`'s trimmed branch) must snapshot `store.history` to a STRING synchronously, at the +// moment they decide to enqueue a write — never read it lazily from inside the queued closure. +// See the onMount `finally` block below for why. +function serializeHistory(history: readonly PromptInfo[]): string { + return history.map((line) => JSON.stringify(line)).join("\n") + "\n" +} +// altimate_change end + // altimate_change start — preserve in-progress prompt while browsing history export type PromptHistoryNavigationState = { index: number @@ -92,14 +116,105 @@ export const { use: usePromptHistory, provider: PromptHistoryProvider } = create init: () => { const paths = useTuiPaths() const historyPath = path.join(paths.state, "prompt-history.jsonl") + // altimate_change start — fixes #1301: a "returning user" signal for the startup migration + // decision in app.tsx, independent of the current project's (30-day-windowed) session list. + // `loaded()` settles (true) once this read finishes either way; `hadHistoryAtStartup()` is a + // ONE-TIME snapshot taken at that moment, not a live "history is non-empty" memo — a prompt + // sent during THIS launch must not retroactively make the launch look like a return visit. + const [loaded, setLoaded] = createSignal(false) + let hadHistoryAtStartup = false + // altimate_change — Codex review round 4 / round 6 (cursor/cubic/kilo): the startup flush and + // every `append()` write are fire-and-forget from `onMount`'s perspective — `loaded()` + // becoming true means the startup flush, if one was needed, has been SNAPSHOTTED and HANDED + // TO the write queue (see the `onMount` `finally` block below), not that it has landed on + // disk yet. An earlier fix tried making `loaded()` also imply "landed on disk" by awaiting + // the write first, but that opened a WORSE window: an `append()` that lands during that await + // still sees `loaded() === false`, takes the early return, and is dropped for good (the + // flush it deferred to has already been snapshotted and sent without it). Track the most + // recently kicked-off write so a caller (tests, primarily) can wait for it via `flushed()` + // below instead of assuming `loaded()` implies it. + let pendingWrite: Promise = Promise.resolve() + // altimate_change start — Cursor review round 5: serialize the startup flush and every + // append's write through one FIFO queue. Before this, each call site reassigned + // `pendingWrite` independently — that tracked only the LAST write kicked off, it never made + // one write wait for the previous one to actually land. Two writes could then run + // concurrently (this onMount flush and a later append, once `loaded()` had already flipped + // true) and race on disk: whichever finished last "wins", not necessarily the one kicked off + // last, so a racing append could be silently clobbered by the still in-flight flush. + // Chaining every write onto `pendingWrite` guarantees strict start-after-previous-settles + // ordering. Round 6 review: every call site now passes a closure over content already + // captured synchronously at enqueue time (a pre-serialized snapshot string for a full + // rewrite, or the already-`structuredClone`d entry for a plain append) — never one that + // lazily re-reads live `store.history` when it finally runs, which used to let a rewrite's + // closure pick up an entry a later, already-queued append would ALSO write, duplicating it. + function queueWrite(write: () => Promise) { + pendingWrite = pendingWrite.then(write) + return pendingWrite + } + // altimate_change end onMount(async () => { - const lines = parsePromptHistory(await readText(historyPath).catch(() => "")) - setStore("history", lines) - - // Rewrite valid retained entries to self-heal corruption and enforce the limit. - if (lines.length > 0) - writeText(historyPath, lines.map((line) => JSON.stringify(line)).join("\n") + "\n").catch(() => {}) + try { + const lines = parsePromptHistory(await readText(historyPath).catch(() => "")) + // altimate_change — Codex review round 4: MERGE, never blind-overwrite. An `append()` + // that ran while this read was in flight already pushed its entry onto `store.history` + // (in-memory only — its file write is deferred, see `append()` below); a plain + // `setStore("history", lines)` here would silently discard that entry the moment the read + // resolves. `lines` (older, from disk) comes first, whatever was already appended this + // launch comes after. + setStore("history", (prev) => mergeStartupHistory(lines, prev)) + // altimate_change — Codex review round 4: captured from the READ RESULT ALONE, before + // `append()` below can have merged anything else into `store.history`. Subtracting a + // count of races that happened DURING the read (the previous fix) was itself unsound: an + // early append's file write can land on disk AFTER the read started but BEFORE it + // resolves, in which case it's already counted in `lines.length` too — one pre-existing + // entry + one early, already-landed append could read as `lines.length === 2`, and + // subtracting the count of 1 wrongly gives `2 - 1 = 1 > 0`... but the reverse also + // happens: NO pre-existing entries + one early append whose write hadn't landed by read + // time gives `lines.length === 0`, and subtracting still gives a negative-clamped 0 — + // except when the write DOES land in between, giving `1 - 1 = 0` for a case that should + // read as "no prior history", by accident rather than by contract. Arithmetic against an + // unbounded race has no correct answer; fixed by construction below instead — `append()` + // defers its FILE write (never the in-memory update) until this read has fully resolved + // and this snapshot has already been taken, so nothing from this launch can reach + // `lines` in the first place. + hadHistoryAtStartup = lines.length > 0 + } finally { + // altimate_change — round 6 review (cursor 3986264135/3986264141, cubic 3986055642, + // kilo 3986287554, coderabbit 3982012207, all independently converging here): the ATOMIC + // transition. `store.history` at this point already reflects the merge above AND + // whatever `append()` calls raced the read (in-memory updates there are always + // immediate, see `append()` below) — a rewrite is needed either to self-heal a + // corrupted/malformed file or to persist those raced appends, and `store.history.length + // > 0` is true for either case. These three steps run SYNCHRONOUSLY, with no `await` + // between them, which is what makes the whole thing safe: + // 1. snapshot `store.history` to a STRING right now, via `serializeHistory` (see its + // declaration above) — never read `store.history` lazily from inside the queued + // write closure. + // 2. hand that snapshot to `queueWrite` — kicked off immediately, NOT awaited, so + // `setLoaded` below never waits on disk I/O. + // 3. flip `loaded()` true. + // Two independent bugs this closes at once: + // - Awaiting the write before `setLoaded(true)` (a prior fix) made `loaded()` also mean + // "landed on disk", but opened a worse window: an `append()` arriving during that + // await still saw `loaded() === false`, took the early return below, and was DROPPED + // — the flush it deferred to had already been sent without it. + // - Reading `store.history` lazily inside the write closure (instead of snapshotting + // synchronously here) let the closure pick up entries a LATER, already-queued + // append() would ALSO write — once both closures actually ran, the same entry landed + // in the file TWICE. + // With the snapshot frozen at this exact synchronous instant: an append that already ran + // is already in the snapshot, and (because `loaded()` was still false when it ran) wrote + // nothing itself — persisted exactly once, by this flush. An append that runs after this + // point sees `loaded() === true` and queues its own write BEHIND this one in the same + // FIFO queue — persisted exactly once too, never overlapping with this flush's content. + if (store.history.length > 0) { + const snapshot = serializeHistory(store.history) + queueWrite(() => writeText(historyPath, snapshot).catch(() => {})) + } + setLoaded(true) + } }) + // altimate_change end const [store, setStore] = createStore({ index: 0, @@ -110,6 +225,12 @@ export const { use: usePromptHistory, provider: PromptHistoryProvider } = create }) return { + // altimate_change start — fixes #1301: see the signal declarations above + loaded, + hadHistoryAtStartup() { + return hadHistoryAtStartup + }, + // altimate_change end // altimate_change start — preserve in-progress prompt while browsing history move(direction: 1 | -1, prompt: PromptInfo) { const result = movePromptHistory({ index: store.index, draft: store.draft }, store.history, direction, prompt) @@ -143,12 +264,40 @@ export const { use: usePromptHistory, provider: PromptHistoryProvider } = create }), ) + // altimate_change start — Codex review round 4: the IN-MEMORY update above always + // happens immediately (so the UI — history navigation, drafts — is unaffected either + // way); only the FILE write is deferred while the startup read is still in flight, so + // this launch's own write cannot land in `historyPath` before that read's + // `hadHistoryAtStartup` snapshot is taken from it (see onMount above). `onMount`'s + // `finally` flushes the merged `store.history` in one write once `loaded()` settles — + // writing here too could race that flush and get silently clobbered by it. `pendingWrite` + // tracking (see its declaration above) lets a caller (tests, primarily) await the write + // via `flushed()` below instead of assuming it already landed once kicked off. + if (!loaded()) return + if (trimmed) { - writeText(historyPath, store.history.map((line) => JSON.stringify(line)).join("\n") + "\n").catch(() => {}) + // altimate_change — round 6 review: same snapshot-at-enqueue reasoning as the onMount + // flush above — `store.history` is captured to a string synchronously, right here, + // rather than lazily inside the queued closure. This call is itself fully synchronous + // (no `await` between the `setStore` above and this `queueWrite`), so no OTHER + // `append()` can interleave with it directly — but a LATER append(), enqueued after + // this one, would still update `store.history` immediately (in-memory) before its own + // write reaches the front of the queue; a lazy read here could pick that entry up too, + // duplicating it once this rewrite's closure and that later append's own queued write + // both eventually run. + const snapshot = serializeHistory(store.history) + queueWrite(() => writeText(historyPath, snapshot).catch(() => {})) return } - appendText(historyPath, JSON.stringify(entry) + "\n").catch(() => {}) + queueWrite(() => appendText(historyPath, JSON.stringify(entry) + "\n").catch(() => {})) + }, + // altimate_change end + // altimate_change start — see `pendingWrite`'s declaration above. Awaiting this settles + // once the most recently kicked-off write has landed (or failed). + flushed() { + return pendingWrite }, + // altimate_change end } }, }) diff --git a/packages/tui/src/ui/dialog.tsx b/packages/tui/src/ui/dialog.tsx index 5ef1d5d451..10f3dd7f3c 100644 --- a/packages/tui/src/ui/dialog.tsx +++ b/packages/tui/src/ui/dialog.tsx @@ -74,11 +74,18 @@ function init() { const renderer = useRenderer() const modeStack = useOpencodeModeStack() - // altimate_change start — allow a modal to veto every dialog replacement/close path - let closeGuard: (() => boolean) | undefined + // altimate_change start — allow a modal to veto every dialog replacement/close path. `reason` + // distinguishes a user dismissal (Escape, via `closeTop("dismiss")`) from a programmatic close + // (`clear()`/`replace()`, whether that is this same dialog closing itself, a click-away, or an + // unrelated feature — command palette, session list — taking over the dialog stack) from a + // Ctrl+C interrupt (via `closeTop("interrupt")`, PR review round 3: Ctrl+C is a "get me out" + // gesture, distinct from Escape's "I decline this dialog specifically" — a guard that treated + // them the same made quitting with Ctrl+C twice while the migration dialog was open persist a + // refusal the user never made, since the guard queued the decline+picker on the FIRST Ctrl+C). + let closeGuard: ((reason: "dismiss" | "interrupt" | "programmatic") => boolean) | undefined - function canClose() { - return closeGuard?.() ?? true + function canClose(reason: "dismiss" | "interrupt" | "programmatic") { + return closeGuard?.(reason) ?? true } // altimate_change end @@ -106,9 +113,11 @@ function init() { }, 1) } - // altimate_change start — centralize guarded single-dialog close behavior - function closeTop() { - if (!canClose()) return false + // altimate_change start — centralize guarded single-dialog close behavior. `reason` defaults to + // "dismiss" (Escape's behavior before Ctrl+C got its own reason below) but every caller now + // passes explicitly. + function closeTop(reason: "dismiss" | "interrupt" = "dismiss") { + if (!canClose(reason)) return false const current = store.stack.at(-1) current?.onClose?.() setStore("stack", store.stack.slice(0, -1)) @@ -126,7 +135,7 @@ function init() { group: "Dialog", cmd: () => { // altimate_change start — preserve selection when the active close guard vetoes Escape - if (!closeTop()) return + if (!closeTop("dismiss")) return if (renderer.getSelection()) { renderer.clearSelection() } @@ -138,8 +147,10 @@ function init() { desc: "Close dialog", group: "Dialog", cmd: () => { - // altimate_change start — preserve selection when the active close guard vetoes Ctrl-C - if (!closeTop()) return + // altimate_change start — preserve selection when the active close guard vetoes Ctrl-C. + // PR review round 3: "interrupt", not "dismiss" — Ctrl+C is a "get me out" gesture, not + // a refusal of whatever dialog happens to be open (see the guard's declaration above). + if (!closeTop("interrupt")) return if (renderer.getSelection()) { renderer.clearSelection() } @@ -149,24 +160,40 @@ function init() { ], })) + // altimate_change start — fixes #1301 (Codex review round 2, P2): shared body for `clear()` + // (a "programmatic" close — used all over the codebase, including a dialog closing itself) and + // `dismiss()` (a "dismiss" close — the ONE caller is the backdrop click, which is just as much + // a user dismissal as Escape and must be reported to the guard the same way. Ctrl+C is a + // separate "interrupt" reason — a "get me out" gesture, not a decline — see `closeTop` below; + // update this comment too if that distinction ever changes). + function clearAll(reason: "dismiss" | "programmatic") { + if (!canClose(reason)) return false + for (const item of store.stack) { + if (item.onClose) item.onClose() + } + batch(() => { + setStore("size", "medium") + setStore("stack", []) + }) + refocus() + return true + } + // altimate_change end + return { + // altimate_change start — fixes #1301 (Codex review round 2, P2): `clear()` is the + // programmatic close path; `dismiss()` is the backdrop click only, wired in + // `DialogProvider`'s `` below — see `clearAll` above. clear() { - // altimate_change start — guard and report bulk dialog closure - if (!canClose()) return false - for (const item of store.stack) { - if (item.onClose) item.onClose() - } - batch(() => { - setStore("size", "medium") - setStore("stack", []) - }) - refocus() - return true - // altimate_change end + return clearAll("programmatic") + }, + dismiss() { + return clearAll("dismiss") }, + // altimate_change end replace(input: any, onClose?: () => void) { // altimate_change start — replacement is a close path and must obey the same guard - if (!canClose()) return false + if (!canClose("programmatic")) return false if (store.stack.length === 0) { focus = renderer.currentFocusedRenderable focus?.blur() @@ -194,7 +221,7 @@ function init() { setStore("size", size) }, // altimate_change start — install and safely dispose the active close guard - guardClose(guard: () => boolean) { + guardClose(guard: (reason: "dismiss" | "interrupt" | "programmatic") => boolean) { closeGuard = guard return () => { if (closeGuard === guard) closeGuard = undefined @@ -242,9 +269,16 @@ export function DialogProvider(props: ParentProps) { onMouseUp={!Flag.OPENCODE_EXPERIMENTAL_DISABLE_COPY_ON_SELECT ? copySelection : undefined} > - value.clear()} size={value.size}> + {/* altimate_change start — fixes #1301: backdrop click is a USER dismissal, same as + Escape. `dismiss()` reports "dismiss" to the close guard, unlike every other + `clear()`/`replace()` call site (self-close, or an unrelated feature taking over + the stack), which stays "programmatic". Ctrl+C is neither: it reports its own + "interrupt" reason (see `closeTop` below) and deliberately does not record a + decline, since quitting the app is not the same as dismissing this dialog. */} + value.dismiss()} size={value.size}> {value.stack.at(-1)!.element} + {/* altimate_change end */} diff --git a/packages/tui/src/util/signal.ts b/packages/tui/src/util/signal.ts index e28c680cd4..ce7deb533f 100644 --- a/packages/tui/src/util/signal.ts +++ b/packages/tui/src/util/signal.ts @@ -1,5 +1,48 @@ import { createEffect, createSignal, on, onCleanup, type Accessor } from "solid-js" +// altimate_change start — Codex re-review round 8 (cycle-stability/ready-pending test coverage) / +// round 9 (stale-revision cancellation): a reactive defer-then-retry primitive — call `.defer()` +// when a caller can't act yet (e.g. a readiness signal is still pending), and the wrapped `retry` +// callback fires automatically, exactly once, the NEXT time `pending()` reads false. Extracted as +// a standalone, importable function so the SAME production code path is exercised by both a real +// consumer (component/prompt/index.tsx's submit gate — see `readyPending`'s declaration there) +// and its test (test/context/ready-pending.test.tsx) — the test was previously a hand-rolled +// reimplementation of this exact shape, which meant reverting the real fix in prompt/index.tsx +// left the test passing regardless, since it never touched production code at all. +// +// `options.getRevision`, if given, is called ONCE at `.defer()` time and again right before +// `retry()` would fire — if the two differ (by JSON equality), `retry()` is skipped entirely +// rather than fired against stale state. The defer-time value is captured SERIALIZED, never as a +// reference: Solid's `unwrap` hands back the store's raw underlying object, the very one later +// edits mutate in place, so holding it and stringifying both sides at retry time compared the +// object to itself and could never see an edit (cursor 3987286236 / cubic 3987320771). This is what +// component/prompt/index.tsx's submit gate uses to snapshot the prompt (text + attachments) at +// the moment a submission defers: without it, a user who deferred prompt A, then edited the box +// to B WITHOUT pressing Enter again, would have B silently auto-submitted the instant readiness +// resolved — a send the user never asked for, not a resend of the one they did. +export function createDeferredRetry( + pending: Accessor, + retry: () => void, + options?: { getRevision?: () => T }, +) { + let deferred = false + let capturedRevision: string | undefined + const snapshot = () => (options?.getRevision ? JSON.stringify(options.getRevision()) : undefined) + createEffect(() => { + if (pending() || !deferred) return + deferred = false + if (options?.getRevision && snapshot() !== capturedRevision) return + retry() + }) + return { + defer() { + deferred = true + capturedRevision = snapshot() + }, + } +} +// altimate_change end + export function createDebouncedSignal(value: T, ms: number): [Accessor, (value: T) => void] { const [get, set] = createSignal(value) let timer: ReturnType | undefined diff --git a/packages/tui/test/cli/tui/dialog-altimate-base.test.tsx b/packages/tui/test/cli/tui/dialog-altimate-base.test.tsx index c9dd241713..3772d2c23d 100644 --- a/packages/tui/test/cli/tui/dialog-altimate-base.test.tsx +++ b/packages/tui/test/cli/tui/dialog-altimate-base.test.tsx @@ -3,9 +3,17 @@ import { createDefaultOpenTuiKeymap } from "@opentui/keymap/opentui" import { testRender, useRenderer } from "@opentui/solid" import { expect, test } from "bun:test" import { onCleanup, onMount } from "solid-js" +import { mkdir } from "node:fs/promises" +import path from "node:path" import { createTuiResolvedConfig } from "../../fixture/tui-runtime" import { TestTuiContexts } from "../../fixture/tui-environment" import { createEventSource, createFetch, directory, json } from "../../fixture/tui-sdk" +// altimate_change — fixes #1301 (Codex review round 2, D): the harness now performs REAL +// kv/model.json writes (see `declinedInKv`/`declinedInModel` below), so it needs a per-mount +// isolated state directory — `TestTuiContexts`'s default `state` path is a single fixed +// `/tmp/opencode/state` shared by every test in the process (see `dialog-scan-gate.test.tsx` for +// the same pattern with a real DialogProvider + kv fixture). +import { tmpdir } from "../../fixture/fixture" import type { OnboardingTelemetryEvent } from "../../../src/context/onboarding-telemetry" async function waitUntil(predicate: () => boolean, timeout = 2_000) { @@ -26,6 +34,16 @@ async function mountConfirm( >) modelAvailable?: boolean origin?: "welcome" | "migration" + // altimate_change start — fixes #1301: broadened migration eligibility test support + // Whether the harness marks first-run active before mounting. Every prior test relied on this + // always being true; migration's telemetry must now also fire when it is NOT (migration is + // reachable on a returning launch, which is never "first run"). + markFirstRun?: boolean + // The free public Zen model presented as the (sole, when `modelAvailable: false`) opencode + // provider model — defaults to the retired Big Pickle id so every existing test is unaffected. + // Swap it to prove the migration copy names whichever free model is actually current. + zenModel?: { id: string; name: string; family?: string } + // altimate_change end } = {}, ) { const [ @@ -39,7 +57,7 @@ async function mountConfirm( }, { OnboardingTelemetryProvider }, { ArgsProvider }, - { KVProvider }, + { KVProvider, useKV }, { ThemeProvider }, { TuiConfigProvider }, { ToastProvider }, @@ -47,7 +65,10 @@ async function mountConfirm( { AltimateBaseConsentProvider }, { ProjectProvider }, { SyncProvider }, - { LocalProvider }, + // altimate_change — fixes #1301 (Codex review round 2, D): `useLocal`/`ALTIMATE_BASE_MIGRATION_DECLINED_KEY` + // let the harness assert the ACTUAL persisted decline state (kv + model.json) an app.tsx + // `onDecline` would produce, instead of only whether a mock callback was invoked. + { LocalProvider, useLocal, ALTIMATE_BASE_MIGRATION_DECLINED_KEY }, { OpencodeKeymapProvider, registerOpencodeKeymap }, { ExitProvider }, { RouteProvider }, @@ -72,12 +93,33 @@ async function mountConfirm( import("../../../src/context/route"), ]) + // altimate_change start — fixes #1301 (Codex review round 2, D): isolated per-mount state dir + // — see the `tmpdir` import comment above. `kv.json` is pre-seeded (matching + // `dialog-scan-gate.test.tsx`) purely to avoid the harmless-but-noisy "Failed to read KV state" + // console error `kv.tsx` logs on a missing file; `model.json`'s reader doesn't log at all, so + // it isn't pre-seeded. + const tmp = await tmpdir() + const state = path.join(tmp.path, "state") + await mkdir(state, { recursive: true }) + await Bun.write(path.join(state, "kv.json"), "{}") + // altimate_change end + resetSetupComplete() - markFirstRunActive() + // altimate_change — fixes #1301: default preserved (every prior test relies on it), but a test + // can now mount without first-run active to prove migration telemetry fires regardless. + if (input.markFirstRun ?? true) markFirstRunActive() const events: OnboardingTelemetryEvent[] = [] const registrations: true[] = [] const declines: true[] = [] let replaceDialog = () => false + // altimate_change — fixes #1301 (Codex review round 2, D): populated inside `OpenConfirm` below + // (rendered inside `KVProvider`/`LocalProvider`), so the harness can assert the actual + // persisted decline state, not only whether a mock callback fired. + let declinedInKv = () => false + let declinedInModel = () => false + // altimate_change — PR #1302 review (CodeRabbit + cubic "Await the atomic writes before + // disposing the state directory"): populated inside `OpenConfirm` below. + let waitForPersistence: () => Promise = () => Promise.resolve() const model = { id: "altimate-base", providerID: "altimate-free", @@ -89,14 +131,17 @@ async function mountConfirm( limit: { context: 65_536, output: 4_096 }, } const provider = { id: "altimate-free", name: "Altimate", models: { "altimate-base": model }, env: [] } + // altimate_change — fixes #1301: the opencode-provider free model defaults to the retired Big + // Pickle id (unchanged for every existing test) but can be swapped to any other free Zen model. + const zenModel = input.zenModel ?? { id: "big-pickle", name: "Big Pickle", family: "glm" } const bigPickle = { ...model, - id: "big-pickle", + id: zenModel.id, providerID: "opencode", - name: "Big Pickle", - family: "glm", + name: zenModel.name, + family: zenModel.family ?? "opencode", } - const openCodeProvider = { id: "opencode", name: "Legacy Zen", models: { "big-pickle": bigPickle }, env: [] } + const openCodeProvider = { id: "opencode", name: "Legacy Zen", models: { [zenModel.id]: bigPickle }, env: [] } const inner = createFetch((url) => { if (url.pathname === "/instance/dispose") return json({}) if (url.pathname === "/config/providers") { @@ -125,17 +170,42 @@ async function mountConfirm( function OpenConfirm() { const dialog = useDialog() + // altimate_change start — fixes #1301 (Codex review round 2, D): mirror app.tsx's REAL + // migration `onDecline` (kv.set + local.model.declineManagedBaseDefault()) instead of only + // recording that the callback fired, so tests can assert the actual persisted state a real + // launch would see — not just that a mock array grew. + const kv = useKV() + const local = useLocal() + declinedInKv = () => kv.get(ALTIMATE_BASE_MIGRATION_DECLINED_KEY, false) + declinedInModel = () => local.model.declinedManagedBaseDefault() + // altimate_change — PR #1302 review (CodeRabbit + cubic; Codex review round 2, P2): both + // real write queues, so `cleanup()` can await the actual persistence instead of a fixed + // delay before disposing the tmp state directory. Safe to `Promise.all` (rather than + // `allSettled`) here: `local.model.persisted()` now internally awaits `allSettled` over + // EVERY outstanding model write (not just the latest — a single reassigned promise + // previously dropped earlier in-flight writes from what this waited for), and `kv.flush()` + // returns kv.tsx's own queued write chain, which already swallows its own errors + // internally — neither can reject. + waitForPersistence = () => Promise.all([local.model.persisted(), kv.flush()]) replaceDialog = () => dialog.replace(() => Session list replacement) onMount(() => dialog.replace(() => ( - declines.push(true)} /> + { + declines.push(true) + kv.set(ALTIMATE_BASE_MIGRATION_DECLINED_KEY, true) + local.model.declineManagedBaseDefault() + }} + /> )), ) + // altimate_change end return null } return ( - + {}}> @@ -193,10 +263,22 @@ async function mountConfirm( setupComplete: useSetupComplete(), registrations: () => registrations, declines: () => declines, + // altimate_change — fixes #1301 (Codex review round 2, D): actual persisted decline state. + declinedInKv: () => declinedInKv(), + declinedInModel: () => declinedInModel(), replaceDialog: () => replaceDialog(), - cleanup() { + async cleanup() { app.renderer.destroy() resetSetupComplete() + // altimate_change — PR #1302 review (CodeRabbit + cubic "Await the atomic writes before + // disposing the state directory"): `local.model`'s `save()` and `kv.tsx`'s `set()` each now + // expose their in-flight write (`persisted()`/`flush()` — see `waitForPersistence` above). + // A decline persisted just before this runs previously still had its write in flight when a + // fixed `Bun.sleep(20)` disposed the tmp dir out from under it (an EINVAL/ENOENT from + // `writeJsonAtomic`, surfacing as an unhandled rejection misattributed to whichever test + // happened to be running when it resolved). Actually awaiting the writes removes the guess. + await waitForPersistence().catch(() => {}) + await tmp[Symbol.asyncDispose]() }, } } @@ -223,7 +305,7 @@ test.serial("Altimate Base shows the privacy disclosure before registration and expect(confirm.registrations()).toHaveLength(0) expect(confirm.events).toEqual([{ name: "altimate_base_confirm_shown", origin: "welcome" }]) } finally { - confirm.cleanup() + await confirm.cleanup() } }) @@ -235,37 +317,205 @@ test.serial("Return declines, because No is the default — it must never regist // else in this suite exercises it, so this path was previously unverified in either direction. confirm.app.mockInput.pressKey("RETURN") await waitUntil(() => confirm.events.some((event) => event.name === "altimate_base_choice")) - expect(confirm.events).toContainEqual({ name: "altimate_base_choice", choice: "cancel" }) + expect(confirm.events).toContainEqual({ name: "altimate_base_choice", choice: "cancel", origin: "welcome" }) // The property that matters: an unread Return cannot opt the installation into request logging. expect(confirm.registrations()).toHaveLength(0) } finally { - confirm.cleanup() + await confirm.cleanup() } }) test.serial( - "the Big Pickle migration reuses consent, stays out of first-run telemetry, and routes No to the picker", + // altimate_change — fixes #1301: migration telemetry is no longer suppressed — see the "even + // when first-run is not active" variant below for why that matters. + "the migration disclosure reuses consent, reports its own telemetry, and routes explicit No to the picker", async () => { const confirm = await mountConfirm({ origin: "migration" }) try { const frame = confirm.app.captureCharFrame() expect(frame).toContain("No — pick something else") expect(frame.replace(/\s+/g, " ")).toContain("Requests and responses may be logged and used") - expect(confirm.events).toEqual([]) + expect(confirm.events).toEqual([{ name: "altimate_base_confirm_shown", origin: "migration" }]) confirm.app.mockInput.pressKey("n") await waitUntil(() => confirm.declines().length === 1) expect(confirm.registrations()).toHaveLength(0) + // altimate_change — fixes #1301 (Codex review round 2, D): assert the ACTUAL persisted + // state (kv + model.json, both through the real `local.model.declineManagedBaseDefault()`), + // not only that a mock callback was invoked. + expect(confirm.declinedInKv()).toBe(true) + expect(confirm.declinedInModel()).toBe(true) + expect(confirm.events).toContainEqual({ name: "altimate_base_choice", choice: "cancel", origin: "migration" }) // altimate_change — "No — pick something else" must actually route somewhere: Big Pickle is // retired, so declining the migration prompt lands the user in the curated picker instead of // silently leaving the dialog cleared (the label used to promise a re-pick that never // happened). await waitUntil(() => confirm.events.some((event) => event.name === "model_picker_shown")) - expect(confirm.events).toEqual([{ name: "model_picker_shown", trigger: "altimate_base_back" }]) + expect(confirm.events).toContainEqual({ name: "model_picker_shown", trigger: "altimate_base_back" }) await confirm.app.renderOnce() expect(confirm.app.captureCharFrame()).toContain("Altimate LLM Gateway") } finally { - confirm.cleanup() + await confirm.cleanup() + } + }, +) + +test.serial( + "migration telemetry fires even when first-run is not active, unlike welcome/model", + async () => { + // altimate_change — fixes #1301: migration is reachable on a returning (non-first-run) + // launch — the whole point of the fix — so its telemetry must not depend on + // `firstRunActive()` the way "welcome"/"model" origins' does. + const confirm = await mountConfirm({ origin: "migration", markFirstRun: false }) + try { + expect(confirm.events).toEqual([{ name: "altimate_base_confirm_shown", origin: "migration" }]) + } finally { + await confirm.cleanup() + } + }, +) + +test.serial( + "Escape on the migration disclosure persists the decline and opens the welcome picker, not a bare dismissal", + async () => { + // altimate_change — fixes #1301: DialogProvider's keymap binding closes the dialog BEFORE the + // component's own `useKeyboard` ever sees Escape/Ctrl-C, so this must route through the close + // guard — see `releaseCloseGuard` in altimate-onboarding.tsx. + const confirm = await mountConfirm({ origin: "migration" }) + try { + // `pressKey("escape")` (lowercase) types the literal LETTERS e-s-c-a-p-e — it is not the + // Escape key (see `KeyCodes.ESCAPE`/`resolveKeyInput` in @opentui/core's mock-keys helper). + // `pressEscape()` sends the actual key. + confirm.app.mockInput.pressEscape() + await waitUntil(() => confirm.declines().length === 1) + expect(confirm.registrations()).toHaveLength(0) + // altimate_change — fixes #1301 (Codex review round 2, D): the actual persisted state a + // real launch's `Provider.defaultModel()`/ACP would read, not only the mock callback. + expect(confirm.declinedInKv()).toBe(true) + expect(confirm.declinedInModel()).toBe(true) + await waitUntil(() => confirm.events.some((event) => event.name === "model_picker_shown")) + await confirm.app.renderOnce() + const frame = confirm.app.captureCharFrame() + expect(frame).toContain("Select a provider") + expect(frame).toContain("Altimate LLM Gateway") + } finally { + await confirm.cleanup() + } + }, +) + +test.serial( + "Ctrl+C on the migration disclosure closes it without deciding anything, unlike Escape", + async () => { + // altimate_change — PR review round 3: Ctrl+C is a "get me out" gesture (quitting the app), + // not "I decline Altimate Base specifically" the way Escape on THIS dialog is. Before + // `dialog.tsx` gave it its own "interrupt" reason, Ctrl+C was treated identically to Escape + // ("dismiss"), so quitting with Ctrl+C twice while this dialog was open queued `no()` (persist + // + picker takeover) on the FIRST Ctrl+C, recording a refusal the user never made. + const confirm = await mountConfirm({ origin: "migration" }) + try { + // The established way this suite sends a real Ctrl+C through the keymap (see the + // busy-state test below) — not `pressKey("c")` alone, which is just the letter "c". + confirm.app.mockInput.pressKey("c", { ctrl: true }) + await confirm.app.renderOnce() + // The dialog closes (it was the only entry on the stack) without being replaced by + // anything — no forced picker takeover, unlike Escape. + expect(confirm.app.captureCharFrame()).not.toContain("Use Altimate Base?") + expect(confirm.declines()).toHaveLength(0) + expect(confirm.registrations()).toHaveLength(0) + expect(confirm.events.some((event) => event.name === "altimate_base_choice")).toBe(false) + expect(confirm.events.some((event) => event.name === "model_picker_shown")).toBe(false) + // altimate_change — the actual persisted state, which is what a real headless/server + // launch's `Provider.defaultModel()`/ACP would read — not only the mock callback. + expect(confirm.declinedInKv()).toBe(false) + expect(confirm.declinedInModel()).toBe(false) + } finally { + await confirm.cleanup() + } + }, +) + +test.serial( + "the visible mouse esc label on the migration disclosure persists the decline and opens the picker, same as keyboard Escape", + async () => { + // altimate_change — fixes #1301 (Codex review round 2, P2): this visible label used to call a + // bare `dialog.clear()` for every origin, including migration — so clicking it silently + // skipped both the decline persistence AND the picker takeover that keyboard Escape produces, + // leaving a later headless/server launch free to pick Base again after a partial + // registration. It must now behave exactly like Escape for `origin === "migration"`. + const confirm = await mountConfirm({ origin: "migration" }) + try { + const frame = confirm.app.captureCharFrame() + expect(frame).toContain("esc") + // The "esc" label sits on the same row as the dialog title, near its right edge. + const escRow = frame.split("\n").findIndex((line) => line.includes("Use Altimate Base?")) + expect(escRow).toBeGreaterThanOrEqual(0) + const escColumn = frame.split("\n")[escRow].indexOf("esc") + // altimate_change — PR #1302 review (cubic P3): if the label ever moves off this row, + // `indexOf` returns -1 and the click silently misses — fail here with the real cause + // instead of a generic "timed out waiting for condition" from the assertion below. + expect(escColumn).toBeGreaterThanOrEqual(0) + await confirm.app.mockMouse.click(escColumn, escRow) + await waitUntil(() => confirm.declines().length === 1) + expect(confirm.registrations()).toHaveLength(0) + expect(confirm.declinedInKv()).toBe(true) + expect(confirm.declinedInModel()).toBe(true) + await waitUntil(() => confirm.events.some((event) => event.name === "model_picker_shown")) + await confirm.app.renderOnce() + expect(confirm.app.captureCharFrame()).toContain("Select a provider") + } finally { + await confirm.cleanup() + } + }, +) + +test.serial( + "a programmatic replace of the migration dialog succeeds and does not persist a decline", + async () => { + // altimate_change — fixes #1301 (Codex review round 2, P2): an unrelated feature (command + // palette, session list) replacing the dialog stack while the migration disclosure is open is + // not the user declining Altimate Base — it never dismissed THIS dialog, unlike keyboard + // Escape/Ctrl+C, the backdrop click (`dialog.tsx`'s `dismiss()`), or the visible mouse "esc" + // label, all of which now route through `no()` (see the tests above). Before the original + // fix, the close guard queued `no()` for every guarded close, including this one. + const confirm = await mountConfirm({ origin: "migration" }) + try { + expect(confirm.replaceDialog()).toBe(true) + await confirm.app.renderOnce() + expect(confirm.app.captureCharFrame()).toContain("Session list replacement") + expect(confirm.declines()).toHaveLength(0) + // altimate_change — fixes #1301 (Codex review round 2, D): the actual persisted state, + // which is what a real headless/server launch would read — not only the mock callback. + expect(confirm.declinedInKv()).toBe(false) + expect(confirm.declinedInModel()).toBe(false) + expect(confirm.registrations()).toHaveLength(0) + expect(confirm.events.some((event) => event.name === "model_picker_shown")).toBe(false) + } finally { + await confirm.cleanup() + } + }, +) + +test.serial( + "the migration copy names the current free model instead of always naming Big Pickle", + async () => { + // altimate_change — fixes #1301: migration now also covers implicit free public Zen + // defaults besides Big Pickle, so the copy must say which model is actually being moved. + // `modelAvailable: false` makes this swapped-in model the ONLY (hence current) provider + // entry, sidestepping any ambiguity in which provider the fallback picks first. + const confirm = await mountConfirm({ + origin: "migration", + modelAvailable: false, + zenModel: { id: "nemotron-3.5-lightning-free", name: "Nemotron 3.5 Lightning (Free)" }, + }) + try { + const flat = confirm.app.captureCharFrame().replace(/\s+/g, " ") + expect(flat).toContain( + "Your default model, Nemotron 3.5 Lightning (Free), is a public free model. Altimate Base is the free model Altimate hosts for data work.", + ) + expect(flat).not.toContain("Big Pickle has been retired.") + } finally { + await confirm.cleanup() } }, ) @@ -275,7 +525,7 @@ test.serial("declining Altimate Base makes no registration request, and Big Pick try { confirm.app.mockInput.pressKey("n") await waitUntil(() => confirm.events.some((event) => event.name === "altimate_base_choice")) - expect(confirm.events).toContainEqual({ name: "altimate_base_choice", choice: "cancel" }) + expect(confirm.events).toContainEqual({ name: "altimate_base_choice", choice: "cancel", origin: "welcome" }) expect(confirm.registrations()).toHaveLength(0) confirm.app.mockInput.pressKey("/") await confirm.app.renderOnce() @@ -285,7 +535,7 @@ test.serial("declining Altimate Base makes no registration request, and Big Pick expect(confirm.app.captureCharFrame()).not.toContain("Big Pickle") expect(confirm.registrations()).toHaveLength(0) } finally { - confirm.cleanup() + await confirm.cleanup() } }) @@ -295,11 +545,15 @@ test.serial("accepting registers once through the private host operation and com confirm.app.mockInput.pressKey("y") await waitUntil(() => confirm.setupComplete()) expect(confirm.registrations()).toHaveLength(1) - expect(confirm.events).toContainEqual({ name: "altimate_base_choice", choice: "accept" }) - expect(confirm.events).toContainEqual({ name: "altimate_base_register_result", result: "success" }) + expect(confirm.events).toContainEqual({ name: "altimate_base_choice", choice: "accept", origin: "welcome" }) + expect(confirm.events).toContainEqual({ + name: "altimate_base_register_result", + result: "success", + origin: "welcome", + }) expect(confirm.events.filter((event) => event.name === "altimate_base_choice")).toHaveLength(1) } finally { - confirm.cleanup() + await confirm.cleanup() } }) @@ -313,7 +567,7 @@ test.serial("registration without a usable model remains incomplete and visibly expect(confirm.setupComplete()).toBe(false) expect(confirm.app.captureCharFrame()).toContain("ready yet. Try again") } finally { - confirm.cleanup() + await confirm.cleanup() } }) @@ -328,10 +582,14 @@ test.serial("rate-limited registration stays recoverable and reports a typed out await confirm.app.renderOnce() expect(confirm.setupComplete()).toBe(false) expect(confirm.registrations()).toHaveLength(1) - expect(confirm.events).toContainEqual({ name: "altimate_base_register_result", result: "rate_limited" }) + expect(confirm.events).toContainEqual({ + name: "altimate_base_register_result", + result: "rate_limited", + origin: "welcome", + }) expect(confirm.app.captureCharFrame()).toContain("Too many Altimate Base") } finally { - confirm.cleanup() + await confirm.cleanup() } }) @@ -356,7 +614,11 @@ test.serial("dismissal keys and backdrop clicks are ignored while registration i expect(confirm.replaceDialog()).toBe(false) await confirm.app.renderOnce() expect(confirm.app.captureCharFrame()).not.toContain("Session list replacement") - confirm.app.mockInput.pressKey("escape") + // altimate_change — fixes #1301: `pressKey("escape")` (lowercase) sends the literal letters + // e-s-c-a-p-e, not the Escape key (see the comment on the migration Escape test below); this + // assertion happened to hold either way since typing those letters while busy is also a + // no-op, but `pressEscape()` is what actually exercises the key this test is named for. + confirm.app.mockInput.pressEscape() await confirm.app.renderOnce() expect(confirm.app.captureCharFrame()).toContain("Setting up…") confirm.app.mockInput.pressKey("c", { ctrl: true }) @@ -369,6 +631,6 @@ test.serial("dismissal keys and backdrop clicks are ignored while registration i finish({ ok: true }) await waitUntil(() => confirm.setupComplete()) } finally { - confirm.cleanup() + await confirm.cleanup() } }) diff --git a/packages/tui/test/component/welcome-panel.test.tsx b/packages/tui/test/component/welcome-panel.test.tsx index 3909255325..75f8118d80 100644 --- a/packages/tui/test/component/welcome-panel.test.tsx +++ b/packages/tui/test/component/welcome-panel.test.tsx @@ -8,6 +8,9 @@ import { FULL_MIN_HEIGHT, FULL_MIN_WIDTH, MEDIUM_MIN_WIDTH } from "../../src/com import { ArgsProvider } from "../../src/context/args" import { ExitProvider } from "../../src/context/exit" import { KVProvider } from "../../src/context/kv" +// altimate_change — fixes #1301 (Codex review, P2): `useReady()` now also calls `useLocal()` +// (`hasUsableFreeDefault`), so `WelcomePanel` needs `LocalProvider` in its tree like the real app. +import { LocalProvider } from "../../src/context/local" import { ProjectProvider } from "../../src/context/project" import { RouteProvider } from "../../src/context/route" import { SDKProvider } from "../../src/context/sdk" @@ -40,7 +43,9 @@ async function renderPanel(availableWidth: number, availableHeight: number) { - + + + diff --git a/packages/tui/test/context/cycle-stability.test.tsx b/packages/tui/test/context/cycle-stability.test.tsx new file mode 100644 index 0000000000..4e9fec9cab --- /dev/null +++ b/packages/tui/test/context/cycle-stability.test.tsx @@ -0,0 +1,294 @@ +/** @jsxImportSource @opentui/solid */ +// altimate_change start — Codex HOLD finding 2 (+ re-review rounds 8-9): `cycle()` must traverse a +// STABLE order, and that order must stay correct as `recent` changes for reasons OTHER than +// cycle() itself. +// +// Passing `{ recent: true }` to `selectModel` (round 6, cubic 3986171198/cursor 3986044810) fixed +// a real cross-surface bug (TUI vs headless/ACP default divergence after a cycle) but, on its +// own, broke `cycle()`'s OWN navigation: reordering `recent` on every pick means the very next +// press reads its "next" index off a list that just reshuffled out from under it. Codex caught +// this by actually executing it: cycling forward through [A, B, C] starting from B went +// B -> A -> B forever instead of visiting every model. The unit test previously at +// test/context/local.test.ts:317 ("cycling persists the launch default via recents order") only +// called `recentModels()` directly — it asserted the persistence half of the fix and would have +// passed on the broken code, never exercising `cycle()` itself. +// +// Round 7's fix (a `cycleOrder` snapshot, re-captured only when the CURRENT model fell out of +// it) still went stale after a PICKER selection that reordered `recent` without also knocking +// the current model out of the old snapshot — Codex's re-review reproduced it: `[A, B, C]`, +// cycle once, then pick D and A via the picker, and D stays permanently unreachable by cycling +// (the round-7 check never re-fires because A is still present in the stale snapshot). The +// `cycleOrderVersion` counter (local.tsx) fixes this. +// +// Round 9 (cubic 3987174885, repo rule: no order-dependent tests): this file used to spread the +// scenario across three `test.serial` blocks sharing one `beforeAll`/`afterAll` mount — cycle() +// is inherently a sequence of interactions, so splitting the scenario into separately-named +// blocks made each one implicitly depend on the ones before it, which broke a `-t` filter or +// `--only-failures` run of just one of them. Folded into ONE `test()`, with the mount local to +// it and labelled phases (a `phase()` helper below just for readable failure messages — plain +// comments would work too, but this makes an assertion failure show exactly which stage of the +// sequence it happened in). A second/third independent heavy provider-tree mount in this same +// file was ALSO found to reproducibly hang during its own bootstrap (a resource-contention issue +// unrelated to anything under test) — folding into one test with one mount sidesteps that too. +// +// Round 9 (cubic 3986917361, repo rule: tests must not touch real global state): `kv.tsx`'s +// `Flock.withLock` lock directory is derived from `Global.Path.state` (packages/core/src/global.ts), +// which used to have no test-isolation override at all — only `Global.Path.home` did. This test's +// `kv.json` FILE itself was always correctly isolated (`paths.state`, via `TestTuiContexts`), but +// the LOCK it takes while reading/writing that file was not — it could still land in the real, +// current developer's global state directory. Setting `OPENCODE_TEST_STATE_HOME` (mirroring +// `OPENCODE_TEST_HOME`'s established pattern, used throughout this codebase's tests) around the +// mount redirects both `Global.Path.state` and `Flock`'s lock root to this test's own throwaway +// temp dir instead. +import { testRender } from "@opentui/solid" +import { expect, test } from "bun:test" +import { mkdir } from "node:fs/promises" +import path from "node:path" +import { tmpdir } from "../fixture/fixture" +import { TestTuiContexts } from "../fixture/tui-environment" +import { createTuiResolvedConfig } from "../fixture/tui-runtime" +import { createEventSource, createFetch, directory, json } from "../fixture/tui-sdk" + +async function waitUntil(predicate: () => boolean, timeout = 2_000) { + const started = Date.now() + while (!predicate()) { + if (Date.now() - started > timeout) throw new Error("timed out waiting for condition") + await Bun.sleep(5) + } +} + +const MODEL_A = { providerID: "opencode", modelID: "model-a" } +const MODEL_B = { providerID: "opencode", modelID: "model-b" } +const MODEL_C = { providerID: "opencode", modelID: "model-c" } +const MODEL_D = { providerID: "opencode", modelID: "model-d" } + +function makeModel(id: string) { + return { + id, + providerID: "opencode", + name: id, + family: "opencode", + status: "active", + capabilities: {}, + cost: { input: 0, output: 0 }, + limit: { context: 65_536, output: 4_096 }, + } +} + +async function mount() { + const [ + { KVProvider }, + { LocalProvider, useLocal }, + { ArgsProvider }, + { ThemeProvider }, + { ToastProvider }, + { SDKProvider }, + { ProjectProvider }, + { SyncProvider }, + { RouteProvider }, + { ExitProvider }, + { TuiConfigProvider }, + ] = await Promise.all([ + import("../../src/context/kv"), + import("../../src/context/local"), + import("../../src/context/args"), + import("../../src/context/theme"), + import("../../src/ui/toast"), + import("../../src/context/sdk"), + import("../../src/context/project"), + import("../../src/context/sync"), + import("../../src/context/route"), + import("../../src/context/exit"), + import("../../src/config"), + ]) + + const tmp = await tmpdir() + const state = path.join(tmp.path, "state") + await mkdir(state, { recursive: true }) + await Bun.write(path.join(state, "kv.json"), "{}") + // Three free models, all persisted as recents in order A, B, C — this is the "order as of TUI + // launch" `cycle()` must traverse, independent of how it self-reorders `recent` on each pick. + // A fourth (D) is registered with the provider but NOT in the initial `recent` — it only + // enters via a later picker selection, in the round-8 regression phase below. + await Bun.write(path.join(state, "model.json"), JSON.stringify({ recent: [MODEL_A, MODEL_B, MODEL_C] })) + + const openCodeProvider = { + id: "opencode", + name: "Legacy Zen", + models: { + "model-a": makeModel("model-a"), + "model-b": makeModel("model-b"), + "model-c": makeModel("model-c"), + "model-d": makeModel("model-d"), + }, + env: [], + } + const agent = { + name: "build", + mode: "primary" as const, + hidden: false, + permission: {}, + options: {}, + } + const inner = createFetch((url) => { + if (url.pathname === "/instance/dispose") return json({}) + if (url.pathname === "/config/providers") return json({ providers: [openCodeProvider], default: {} }) + if (url.pathname === "/provider") return json({ all: [openCodeProvider], default: {}, connected: ["opencode"] }) + if (url.pathname === "/agent") return json([agent]) + if (url.pathname === "/project/proj_test/directories") return json([]) + return undefined + }) + const source = createEventSource() + + let localAccessor: ReturnType | undefined + function Capture() { + localAccessor = useLocal() + return null + } + + const app = await testRender(() => ( + + {}}> + + + + + + + + + + + + + + + + + + + + + + + + )) + await app.renderOnce() + await waitUntil(() => localAccessor !== undefined && localAccessor.model.ready) + const local = localAccessor! + + // `fallbackModel()` resolves the launch default to `recent[0]` (model-a) with nothing else + // configured — confirm the harness actually started where the test assumes before asserting + // anything about `cycle()`. + await waitUntil(() => local.model.current()?.modelID === "model-a") + // Move to B WITHOUT touching `recent`'s order (no `recent: true`) — matches Codex's repro + // shape: current = B, persisted recent order = [A, B, C]. + local.model.set(MODEL_B) + await waitUntil(() => local.model.current()?.modelID === "model-b") + + return { + local, + async cleanup() { + app.renderer.destroy() + await local.model.persisted().catch(() => {}) + await tmp[Symbol.asyncDispose]() + }, + } +} + +/** Labels a phase for a clearer assertion-failure message; otherwise a no-op. */ +function phase(name: string, fn: () => void | Promise) { + try { + return fn() + } catch (err) { + throw err instanceof Error ? new Error(`[phase: ${name}] ${err.message}`, { cause: err }) : err + } +} + +test("cycle() traverses a stable order, re-discovers picker-added entries after recents reorder, and persists picks to the front of recent (Codex HOLD finding 2 + re-review rounds 8-9)", async () => { + const originalStateHome = process.env.OPENCODE_TEST_STATE_HOME + const isolatedState = await tmpdir() + process.env.OPENCODE_TEST_STATE_HOME = isolatedState.path + + const mounted = await mount() + try { + // Phase 1 (Codex HOLD finding 2): starting state (from `mount()`) is current = B, + // recent = [A, B, C]. Forward cycling must visit C then A (not bounce back to A immediately + // — the bug Codex's own execution caught was B -> A -> B) and complete the traversal back to + // B on the third press, having visited every one of the three models exactly once. + // Ends: current = B, recent = [B, A, C]. + await phase("1: stable traversal order", async () => { + const visited: string[] = [] + mounted.local.model.cycle(1) + visited.push(mounted.local.model.current()!.modelID) + mounted.local.model.cycle(1) + visited.push(mounted.local.model.current()!.modelID) + mounted.local.model.cycle(1) + visited.push(mounted.local.model.current()!.modelID) + + expect(visited).toEqual(["model-c", "model-a", "model-b"]) + expect(new Set(visited).size).toBe(3) + }) + + // Phase 2: continues from phase 1 (current = B, recent = [B, A, C], cycleOrder still + // [A, B, C] — unchanged, nothing but cycle() itself has written to `recent` so far). Every + // cycle() pick must still move the picked model to the front of PERSISTED `recent` — that's + // the only state headless/ACP default resolution reads. + // Ends: current = C, recent = [C, B, A]. + await phase("2: picks move to the front of persisted recent", async () => { + mounted.local.model.cycle(1) + await waitUntil(() => mounted.local.model.recent()[0]?.modelID === "model-c") + expect(mounted.local.model.recent()[0]).toEqual({ providerID: "opencode", modelID: "model-c" }) + }) + + // Phase 3 (Codex re-review round 8): continues from phase 2 (current = C, + // recent = [C, B, A], cycleOrder still [A, B, C] — still unchanged). Reproduces Codex's exact + // repro from here: pick D and A via the PICKER (an explicit /model selection with + // `recent: true`, like DialogModel uses). Round 7's fix only re-captured `cycleOrder` when + // the CURRENT model fell OUT of the stale snapshot — A stays present in the stale + // `[A, B, C]` snapshot throughout, so that check never fires, and D — never in that snapshot + // at all — stays permanently unreachable by cycling. `cycleOrderVersion` (its declaration in + // local.tsx) fixes this: it also invalidates on ANY external write to `recent`, picker + // included. + await phase("3: picker selections reorder recent", async () => { + mounted.local.model.set(MODEL_D, { recent: true }) + await waitUntil(() => mounted.local.model.current()?.modelID === "model-d") + mounted.local.model.set(MODEL_A, { recent: true }) + await waitUntil(() => mounted.local.model.current()?.modelID === "model-a") + expect(mounted.local.model.recent().map((m) => m.modelID)).toEqual([ + "model-a", + "model-d", + "model-c", + "model-b", + ]) + }) + + // Phase 4: cycleOrder must re-capture from the CURRENT [A, D, C, B] here (recentsVersion + // moved past cycleOrderVersion since the last cycle() call, from phase 3's two picker picks) + // — forward from A lands on D. The OLD bug: the stale `[A, B, C]` snapshot's "next after A" + // was B, and D was never reachable from it at all. + await phase("4: cycle() re-discovers the picker-added model", async () => { + mounted.local.model.cycle(1) + await waitUntil(() => mounted.local.model.current()?.modelID === "model-d") + }) + + // Phase 5: the newly re-captured order stays stable for subsequent presses, same guarantee + // as phase 1 — visits C then B then wraps back to A. + await phase("5: the re-captured order stays stable", async () => { + const visitedAfterD: string[] = [] + mounted.local.model.cycle(1) + visitedAfterD.push(mounted.local.model.current()!.modelID) + mounted.local.model.cycle(1) + visitedAfterD.push(mounted.local.model.current()!.modelID) + mounted.local.model.cycle(1) + visitedAfterD.push(mounted.local.model.current()!.modelID) + expect(visitedAfterD).toEqual(["model-c", "model-b", "model-a"]) + }) + } finally { + await mounted.cleanup() + if (originalStateHome === undefined) delete process.env.OPENCODE_TEST_STATE_HOME + else process.env.OPENCODE_TEST_STATE_HOME = originalStateHome + await isolatedState[Symbol.asyncDispose]() + } +}) +// altimate_change end diff --git a/packages/tui/test/context/local.test.ts b/packages/tui/test/context/local.test.ts index e3c2bb7cd3..ca128b6e88 100644 --- a/packages/tui/test/context/local.test.ts +++ b/packages/tui/test/context/local.test.ts @@ -1,15 +1,48 @@ import { expect, test } from "bun:test" +// altimate_change — cubic review (3986532221): the real, module-level onboarding signals, to +// test the app.tsx call site's discriminator computation against the ACTUAL production functions +// rather than only the pure `shouldSkipOnboardingAtStartup` predicate in isolation. +import { + markFirstRunActive, + markSetupComplete, + resetSetupComplete, + useFirstRunOpenedThisLaunch, + useSetupComplete, +} from "../../src/component/altimate-onboarding" import { allowsManagedBaseDefault, ALTIMATE_BASE_MODEL, isConfirmedExplicitSelection, isExistingBigPickleSelection, + // altimate_change start — fixes #1301: broaden legacy-default migration eligibility + isFreeZenModel, + shouldOfferManagedBaseDefault, + // altimate_change end + // altimate_change start — fixes #1301 (Codex review, P2): usable-free-default predicate + isUsableFreeDefault, + // altimate_change end + // altimate_change start — Kilo review round 6: kv.ready gate for hasUsableFreeDefault() + hasUsableFreeDefaultGated, + // altimate_change end + // altimate_change start — Kilo review round 6: app.tsx startup onboarding-skip discriminator + shouldSkipOnboardingAtStartup, + shouldFireFirstRunFunnelAtStartup, + // altimate_change end + // altimate_change start — fixes #1301 (Codex review round 2, P1): migration correctness + isOwnPastPickOfFreeDefault, + shouldMoveAgentModelDuringMigration, + // altimate_change end + // altimate_change start — PR #1302 Codex review round 2 + isLegacyBigPickleModel, + isMigrationStillEligibleAfterCapture, + // altimate_change end LEGACY_BIG_PICKLE_MODEL, migrateLegacyRecentModels, parseModel, recentModels, shouldMigrateLegacyDefault, } from "../../src/context/local" +import type { ConnectedProviderShape } from "../../src/util/connected" test("parses model IDs containing slashes", () => { expect(parseModel("provider/family/model")).toEqual({ @@ -93,3 +126,395 @@ test("replaces Big Pickle recents while preserving every unrelated model and ord { providerID: "openai", modelID: "gpt-5" }, ]) }) + +// altimate_change start — fixes #1301: offer Altimate Base to every user riding an implicit free +// OpenCode Zen default, not only the retired Big Pickle id. +const NEMOTRON = { providerID: "opencode", modelID: "nemotron-3.5-lightning-free" } as const +const ZEN_PAID = { providerID: "opencode", modelID: "zen-paid" } as const + +function providersFixture(): ConnectedProviderShape[] { + return [ + { + id: "opencode", + models: { + "nemotron-3.5-lightning-free": { cost: undefined }, + "big-pickle": { cost: { input: 0 } }, + "zen-paid": { cost: { input: 3 } }, + }, + }, + { + id: "anthropic", + models: { + "claude-sonnet": { cost: { input: 3 } }, + }, + }, + ] +} + +test("identifies a free OpenCode Zen model regardless of cost being zero or absent", () => { + const providers = providersFixture() + expect(isFreeZenModel(NEMOTRON, providers)).toBe(true) + expect(isFreeZenModel(LEGACY_BIG_PICKLE_MODEL, providers)).toBe(true) +}) + +test("does not treat a paid Zen model, another provider, or a missing catalogue entry as free", () => { + const providers = providersFixture() + expect(isFreeZenModel(ZEN_PAID, providers)).toBe(false) + expect(isFreeZenModel({ providerID: "anthropic", modelID: "claude-sonnet" }, providers)).toBe(false) + expect(isFreeZenModel({ providerID: "opencode", modelID: "does-not-exist" }, providers)).toBe(false) + expect(isFreeZenModel(undefined, providers)).toBe(false) +}) + +test("offers Altimate Base for an implicit free default but never for an explicit one or an allowlisted project", () => { + const isFree = (model: { providerID: string; modelID: string }) => + isFreeZenModel(model, providersFixture()) || model.modelID === LEGACY_BIG_PICKLE_MODEL.modelID + + // Implicit free Zen default (the case the old Big-Pickle-only, recent-gated check missed). + expect(shouldOfferManagedBaseDefault(NEMOTRON, false, {}, isFree)).toBe(true) + // Implicit Big Pickle default still qualifies too. + expect(shouldOfferManagedBaseDefault(LEGACY_BIG_PICKLE_MODEL, false, {}, isFree)).toBe(true) + // A deliberate (picker-driven or CLI/config) choice is never overridden. + expect(shouldOfferManagedBaseDefault(NEMOTRON, true, {}, isFree)).toBe(false) + // A project provider allowlist that excludes the managed provider is respected. + expect(shouldOfferManagedBaseDefault(NEMOTRON, false, { anthropic: {} }, isFree)).toBe(false) + // No current model at all (e.g. no provider connected) has nothing to offer. + expect(shouldOfferManagedBaseDefault(undefined, false, {}, isFree)).toBe(false) +}) + +test("migrateLegacyRecentModels also drops the previous free default so cycling cannot bounce back onto it", () => { + expect( + migrateLegacyRecentModels( + [NEMOTRON, { providerID: "anthropic", modelID: "claude-sonnet" }, LEGACY_BIG_PICKLE_MODEL], + NEMOTRON, + ), + ).toEqual([ALTIMATE_BASE_MODEL, { providerID: "anthropic", modelID: "claude-sonnet" }]) + // Without a `previous`, behavior is unchanged from before (only Big Pickle is dropped). + expect(migrateLegacyRecentModels([NEMOTRON, LEGACY_BIG_PICKLE_MODEL])).toEqual([ALTIMATE_BASE_MODEL, NEMOTRON]) +}) +// altimate_change end + +// altimate_change start — fixes #1301 (Codex review, P1): explicitness must be judged against +// the SAME model eligibility is judged against (`fallbackModel()`, the launch default), not +// `currentModel()` — which can be a session-restored model unrelated to the launch default. +test("explicitness must be checked against the launch default, not a session-restored model", () => { + const isFree = (model: { providerID: string; modelID: string }) => isFreeZenModel(model, providersFixture()) + const explicitDefault = NEMOTRON + const fallbackModel = NEMOTRON // the launch default the user explicitly chose + const restoredSessionModel = { providerID: "anthropic", modelID: "claude-sonnet" } // an unrelated open conversation + + // Correct: explicitness checked against the SAME model eligibility evaluates (`fallbackModel`). + // `usesImplicitFreeDefault()` in local.tsx now does exactly this via `hasExplicitDefault()`. + const explicitAgainstFallback = isConfirmedExplicitSelection(fallbackModel, explicitDefault) + expect(explicitAgainstFallback).toBe(true) + expect(shouldOfferManagedBaseDefault(fallbackModel, explicitAgainstFallback, {}, isFree)).toBe(false) + + // The bug this guards against: checking explicitness against `currentModel()` — here standing + // in for a restored session on a different, unrelated conversation — finds no match, + // misclassifies the deliberate Nemotron pick as implicit, and `usesImplicitFreeDefault()` would + // wrongly become eligible to migrate, overwriting the restored conversation's model with Base. + const explicitAgainstRestoredSession = isConfirmedExplicitSelection(restoredSessionModel, explicitDefault) + expect(explicitAgainstRestoredSession).toBe(false) + expect(shouldOfferManagedBaseDefault(fallbackModel, explicitAgainstRestoredSession, {}, isFree)).toBe(true) +}) +// altimate_change end + +// altimate_change start — fixes #1301 (Codex review, P2): a free default the user chose on +// purpose, or already declined migrating away from, is usable — not "un-onboarded". +test("isUsableFreeDefault: usable when explicit or previously declined, never when neither", () => { + const isValid = () => true + const isFree = (model: { providerID: string; modelID: string }) => isFreeZenModel(model, providersFixture()) + + // Explicitly chosen, never declined: usable. + expect(isUsableFreeDefault(NEMOTRON, isValid, isFree, true, false)).toBe(true) + // Not explicit, but a previous decline is on record (kv key or model.json flag): usable. + expect(isUsableFreeDefault(NEMOTRON, isValid, isFree, false, true)).toBe(true) + // Neither explicit nor declined: still "un-onboarded" — not usable. + expect(isUsableFreeDefault(NEMOTRON, isValid, isFree, false, false)).toBe(false) + // A paid/non-free model is never usable via this path regardless of explicitness/decline. + expect(isUsableFreeDefault(ZEN_PAID, isValid, isFree, true, true)).toBe(false) + // An invalid (no longer offered) model is never usable. + expect(isUsableFreeDefault(NEMOTRON, () => false, isFree, true, true)).toBe(false) + // No current model at all. + expect(isUsableFreeDefault(undefined, isValid, isFree, true, true)).toBe(false) +}) +// altimate_change end + +// altimate_change start — fixes #1301 (Codex review round 2, P1): `migrateLegacyDefault()` in +// local.tsx is closure-internal and needs `LocalProvider`/`SyncProvider`/SDK mocks to exercise +// directly (the existing dialog test harness in dialog-altimate-base.test.tsx does not go through +// this function at all — it mounts the dialog with a stubbed `onDecline`, never accept). Per +// review guidance, this is the pure-function test standing in for that: it exercises the EXACT +// predicate `migrateLegacyDefault()` now calls (`shouldMoveAgentModelDuringMigration`), not a +// hand-rolled comparison, so a change to that predicate's logic is caught here even without a +// full-context test. +test("shouldMoveAgentModelDuringMigration: only moves a conversation still on the implicit default", () => { + const previous = NEMOTRON // the implicit free default being migrated away from + const restoredSession = { providerID: "anthropic", modelID: "claude-sonnet" } // an unrelated open conversation + + // Still on the implicit default (the common case: no session restored) — migrate it. + expect(shouldMoveAgentModelDuringMigration(previous, previous)).toBe(true) + // No current model at all (e.g. agent has no per-agent model set yet) — nothing to preserve. + expect(shouldMoveAgentModelDuringMigration(undefined, previous)).toBe(true) + // A restored conversation on a DIFFERENT model must be left alone — this is the regression: + // migration is a decision about the DEFAULT, not about overwriting an unrelated open thread. + expect(shouldMoveAgentModelDuringMigration(restoredSession, previous)).toBe(false) + // No captured `previous` at all (should not happen in practice — `usesLegacyDefault()` already + // requires a defined `fallbackModel()` — but fail closed rather than move an unrelated model). + expect(shouldMoveAgentModelDuringMigration(restoredSession, undefined)).toBe(false) +}) + +test("isOwnPastPickOfFreeDefault: an older picker-written recent is the user's own pick, not implicit", () => { + // An older Nemotron recent (predates the `explicitDefault` marker) is still the user's own past + // pick — silent migration when Base is registered must not sweep it up without asking. + expect(isOwnPastPickOfFreeDefault(NEMOTRON, [NEMOTRON, { providerID: "anthropic", modelID: "claude-sonnet" }])).toBe( + true, + ) + // Not in recents at all: a genuinely implicit default, silent migration proceeds as before. + expect(isOwnPastPickOfFreeDefault(NEMOTRON, [{ providerID: "anthropic", modelID: "claude-sonnet" }])).toBe(false) + expect(isOwnPastPickOfFreeDefault(NEMOTRON, [])).toBe(false) + // Big Pickle is deliberately excluded — recents written before this distinction existed were + // always silently migrated, and that stays unchanged ("today's behaviour"). + expect(isOwnPastPickOfFreeDefault(LEGACY_BIG_PICKLE_MODEL, [LEGACY_BIG_PICKLE_MODEL])).toBe(false) + // No current model at all. + expect(isOwnPastPickOfFreeDefault(undefined, [NEMOTRON])).toBe(false) +}) +// altimate_change end + +// altimate_change start — PR #1302 Codex review round 2, P1: `cycle()` (the recent-model +// shortcut) sets `explicitDefault` to whichever model was cycled TO, without reordering +// `recent` — so `fallbackModel()` (the LAUNCH default) can still resolve to the model cycled +// FROM. Usability (`hasUsableFreeDefault()`) must therefore compare explicitness against +// `currentModel()` (`hasExplicitModel()`'s comparison), not `fallbackModel()` +// (`hasExplicitDefault()`'s — the right comparison for MIGRATION eligibility, the wrong one for +// "is the model in use right now usable"). +test("readiness after cycling: explicitness must be judged against the model in use, not the launch default", () => { + const isFree = (model: { providerID: string; modelID: string }) => + isLegacyBigPickleModel(model) || isFreeZenModel(model, providersFixture()) + const launchDefault = NEMOTRON // A: what fallbackModel() still resolves to after cycling + const cycledTo = LEGACY_BIG_PICKLE_MODEL // B: the current model, and what explicitDefault now is + + // Correct: explicitness checked against the model actually in use sees the deliberate cycle + // and stays usable — this is `hasExplicitModel()`'s comparison. + const explicitAgainstCurrent = isConfirmedExplicitSelection(cycledTo, cycledTo) + expect(explicitAgainstCurrent).toBe(true) + expect(isUsableFreeDefault(cycledTo, () => true, isFree, explicitAgainstCurrent, false)).toBe(true) + + // The bug this guards against: checking explicitness against the LAUNCH default instead + // (`hasExplicitDefault()`'s comparison) finds no match — `explicitDefault` is B, not A — so + // usability wrongly flips false for a model the user just deliberately picked, flipping + // `useReady()` true→false and reopening the picker (clearing the prompt) on the next submit. + const explicitAgainstLaunchDefault = isConfirmedExplicitSelection(launchDefault, cycledTo) + expect(explicitAgainstLaunchDefault).toBe(false) + expect(isUsableFreeDefault(cycledTo, () => true, isFree, explicitAgainstLaunchDefault, false)).toBe(false) +}) +// altimate_change end + +// altimate_change start — round 6 review (cursor 3986044810/3986264141, cubic 3986055646, +// kilo 3986171198, all independently converging): `cycle()` now passes `{ explicit: true, +// recent: true }` so the cycled-to model moves to the FRONT of `recent` — the only state +// headless/ACP default resolution (`Provider.readDefaultModelState()`, +// `defaultModelFromConfig()`) reads. A prior fix instead made `fallbackModel()` prefer a +// persisted `explicitDefault` over `recent`'s order, without teaching the server about that +// TUI-only marker at all — so the TUI and server could resolve two different launch defaults +// from the same `model.json` after a cycle. Reverted; `recent`'s order is the single source of +// truth for every surface. +test("cycling persists the launch default via recents order, not a TUI-only marker", () => { + const recent = [NEMOTRON, LEGACY_BIG_PICKLE_MODEL] + const cycledTo = LEGACY_BIG_PICKLE_MODEL // B: what cycle(1) from NEMOTRON selects + + // This mirrors exactly what `cycle()` → `selectModel(val, { recent: true })` persists: + // `recentModels(cycledTo, recent)` moves B to the front, same as any other deliberate pick + // (`cycleFavorite`, `/model`) already does. + const persisted = recentModels(cycledTo, recent) + expect(persisted).toEqual([LEGACY_BIG_PICKLE_MODEL, NEMOTRON]) + + // `fallbackModel()`'s `recent` loop (TUI) and `Provider.readDefaultModelState()` / + // `defaultModelFromConfig()` (headless/ACP, server-side) all resolve the launch default to the + // FIRST valid entry in `recent` — so after a cycle, every surface reading the same persisted + // array agrees on B, with no separate marker for the server to not know about. + expect(persisted[0]).toEqual(cycledTo) +}) +// altimate_change end + +// altimate_change start — Kilo review round 6 (3986171192) / Codex HOLD finding 1: `hasUsableFreeDefault()` +// used to read the kv migration-decline key with no `kv.ready` gate. A pre-0.11.x decliner whose +// refusal lives ONLY in kv (no `explicitDefault`, no picker-written recent, legacy Big Pickle so +// `hasOwnPickOfImplicitDefault()` is also false) reads as "not declined" before kv hydrates. +// A FIRST fix attempt made an unready kv read as `true` ("assume usable") — Codex caught that +// going the WRONG direction: it makes `useReady()` true immediately, before onboarding/migration +// ever runs, so `--prompt` (or a fast manual submit) sails straight through to the implicit +// public Zen default — trading a false negative (discarded input) for a false positive (skipped +// onboarding/migration), which is worse. The correct third state is `"pending"`, not a boolean +// guess either way — see `hasUsableFreeDefaultGated`'s declaration in local.tsx, and +// `useReadyPending()`/the submit-gate defer logic in component/prompt/index.tsx for how the ONE +// caller that must see `"pending"` (the prompt submit gate) uses it to defer without discarding. +test("hasUsableFreeDefault reports 'pending' (not a boolean guess) while kv is unready", () => { + // kv not ready yet: neither `true` nor `false` — explicitly "don't know yet." + expect(hasUsableFreeDefaultGated(false, () => false)).toBe("pending") + expect(hasUsableFreeDefaultGated(false, () => true)).toBe("pending") + // kv ready: the underlying computation is authoritative. + expect(hasUsableFreeDefaultGated(true, () => false)).toBe(false) + expect(hasUsableFreeDefaultGated(true, () => true)).toBe(true) +}) +// altimate_change end + +// altimate_change start — Kilo review round 6 (3986171188): app.tsx's startup effect used to +// latch "no onboarding needed this launch" purely off `hasExistingLegacySelection() || +// hasUsableFreeDefault()`, skipping the `onboardingReady()` branch (funnel telemetry + +// `openScanGate()`) even when THIS launch's own impatient-user setup — not a returning user's +// persisted state — is what made that true. `setupCompleteThisLaunch` is the fix. +test("shouldSkipOnboardingAtStartup: a same-launch setup must not swallow the onboardingReady() branch", () => { + // A genuine returning user: legacy/free-default signal true, but nothing was set up THIS + // launch — skip onboarding, as before. + expect(shouldSkipOnboardingAtStartup(true, false, false)).toBe(true) + expect(shouldSkipOnboardingAtStartup(false, true, false)).toBe(true) + + // The regression this guards: an impatient first-run user's own submit-before-ready flow made + // `hasUsableFreeDefault()` (or `hasExistingLegacySelection()`) true THIS launch, via + // `setupComplete()`. Must NOT skip — `onboardingReady()` needs to see this to fire telemetry + // and the scan gate. + expect(shouldSkipOnboardingAtStartup(true, false, true)).toBe(false) + expect(shouldSkipOnboardingAtStartup(false, true, true)).toBe(false) + + // Neither signal true: nothing to skip either way. + expect(shouldSkipOnboardingAtStartup(false, false, false)).toBe(false) + expect(shouldSkipOnboardingAtStartup(false, false, true)).toBe(false) +}) +// altimate_change end + +// altimate_change start — cubic review (3986532221): the fix above still passed a bare +// `setupComplete()` at the app.tsx call site, which is a GLOBAL flag `markSetupComplete()` sets +// for ANY model selection, not only a first-run one. A RETURNING user (existing +// legacy/free-default selection) who does an ordinary `/model` switch while app.tsx's startup +// effect is still settling made `setupComplete()` true too, which used to fall through to the +// `onboardingReady()` branch and fire `onboarding_started`/`onboarding_completed`/ +// `scan_gate_shown` telemetry plus `openScanGate()` for a routine model change — not a first run. +// `firstRunOpenedThisLaunch()` (altimate-onboarding.tsx) is a one-way latch set only when the +// first-run picker itself actually opens THIS launch (app.tsx's own fallthrough, or the prompt +// gate's equivalent in component/prompt/index.tsx); `setupComplete() && firstRunOpenedThisLaunch()` +// — what app.tsx now actually passes — is the correct "did first-run genuinely complete this +// launch" signal. These tests exercise the REAL production signals (not synthetic booleans), +// asserting `shouldSkipOnboardingAtStartup` receives the correctly-computed discriminator: `skip +// === true` means app.tsx's startup effect returns BEFORE ever reaching the telemetry/scan-gate +// branch, so it is the direct proxy for "no onboarding telemetry, no scan gate" at this call site. +test("a returning user's routine /model switch during the startup race window fires no onboarding telemetry or scan gate", () => { + resetSetupComplete() + try { + // Returning user: has an existing legacy/free-default selection (`hasExistingLegacySelection` + // true below). They switch models via an ORDINARY `/model` pick — NOT through the first-run + // picker — while app.tsx's startup effect is still settling. `markSetupComplete()` fires for + // this exactly as it does for every model pick, first-run or not. + markSetupComplete() + const setupComplete = useSetupComplete() + const firstRunOpenedThisLaunch = useFirstRunOpenedThisLaunch() + expect(setupComplete()).toBe(true) + expect(firstRunOpenedThisLaunch()).toBe(false) + + // Mirrors app.tsx's actual call site exactly. + const skip = shouldSkipOnboardingAtStartup(true, false, setupComplete() && firstRunOpenedThisLaunch()) + expect(skip).toBe(true) + } finally { + resetSetupComplete() + } +}) + +test("a genuine impatient first-run completion (the prompt gate opened this launch) still fires the onboardingReady() branch", () => { + resetSetupComplete() + try { + // The prompt gate (component/prompt/index.tsx's `!ready()` branch) — or app.tsx's own + // startup fallthrough — actually opened the first-run picker THIS launch... + markFirstRunActive() + // ...and the user picked a free model there, completing it. + markSetupComplete() + const setupComplete = useSetupComplete() + const firstRunOpenedThisLaunch = useFirstRunOpenedThisLaunch() + expect(setupComplete()).toBe(true) + expect(firstRunOpenedThisLaunch()).toBe(true) + + const skip = shouldSkipOnboardingAtStartup(false, true, setupComplete() && firstRunOpenedThisLaunch()) + expect(skip).toBe(false) + } finally { + resetSetupComplete() + } +}) +// altimate_change end + +// altimate_change start — Codex re-review round 9: app.tsx's OTHER onboarding-completion check — +// the `onboardingReady()` branch's own `if (setupComplete() && firstRunOpenedThisLaunch())`, a +// few lines below `shouldSkipOnboardingAtStartup`'s call site — had the exact same class of bug +// cubic caught there (3986532221), just reached from a different starting condition: a bare +// `setupComplete()` check. `onboardingReady()` (`useReady()`) is also true via `connected()`, not +// only via `hasUsableFreeDefault()` — so a RETURNING user with a configured legacy default (e.g. +// Big Pickle) who switches `/model` to a PAID model has BOTH of `shouldSkipOnboardingAtStartup`'s +// skip predicates false (paid picks aren't covered by either `hasExistingLegacySelection()` or +// `hasUsableFreeDefault()`) — falling through to THIS branch — while `onboardingReady()` is +// already true via `connected()`. If that `/model` switch raced app.tsx's startup effect, a bare +// `setupComplete()` read as true even though no first-run picker ever opened, firing onboarding +// telemetry and the scan gate for an ordinary provider switch. Fixed the same way: require +// `firstRunOpenedThisLaunch()` alongside `setupComplete()`. +test("onboardingReady() branch: a returning user's /model switch to a paid model, with both shouldSkipOnboardingAtStartup predicates false, fires no onboarding telemetry or scan gate", () => { + resetSetupComplete() + try { + // Both of shouldSkipOnboardingAtStartup's skip predicates are false — a paid /model pick, + // unlike a free one, is covered by neither `hasExistingLegacySelection()` nor + // `hasUsableFreeDefault()` — so app.tsx falls through past that branch and reaches this one. + expect(shouldSkipOnboardingAtStartup(false, false, false)).toBe(false) + + // The paid /model switch itself: markSetupComplete() fires for it exactly as it does for + // every pick, but the first-run picker was never involved. + markSetupComplete() + const setupComplete = useSetupComplete() + const firstRunOpenedThisLaunch = useFirstRunOpenedThisLaunch() + expect(setupComplete()).toBe(true) + expect(firstRunOpenedThisLaunch()).toBe(false) + + // The SAME predicate app.tsx's onboardingReady() branch calls — must be false, or onboarding + // telemetry and the scan gate fire for a routine provider switch. + expect(shouldFireFirstRunFunnelAtStartup(setupComplete(), firstRunOpenedThisLaunch())).toBe(false) + } finally { + resetSetupComplete() + } +}) + +test("onboardingReady() branch: a genuine first-run completion (the picker opened this launch) still fires onboarding telemetry and the scan gate", () => { + resetSetupComplete() + try { + // The first-run picker (app.tsx's own startup fallthrough, or the prompt gate's equivalent) + // actually opened THIS launch, and the user completed setup there. + markFirstRunActive() + markSetupComplete() + const setupComplete = useSetupComplete() + const firstRunOpenedThisLaunch = useFirstRunOpenedThisLaunch() + expect(setupComplete()).toBe(true) + expect(firstRunOpenedThisLaunch()).toBe(true) + + expect(shouldFireFirstRunFunnelAtStartup(setupComplete(), firstRunOpenedThisLaunch())).toBe(true) + } finally { + resetSetupComplete() + } +}) +// altimate_change end + +// altimate_change start — PR #1302 Codex review round 2, P2: `migrateLegacyDefault({ from })`'s +// captured `from` must not bypass free-model validation entirely. +test("isMigrationStillEligibleAfterCapture: only the launch-default-unchanged or registration-induced-Base transitions stay eligible", () => { + const from = NEMOTRON + + // Still exactly `from`: the ordinary case (nothing changed while the dialog was open). + expect(isMigrationStillEligibleAfterCapture(from, from, false, {})).toBe(true) + // Registration itself moved the launch default to Base: the expected post-registration state. + expect(isMigrationStillEligibleAfterCapture(ALTIMATE_BASE_MODEL, from, false, {})).toBe(true) + // A provider refresh moved the launch default to some OTHER (in particular PAID) model for an + // unrelated reason — this must NOT stay eligible, or accept would insert Base on top of a + // default that changed out from under it. + expect(isMigrationStillEligibleAfterCapture(ZEN_PAID, from, false, {})).toBe(false) + expect( + isMigrationStillEligibleAfterCapture({ providerID: "anthropic", modelID: "claude-sonnet" }, from, false, {}), + ).toBe(false) + // Explicit or allowlist-excluded still block it regardless of which model `fallbackModel()` is. + expect(isMigrationStillEligibleAfterCapture(from, from, true, {})).toBe(false) + expect(isMigrationStillEligibleAfterCapture(from, from, false, { anthropic: {} })).toBe(false) + // No current fallback at all (e.g. no provider connected any more). + expect(isMigrationStillEligibleAfterCapture(undefined, from, false, {})).toBe(false) +}) +// altimate_change end diff --git a/packages/tui/test/context/ready-pending.test.tsx b/packages/tui/test/context/ready-pending.test.tsx new file mode 100644 index 0000000000..9254ab40f9 --- /dev/null +++ b/packages/tui/test/context/ready-pending.test.tsx @@ -0,0 +1,250 @@ +/** @jsxImportSource @opentui/solid */ +// altimate_change start — Codex HOLD finding 1 (+ re-review round 8): coverage for the kv.ready +// "pending" defer path. +// +// `hasUsableFreeDefaultGated`'s own unit test (local.test.ts) proves the pure gate itself reports +// `"pending"` (not a boolean guess either way) while kv is unready. This file proves the +// CONSEQUENCE: a submit attempted while `useReadyPending()` is true must be neither sent early +// (Codex's finding — skips onboarding/migration) nor discarded (Kilo's original finding), but +// deferred and automatically retried once pending resolves — using `createDeferredRetry` +// (util/signal.ts), the SAME production primitive `component/prompt/index.tsx`'s submit gate +// calls, against a manually-controlled signal standing in for `useReadyPending()`/`useReady()`. +// +// Codex re-review round 8: this file originally re-implemented its own copy of the defer+retry +// flag/effect shape rather than importing the real one — meaning reverting the actual fix in +// prompt/index.tsx left this test passing regardless, since it never touched production code at +// all. `createDeferredRetry` was extracted specifically to close that gap: `DeferThenRetryHarness` +// below now calls it directly, so a regression in the SHARED primitive (or its removal from the +// real submit gate) is exactly what this test would need to still be testing anything. +// +// Mounted via `testRender` (a bare component, no context providers) rather than a plain +// `createRoot()` call: bare `solid-js` imported outside `@opentui/solid`'s render pipeline +// resolves to its SSR build in this test environment, whose effects run once at creation and +// never re-fire on a later signal write — `testRender` is what gives this file the real, +// client-reactive `solid-js` runtime the production code actually runs under. +// +// IMPORTANT — this is deliberately NOT an end-to-end mount of `` inside the real provider +// tree, and that is a documented finding, not an oversight: `KVProvider` and `LocalProvider` are +// both built on `createSimpleContext` (context/helper.tsx), whose `provider` wraps `children` in +// ``. Since both providers expose a +// `ready` getter, NEITHER renders its children — and ``/``/`` sit nested inside +// both, per app.tsx's provider tree — until `kv.ready` AND `local.model.ready` are already true. +// Verified empirically while writing this test: a capture component mounted inside the real +// `` tree never observes `kv.ready === false` — it simply never renders +// until `kv.ready` is already true, because `Show` withholds its children rather than rendering +// them and letting a child branch on readiness itself. That means the exact "prompt gate fires +// while kv is still hydrating" window Kilo originally flagged is very likely NOT reachable through +// the actual interactive Prompt path in the current codebase — reported alongside this file. The +// fix is kept anyway (a `"pending"` third state is a more honest contract than guessing a boolean +// either way, costs nothing, and is defense-in-depth against this invariant ever changing), and +// this test validates the MECHANISM directly — via the real shared primitive — rather than +// asserting an end-to-end scenario that cannot currently be constructed through the real provider +// tree. +import { testRender } from "@opentui/solid" +import { expect, test } from "bun:test" +import { createSignal } from "solid-js" +import { createDeferredRetry } from "../../src/util/signal" + +async function waitUntil(predicate: () => boolean, timeout = 2_000) { + const started = Date.now() + while (!predicate()) { + if (Date.now() - started > timeout) throw new Error("timed out waiting for condition") + await Bun.sleep(5) + } +} + +/** Mirrors component/prompt/index.tsx's `submitInner()` gate, built on the SAME shared + * `createDeferredRetry` primitive the real submit gate uses (see this file's header comment). */ +function DeferThenRetryHarness(props: { + ready: () => boolean + pending: () => boolean + promptText: () => string + setPromptText: (value: string) => void + /** The SAME object on every call, mutated in place by `setPromptText` — exactly what + * `unwrap(store.prompt)` hands component/prompt/index.tsx (cursor 3987286236). */ + livePrompt: () => { input: string } + onSend: (value: string) => void + onDiscard: () => void + exposeSubmit: (fn: () => boolean) => void +}) { + function attemptSubmit() { + if (!props.promptText()) return false + if (!props.ready()) { + if (props.pending()) { + deferredSubmit.defer() + return false + } + props.setPromptText("") + props.onDiscard() + return false + } + props.onSend(props.promptText()) + props.setPromptText("") + return true + } + // altimate_change — Codex re-review round 9 / cursor 3987286236: `getRevision` mirrors + // component/prompt/index.tsx's real usage exactly — it returns the store's LIVE raw object (the + // same reference every call, mutated in place by edits), not a fresh string. A primitive that + // merely held that reference and stringified both sides at retry time compared the object to + // itself and never saw an edit; this harness shape is what makes the edited-while-deferred test + // below fail on that bug. + const deferredSubmit = createDeferredRetry(props.pending, () => void attemptSubmit(), { + getRevision: () => props.livePrompt(), + }) + props.exposeSubmit(attemptSubmit) + return null +} + +async function mountHarness(options: { initialPending: boolean; willBeReady: boolean; promptText?: string }) { + const [pending, setPending] = createSignal(options.initialPending) + const ready = () => !pending() && options.willBeReady + const [promptText, setPromptTextSignal] = createSignal(options.promptText ?? "hello from before kv.ready") + // One object, mutated in place — see `livePrompt` on the harness props. + const livePrompt = { input: promptText() } + const setPromptText = (value: string) => { + livePrompt.input = value + setPromptTextSignal(value) + } + const submitSpy: string[] = [] + let discarded = false + let submit: (() => boolean) | undefined + + const app = await testRender(() => ( + livePrompt} + onSend={(value) => submitSpy.push(value)} + onDiscard={() => { + discarded = true + }} + exposeSubmit={(fn) => { + submit = fn + }} + /> + )) + await app.renderOnce() + await waitUntil(() => submit !== undefined) + + return { + attemptSubmit: () => submit!(), + setPending, + promptText, + setPromptText, + submitSpy, + discarded: () => discarded, + cleanup() { + app.renderer.destroy() + }, + } +} + +test.serial( + "defer-then-retry: a submit issued while pending is neither sent nor lost, and resolves once pending clears (usable)", + async () => { + const h = await mountHarness({ initialPending: true, willBeReady: true }) + try { + // The submit attempted while pending: deferred, not sent, not discarded. + expect(h.attemptSubmit()).toBe(false) + expect(h.submitSpy).toEqual([]) + expect(h.promptText()).toBe("hello from before kv.ready") + + // kv resolves ("declined" — usable): the retry effect fires automatically, with no second + // `attemptSubmit()` call from the test — proving the RETRY is automatic, not manual. + h.setPending(false) + await waitUntil(() => h.submitSpy.length > 0) + expect(h.submitSpy).toEqual(["hello from before kv.ready"]) + expect(h.promptText()).toBe("") + } finally { + h.cleanup() + } + }, +) + +test.serial( + "defer-then-retry: a submit issued while pending is neither sent nor lost, and is correctly discarded once pending clears (not usable)", + async () => { + const h = await mountHarness({ initialPending: true, willBeReady: false }) + try { + expect(h.attemptSubmit()).toBe(false) + expect(h.submitSpy).toEqual([]) + expect(h.promptText()).toBe("hello from before kv.ready") + expect(h.discarded()).toBe(false) + + // kv resolves to "not declined", and nothing else makes this launch ready — the deferred + // retry re-evaluates `ready()` fresh and correctly finds it still false, taking the + // discard branch (matching the real gate's picker-reopen path) rather than sending stale + // input through. + h.setPending(false) + await waitUntil(() => h.discarded()) + expect(h.submitSpy).toEqual([]) + expect(h.promptText()).toBe("") + } finally { + h.cleanup() + } + }, +) + +test.serial( + "defer-then-retry: a submission edited (not resubmitted) while deferred is NOT auto-sent once pending clears (Codex re-review round 9)", + async () => { + // Defer prompt A, then edit the box to B WITHOUT pressing Enter again — the user reconsidering + // mid-defer. Once pending resolves, the retry must be canceled (`getRevision` sees A != B at + // retry time), not fire and silently send B — a send the user never asked for. + const h = await mountHarness({ initialPending: true, willBeReady: true, promptText: "A" }) + try { + expect(h.attemptSubmit()).toBe(false) + expect(h.submitSpy).toEqual([]) + expect(h.promptText()).toBe("A") + + h.setPromptText("B") + + h.setPending(false) + // Deterministic window for the retry effect to have fired if it were going to — asserting + // an absence needs a bounded wait, not `waitUntil` (which only proves a positive). + await Bun.sleep(100) + expect(h.submitSpy).toEqual([]) + expect(h.promptText()).toBe("B") + } finally { + h.cleanup() + } + }, +) + +test.serial( + "defer-then-retry: a submission that is deferred and then re-deferred unchanged still sends once pending clears", + async () => { + // Guards against an overzealous fix: identical text at defer-time and retry-time (nothing + // edited) must still send normally — and pressing Enter TWICE while pending (Kilo 3987319604: + // two `.defer()` calls, the revision re-captured each time) must still yield exactly one send. + const h = await mountHarness({ initialPending: true, willBeReady: true, promptText: "unchanged" }) + try { + expect(h.attemptSubmit()).toBe(false) + expect(h.attemptSubmit()).toBe(false) + expect(h.submitSpy).toEqual([]) + h.setPending(false) + await waitUntil(() => h.submitSpy.length > 0) + // Bounded settle so a second (duplicate) retry would have had time to show up. + await Bun.sleep(50) + expect(h.submitSpy).toEqual(["unchanged"]) + expect(h.promptText()).toBe("") + } finally { + h.cleanup() + } + }, +) + +test.serial("defer-then-retry: a submit issued once already ready sends immediately, no defer", async () => { + const h = await mountHarness({ initialPending: false, willBeReady: true, promptText: "hello, already ready" }) + try { + expect(h.attemptSubmit()).toBe(true) + expect(h.submitSpy).toEqual(["hello, already ready"]) + expect(h.promptText()).toBe("") + expect(h.discarded()).toBe(false) + } finally { + h.cleanup() + } +}) +// altimate_change end diff --git a/packages/tui/test/prompt/history-startup-race.test.tsx b/packages/tui/test/prompt/history-startup-race.test.tsx new file mode 100644 index 0000000000..2f7abf029e --- /dev/null +++ b/packages/tui/test/prompt/history-startup-race.test.tsx @@ -0,0 +1,353 @@ +/** @jsxImportSource @opentui/solid */ +// altimate_change start — Codex review rounds 2 and 4: `hadHistoryAtStartup()` has no latency +// bound or ordering guarantee against `append()` — a prompt submitted before the startup read +// settles (`routes/home.tsx` can auto-submit without waiting for history; the prompt saves drafts +// with no readiness gate either) could otherwise write an entry that the SAME read then picks up, +// making a genuinely fresh launch look like a returning one (round 2's finding), OR corrupt the +// COUNT-based fix that round introduced — subtracting how many appends raced the read is unsound +// because an early append's write can land on disk either before or after the read resolves, +// with no bound either way (round 4's finding: one pre-existing entry + one early append whose +// write lands late reads as `lines.length === 1`, and subtracting 1 wrongly gives `0`). Fixed by +// construction instead: `append()` defers its FILE write (never the in-memory store update) while +// `!loaded()`, so nothing from this launch can reach the read's `lines` at all; `onMount`'s +// `finally` flushes the merged `store.history` in one write once the snapshot is captured. These +// first tests cover both completion orders — a genuinely fresh launch, and a returning one — and +// assert the deferred write actually lands on disk once `loaded()` settles. +// +// altimate_change — Codex HOLD finding 3 (round 7): the two tests that used to live here for the +// round-6 flush/append races ("an append that lands before loaded() flips…" and "…while the +// startup flush is still in flight…") did not actually establish either race — POLLING +// `loaded()` via `Bun.sleep(5)` does not keep the startup write in flight; by the time the poll +// observes `loaded() === true`, the flush's tiny write has very likely already completed on real +// disk. Codex proved this by executing them: both passed even against 0370cfa's dropped-append +// bug (`await` before `setLoaded(true)`), which they were supposed to guard against. Replaced +// below with three tests built on CONTROLLABLE barriers — `spyOn(persistence, "readText"/"writeText")` +// returning a manually-resolved deferred promise — so each scenario is reproduced by +// construction, not by timing luck: (a) append while the startup READ is still pending, (b) +// append while the startup REWRITE's write is still in flight, (c) append SYNCHRONOUSLY in the +// same reactive tick `loaded()` flips (the specific shape that reproduces the lazy-snapshot +// duplicate bug — see that test's own comment for why timing this precisely matters). All three +// were confirmed to FAIL against the relevant old behavior and PASS on HEAD before being kept; +// see this session's report for the exact failure output. +import { testRender } from "@opentui/solid" +import { expect, spyOn, test } from "bun:test" +import { mkdir } from "node:fs/promises" +import path from "node:path" +import { createEffect } from "solid-js" +import { TestTuiContexts } from "../fixture/tui-environment" +import { tmpdir } from "../fixture/fixture" +import * as persistence from "../../src/util/persistence" +import { PromptHistoryProvider, usePromptHistory, parsePromptHistory } from "../../src/prompt/history" + +async function waitUntil(predicate: () => boolean, timeout = 2_000) { + const started = Date.now() + while (!predicate()) { + if (Date.now() - started > timeout) throw new Error("timed out waiting for condition") + await Bun.sleep(5) + } +} + +function deferred() { + let resolve!: (value: T) => void + const promise = new Promise((res) => { + resolve = res + }) + return { promise, resolve } +} + +async function mountWithRacingAppend(existing?: string) { + const tmp = await tmpdir() + const state = path.join(tmp.path, "state") + await mkdir(state, { recursive: true }) + const historyPath = path.join(state, "prompt-history.jsonl") + if (existing !== undefined) await Bun.write(historyPath, existing) + + let history: ReturnType | undefined + let appended = false + + function Capture() { + history = usePromptHistory() + // Called synchronously during the SAME initial render pass that mounts + // `PromptHistoryProvider` — necessarily before its `onMount`'s `await readText(...)` (real + // disk I/O) can possibly have resolved, reproducing the race deterministically rather than by + // timing luck. + history!.append({ input: "first prompt of this launch", parts: [] }) + appended = true + return null + } + + const app = await testRender(() => ( + + + + + + )) + await app.renderOnce() + expect(appended).toBe(true) + return { + historyPath, + history: history!, + async cleanup() { + app.renderer.destroy() + await tmp[Symbol.asyncDispose]() + }, + } +} + +test.serial( + "a prompt appended before the startup read settles does not count as pre-existing history (no prior history)", + async () => { + const mounted = await mountWithRacingAppend() + try { + await waitUntil(() => mounted.history.loaded()) + // The append's own write eventually lands (it's in `history()`), but it must not be + // mistaken for history that existed BEFORE this launch. + expect(mounted.history.hadHistoryAtStartup()).toBe(false) + // The deferred write actually reaches disk once it settles — this is the fix: the write + // was deferred, not dropped. `loaded()` alone does not guarantee the write already landed + // (it's fire-and-forget), so this awaits `flushed()` rather than reading immediately. + await mounted.history.flushed() + const onDisk = parsePromptHistory(await Bun.file(mounted.historyPath).text()) + expect(onDisk).toEqual([{ input: "first prompt of this launch", parts: [] }]) + } finally { + await mounted.cleanup() + } + }, +) + +test.serial( + "a prompt appended before the startup read settles does not hide real pre-existing history", + async () => { + // Codex review round 4: the count-subtraction fix this replaces could turn THIS case into a + // false negative — one pre-existing entry plus one early append whose write happened to land + // before the read resolves reads as `lines.length === 1`, and subtracting the appended count + // (1) wrongly gave `0`, misclassifying a genuine returning user as fresh. + const existing = JSON.stringify({ input: "from a previous launch", parts: [] }) + "\n" + const mounted = await mountWithRacingAppend(existing) + try { + await waitUntil(() => mounted.history.loaded()) + expect(mounted.history.hadHistoryAtStartup()).toBe(true) + // Both the pre-existing entry and the deferred append are on disk once the flush settles. + await mounted.history.flushed() + const onDisk = parsePromptHistory(await Bun.file(mounted.historyPath).text()) + expect(onDisk).toEqual([ + { input: "from a previous launch", parts: [] }, + { input: "first prompt of this launch", parts: [] }, + ]) + } finally { + await mounted.cleanup() + } + }, +) + +test.serial("real pre-existing history is still recognized when nothing races ahead of the read", async () => { + const tmp = await tmpdir() + const state = path.join(tmp.path, "state") + await mkdir(state, { recursive: true }) + await Bun.write( + path.join(state, "prompt-history.jsonl"), + JSON.stringify({ input: "from a previous launch", parts: [] }) + "\n", + ) + + let history: ReturnType | undefined + function Capture() { + history = usePromptHistory() + return null + } + + const app = await testRender(() => ( + + + + + + )) + try { + await waitUntil(() => history!.loaded()) + expect(history!.hadHistoryAtStartup()).toBe(true) + } finally { + app.renderer.destroy() + await tmp[Symbol.asyncDispose]() + } +}) + +// altimate_change start — Codex HOLD finding 3: barrier-controlled races (see file header). +async function mountBare(existing?: string) { + const tmp = await tmpdir() + const state = path.join(tmp.path, "state") + await mkdir(state, { recursive: true }) + const historyPath = path.join(state, "prompt-history.jsonl") + if (existing !== undefined) await Bun.write(historyPath, existing) + + let history: ReturnType | undefined + function Capture() { + history = usePromptHistory() + return null + } + const app = await testRender(() => ( + + + + + + )) + await app.renderOnce() + return { + historyPath, + history: history!, + async cleanup() { + app.renderer.destroy() + await tmp[Symbol.asyncDispose]() + }, + } +} + +function diskLines(text: string) { + return text.split("\n").filter(Boolean) +} + +test.serial( + "barrier: append while the startup READ is pending is persisted exactly once, in FIFO order", + async () => { + const read = deferred() + const readSpy = spyOn(persistence, "readText").mockImplementation(() => read.promise) + const mounted = await mountBare() + try { + expect(mounted.history.loaded()).toBe(false) + mounted.history.append({ input: "appended while read pending", parts: [] }) + // The in-memory update is immediate regardless of `loaded()` — the read is still pending, + // so no file write has happened yet either way. + expect(mounted.history.loaded()).toBe(false) + + read.resolve(JSON.stringify({ input: "from a previous launch", parts: [] }) + "\n") + await waitUntil(() => mounted.history.loaded()) + await mounted.history.flushed() + + const text = await Bun.file(mounted.historyPath).text() + const onDisk = parsePromptHistory(text) + expect(onDisk).toEqual([ + { input: "from a previous launch", parts: [] }, + { input: "appended while read pending", parts: [] }, + ]) + const lines = diskLines(text) + expect(lines.length).toBe(2) + expect(new Set(lines).size).toBe(2) + } finally { + readSpy.mockRestore() + await mounted.cleanup() + } + }, +) + +test.serial( + "barrier: append while the startup REWRITE's write is still in flight is persisted exactly once, in FIFO order", + async () => { + const originalWriteText = persistence.writeText + const gate = deferred() + const writeSpy = spyOn(persistence, "writeText").mockImplementation(async (filePath, content) => { + await gate.promise + return originalWriteText(filePath, content) + }) + const existing = JSON.stringify({ input: "from a previous launch", parts: [] }) + "\n" + const mounted = await mountBare(existing) + try { + // The read resolves normally (real, unmocked disk read); onMount's `finally` then + // synchronously snapshots + enqueues the (now gated) flush write + flips `loaded()` — all + // before the gated `writeText` call has done anything beyond starting to await the gate. + await waitUntil(() => mounted.history.loaded()) + + // Confirm the flush's write is genuinely still in flight (gated), not merely "probably + // still running" — `flushed()` must not have settled yet. + let flushSettled = false + void mounted.history.flushed().then(() => { + flushSettled = true + }) + await Bun.sleep(20) + expect(flushSettled).toBe(false) + + // Append NOW, deterministically while the flush's write is gated/in flight. + mounted.history.append({ input: "appended during in-flight rewrite", parts: [] }) + + // Release the gate: the flush's write proceeds first; the append's own queued `appendText` + // runs strictly AFTER it (FIFO, via `queueWrite`), never concurrently. + gate.resolve() + await waitUntil(() => flushSettled) + await mounted.history.flushed() + + const text = await Bun.file(mounted.historyPath).text() + const onDisk = parsePromptHistory(text) + expect(onDisk).toEqual([ + { input: "from a previous launch", parts: [] }, + { input: "appended during in-flight rewrite", parts: [] }, + ]) + const lines = diskLines(text) + expect(lines.length).toBe(2) + expect(new Set(lines).size).toBe(2) + } finally { + writeSpy.mockRestore() + await mounted.cleanup() + } + }, +) + +test.serial( + "barrier: an append synchronous with the SAME reactive tick loaded() flips is not duplicated by the flush", + async () => { + // The lazy-snapshot duplicate bug (cursor 3986264135, cubic 3986055642) required the append + // to land in-memory BEFORE the flush's write closure actually evaluated its content — which, + // under the old lazy-read code, happened on the very next microtask after `setLoaded(true)`, + // not after any `Bun.sleep`-based poll could observe `loaded()`. Solid's `createEffect` + // reacting to a signal read re-runs SYNCHRONOUSLY, in the same call stack as the `setLoaded` + // that triggered it — so an effect watching `loaded()` that calls `append()` the instant it + // becomes true reproduces that exact race by construction: the append happens before the + // queued flush closure's `.then()` microtask has had a chance to run, whether that closure + // reads `store.history` lazily (old, buggy) or was already handed a frozen snapshot before + // `setLoaded` ran (current `history.tsx` — the snapshot is computed BEFORE `setLoaded`, in + // the same synchronous block, so this effect's append can never be included in it). + const tmp = await tmpdir() + const state = path.join(tmp.path, "state") + await mkdir(state, { recursive: true }) + const historyPath = path.join(state, "prompt-history.jsonl") + await Bun.write(historyPath, JSON.stringify({ input: "from a previous launch", parts: [] }) + "\n") + + let history: ReturnType | undefined + let appended = false + function Capture() { + history = usePromptHistory() + createEffect(() => { + if (!history!.loaded() || appended) return + appended = true + history!.append({ input: "appended in the same tick loaded() flipped", parts: [] }) + }) + return null + } + const app = await testRender(() => ( + + + + + + )) + try { + await app.renderOnce() + await waitUntil(() => appended) + await waitUntil(() => history!.loaded()) + await history!.flushed() + + const text = await Bun.file(historyPath).text() + const onDisk = parsePromptHistory(text) + expect(onDisk).toEqual([ + { input: "from a previous launch", parts: [] }, + { input: "appended in the same tick loaded() flipped", parts: [] }, + ]) + const lines = diskLines(text) + expect(lines.length).toBe(2) + expect(new Set(lines).size).toBe(2) + } finally { + app.renderer.destroy() + await tmp[Symbol.asyncDispose]() + } + }, +) +// altimate_change end