Repository navigation
fix(model-aliases): back custom-alias store with globalThis (#5777 follow-up) - #5821
Conversation
…llow-up) #5777 self-healed the GET /api/settings/model-aliases symptom at the route layer, but the root cause remained: modelDeprecation.ts held _customAliases in a plain module-level let, which webpack duplicates across the startup and app-route module graphs (same class as #5312). Startup hydration landed on one copy; the API route read the other (empty) one. Back the store with globalThis (__omniroute_customAliases__) so both instances share one store — the exact pattern already used by thinkingBudget.ts/backgroundTaskDetector.ts (#5312). The route-layer DB self-heal from #5777 stays as a harmless fallback. Extends #5777 (thanks @jleonar2). Regression: tests/unit/model-aliases-globalthis-5777.test.ts (fails on the plain-let store: never populates globalThis, never reads a sibling instance's write).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request migrates the custom model aliases store from a module-level variable to a globalThis-backed store to prevent state duplication across separate Webpack module graphs, and adds corresponding unit tests. The review feedback recommends improving type safety and security by declaring the global variable properly and using Object.create(null) to avoid prototype pollution, alongside refactoring the tests to leverage these type-safe globals.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const CUSTOM_ALIASES_GLOBAL_KEY = "__omniroute_customAliases__"; | ||
| const _aliasStore = globalThis as unknown as Record< | ||
| string, | ||
| Record<string, string> | undefined | ||
| >; | ||
|
|
||
| function customAliases(): Record<string, string> { | ||
| if (!_aliasStore[CUSTOM_ALIASES_GLOBAL_KEY]) { | ||
| _aliasStore[CUSTOM_ALIASES_GLOBAL_KEY] = {}; | ||
| } | ||
| return _aliasStore[CUSTOM_ALIASES_GLOBAL_KEY]!; | ||
| } | ||
|
|
||
| /** | ||
| * Set custom aliases (called from settings API or startup). | ||
| */ | ||
| export function setCustomAliases(aliases: Record<string, string>): void { | ||
| _customAliases = { ...aliases }; | ||
| _aliasStore[CUSTOM_ALIASES_GLOBAL_KEY] = { ...aliases }; | ||
| } |
There was a problem hiding this comment.
Type Safety & Security Improvement: Use declare global and Object.create(null)
- Type Safety: Casting
globalThistounknownand then to a genericRecordis a bit of a code smell and bypasses TypeScript's type system. Instead, we can usedeclare global { var ... }which is the idiomatic TypeScript way to declare global variables onglobalThis. - Prototype Pollution & Lookup Protection: Using a plain object
{}for custom aliases makes it vulnerable to prototype lookup issues (e.g., if a model ID is"toString"or"hasOwnProperty", it will resolve to the prototype function instead ofundefined). By initializing the store withObject.create(null), we create a prototype-less dictionary that is completely immune to prototype lookup bugs and prototype pollution.
| const CUSTOM_ALIASES_GLOBAL_KEY = "__omniroute_customAliases__"; | |
| const _aliasStore = globalThis as unknown as Record< | |
| string, | |
| Record<string, string> | undefined | |
| >; | |
| function customAliases(): Record<string, string> { | |
| if (!_aliasStore[CUSTOM_ALIASES_GLOBAL_KEY]) { | |
| _aliasStore[CUSTOM_ALIASES_GLOBAL_KEY] = {}; | |
| } | |
| return _aliasStore[CUSTOM_ALIASES_GLOBAL_KEY]!; | |
| } | |
| /** | |
| * Set custom aliases (called from settings API or startup). | |
| */ | |
| export function setCustomAliases(aliases: Record<string, string>): void { | |
| _customAliases = { ...aliases }; | |
| _aliasStore[CUSTOM_ALIASES_GLOBAL_KEY] = { ...aliases }; | |
| } | |
| declare global { | |
| var __omniroute_customAliases__: Record<string, string> | undefined; | |
| } | |
| function customAliases(): Record<string, string> { | |
| if (!globalThis.__omniroute_customAliases__) { | |
| globalThis.__omniroute_customAliases__ = Object.create(null) as Record<string, string>; | |
| } | |
| return globalThis.__omniroute_customAliases__; | |
| } | |
| /** | |
| * Set custom aliases (called from settings API or startup). | |
| */ | |
| export function setCustomAliases(aliases: Record<string, string>): void { | |
| const store = Object.create(null) as Record<string, string>; | |
| Object.assign(store, aliases); | |
| globalThis.__omniroute_customAliases__ = store; | |
| } |
| const GLOBAL_KEY = "__omniroute_customAliases__"; | ||
| const g = globalThis as unknown as Record<string, Record<string, string> | undefined>; | ||
|
|
||
| const modelDeprecation = await import("../../open-sse/services/modelDeprecation.ts"); | ||
|
|
||
| test.beforeEach(() => { | ||
| delete g[GLOBAL_KEY]; | ||
| }); | ||
|
|
||
| test.after(() => { | ||
| delete g[GLOBAL_KEY]; | ||
| }); | ||
|
|
||
| test("#5777: setCustomAliases writes through globalThis (store is not a per-module let)", () => { | ||
| modelDeprecation.setCustomAliases({ "old-model": "new-model" }); | ||
|
|
||
| // A plain module-level `let` would never populate globalThis; the globalThis-backed | ||
| // store does. This is what lets a second webpack module instance see the write. | ||
| assert.deepEqual( | ||
| g[GLOBAL_KEY], | ||
| { "old-model": "new-model" }, | ||
| "custom aliases must live on globalThis so both webpack module graphs share them" | ||
| ); | ||
| }); | ||
|
|
||
| test("#5777: reads reflect a value written by another module instance via globalThis", () => { | ||
| // Simulate the OTHER webpack module instance (the startup/instrumentation graph) | ||
| // hydrating the shared store — this module instance did NOT call setCustomAliases. | ||
| g[GLOBAL_KEY] = { "claude-opus-4-8": "mimo/mimo-v2.5-pro" }; | ||
|
|
||
| // With the plain-`let` store this module's own copy would still be empty (the #5777 | ||
| // bug). With the globalThis backing the read reflects the other instance's write. | ||
| assert.deepEqual(modelDeprecation.getCustomAliases(), { | ||
| "claude-opus-4-8": "mimo/mimo-v2.5-pro", | ||
| }); | ||
| assert.equal(modelDeprecation.resolveModelAlias("claude-opus-4-8"), "mimo/mimo-v2.5-pro"); | ||
| assert.equal( | ||
| modelDeprecation.getAllAliases()["claude-opus-4-8"], | ||
| "mimo/mimo-v2.5-pro", | ||
| "getAllAliases must merge the globalThis-backed custom aliases over built-ins" | ||
| ); | ||
| }); | ||
|
|
||
| test("#5777: addCustomAlias / removeCustomAlias mutate the shared globalThis store", () => { | ||
| modelDeprecation.setCustomAliases({}); | ||
| modelDeprecation.addCustomAlias("foo", "bar"); | ||
| assert.equal(g[GLOBAL_KEY]?.foo, "bar", "addCustomAlias must mutate the globalThis store"); | ||
| assert.equal(modelDeprecation.resolveModelAlias("foo"), "bar"); | ||
|
|
||
| assert.equal(modelDeprecation.removeCustomAlias("foo"), true); | ||
| assert.equal(g[GLOBAL_KEY]?.foo, undefined, "removeCustomAlias must clear it from globalThis"); | ||
| assert.equal(modelDeprecation.removeCustomAlias("missing"), false); | ||
| }); |
There was a problem hiding this comment.
Refactor: Use Static Import and Direct Global Access
Now that __omniroute_customAliases__ is declared globally in modelDeprecation.ts, we can statically import the module and access globalThis.__omniroute_customAliases__ directly with full type safety. This eliminates the need for dynamic await import and unsafe type casting (as unknown as Record<...>) in the test file.
import * as modelDeprecation from "../../open-sse/services/modelDeprecation.ts";
test.beforeEach(() => {
delete globalThis.__omniroute_customAliases__;
});
test.after(() => {
delete globalThis.__omniroute_customAliases__;
});
test("#5777: setCustomAliases writes through globalThis (store is not a per-module let)", () => {
modelDeprecation.setCustomAliases({ "old-model": "new-model" });
// A plain module-level `let` would never populate globalThis; the globalThis-backed
// store does. This is what lets a second webpack module instance see the write.
assert.deepEqual(
globalThis.__omniroute_customAliases__,
{ "old-model": "new-model" },
"custom aliases must live on globalThis so both webpack module graphs share them"
);
});
test("#5777: reads reflect a value written by another module instance via globalThis", () => {
// Simulate the OTHER webpack module instance (the startup/instrumentation graph)
// hydrating the shared store — this module instance did NOT call setCustomAliases.
globalThis.__omniroute_customAliases__ = { "claude-opus-4-8": "mimo/mimo-v2.5-pro" };
// With the plain-`let` store this module's own copy would still be empty (the #5777
// bug). With the globalThis backing the read reflects the other instance's write.
assert.deepEqual(modelDeprecation.getCustomAliases(), {
"claude-opus-4-8": "mimo/mimo-v2.5-pro",
});
assert.equal(modelDeprecation.resolveModelAlias("claude-opus-4-8"), "mimo/mimo-v2.5-pro");
assert.equal(
modelDeprecation.getAllAliases()["claude-opus-4-8"],
"mimo/mimo-v2.5-pro",
"getAllAliases must merge the globalThis-backed custom aliases over built-ins"
);
});
test("#5777: addCustomAlias / removeCustomAlias mutate the shared globalThis store", () => {
modelDeprecation.setCustomAliases({});
modelDeprecation.addCustomAlias("foo", "bar");
assert.equal(globalThis.__omniroute_customAliases__?.foo, "bar", "addCustomAlias must mutate the globalThis store");
assert.equal(modelDeprecation.resolveModelAlias("foo"), "bar");
assert.equal(modelDeprecation.removeCustomAlias("foo"), true);
assert.equal(globalThis.__omniroute_customAliases__?.foo, undefined, "removeCustomAlias must clear it from globalThis");
assert.equal(modelDeprecation.removeCustomAlias("missing"), false);
});…inference#5777 follow-up) (cheaper-inference#5821) cheaper-inference#5777 self-healed the GET /api/settings/model-aliases symptom at the route layer, but the root cause remained: modelDeprecation.ts held _customAliases in a plain module-level let, which webpack duplicates across the startup and app-route module graphs (same class as cheaper-inference#5312). Startup hydration landed on one copy; the API route read the other (empty) one. Back the store with globalThis (__omniroute_customAliases__) so both instances share one store — the exact pattern already used by thinkingBudget.ts/backgroundTaskDetector.ts (cheaper-inference#5312). The route-layer DB self-heal from cheaper-inference#5777 stays as a harmless fallback. Extends cheaper-inference#5777 (thanks @jleonar2). Regression: tests/unit/model-aliases-globalthis-5777.test.ts (fails on the plain-let store: never populates globalThis, never reads a sibling instance's write).
Follow-up to #5777 (thanks @jleonar2). #5777 self-healed the
GET /api/settings/model-aliasessymptom at the route layer; this fixes the root cause.Root cause
open-sse/services/modelDeprecation.tsheld_customAliasesin a plain module-levellet, which webpack duplicates across the startup (instrumentation.ts) and app-route module graphs (same class as #5312). Startup hydration landed on one copy; the API route read the other, empty one.Fix
Back the store with
globalThis(__omniroute_customAliases__) so both module instances share one store — the exact pattern already applied tothinkingBudget.ts/backgroundTaskDetector.ts(#5312). The route-layer DB self-heal from #5777 stays as a harmless fallback, and the false-empty ambiguity is moot (the DB is a singleton, so a cleared state reads{}).Regression (TDD, RED→GREEN)
tests/unit/model-aliases-globalthis-5777.test.ts— 3 cases proven failing on the plain-letstore (never populates globalThis; never reads a sibling instance's write) and passing with the fix. #5777'smodel-aliases-settings-route-selfheal.test.tsstays green (fallback intact). 6/6.