fix(catalog): single-flight gather, prewarm, and readable slash aliases - #738
Conversation
Share one live discovery per provider-set key (Map), prewarm after bind, and encode OpenRouter-style model ids with ~ so Claude Available models stay readable.
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughClaude aliases now encode slash-containing model IDs with ChangesAlias encoding and discovery
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant handleStart
participant scheduleCatalogPrewarm
participant gatherRoutedModels
participant gatherInflight
participant ProviderDiscovery
handleStart->>scheduleCatalogPrewarm: schedule after successful server bind
scheduleCatalogPrewarm->>gatherRoutedModels: load config and gather catalog
gatherRoutedModels->>gatherInflight: create or reuse configuration flight
gatherInflight->>ProviderDiscovery: fetch routed provider models
ProviderDiscovery-->>gatherInflight: return models and combo omissions
gatherInflight-->>gatherRoutedModels: provide shared result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/claude/alias.ts`:
- Around line 12-15: The tilde decoder in alias resolution breaks previously
issued aliases such as those handled by resolveInboundModel. Introduce a
versioned alias prefix/encoding or explicit migration path so legacy
literal-tilde aliases remain unchanged while new slash encoding decodes
correctly; update resolveAlias and related alias generation accordingly, and add
a regression test covering legacy aliases.
In `@src/codex/catalog/provider-fetch.ts`:
- Around line 68-75: The gatherFlightKey-based single-flight key omits
configuration inputs that affect the catalog, allowing distinct snapshots to
share incorrect results. Refactor the catalog flow to single-flight only
provider `/models` fetches and aggregate each caller’s combos, custom models,
adapters, context caps, and credential/account-dependent discovery separately,
or expand the key with a complete non-secret fingerprint; add a focused
regression near the existing catalog tests using identical provider identity
with differing context caps or custom models.
- Around line 604-609: Update the promise/flight flow around the resolved models
so each gather returns both models and its own combo omissions, rather than
reading getLastComboCatalogOmissions() after awaiting. Destructure the resolved
`{ models, omissions }` and copy that flight-local omissions list into every
caller’s options.comboOmissions, while preserving concurrent per-key behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c2902bb3-0fe4-4339-b017-58a34765f076
📒 Files selected for processing (7)
src/claude/alias.tssrc/cli/index.tssrc/codex/catalog.tssrc/codex/catalog/provider-fetch.tssrc/codex/catalog/sync.tstests/claude-alias.test.tstests/gather-routed-models-single-flight.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 818fcd867d
ℹ️ 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".
| if (sep <= 0) return null; | ||
| const provider = rest.slice(0, sep); | ||
| const model = rest.slice(sep + 2); | ||
| const model = decodeModelId(rest.slice(sep + 2)); |
There was a problem hiding this comment.
Existing releases allowed routed model IDs containing ~, so an alias such as claude-ocx-demo--old~model may already be persisted in Claude Code settings and previously resolved to demo/old~model. Unconditionally decoding every tilde now changes that saved selection to demo/old/model, potentially routing requests to a different or nonexistent model. Use a versioned or unambiguous encoding while retaining legacy tilde decoding semantics.
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
| .map(([name, prov]) => `${name}\0${prov.liveModels === false ? "0" : "1"}\0${prov.baseUrl ?? ""}`) | ||
| .sort() | ||
| .join("\n"); | ||
| return `${providers}\n#${config.modelCacheTtlMs ?? DEFAULT_MODEL_CACHE_TTL_MS}`; |
There was a problem hiding this comment.
When concurrent callers have the same provider name, base URL, live-model flag, and TTL but differ in fields such as providers[*].models, catalog hints, customModels, or combos, the second caller joins the first promise and receives results derived entirely from the first configuration. This can occur when the startup prewarm overlaps a dashboard config update, causing that update's refresh to write a catalog without the newly added models or combos. Either single-flight only the upstream fetch portion and derive each caller's catalog separately, or key every input that affects the returned models.
Useful? React with 👍 / 👎.
| const models = await promise; | ||
| if (options?.comboOmissions) { | ||
| const last = getLastComboCatalogOmissions(); | ||
| options.comboOmissions.length = 0; | ||
| options.comboOmissions.push(...last); |
There was a problem hiding this comment.
When two distinct flight keys finish concurrently, each uncached gather replaces the process-global omission list, and an awaiting caller then reads whichever flight updated that global last rather than the omissions belonging to its own promise. A catalog sync can consequently report another configuration's combo omissions—or suppress its own—even though its model array came from the correct flight. Store models and omissions together in the per-key promise and copy that flight-local result to each joiner.
Useful? React with 👍 / 👎.
| if (!modelId) return null; | ||
| const encoded = encodeModelId(modelId); | ||
| if (encoded === null) return null; | ||
| return `${CLAUDE_ALIAS_PREFIX}${provider}--${encoded}`; |
There was a problem hiding this comment.
The new encoding makes slash-containing model IDs readable, but docs-site/src/content/docs/guides/claude-code.md:194-196 still says models containing / cannot use the readable form and fall back to a hash; the Korean, Chinese, Russian, and Japanese pages repeat that claim. Users troubleshooting OpenRouter selections will therefore be told to expect a different ID family than the proxy now emits. Update the English alias grammar with the ~ encoding and keep each translation consistent.
AGENTS.md reference: AGENTS.md:L167-L168
Useful? React with 👍 / 👎.
| void import("../codex/catalog").then(({ gatherRoutedModels }) => { | ||
| gatherRoutedModels(loadConfig()).catch(() => {}); | ||
| }); |
There was a problem hiding this comment.
The new production behavior is wired only inside handleStart, while the added tests call gatherRoutedModels directly and never verify that a successful bind triggers exactly one background prewarm or that the first /v1/models/sync caller joins it. A refactor or retry-path change can therefore silently remove or duplicate the startup request while all new tests remain green. Add a focused startup-level regression with injectable bind/gather dependencies that asserts ordering and single-flight joining.
AGENTS.md reference: AGENTS.md:L164-L166
Useful? React with 👍 / 👎.
Encode / as ~s and literal ~ as ~t so legacy bare-~ aliases keep resolving. Hash catalog-affecting config into the gather flight key and return flight-local combo omissions so concurrent keys cannot cross-contaminate.
|
@coderabbitai review |
|
✅ Action performedReview finished.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/claude/alias.ts`:
- Around line 40-60: Introduce a versioned alias format or explicit migration
path so legacy aliases containing literal "~s" and "~t" remain unchanged while
new aliases decode escapes through decodeModelId. Update inbound resolution in
src/claude/alias.ts lines 80-88 to select the correct compatibility path,
document the complete policy in src/claude/alias.ts lines 12-18, and add legacy
"~s" and "~t" round-trip assertions in tests/claude-alias.test.ts lines 53-57.
In `@src/codex/catalog/provider-fetch.ts`:
- Around line 82-123: Update providerCatalogFingerprint() to include each
provider’s selectedModels, and update gatherFlightKey() to include
config.disabledModels in the stable fingerprint input. Preserve the existing
sorting and hashing behavior so configurations differing in either visibility
filter produce distinct flight keys.
In `@tests/claude-alias.test.ts`:
- Around line 53-57: Add focused regression assertions in the test covering
aliasForRoute and resolveAlias for legacy model aliases containing ~s and ~t,
including demo--old~smodel and demo--old~tmodel. Verify both aliases resolve to
demo/old~smodel and demo/old~tmodel respectively, while preserving the existing
encoded-alias expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 92896110-0839-45f6-93b3-653bba93771e
📒 Files selected for processing (12)
docs-site/src/content/docs/guides/claude-code.mddocs-site/src/content/docs/ja/guides/claude-code.mddocs-site/src/content/docs/ko/guides/claude-code.mddocs-site/src/content/docs/ru/guides/claude-code.mddocs-site/src/content/docs/zh-cn/guides/claude-code.mdsrc/claude/alias.tssrc/cli/catalog-prewarm.tssrc/cli/index.tssrc/codex/catalog/provider-fetch.tstests/claude-alias.test.tstests/cli-catalog-prewarm.test.tstests/gather-routed-models-single-flight.test.ts
| test("literal '~' in model ids encodes as '~t' and legacy bare '~' still resolves", () => { | ||
| expect(aliasForRoute("demo", "old~model")).toBe(`${CLAUDE_ALIAS_PREFIX}demo--old~tmodel`); | ||
| expect(resolveAlias(`${CLAUDE_ALIAS_PREFIX}demo--old~tmodel`)).toBe("demo/old~model"); | ||
| // Pre-slash-encoding aliases kept literal tildes in the model portion. | ||
| expect(resolveAlias(`${CLAUDE_ALIAS_PREFIX}demo--old~model`)).toBe("demo/old~model"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add regression cases for legacy ~s and ~t.
This test only covers an unambiguous bare tilde. Add aliases such as demo--old~smodel and demo--old~tmodel; the current decoder incorrectly interprets them as new escapes. As per path instructions, shared behavior changes require focused regression coverage near the affected subsystem.
Proposed test additions
expect(resolveAlias(`${CLAUDE_ALIAS_PREFIX}demo--old~model`)).toBe("demo/old~model");
+ expect(resolveAlias(`${CLAUDE_ALIAS_PREFIX}demo--old~smodel`)).toBe("demo/old~smodel");
+ expect(resolveAlias(`${CLAUDE_ALIAS_PREFIX}demo--old~tmodel`)).toBe("demo/old~tmodel");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/claude-alias.test.ts` around lines 53 - 57, Add focused regression
assertions in the test covering aliasForRoute and resolveAlias for legacy model
aliases containing ~s and ~t, including demo--old~smodel and demo--old~tmodel.
Verify both aliases resolve to demo/old~smodel and demo/old~tmodel respectively,
while preserving the existing encoded-alias expectations.
Source: Path instructions
|
@coderabbitai review |
✅ Action performedReview finished.
|
Merge-ready summaryReview follow-ups from CodeRabbit/Codex that were necessary or useful are addressed. Remaining bot notes are either already fixed on tip, intentionally deferred (e.g. versioned alias family for pathological Merge order (bottom-up): #738 → #739 → #740 → #741. After each merge, retarget the next PR to Codex cloud review is currently usage-capped on this account; local/CI verification was used instead for later rounds. This PR (#738)
Merge-ready. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/claude-alias.test.ts (1)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing regression test for
aliasForNativetilde encoding.This diff covers tilde encoding for
aliasForRoute/claudeCodeAlias(Lines 53-72, 138) but there's no assertion foraliasForNativewith a slug containing~, even though the line-range summary statesaliasForNativewas changed to encode literal~(via the same~tscheme) so it's representable. Add a case near the existing native tests (Lines 25-29) such as:🧪 Suggested addition
test("native slugs use the pseudo-provider and resolve to bare ids", () => { const alias = aliasForNative("gpt-5.5"); expect(alias).toBe(`${CLAUDE_ALIAS_PREFIX}native--gpt-5.5`); expect(resolveAlias(alias!)).toBe("gpt-5.5"); + const tildeAlias = aliasForNative("gpt-5.5~beta"); + expect(tildeAlias).toBe(`${CLAUDE_ALIAS_PREFIX}native--gpt-5.5~tbeta`); + expect(resolveAlias(tildeAlias!)).toBe("gpt-5.5~beta"); });As per path instructions, "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
Also applies to: 117-139
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/claude-alias.test.ts` around lines 25 - 29, Add a focused regression assertion near the existing native-slug test for aliasForNative using a slug containing a literal “~”. Verify the generated alias applies the established “~t” encoding and that resolveAlias converts it back to the original slug, preserving the existing native alias behavior.Source: Path instructions
src/claude/alias.ts (1)
12-22: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftLegacy
~s/~tcollision is now documented, not fixed — confirm this is the accepted final decision.The docstring now explicitly states that historical model IDs literally containing the two-char sequences
~s/~tare not preserved on decode, andresolveAlias(Line 90, viadecodeModelId) unconditionally treats~s→/and~t→~. This is exactly the compatibility gap flagged in the earlier review round, which asked for a versioned alias prefix or explicit migration strategy before enabling the decoder. The team's response here is to document the behavior and add a test (tests/claude-alias.test.tsLines 60-72) that asserts the breaking decode rather than to preserve it.Given
resolveInboundModelcallsresolveAliasand returns immediately on a hit, any live picker alias that happened to contain a literal~s/~ttwo-char run pre-dating this change will now silently resolve to the wrong provider/model. If this trade-off (rare occurrence, acceptable breakage) is intentional and signed off, this is fine as documented; if not, the original ask for a versioned encoding still stands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/claude/alias.ts` around lines 12 - 22, Prevent legacy alias collisions by adding a versioned encoding marker and making decodeModelId apply ~s/~t decoding only to aliases carrying that marker. Update the alias encoding and resolveAlias flow to preserve unversioned historical model IDs literally, and revise tests to cover both versioned aliases and legacy collision cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/gather-routed-models-single-flight.test.ts`:
- Around line 246-339: Reduce the duplicated gate-promise and mocked-fetch setup
in the selectedModels and disabledModels tests by extracting a small local
helper, such as makeGatedFetch, that returns the fetch-count state and release
control. Update both tests to use the helper while preserving their independent
fetch-count assertions and existing configuration-specific behavior.
---
Outside diff comments:
In `@src/claude/alias.ts`:
- Around line 12-22: Prevent legacy alias collisions by adding a versioned
encoding marker and making decodeModelId apply ~s/~t decoding only to aliases
carrying that marker. Update the alias encoding and resolveAlias flow to
preserve unversioned historical model IDs literally, and revise tests to cover
both versioned aliases and legacy collision cases.
In `@tests/claude-alias.test.ts`:
- Around line 25-29: Add a focused regression assertion near the existing
native-slug test for aliasForNative using a slug containing a literal “~”.
Verify the generated alias applies the established “~t” encoding and that
resolveAlias converts it back to the original slug, preserving the existing
native alias behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 93915070-a7eb-4591-a4c4-b50c794ba7b9
📒 Files selected for processing (4)
src/claude/alias.tssrc/codex/catalog/provider-fetch.tstests/claude-alias.test.tstests/gather-routed-models-single-flight.test.ts
| test("selectedModels-only config changes do not share a flight", async () => { | ||
| let fetchCount = 0; | ||
| let release!: () => void; | ||
| const gate = new Promise<void>(resolve => { | ||
| release = resolve; | ||
| }); | ||
|
|
||
| globalThis.fetch = (async () => { | ||
| fetchCount += 1; | ||
| await gate; | ||
| return new Response( | ||
| JSON.stringify({ data: [{ id: "keep-me" }, { id: "drop-me" }] }), | ||
| { status: 200, headers: { "content-type": "application/json" } }, | ||
| ); | ||
| }) as typeof fetch; | ||
|
|
||
| const base: OcxConfig = { | ||
| port: 10100, | ||
| defaultProvider: "p", | ||
| providers: { | ||
| p: { | ||
| adapter: "openai-chat", | ||
| baseUrl: "https://sel.example.test/v1", | ||
| models: [], | ||
| }, | ||
| }, | ||
| }; | ||
| const withSel: OcxConfig = { | ||
| ...base, | ||
| providers: { | ||
| p: { | ||
| ...base.providers.p, | ||
| selectedModels: ["keep-me"], | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| const first = gatherRoutedModels(base); | ||
| const second = gatherRoutedModels(withSel); | ||
| await Promise.resolve(); | ||
| // Distinct flight keys => two live discoveries (cannot reuse unfiltered result). | ||
| expect(fetchCount).toBe(2); | ||
| release(); | ||
| const [all, withSelModels] = await Promise.all([first, second]); | ||
| // gather itself is unfiltered; visibility is applied by callers. | ||
| expect(all.map(m => m.id).sort()).toEqual(["drop-me", "keep-me"]); | ||
| expect(withSelModels.map(m => m.id).sort()).toEqual(["drop-me", "keep-me"]); | ||
| expect(filterCatalogVisibleModels(all, base).map(m => m.id).sort()).toEqual(["drop-me", "keep-me"]); | ||
| expect(filterCatalogVisibleModels(withSelModels, withSel).map(m => m.id)).toEqual(["keep-me"]); | ||
| }); | ||
|
|
||
| test("disabledModels-only config changes do not share a flight", async () => { | ||
| let fetchCount = 0; | ||
| let release!: () => void; | ||
| const gate = new Promise<void>(resolve => { | ||
| release = resolve; | ||
| }); | ||
|
|
||
| globalThis.fetch = (async () => { | ||
| fetchCount += 1; | ||
| await gate; | ||
| return new Response( | ||
| JSON.stringify({ data: [{ id: "keep-me" }, { id: "drop-me" }] }), | ||
| { status: 200, headers: { "content-type": "application/json" } }, | ||
| ); | ||
| }) as typeof fetch; | ||
|
|
||
| const base: OcxConfig = { | ||
| port: 10100, | ||
| defaultProvider: "p", | ||
| providers: { | ||
| p: { | ||
| adapter: "openai-chat", | ||
| baseUrl: "https://dis.example.test/v1", | ||
| models: [], | ||
| }, | ||
| }, | ||
| }; | ||
| const withDisabled: OcxConfig = { | ||
| ...base, | ||
| disabledModels: ["p/drop-me"], | ||
| }; | ||
|
|
||
| const first = gatherRoutedModels(base); | ||
| const second = gatherRoutedModels(withDisabled); | ||
| await Promise.resolve(); | ||
| expect(fetchCount).toBe(2); | ||
| release(); | ||
| const [all, withDisabledModels] = await Promise.all([first, second]); | ||
| expect(all.map(m => m.id).sort()).toEqual(["drop-me", "keep-me"]); | ||
| expect(withDisabledModels.map(m => m.id).sort()).toEqual(["drop-me", "keep-me"]); | ||
| expect(filterCatalogVisibleModels(all, base).map(m => m.id).sort()).toEqual(["drop-me", "keep-me"]); | ||
| expect(filterCatalogVisibleModels(withDisabledModels, withDisabled).map(m => m.id)).toEqual(["keep-me"]); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Good regression coverage for the two new fingerprint fields.
Both tests correctly isolate selectedModels and disabledModels respectively and assert 2 separate upstream fetches, confirming the fingerprint changes in provider-fetch.ts actually prevent cross-config sharing. As per path instructions, this is exactly the "focused regression test near the existing tests for that subsystem" expected for a shared-routing behavior change.
Minor nitpick: the gate-promise + mock-fetch + base-config scaffolding (Lines 247-272 and 298-323) is duplicated almost verbatim between the two tests; a small local helper (e.g. makeGatedFetch()) could reduce repetition if more isolation-field tests are added later.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/gather-routed-models-single-flight.test.ts` around lines 246 - 339,
Reduce the duplicated gate-promise and mocked-fetch setup in the selectedModels
and disabledModels tests by extracting a small local helper, such as
makeGatedFetch, that returns the fetch-count state and release control. Update
both tests to use the helper while preserving their independent fetch-count
assertions and existing configuration-specific behavior.
Source: Path instructions
Summary
gatherRoutedModelsin-flight by provider-set so concurrent gathers with different keys cannot evict each other/model ids as~in Claude discovery aliases (readable chips, not desktop-3p hashes)Stack
dev) — fix(catalog): single-flight gather, prewarm, and readable slash aliases #738Merge bottom-up (738 → 739 → 740 → 741). After each merge, retarget the next PR to
devif GitHub does not auto-retarget.Test plan
bun test tests/claude-alias.test.ts tests/gather-routed-models-single-flight.test.tsGET /v1/modelsafter listen joins prewarm (no duplicate provider sweep)claude-ocx-…~…aliasesSummary by CodeRabbit
/, with stable, reversible encoding and correct alias resolution (including safe handling of~).~behavior and correct--splitting/resolution fallback behavior.