Repository navigation
fix(providers): stop the Antigravity rename migration re-announcing itself on every boot - #5077
Conversation
…tself The rename migration and the OAuth preset reconciliation disagreed about nine retired Antigravity Flash ids, and startServer runs both, in that order. ANTIGRAVITY_MODEL_CONTEXT_WINDOWS derives a window for every compatibility alias, so the google-antigravity preset carries a modelContextWindows key for each retired id. The migration renamed those keys and persisted, applyOAuthPresetCatalog copied the preset's record back over them in the same boot, and the next start found them again: nine [model-rename-migration] lines on every ocx start, for a user who never selected any of those nine models. Skip a field the registry's own seed still publishes keyed by the retired id. Those entries are deliberate (a request naming gemini-3.6-flash routes to 3.7 and needs a window under the id it asked for), and reconciliation owns that record anyway, so the migration has nothing to repair there. Every field the user owns (models, selectedModels, retainModels, defaultModel, disabledModels) is still renamed and still reported once. Closes #5066
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe migration now skips registry-seeded retired IDs when the saved field already contains the replacement ID. Regression tests cover repeated startup, user-selected IDs, registry-published metadata, and all shipped renames. ChangesModel rename migration
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The migration preserves registry compatibility metadata while continuing to repair user-selected retired IDs, with no remaining actionable risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dfc1f303f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // replacement — and only in the fields the registry does not itself republish | ||
| // keyed by that retired id (see `registrySeededFields`, issue #5066). Everything |
There was a problem hiding this comment.
This changes the startup/config-migration contract for the owned src/providers/ area, but the commit updates none of the documents mapped to that area in structure/INDEX.md (runtime.md, subagents.md, transports/inventory.md, providers-and-adapters.md, and providers/xai-grok.md). Update those documents in this change so the maintained architecture description records which registry-owned fields the rename migration intentionally leaves untouched.
AGENTS.md reference: src/AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
| for (const field of MODEL_KEYED_RECORDS) { | ||
| if (registrySeeded.has(field)) continue; | ||
| const next = rename.dropReasoningEffortMap && field === "modelReasoningEffortMap" |
There was a problem hiding this comment.
When a saved google-antigravity row has a non-oauth or legacy-absent authMode, reconcileOAuthProviders does not republish modelContextWindows, and enrichProviderFromRegistry only supplies that record when it is missing. Nevertheless, this unconditional skip treats the field as registry-owned merely because the seed contains the retired ID. For a row with defaultModel: "gemini-3.6-flash" and a user override such as modelContextWindows: {"gemini-3.6-flash": 262144}, the migration now changes the default to 3.7 but strands the override under 3.6, so routing falls back to the registry's 3.7 window. Restrict the skip to rows actually eligible for OAuth reconciliation, or distinguish the registry seed shape from user-owned values.
AGENTS.md reference: src/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
The first cut skipped any field the registry seed published keyed by the retired id, which also skipped a row that saved only the retired key. That row needs the value carried onto the supported id, and dropping it blanks the record instead of moving it (test 4/4 caught it at gemini-37-flash-migration.test.ts:211). Require both conditions now: the registry seed still publishes the retired id in the field, AND the saved field already carries the supported id, so the rename would only delete a key reconciliation puts straight back. The reporter's post-reconcile row satisfies both for all nine ids; a row with just the retired key does not, and is still migrated and still reported.
abhisheksharma2411
left a comment
abhisheksharma2411
left a comment
There was a problem hiding this comment.
The diagnosis is the valuable part here and it's exactly right — this was never a persistence failure, it was two boot phases disagreeing. runModelRenameStartupMigration deletes the keys, reconcileOAuthProviders → applyOAuthPresetCatalog copies the preset's record straight back, and the next boot finds them again. A migration that loses to the second half of its own boot is a genuinely hard thing to see from the symptom, and "nine messages from a user who never selected nine models" is the tell that makes it obvious in hindsight.
Traced the root cause to confirm the premise rather than take it on trust, and it holds:
export const ANTIGRAVITY_MODEL_CONTEXT_WINDOWS = {
"gemini-3.8-flash": …, "gemini-3.7-flash": …, "gemini-3.1-pro": …,
...ANTIGRAVITY_WIRE_MODEL_CONTEXT_WINDOWS,
...Object.fromEntries(Object.entries(ANTIGRAVITY_MODEL_ALIASES).map(…)), // <- every alias
};
export const ANTIGRAVITY_MODEL_ALIASES = {
...ANTIGRAVITY_VISIBLE_MODEL_ALIASES,
...ANTIGRAVITY_COMPATIBILITY_MODEL_ALIASES, // <- includes the retired ids
};So the seed publishes a window for every compatibility alias, retired ones included, which is precisely why the delete can never converge.
Worth noting the sibling map does not do this. ANTIGRAVITY_MODEL_INPUT_MODALITIES, twenty lines below, lists live ids explicitly and derives nothing. Two maps describing the same model set, one curated and one derived-over-all-aliases — that asymmetry is the root cause, and it'll produce the next instance of this bug in whatever field gets added next.
I'd still land this rather than chase that, and I think the PR chose correctly: narrowing ANTIGRAVITY_MODEL_CONTEXT_WINDOWS to live ids would strip the window from requests that still use a retired alias, which is a behaviour change on the request path to fix a logging and config-churn problem. Suppressing at the migration is the smaller blast radius. But it's worth a line in the code or the issue saying the derivation is the upstream cause, so the next person doesn't re-derive the whole diagnosis.
On isRegistryResidue itself: requiring both conditions — registry still publishes the retired id, and the saved field already carries the supported id — is the right pair. It also self-heals: the day the registry stops seeding a retired alias, publishes goes false, the migration resumes, and the key finally gets cleaned up without anyone revisiting this function. Worth saying that in the doc comment, because "skip the rename" reads like a permanent exemption and it isn't.
140 lines of test for 55 lines of fix is the right ratio for a boot-path migration.
2 participants
Summary
Starting the proxy with a
google-antigravityprovider printed the same nine[model-rename-migration] renamed "google-antigravity/gemini-3.x-flash..."lines on everysingle boot (#5066). The migration was not failing to persist; it was losing to the second half
of its own boot.
startServerrunsrunModelRenameStartupMigrationand thenreconcileOAuthProviders.ANTIGRAVITY_MODEL_CONTEXT_WINDOWSderives a context window for every compatibility alias, sothe Antigravity OAuth preset carries a
modelContextWindowskey for all nine retired Flash idsbeside the live ones. The migration deleted those keys and wrote the config;
applyOAuthPresetCatalogthen copied the preset's record straight back over them and wrote theconfig again; the next boot found them and said so. That is also why the report shows nine
messages from a user who never selected nine models: the ids came from the registry, not from
anything they had chosen.
The fix is one guard in the projection, the mirror of the
registrySeedsTargetcheck already inthat file. A field is skipped only when both of these hold:
providerConfigSeed, the row both the OAuth preset andenrichProviderFromRegistrycontribute) still publishes the retired id in that field, so theentry arrives in the config on its own; and
with no value left to carry forward.
The second condition is what keeps it narrow. A row that saved
modelContextWindows: { "gemini-3.6-flash": 1048576 }and nothing else still gets that valuemoved onto
gemini-3.7-flash, because dropping it there would blank the record instead ofmoving it. The retired registry entries themselves are deliberate and stay put: a request that
still names
gemini-3.6-flashroutes to 3.7 and needs a window under the id it asked for.Everything the user owns is unchanged.
models,selectedModels,retainModels,defaultModelanddisabledModelsare still renamed, and a user who has not started the proxysince the rename shipped still gets the repair and still sees it reported -- once.
Deliberately not done: no new store, no unconditional silencing of the log, no change to which
aliases are retired, and no change to
ANTIGRAVITY_MODEL_CONTEXT_WINDOWSitself (those windowsare pinned by
tests/adapters/google/gemini-37-flash-migration.test.tsfor a reason).Closes #5066
Verification
No local suite was run: this checkout is under a no-local-execution rule after an earlier local
run destroyed the operator's real
~/.opencodex. Correctness rests on static tracing plus hostedCI at the exact head.
src/server/index.ts(migration, then reconcile),src/providers/model-rename-migration.ts,src/oauth/index.ts(applyOAuthPresetCatalog,OAUTH_RECONCILE_FIELDS),src/providers/antigravity-models.ts,src/providers/derive.ts.modelContextWindowsis the only reconciled field the preset publishes keyed by a retired id,which is exactly the nine messages reported.
enrichProviderFromRegistryis all-or-nothing formodelContextWindowsand thatloadConfigdoes not enrich at all, so the writer between the two boots is reconciliation.qwen3.8-max-previewappears nowherein
src/outside the rename table, so [Bug] Registry model renames never reach existing key-auth provider configs (qwen3.8-max-preview persists) #1610 coverage is untouched.tests/providers/model-rename-migration.test.ts(basename alreadyregistered in
scripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json,so no layout change is needed):
OPENCODEX_HOME: save a synthetic Antigravity config, runmigrate-then-reconcile, assert reconciliation put the retired keys on disk, then assert the
second boot renames nothing and prints nothing. Red on
devat that last assertion.selectedModelsentry is still repaired and still reported once.providerConfigSeedmust never be something the migration wants to rewrite. Red ondevforall nine Antigravity renames.
test 4/4caught that it also skipped a rowsaving only the retired key, which is the case that must still be migrated. That is the second
condition above, and
gemini-37-flash-migration.test.ts:211is the regression test for it.tests/fixtures/file-size-baseline.json,and the test file is 418 lines against the 2000-line threshold for untracked files.
origin/devat 6044a9a.Checklist
line that stops repeating; nothing in
docs-site/describes it.)credential or workflow surface is touched; the change narrows which config fields a startup
migration rewrites.)
Summary by CodeRabbit