fix(cache): declare the provider env overrides that must invalidate the cache - #927
Open
ozymandiashh wants to merge 4 commits into
Open
fix(cache): declare the provider env overrides that must invalidate the cache#927ozymandiashh wants to merge 4 commits into
ozymandiashh wants to merge 4 commits into
Conversation
…he cache Nine providers honor an env var that relocates where discovery looks, but the var was never declared in PROVIDER_ENV_VARS, so computeEnvFingerprint() did not hash it and the provider's cache section survived the change: sessions parsed from the old root kept being reported and the new root was never read, with no diagnostic anywhere (getagentseal#920, same silent-wrong-numbers family as getagentseal#874). Declare every env var that changes what a provider discovers or how its sessions parse, including the platform path vars that resolve a discovery root on Windows and Linux, and the CodeBurn-side directory overrides. Ambient platform vars (APPDATA, LOCALAPPDATA, XDG_CONFIG_HOME, XDG_DATA_HOME) are set by the OS or the desktop session for everyone, so doctor must not name them as a deliberate override: without the guard every Windows user would be told Claude and Copilot discovery runs under an override. They stay in the fingerprint - a change to them does move the discovery root - but doctor skips them when collecting overrides, and the probed paths it already prints show where CodeBurn looked.
The nine undeclared overrides in getagentseal#920 all slipped through the same way: the declaration lives in one file and the read in another, and nothing tied them together. Add the static guard the issue asked for - every process.env read in src/providers is either declared in PROVIDER_ENV_VARS for the provider(s) that file serves, or allowlisted with a reason. It resolves bracket literals, dot access and `process.env[CONST]` indirection (open-design's ENV_DIR), and fails loudly on any read it cannot resolve to a name rather than skipping it, since a silently skipped read is how this class of defect survives. A read-bearing provider file missing from the file-to-provider map fails too, so a new provider cannot join without being mapped. A second assertion catches a PROVIDER_ENV_VARS key that is not a registered provider name, which declares nothing and fails just as silently. Plus the direct regression: each of the nine reported (provider, var) pairs must move the fingerprint, with codex/CODEX_HOME as the control the issue used, and the round trip asserted so the hash stays a pure function of the environment.
Five findings from a cross-model review of the previous two commits, each verified on the code before acting: Copilot is no longer declared. Declaring anything for it changes its fingerprint, and getOrCreateProviderSection keeps only cached entries whose source path is gone - but OTel discovery returns one source per DB file (copilot.ts:1935) and that DB keeps existing, so the entry would be dropped and re-parsed, destroying conversations Copilot has since pruned from the DB that only the cache still holds. Trading a staleness bug for a data-loss bug is a bad trade; copilot waits for the durable carry-forward to merge instead of drop, and its reads are allowlisted with that reason. The Vercel gateway credentials ARE declared, reversing the previous commit's reasoning, which was wrong: servedSources is seeded with every discovered source (parser.ts:2875) before the network branch, and the network re-fetch (parser.ts:2888) only runs when !readOnly, so a read-only refresh serves the cached report and an undeclared credential keeps reporting the previous account's usage after a swap. Doctor redacts credential values so a key can never reach terminal output or the JSON report. AMBIENT_ENV_VARS narrows to APPDATA and LOCALAPPDATA. Windows sets those for every process so they carry no intent, but the XDG vars are opt-in and do: suppressing them made doctor answer a deliberately relocated XDG_DATA_HOME with "tool likely not installed", which is worse than the noise it avoided. The guard's allowlist is keyed by file and var, not var alone - a var allowlisted for one file silenced every other file's undeclared read of it. Cursor drops its stale XDG_DATA_HOME declaration, which it never reads; its fingerprint already changes here, so this costs no extra migration. cursor-agent keeps its equally stale one, since removing it would force a re-parse to fix nothing.
Round 2 of the independent review proved five things by mutation: it broke the behavior and the tests stayed green. Every one is now pinned. The most important invariant in this change was the least guarded. Copilot must have NO entry in PROVIDER_ENV_VARS - declaring any of its nine reads moves its fingerprint and re-opens the durable history-loss path - but only one of the nine was covered, so declaring any of the other eight passed the whole suite. Now the absence of the entry is asserted directly, and all nine vars are table-tested for fingerprint stability. Doctor stops blaming parse-only overrides for a failed discovery. CODEBURN_CURSOR_MAX_BUBBLES caps how many bubbles Cursor parses and KIMI_MODEL_NAME renames an attributed model; neither relocates anything, so "NOTHING FOUND (override CODEBURN_CURSOR_MAX_BUBBLES set...)" pointed the user at the wrong thing. Both join NON_DISCOVERY_ENV_VARS, which exists for exactly this, and both still appear in Details - only the verdict's blame line changes. The secret-redaction and ambient-suppression tests are table-driven over both names each covers, since removing either second name (VERCEL_OIDC_TOKEN, LOCALAPPDATA) previously leaked or surfaced it with every test still passing. The changelog no longer claims a one-time re-parse for the Vercel gateway: it is a network provider re-fetched on every writable run, so its declaration is a read-only-path correction, not a migration. Fourteen file-backed providers migrate once.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #920.
computeEnvFingerprint()hashes only whatPROVIDER_ENV_VARSdeclares, so the nine providers that honor an undeclared env var never invalidated their cache section: pointKIRO_HOMEat a second profile and CodeBurn keeps serving the first profile's sessions, reports nothing from the new root, and says nothing about it. Declared now — the nine reported overrides, the adjacent OS-set path vars that resolve a discovery root on Windows and Linux (Claude, IBM Bob, Open Design, Kilo Code), and Cursor's parse budget. Fourteen file-backed providers re-parse once as a result, which the changelog calls out.Two things the issue did not anticipate, both found by review and verified on the code before acting:
Copilot is deliberately left undeclared. Declaring any of its nine reads moves its fingerprint, and
getOrCreateProviderSectionkeeps only cached entries whose source path is gone — but OTel discovery returns one source per DB file (copilot.ts:1935) and that DB keeps existing, so the entry would be dropped and re-parsed, destroying conversations Copilot has since pruned from the DB that only the cache still holds. That trades a staleness bug for a data-loss bug. Its reads are allowlisted with that reason, the map documents it, and a test pins the invariant so nobody "completes" the map before the durable carry-forward learns to merge instead of drop. Follow-up below.The Vercel gateway credential is declared, despite being a network provider.
servedSourcesis seeded with every discovered source (parser.ts:2875) before the network branch, and the re-fetch (parser.ts:2888) only runs when!readOnly— so a read-only refresh serves the cached report straight from the section, and an undeclared credential keeps reporting the previous account's usage after a swap. Doctor redacts credential values to<set>at collect time, so a key cannot reach terminal output or the JSON report.codeburn doctorneeded a matching model, since it treats every declared-and-set var as a deliberate override. It now distinguishes three kinds: ambient Windows paths (APPDATA,LOCALAPPDATA) are hidden — Windows sets them for every process, so naming them would tell every Windows user their discovery runs under an override; parse-only settings (CODEBURN_CURSOR_MAX_BUBBLES,KIMI_MODEL_NAME) stay in Details but never get blamed in a NOTHING FOUND verdict, since neither relocates anything; the XDG vars stay fully visible, because they are opt-in and a set value is a real user override — suppressing them made doctor answer a deliberately relocatedXDG_DATA_HOMEwith "tool likely not installed".The issue asked for a test that stops this recurring, and
tests/provider-env-declarations.test.tsis it: everyprocess.envread insrc/providersmust be declared for the provider(s) that file serves, or allowlisted with a reason. It resolves bracket literals, dot access andprocess.env[CONST]indirection (open-design'sENV_DIR), fails loudly on any read it cannot resolve rather than skipping it, and fails when a read-bearing provider file is missing from the file-to-provider map. Allowlist entries are keyedfile:VAR, so silencing a var in one file cannot mask another file's undeclared read.Verification.
tsc --noEmitclean; 117 tests green across the three affected files. Every new test was checked by breaking what it protects and confirming it goes red — the guard against five planted defects (dot access, dynamic key, bracket literal, a removed declaration, an unmapped provider file), and each doctor/cache invariant against removal of the exact entry it pins. The nine reported (provider, var) pairs each move the fingerprint, withcodex/CODEX_HOMEas the control the issue used. Note for CI: this repo's full suite is flaky under parallel load independent of this change — on a clean checkout at the base commit it fails a varying set (cache-refresh-lock,cli-json-daily,spend-flow), all passing in isolation.Follow-ups, deliberately not in this PR:
PROVIDER_PARSE_VERSIONSbump — can lose pruned OTel history.quickdesksetsdurableSources: truebut is absent fromDURABLE_PROVIDER_NAMES. Untouched by this PR (neither its declarations nor its parse version change here), but it is the same latent hazard.cursor-agentdeclaresXDG_DATA_HOMEwithout reading it. Kept deliberately — removing it would force a re-parse to fix nothing — but worth cleaning up whenever that provider next migrates.