diff --git a/CHANGELOG.md b/CHANGELOG.md index 98c38076..ea7a3628 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed - Claude Desktop and Cowork sessions are discovered for Windows Microsoft Store (MSIX) installs. (#611) +- **Nine providers served silently stale numbers after you pointed their env override at a different profile or root.** Kiro, Grok, Kimi, Mux, Mistral Vibe, Zerostack, Codebuff, Goose and Crush each honor an env var that relocates where discovery looks, but the var was never declared in the provider env fingerprint, so the cache section survived the change and kept reporting sessions parsed from the old root — with no diagnostic anywhere. The fix declares those vars, the adjacent OS-set path variables that resolve a discovery root for Claude, IBM Bob, Open Design and Kilo Code on Windows and Linux, Cursor's parse-budget override, and the Vercel AI Gateway credential — which must invalidate the fingerprint because a read-only refresh serves the cached report and would otherwise keep reporting the previous account's usage after a swap. Your next run re-parses the fourteen file-backed providers whose declarations changed — the nine above plus Claude, Cursor, Open Design, IBM Bob and Kilo Code — once, and only once; the Vercel AI Gateway declaration is a read-only-path correction, not a migration (its report is re-fetched on every writable run anyway); Copilot is deliberately NOT included, because declaring its overrides would force a re-parse that can drop OTel history only the cache still holds; `codeburn doctor` names deliberate overrides including the XDG_* vars, never the Windows ambient APPDATA / LOCALAPPDATA, and redacts credential values. (#920) ## 0.9.19 - 2026-07-20 diff --git a/packages/cli/src/doctor.ts b/packages/cli/src/doctor.ts index 818da52a..43dae64e 100644 --- a/packages/cli/src/doctor.ts +++ b/packages/cli/src/doctor.ts @@ -99,10 +99,37 @@ const PARSE_CALL_CAP = 500 // (readdir/stat only) still runs, so session counts stay meaningful. const PARSE_SPAWNS = new Set(['antigravity']) -// CodeBurn's own cache location: listed in PROVIDER_ENV_VARS for cache -// fingerprinting, but it is not a discovery path, so it must never be blamed -// in a NOTHING FOUND hint. -const NON_DISCOVERY_ENV_VARS = new Set(['CODEBURN_CACHE_DIR']) +// Vars listed in PROVIDER_ENV_VARS for cache fingerprinting that are NOT +// discovery paths: a change to them can never explain "nothing was +// discovered", so they must never be blamed in a NOTHING FOUND hint. +// - CODEBURN_CACHE_DIR: CodeBurn's own cache location — where the cache +// file lives, not where sessions are discovered. +// - CODEBURN_CURSOR_MAX_BUBBLES: caps how many bubbles Cursor parses +// (src/providers/cursor.ts:530) — a parse budget, not a discovery root. +// - KIMI_MODEL_NAME: renames the model attributed to Kimi sessions +// (src/providers/kimi.ts:125) — attribution, not discovery. +// All three still appear in the Details block; only the verdict's blame line +// is cleared of them. +const NON_DISCOVERY_ENV_VARS = new Set(['CODEBURN_CACHE_DIR', 'CODEBURN_CURSOR_MAX_BUBBLES', 'KIMI_MODEL_NAME']) + +// Ambient platform paths (set by the OS or desktop session for everyone), not +// deliberate user overrides: Windows sets APPDATA and LOCALAPPDATA for every +// process, so they carry no user intent and doctor must not name them as an +// override. The XDG_* vars are the opposite — they are opt-in on Linux, so a +// set value IS a deliberate user override and stays visible: with XDG_DATA_HOME +// pointed at a missing dir, blaming the install instead of the override +// (the pre-#920 behavior) told the user the tool was missing when they had +// deliberately relocated it. All of them are still fingerprinted — a change +// to any of them does move the discovery root, so the cache must invalidate — +// and the probed paths doctor already prints show exactly where CodeBurn +// looked. +const AMBIENT_ENV_VARS = new Set(['APPDATA', 'LOCALAPPDATA']) + +// Credential names whose VALUE must never be printed: knowing whether the +// credential is set is a useful diagnostic, but the value is a live secret. +// Redact at collect time so BOTH the text render and the JSON report are +// covered, and doctor can never leak a key into a bug report or a paste. +const SECRET_ENV_VARS = new Set(['AI_GATEWAY_API_KEY', 'VERCEL_OIDC_TOKEN']) // ── Collect (pure, testable) ───────────────────────────────────────────── @@ -110,8 +137,11 @@ function collectEnvOverrides(providerName: string): DoctorEnvOverride[] { const vars = PROVIDER_ENV_VARS[providerName] ?? [] const out: DoctorEnvOverride[] = [] for (const name of vars) { + if (AMBIENT_ENV_VARS.has(name)) continue const value = process.env[name] - if (value !== undefined && value !== '') out.push({ name, value }) + if (value !== undefined && value !== '') { + out.push(SECRET_ENV_VARS.has(name) ? { name, value: '' } : { name, value }) + } } return out } diff --git a/packages/cli/src/session-cache.ts b/packages/cli/src/session-cache.ts index 461dd461..1a007148 100644 --- a/packages/cli/src/session-cache.ts +++ b/packages/cli/src/session-cache.ts @@ -168,24 +168,60 @@ const CACHE_FILE = `session-cache.v${CACHE_VERSION}.json` const LEGACY_CACHE_FILE = 'session-cache.json' const TEMP_FILE_MAX_AGE_MS = 5 * 60 * 1000 +// Env vars that change what a provider discovers or how its sessions parse. +// computeEnvFingerprint hashes exactly these to decide when a provider's cache +// section is stale; a var read by the provider but missing here means changing +// it serves the old section silently, reporting nothing from the new root. +// Two reads in src/providers/ are deliberately absent: CODEBURN_VERBOSE +// (opencode.ts:151, kilo-code.ts:94) only changes logging verbosity, never +// parsed output. +// +// Copilot is deliberately NOT declared here. Declaring any CODEBURN_COPILOT_* +// var would change its fingerprint, and on a fingerprint change +// getOrCreateProviderSection (src/parser.ts:1270) keeps only the cached +// entries whose source path no longer exists — but copilot's OTel discovery +// returns one source per DB file ({ path: dbPath }, src/providers/copilot.ts:431) +// and that DB keeps existing, so its cached entry would be dropped and +// re-parsed, destroying conversations Copilot has since pruned from the DB +// that only the cache still holds (see DURABLE_PROVIDER_NAMES below). Do not +// "complete" the map for copilot until the durable carry-forward learns to +// merge instead of drop. export const PROVIDER_ENV_VARS: Record = { - claude: ['CLAUDE_CONFIG_DIRS', 'CLAUDE_CONFIG_DIR'], + claude: ['CLAUDE_CONFIG_DIRS', 'CLAUDE_CONFIG_DIR', 'CODEBURN_DESKTOP_SESSIONS_DIR', 'APPDATA', 'LOCALAPPDATA'], + codebuff: ['CODEBUFF_DATA_DIR'], codewhale: ['CODEWHALE_HOME'], codex: ['CODEX_HOME'], hermes: ['HERMES_HOME'], 'lingtai-tui': ['LINGTAI_HOME', 'LINGTAI_TUI_HOME', 'LINGTAI_TUI_GLOBAL_DIR'], droid: ['FACTORY_DIR'], - cursor: ['XDG_DATA_HOME'], + cursor: ['CODEBURN_CURSOR_MAX_BUBBLES'], + // XDG_DATA_HOME is stale here (cursor-agent never reads it) but deliberately + // kept: removing it would force a re-parse to fix nothing. 'cursor-agent': ['XDG_DATA_HOME'], + 'open-design': ['CODEBURN_OPEN_DESIGN_DIR', 'APPDATA'], opencode: ['XDG_DATA_HOME', 'OPENCODE_DATA_DIR', 'OPENCODE_DB_PREFIX'], - goose: ['XDG_DATA_HOME'], - crush: ['XDG_DATA_HOME'], + goose: ['XDG_DATA_HOME', 'GOOSE_PATH_ROOT'], + grok: ['GROK_HOME'], + crush: ['XDG_DATA_HOME', 'CRUSH_GLOBAL_DATA', 'LOCALAPPDATA'], warp: ['WARP_DB_PATH'], antigravity: ['CODEBURN_CACHE_DIR'], + 'kilo-code': ['XDG_DATA_HOME'], + kimi: ['KIMI_SHARE_DIR', 'KIMI_MODEL_NAME'], + kiro: ['KIRO_HOME'], + 'mistral-vibe': ['VIBE_HOME'], + mux: ['MUX_ROOT', 'CODEBURN_MUX_DIR'], qwen: ['QWEN_DATA_DIR'], - 'ibm-bob': ['XDG_CONFIG_HOME'], + 'ibm-bob': ['XDG_CONFIG_HOME', 'APPDATA'], quickdesk: ['QUICKWORK_HOME'], kimicode: ['KIMI_CODE_HOME'], + zerostack: ['ZS_DATA_DIR', 'XDG_DATA_HOME'], + // The gateway credential is a deliberate user override and MUST move the + // fingerprint: a read-only refresh (the refresh-lock fallback) serves the + // cached report straight from the section (parser.ts:1442 seeds servedSources + // before the network re-fetch at parser.ts:1455, which only runs when + // !readOnly), so an undeclared credential would keep serving the previous + // account's usage after a swap — the exact #920 defect. + 'vercel-gateway': ['AI_GATEWAY_API_KEY', 'VERCEL_OIDC_TOKEN'], } // Names of providers whose cache entries are never evicted when source files diff --git a/packages/cli/tests/doctor.test.ts b/packages/cli/tests/doctor.test.ts index d8ca4113..d7b49c35 100644 --- a/packages/cli/tests/doctor.test.ts +++ b/packages/cli/tests/doctor.test.ts @@ -5,6 +5,7 @@ import { tmpdir } from 'os' import { collectDoctorReport, renderDoctorTable, renderDoctorJson } from '../src/doctor.js' import { createCodexProvider } from '../src/providers/codex.js' +import { createOpenCodeProvider } from '../src/providers/opencode.js' import { emptyCache, type SessionCache } from '../src/session-cache.js' import type { Provider, ProbeRoot, SessionSource } from '../src/providers/types.js' @@ -141,6 +142,118 @@ describe('collectDoctorReport - env override', () => { else process.env['CODEX_HOME'] = prev } }) + + it('names a deliberate XDG_DATA_HOME override pointing at a missing dir, blaming the override not the install (opencode)', async () => { + const prev = process.env['XDG_DATA_HOME'] + const bogus = join(tmpDir, 'xdg-missing') + process.env['XDG_DATA_HOME'] = bogus + try { + // Construct after setting env so the provider resolves XDG_DATA_HOME + // (src/providers/opencode.ts:42 reads it to resolve the data dir). + const provider = createOpenCodeProvider() + const report = await collectDoctorReport('all', { providers: [provider], cache: emptyCache() }) + const r = only(report, 'opencode') + + expect(r.envOverrides).toContainEqual({ name: 'XDG_DATA_HOME', value: bogus }) + expect(r.status).toBe('empty') + // Regression (Ruling 3 of lane 04): with XDG_DATA_HOME treated as an + // ambient OS var, doctor skipped it and the verdict blamed the install + // ("tool likely not installed") instead of the override the user set. + expect(r.verdict).toContain('override XDG_DATA_HOME set') + expect(r.verdict).toContain('does not exist') + } finally { + if (prev === undefined) delete process.env['XDG_DATA_HOME'] + else process.env['XDG_DATA_HOME'] = prev + } + }) + + // Windows sets APPDATA and LOCALAPPDATA for every process, so neither + // carries user intent: both are fingerprinted (a change moves the discovery + // root) but must never be named as a deliberate override (Ruling 3 of lane + // 04). Table-driven over both so removing either from AMBIENT_ENV_VARS + // fails a test instead of leaking it into the overrides list. + for (const varName of ['APPDATA', 'LOCALAPPDATA']) { + it(`does not name ${varName} as an override for a provider that declares it`, async () => { + const prev = process.env[varName] + process.env[varName] = join(tmpDir, varName.toLowerCase()) + try { + const provider = fakeProvider({ name: 'claude', displayName: 'Claude' }) + const report = await collectDoctorReport('all', { providers: [provider], cache: emptyCache() }) + const r = only(report, 'claude') + + expect(r.envOverrides.some(o => o.name === varName)).toBe(false) + } finally { + if (prev === undefined) delete process.env[varName] + else process.env[varName] = prev + } + }) + } + + // Every credential in SECRET_ENV_VARS must be redacted at collect time so + // neither the text render nor the JSON report can leak it (Ruling 2 of lane + // 04). Table-driven over both, so a credential added to the set without a + // redaction test fails here instead of leaking into a bug report. + for (const varName of ['AI_GATEWAY_API_KEY', 'VERCEL_OIDC_TOKEN']) { + it(`redacts credential values (${varName}) from overrides, the table render, and the JSON report`, async () => { + const secret = `sk-live-${varName}-value-12345` + const prev = process.env[varName] + const sibling = varName === 'AI_GATEWAY_API_KEY' ? 'VERCEL_OIDC_TOKEN' : 'AI_GATEWAY_API_KEY' + const prevSibling = process.env[sibling] + process.env[varName] = secret + // Isolate the case under test: a stray ambient sibling must not change + // what this case observes. + delete process.env[sibling] + try { + const provider = fakeProvider({ name: 'vercel-gateway', displayName: 'Vercel AI Gateway', network: true }) + const report = await collectDoctorReport('all', { providers: [provider], cache: emptyCache() }) + const r = only(report, 'vercel-gateway') + + // The "is this credential set?" diagnostic is useful; the value is a + // live secret and must never leave doctor (Ruling 2 of lane 04). + expect(r.envOverrides).toContainEqual({ name: varName, value: '' }) + expect(r.envOverrides.some(o => o.value.includes(secret))).toBe(false) + const table = renderDoctorTable(report, { color: false }) + expect(table).toContain(`${varName}=`) + expect(table).not.toContain(secret) + expect(renderDoctorJson(report)).not.toContain(secret) + } finally { + if (prev === undefined) delete process.env[varName] + else process.env[varName] = prev + if (prevSibling === undefined) delete process.env[sibling] + else process.env[sibling] = prevSibling + } + }) + } + + // CODEBURN_CURSOR_MAX_BUBBLES caps how many bubbles Cursor parses + // (src/providers/cursor.ts:530) and KIMI_MODEL_NAME renames the model + // attributed to Kimi sessions (src/providers/kimi.ts:125): both are + // fingerprinted but cannot explain why nothing was discovered, so the + // verdict must not name them — while Details still lists them, because they + // ARE overrides in force. Each is asserted through the provider that + // declares it. + for (const [varName, providerName, displayName, value] of [ + ['CODEBURN_CURSOR_MAX_BUBBLES', 'cursor', 'Cursor', '5000'], + ['KIMI_MODEL_NAME', 'kimi', 'Kimi', 'kimi-latest-920'], + ] as const) { + it(`does not blame ${varName} for an empty ${displayName} (not a discovery path)`, async () => { + const prev = process.env[varName] + process.env[varName] = value + try { + const provider = fakeProvider({ name: providerName, displayName }) + const report = await collectDoctorReport('all', { providers: [provider], cache: emptyCache() }) + const r = only(report, providerName) + + expect(r.envOverrides).toContainEqual({ name: varName, value }) + expect(r.verdict).not.toContain(varName) + const table = renderDoctorTable(report, { color: false }) + expect(table).toContain(`${varName}=${value}`) + } finally { + if (prev === undefined) delete process.env[varName] + else process.env[varName] = prev + } + }) + } }) // ── Synthetic edge cases ─────────────────────────────────────────────────── diff --git a/packages/cli/tests/provider-env-declarations.test.ts b/packages/cli/tests/provider-env-declarations.test.ts new file mode 100644 index 00000000..648b0955 --- /dev/null +++ b/packages/cli/tests/provider-env-declarations.test.ts @@ -0,0 +1,249 @@ +// Static guard for issue #920: every `process.env` read inside +// src/providers/*.ts must be declared in PROVIDER_ENV_VARS for every provider +// whose cache section that file's reads affect — or be allowlisted below with +// a reason. An env var that changes what a provider discovers or how its +// sessions parse but is not fingerprinted means the cache section survives +// the change and serves silently stale numbers, exactly the defect class #920 +// reported (nine providers slipped through it). +// +// Scoping rule for the allowlist: an entry is keyed '.ts:' and +// silences exactly one var in exactly one file. The same var read in any +// other file is checked against the declarations like every other read, so an +// entry can never mask a second file's undeclared read — the failure mode the +// original global-keyed allowlist had (Ruling 4 of lane 04). +import { describe, expect, it } from 'vitest' +import { readdirSync, readFileSync } from 'fs' +import { dirname, join } from 'path' +import { fileURLToPath } from 'url' + +import { PROVIDER_ENV_VARS } from '../src/session-cache.js' +import { getAllProviders } from '../src/providers/index.js' + +// ── src/providers/ → provider registry name(s) ──────────────────── +// The provider(s) whose cache section the file's env reads affect. Derived +// from the real code on the extraction branch (packages/cli); registry names +// come from src/providers/index.ts. Do NOT infer this from the filename at +// runtime — the two diverge (e.g. the shared sqlite-session-parser.ts serves +// two providers). A file that contains env reads and is missing here fails +// the guard: add it, with the provider(s) the reads serve. +const FILE_PROVIDERS: Record = { + 'claude.ts': ['claude'], + 'codebuff.ts': ['codebuff'], + 'codewhale.ts': ['codewhale'], + 'codex.ts': ['codex'], + 'copilot.ts': ['copilot'], + 'droid.ts': ['droid'], + 'hermes.ts': ['hermes'], + 'lingtai-tui.ts': ['lingtai-tui'], + // Its only literal read is CODEBURN_CURSOR_MAX_BUBBLES (cursor.ts:530). + 'cursor.ts': ['cursor'], + // The ENV_DIR const (open-design.ts:11) resolves to CODEBURN_OPEN_DESIGN_DIR. + 'open-design.ts': ['open-design'], + 'opencode.ts': ['opencode'], + 'goose.ts': ['goose'], + 'grok.ts': ['grok'], + 'crush.ts': ['crush'], + 'warp.ts': ['warp'], + 'antigravity.ts': ['antigravity'], + 'kilo-code.ts': ['kilo-code'], + 'kimi.ts': ['kimi'], + 'kiro.ts': ['kiro'], + 'mistral-vibe.ts': ['mistral-vibe'], + 'mux.ts': ['mux'], + 'qwen.ts': ['qwen'], + 'ibm-bob.ts': ['ibm-bob'], + 'quickdesk.ts': ['quickdesk'], + 'kimicode.ts': ['kimicode'], + 'zerostack.ts': ['zerostack'], + // Registered (lazy) network provider; its credential reads are declared in + // PROVIDER_ENV_VARS (session-cache.ts) so a read-only refresh that serves + // the cached report (parser.ts:1442/1455) cannot keep serving the previous + // account's usage after a swap. + 'vercel-gateway.ts': ['vercel-gateway'], +} + +// ── Allowlisted reads ──────────────────────────────────────────────────── +// Reads that must NOT invalidate a cache section, one-line reason each. +// Scoping rule: a key is '.ts:' — it silences exactly one var in +// exactly one file, and a read of the same var anywhere else is still checked +// against the declarations (see the header comment). If you add an entry here, +// the guard goes silent for that var in that file — the reason must say +// exactly why a change to it cannot make a cached section stale. +// Reason shared by every copilot.ts entry (Ruling 1 of lane 04): copilot is +// deliberately undeclared in PROVIDER_ENV_VARS. Declaring any of its reads +// would change the copilot fingerprint, and on a fingerprint change +// getOrCreateProviderSection (src/parser.ts:1270) keeps only cached entries +// whose source path no longer exists — but OTel discovery returns one source +// per DB file ({ path: dbPath }, copilot.ts:431) and that DB keeps existing, +// so the cached entry is dropped and re-parsed, destroying conversations +// Copilot has since pruned from the DB that only the cache still holds. +// Deferred until the durable carry-forward learns to merge instead of drop. +const COPILOT_DEFERRED = 'deferred (Ruling 1): declaring it would force the durable re-parse that loses pruned OTel history' +const ALLOWLIST: Record = { + 'opencode.ts:CODEBURN_VERBOSE': 'opencode.ts:151 — logging verbosity only; changes no discovered path and no parsed value', + 'kilo-code.ts:CODEBURN_VERBOSE': 'kilo-code.ts:94 — logging verbosity only; changes no discovered path and no parsed value', + 'copilot.ts:CODEBURN_COPILOT_SESSION_STATE_DIR': COPILOT_DEFERRED, + 'copilot.ts:CODEBURN_COPILOT_OTEL_DB': COPILOT_DEFERRED, + 'copilot.ts:CODEBURN_COPILOT_JETBRAINS_DIR': COPILOT_DEFERRED, + 'copilot.ts:CODEBURN_COPILOT_WS_STORAGE_DIR': COPILOT_DEFERRED, + 'copilot.ts:CODEBURN_COPILOT_GLOBAL_STORAGE_DIR': COPILOT_DEFERRED, + 'copilot.ts:CODEBURN_COPILOT_DISABLE_OTEL': COPILOT_DEFERRED, + 'copilot.ts:APPDATA': COPILOT_DEFERRED, + 'copilot.ts:XDG_CONFIG_HOME': COPILOT_DEFERRED, + 'copilot.ts:LOCALAPPDATA': COPILOT_DEFERRED, +} + +// ── Static extraction ─────────────────────────────────────────────────── + +// Resolved relative to this test file, never the process cwd. +const PROVIDERS_DIR = join(dirname(fileURLToPath(import.meta.url)), '..', 'src', 'providers') + +type EnvRead = { varName: string; line: number } + +// `const IDENT = 'NAME'` string declarations, used to resolve +// `process.env[IDENT]` reads (open-design.ts does this with ENV_DIR). +const STRING_CONST = /const\s+([A-Za-z_$][A-Za-z0-9_$]*)\s*=\s*(['\"])([^'\"]*)\2/g + +function extractEnvReads(source: string): { reads: EnvRead[]; unresolvable: Array<{ line: number; expr: string }> } { + const consts = new Map() + for (const m of source.matchAll(STRING_CONST)) consts.set(m[1]!, m[3]!) + + const reads: EnvRead[] = [] + const unresolvable: Array<{ line: number; expr: string }> = [] + const anyRead = /process\.env/g + for (const m of source.matchAll(anyRead)) { + const line = source.slice(0, m.index).split('\n').length + const rest = source.slice(m.index + 'process.env'.length) + // The expression as written, for failure messages. + const expr = rest.trim().split(/[;\n]/)[0]! + + if (rest.trimStart().startsWith('[')) { + const bracket = rest.slice(rest.indexOf('[')) + const literal = /^\[\s*(['\"])([A-Z0-9_]+)\1\s*\]/.exec(bracket) + if (literal) { + reads.push({ varName: literal[2]!, line }) + continue + } + const ident = /^\[\s*([A-Za-z_$][A-Za-z0-9_$]*)\s*\]/.exec(bracket) + if (ident) { + const resolved = consts.get(ident[1]!) + if (resolved) { + reads.push({ varName: resolved, line }) + continue + } + unresolvable.push({ line, expr: `process.env[${ident[1]}]` }) + continue + } + unresolvable.push({ line, expr: `process.env${expr}` }) + continue + } + + if (rest.trimStart().startsWith('.')) { + const dot = /^\.\s*([A-Za-z_$][A-Za-z0-9_$]*)/.exec(rest) + if (dot) { + reads.push({ varName: dot[1]!, line }) + continue + } + } + + // Bare `process.env` or any other form: cannot name a var — fail loudly, + // an unresolvable read must never be silently skipped. + unresolvable.push({ line, expr: `process.env${expr}` }) + } + return { reads, unresolvable } +} + +function failWith(problems: string[]): void { + if (problems.length > 0) throw new Error(`\n${problems.join('\n\n')}`) +} + +describe('provider env declarations (#920)', () => { + it('every process.env read in src/providers is declared for the provider(s) it serves', () => { + const problems: string[] = [] + + for (const entry of readdirSync(PROVIDERS_DIR, { withFileTypes: true })) { + if (!entry.isFile() || !entry.name.endsWith('.ts')) continue + + const source = readFileSync(join(PROVIDERS_DIR, entry.name), 'utf8') + const { reads, unresolvable } = extractEnvReads(source) + if (reads.length === 0 && unresolvable.length === 0) continue + + const served = FILE_PROVIDERS[entry.name] + if (!served) { + problems.push( + `src/providers/${entry.name} reads env vars (${reads.map(r => r.varName).join(', ')}) but is missing from FILE_PROVIDERS — add it with the provider(s) whose cache section these reads affect.`, + ) + continue + } + + for (const { line, expr } of unresolvable) { + problems.push( + `src/providers/${entry.name}:${line}: unresolvable env read \`${expr}\` — resolve it to a literal name (e.g. \`const IDENT = 'NAME'\` in the same file) so the guard can verify it is declared; an unresolvable read must never be silently skipped.`, + ) + } + + for (const { varName, line } of reads) { + // File-scoped: an allowlist entry silences this var in this file only + // (see the header comment); a read of the same var in another file + // must be declared or allowlisted there. + if (ALLOWLIST[`${entry.name}:${varName}`]) continue + for (const provider of served) { + if (!(PROVIDER_ENV_VARS[provider] ?? []).includes(varName)) { + problems.push( + `provider '${provider}' reads process.env['${varName}'] at src/providers/${entry.name}:${line} but it is not declared in PROVIDER_ENV_VARS['${provider}'] — declare it there (it changes what the provider discovers or how its sessions parse) or add '${entry.name}:${varName}' to ALLOWLIST with a reason.`, + ) + } + } + } + } + + failWith(problems) + }) + + it('every PROVIDER_ENV_VARS key is a real provider name from the registry', async () => { + const names = new Set((await getAllProviders()).map(p => p.name)) + const problems: string[] = [] + for (const key of Object.keys(PROVIDER_ENV_VARS)) { + if (!names.has(key)) { + // A typo'd key declares nothing and fails silently — the same defect + // class #920 fixed. Do NOT delete the key or weaken the assertion; + // surface it so the registry or the key gets corrected. + problems.push(`PROVIDER_ENV_VARS key '${key}' is not a registered provider name — a typo'd key declares nothing and fails silently.`) + } + } + failWith(problems) + expect(problems).toEqual([]) + }) + + it('allowlist entries are file-scoped: every key is .ts: shaped, names a real file, and names a var that file actually reads', () => { + const problems: string[] = [] + const providerFiles = new Set( + readdirSync(PROVIDERS_DIR, { withFileTypes: true }) + .filter(e => e.isFile() && e.name.endsWith('.ts')) + .map(e => e.name), + ) + + for (const key of Object.keys(ALLOWLIST)) { + const match = /^([A-Za-z0-9._-]+\.ts):([A-Z0-9_]+)$/.exec(key) + if (!match) { + // A global-keyed entry would mask an undeclared read of the same var + // in any other file (the pre-lane-04 failure mode). Reject it here so + // the scoping rule is enforced, not just documented. + problems.push(`ALLOWLIST key '${key}' is not '.ts:' shaped — an allowlist entry must silence exactly one var in exactly one file.`) + continue + } + const [, fileName, varName] = match + if (!providerFiles.has(fileName!)) { + problems.push(`ALLOWLIST key '${key}' names '${fileName}', which is not a file in src/providers — the entry silences nothing and must be removed.`) + continue + } + const { reads } = extractEnvReads(readFileSync(join(PROVIDERS_DIR, fileName!), 'utf8')) + if (!reads.some(r => r.varName === varName)) { + problems.push(`ALLOWLIST key '${key}' names var '${varName}' but src/providers/${fileName} never reads it — dead entry; remove it.`) + } + } + + failWith(problems) + expect(problems).toEqual([]) + }) +}) diff --git a/packages/cli/tests/session-cache.test.ts b/packages/cli/tests/session-cache.test.ts index 2c736b22..73afbcec 100644 --- a/packages/cli/tests/session-cache.test.ts +++ b/packages/cli/tests/session-cache.test.ts @@ -6,6 +6,7 @@ import { basename, join } from 'path' import { CACHE_VERSION, + PROVIDER_ENV_VARS, type CachedCall, type CachedFile, type CachedTurn, @@ -281,6 +282,129 @@ describe('computeEnvFingerprint', () => { }) }) +// ── provider env overrides invalidate the fingerprint (#920) ───────────── + +describe('provider env overrides invalidate the fingerprint (#920)', () => { + // Nine providers honored an env var that relocates where discovery looks + // without the var being declared in PROVIDER_ENV_VARS, so + // computeEnvFingerprint did not hash it and the cache section survived the + // change: sessions parsed from the old root kept being reported and the new + // root was never read. Each pair below must change the fingerprint when the + // var is set. codex/CODEX_HOME is the control — it already worked and must + // keep working. + const CASES: Array<[provider: string, varName: string]> = [ + ['kiro', 'KIRO_HOME'], + ['grok', 'GROK_HOME'], + ['kimi', 'KIMI_SHARE_DIR'], + ['mux', 'MUX_ROOT'], + ['mistral-vibe', 'VIBE_HOME'], + ['zerostack', 'ZS_DATA_DIR'], + ['codebuff', 'CODEBUFF_DATA_DIR'], + ['goose', 'GOOSE_PATH_ROOT'], + ['crush', 'CRUSH_GLOBAL_DATA'], + ['codex', 'CODEX_HOME'], + ] + const VARS = CASES.map(([, varName]) => varName) + + // Save and restore every var we touch (beforeEach/afterEach), so a leaked + // env var never breaks unrelated tests in the same worker — and an ambient + // value never makes the "unset" case a lie. + const saved = new Map() + + beforeEach(() => { + for (const varName of VARS) { + saved.set(varName, process.env[varName]) + delete process.env[varName] + } + }) + + afterEach(() => { + for (const varName of VARS) { + const original = saved.get(varName) + if (original === undefined) delete process.env[varName] + else process.env[varName] = original + } + }) + + for (const [provider, varName] of CASES) { + it(`changes the ${provider} fingerprint when ${varName} is set`, () => { + const unset = computeEnvFingerprint(provider) + process.env[varName] = '/tmp/codeburn-920-override' + const set = computeEnvFingerprint(provider) + expect(set).not.toBe(unset) + // Round trip: restoring the variable to its original state restores the + // original fingerprint, so the hash is a pure function of the + // environment. + delete process.env[varName] + expect(computeEnvFingerprint(provider)).toBe(unset) + }) + } + + it('changes the vercel-gateway fingerprint when AI_GATEWAY_API_KEY is set', () => { + const prev = process.env['AI_GATEWAY_API_KEY'] + try { + const unset = computeEnvFingerprint('vercel-gateway') + process.env['AI_GATEWAY_API_KEY'] = 'sk-liv...-abc' + const set = computeEnvFingerprint('vercel-gateway') + expect(set).not.toBe(unset) + delete process.env['AI_GATEWAY_API_KEY'] + expect(computeEnvFingerprint('vercel-gateway')).toBe(unset) + } finally { + if (prev === undefined) delete process.env['AI_GATEWAY_API_KEY'] + else process.env['AI_GATEWAY_API_KEY'] = prev + } + }) + + // Copilot is deliberately NOT declared in PROVIDER_ENV_VARS (Ruling 1 of + // lane 04): its OTel discovery returns one source per DB file + // ({ path: dbPath }, src/providers/copilot.ts:431), and the durable + // carry-forward in getOrCreateProviderSection (src/parser.ts:1270) drops + // every cached entry whose source still exists on a fingerprint change — so + // declaring any CODEBURN_COPILOT_* var would force a re-parse that destroys + // conversations Copilot has since pruned from the DB, which only the cache + // still holds. The fingerprint must therefore NOT move when one is set. + // This reads as intent, not as an oversight — and the assertions below pin + // the WHOLE invariant (no entry at all, plus every one of the nine deferred + // reads), so a future "completing" edit fails a test instead of silently + // re-opening the durable history-loss path. + describe('copilot is deliberately undeclared in PROVIDER_ENV_VARS', () => { + it('has no PROVIDER_ENV_VARS entry at all', () => { + expect(PROVIDER_ENV_VARS['copilot']).toBeUndefined() + }) + + // The nine reads copilot.ts performs whose declaration is deferred (each + // is allowlisted in tests/provider-env-declarations.test.ts): setting any + // of them must leave the copilot fingerprint untouched. + const DEFERRED_COPILOT_VARS = [ + 'CODEBURN_COPILOT_SESSION_STATE_DIR', + 'CODEBURN_COPILOT_OTEL_DB', + 'CODEBURN_COPILOT_JETBRAINS_DIR', + 'CODEBURN_COPILOT_WS_STORAGE_DIR', + 'CODEBURN_COPILOT_GLOBAL_STORAGE_DIR', + 'CODEBURN_COPILOT_DISABLE_OTEL', + 'APPDATA', + 'LOCALAPPDATA', + 'XDG_CONFIG_HOME', + ] + + for (const varName of DEFERRED_COPILOT_VARS) { + it(`does not move the copilot fingerprint when ${varName} is set (deliberately undeclared)`, () => { + const prev = process.env[varName] + try { + const before = computeEnvFingerprint('copilot') + process.env[varName] = `/tmp/codeburn-copilot-920/${varName}` + expect(computeEnvFingerprint('copilot')).toBe(before) + delete process.env[varName] + expect(computeEnvFingerprint('copilot')).toBe(before) + } finally { + if (prev === undefined) delete process.env[varName] + else process.env[varName] = prev + } + }) + } + }) +}) + // ── fingerprintFile ──────────────────────────────────────────────────── describe('fingerprintFile', () => {