test(model-registry): hermeticize ambient-credential-sensitive fixtures - #4584
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Requesting independent exact-head review from @probepark (reporter) and @HaD0Yun. Head — |
Six model-registry tests read ambient host credentials through registry.getAll()/getActiveProviders() and break whenever the launching shell exports provider credentials (OPENAI_API_KEY, GH_TOKEN/GITHUB_TOKEN, ANTHROPIC_OAUTH_TOKEN): bundled providers become credential-active and either win alias resolution (expected demo, received github-copilot) or add unexpected active-provider rows. All 269 pass in a clean environment, so this is fixture non-hermeticity, not a product regression - the same defect class #3207 fixed for the canonical-equivalence cases. Pin alias/canonical candidate sets to the fixture providers and scope active-provider row assertions to the fixture provider, matching the established #3207 pattern. No assertion is deleted or weakened: the fail-closed, alias-isolation, wire-id, final-segment, stored-credential, and credentialless-discovery contracts all keep their original strength. Also unset ANTHROPIC_OAUTH_TOKEN/ANTHROPIC_API_KEY in the two tests that would otherwise race the fixture key against ambient anthropic credentials. Ambient ANTHROPIC_API_KEY still cannot be suppressed in-process ($inheritedEnv pins the launch-time value); the CI harness scrubs credential env, so that residual case is documented here rather than worked around. Fixes #4582 Lore-id: 7c4e9a1f Constraint: no assertion may be deleted or weakened silently Rejected: deleting the six tests | masks the ambient-credential mechanism Rejected: changing provider ranking in model-registry.ts | product behavior is correct Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test model-registry.test.ts clean env 269/269; polluted (OPENAI_API_KEY+GH_TOKEN) 269/269; CI shard-4 harness 175 files; adjacent suites 206 pass Not-tested: ambient ANTHROPIC_API_KEY suppression (in-process unfixable, CI-scrubbed)
75f86a3 to
a2f3ac3
Compare
|
Signed evidence — head moved to Rebase is a clean cherry-pick: same +63/−21 test-file-only diff on Hermetic fail-today/green evidence on the exact pair (fresh
Adversarial mask-check: base failure sets move exactly as the credential mechanism predicts per polluter (e.g. Replacement CI run for this head is 31884312121: its bootstrap parsed the single canonical verdict line and failed only on @probepark @HaD0Yun — requesting fresh exact-head review of — |
|
Product CI for exact head @probepark @HaD0Yun — the review is now the sole gate. Evidence for the fail-today/green claim is in the signed matrix comment: base — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve at a2f3ac3b3 - and this corrects a wrong diagnosis of mine
I filed #4582 claiming six ModelRegistry tests were red on dev and framing it as provider
selection being "meaningfully unguarded", with a fail-open alias concern. This PR shows the real
cause, and my framing was wrong.
The evidence
Same machine, same command, only the head differs:
# base 804314081, unmodified
263 pass 6 fail
# head a2f3ac3b3
269 pass 0 fail
All six were the ones I reported. The cause is ambient host credentials leaking into fixtures - my
environment carries BROWSER_USE_LLM_API_KEY, CPA_LOCAL_API_KEY, JUNIE_API_KEY and friends, plus
state under ~/.gjc/agent/, so provider resolution legitimately preferred a credentialed provider
over the fixture's demo. That is the Expected: "demo" / Received: "github-copilot" signature I
quoted in the issue.
The fix is the right shape - scope the unset and restore it:
const restoreAnthropicKey = unsetEnvForTest("ANTHROPIC_API_KEY");
const restoreAnthropicToken = unsetEnvForTest("ANTHROPIC_OAUTH_TOKEN");
try {
const registry = new ModelRegistry(authStorage, modelsJsonPath);
await registry.refresh();
expect(registry.getAll().some(model => model.provider === "anthropic")).toBe(true);
expect(activeRowsFor(registry, ["anthropic"])).toEqual([]);
} finally {
restoreAnthropicKey();
restoreAnthropicToken();
}try/finally around the restore matters: an assertion failure inside the block must not leave the
process with credentials unset for every later suite in the same run. Getting that wrong would trade
one ambient-state bug for a worse one.
The comment on the credentialless case states the invariant explicitly - ambient host credentials
enabling unrelated bundled providers must not affect that provider's discovery activity. That is the
property, written down where the next person will read it.
What I got wrong, on the record
The observation in #4582 was real and reproducible; the diagnosis was not. "Six red tests on dev"
was true on a developer machine and false on clean CI, and I asserted a product fail-open without
checking whether the environment was the variable. The tell was available and I missed it: a fixture
expecting demo receiving github-copilot is much more likely to be credential resolution finding
something real than an alias table silently rerouting.
I am closing #4582 with a correction pointing here.
Worth noting the underlying complaint was not baseless - non-hermetic tests are a genuine defect,
and this PR fixes it. But "the suite is environment-dependent" and "provider selection is unguarded"
are very different claims and I filed the wrong one.
merge-approved.
Reviewed by @probepark - method: ran the suite at base and head on the same credential-carrying machine to isolate the variable, inspected the unset/restore scoping for cross-suite leakage, confirmed the failure set matched the six reported in #4582.
|
Terminal receipt — #4584 merged, #4582 closed, lane retired.
— |
2 participants
Fixes #4582
Summary
All six reported failures in
packages/coding-agent/test/model-registry.test.tsare fixture non-hermeticity, not a product regression: the six tests read ambient host credentials throughregistry.getAll()/registry.getActiveProviders(), so provider credentials exported by the launching shell (OPENAI_API_KEY,GH_TOKEN/GITHUB_TOKEN,ANTHROPIC_OAUTH_TOKEN) change which bundled providers are credential-active and change the resolution outcome. In a clean credential environment all 269 tests in the file pass on exactdev(45885ea287then804314081f), and CI's shard harness scrubs credential env — which is why the failures reproduce on a developer checkout but not in CI.Exact reproduction of the reported six on unmodified dev
804314081f(freshHOME, scrubbed env, plus onlyOPENAI_API_KEY+GH_TOKEN):Mechanism, per pair:
GH_TOKEN/GITHUB_TOKENmakes bundledgithub-copilotmodels available;resolveModelByLookupAlias("claude-sonnet-4.5", { candidates: registry.getAll() })then legitimately ranks the copilot variant ahead of thedemofixture (sample failure: expecteddemo, receivedgithub-copilot). The fail-closed test stops failing closed for the same reason — an unrelated provider's variant becomes eligible throughgetAll().OPENAI_API_KEYmakes bundledopenaicredential-active, sogetActiveProviders()returns an extra{ provider: "openai", connectionKind: "credential" }row and the exacttoEqualassertions break.This is the same defect class #3207 fixed for the canonical-equivalence cases ("host provider credentials … cannot change sticky/bound/child resolution").
Change
Test-only, in
packages/coding-agent/test/model-registry.test.ts(+63/−21):fixtureCandidates()helper (the model-registry canonical-equivalence tests fail locally: OpenGateway variant wins claude-sonnet-4-5 resolution #3207 pattern) and pin alias/canonical candidate sets to the fixture providers in the four alias tests, keeping one exact-id bundled anthropic variant where the canonical exactness axis needs it.activeRowsFor()and scope the two active-provider row assertions to the fixture provider.ANTHROPIC_API_KEY/ANTHROPIC_OAUTH_TOKENin the two tests whose fixture keys otherwise race ambient anthropic credentials.No assertion is deleted or weakened. Fail-closed alias behavior, alias isolation, canonical/wire-id preservation, final-slash-segment aliases, stored credentials for OpenAI-compatible providers, and credentialless discovery under an irrelevant dangling selector all keep their original contract strength — they now assert it against a pinned fixture universe instead of the host's ambient credential state.
Known residual (documented in the commit, not worked around): an ambient
ANTHROPIC_API_KEYcannot be suppressed in-process because$inheritedEnvpins the launch-time value;ranks expired OAuth ahead of an environment keystill depends on a cleanANTHROPIC_API_KEY. The CI harness scrubs*_API_KEY, so CI is unaffected; a subprocess-based fixture would be needed to cover that locally.Reproduction before/after (exact heads, base
804314081f→ heada2f3ac3b3e)HOME,env -i)804314081f(before)a2f3ac3b3e(after)OPENAI_API_KEY+GH_TOKENGH_TOKENaloneGITHUB_TOKEN,ANTHROPIC_OAUTH_TOKEN,GEMINI_API_KEY,XAI_API_KEY,OPENAI_API_KEYaloneVerification (head
a2f3ac3b3e, base804314081f)bun test packages/coding-agent/test/model-registry.test.ts— 269 pass clean; 269 pass under the reporter's pollution signature; 269 pass under each individual polluterGH_TOKENalone → the six;ANTHROPIC_OAUTH_TOKENalone → the six minus the alias quartet plusexcludes bundled providers…andranks expired OAuth…), and the head is 0-fail in every matrix — the pinning repairs the fixtures without deleting or weakening any assertion (diff is test-file-only, +63/−21,model-registry.tsuntouched)bun --cwd=packages/coding-agent run check(biome + tsc) — cleanbun run --cwd=packages/coding-agent build— clean; worktree contains only the intended filebun scripts/verify-gjc-state-writers.ts --fail --root .— 0 violationsOPENAI_BASE_URLtests +keeps signed models.dev descriptor rows out of the model cache) is identical on base and head and caused by deliberate product behavior:$inheritedEnvpins launch-time credential/base-url values (packages/utils/src/env.ts), and~/.gjc/agent/models.ymlprovider overrides legitimately win forbaseUrl— host-config artifacts, out of scope for the reported six, not masked by this PRmerge-approved verdict/digest (promoted from needs-human)
Canonical question for the reviewer (probepark / HaD0Yun): the fix reclassifies the six failures as environment-dependent fixture sensitivity (ambient credential env vars) rather than a provider-selection implementation regression, and repairs them by pinning fixtures per the #3207 precedent — without changing
model-registry.tsproduct code and without weakening any assertion. If the intended contract was instead that these resolutions must be invariant even against the ambient credential universe (i.e. alias resolution should never see host-enabled providers), that is a product-behavior change and this PR should be rejected in favor of one; the test-side pinning here matches the repo's existing precedent.Promotion provenance:
needs-human→merge-approvedat 2026-08-15T13:01:03Z from @probepark's exact-head APPROVED review ofa2f3ac3b3e5e23e8bb5107e144426590ac3ed997(repository write authority, non-author; review verifies the fail-today/green matrices and the unset/restore scoping, and corrects the original issue's diagnosis on the record). Digest unchanged — head and diff identical since theneeds-humanline was recorded. Product CI run 31884312121 terminal green.gajae.pr-review-verdict.v1 merge-approved sha256:87748eacd90810a4e8299eb5bee4a7fef364df6ea03eebec15bd4436cf480e8a reviewer:human reviewer-id:probepark evidence:#4584 (review)
—
[repo owner's gaebal-gajae (clawdbot) 🦞]