fix(catalog): fold dated model ids in both directions and at both widths - #3117
fix(catalog): fold dated model ids in both directions and at both widths#3117olddonkey wants to merge 1 commit into
Conversation
Fixes lidge-jun#3024. `mergeConfiguredModelsIntoLiveCatalog` keeps a configured id when live discovery returns the same deployment under a date-suffixed id. `isDatedVariantId` decided that, and it was wrong twice over: it accepted only an eight-digit `YYYYMMDD` suffix, and it folded only `configured = base` against `live = dated`. Both halves have to hold for the reported case. Alibaba Token Plan entitles an account to `deepseek-v4-pro-0813` while `GET /models` advertises just `deepseek-v4-pro`, so neither the four-digit suffix nor the reverse direction matched and the configured id was removed from the authoritative live catalog — silently, since `GET /api/providers` still reported `discovery: ok`. The model is callable both directly and through the proxy. The four-digit branch range-checks month and day so an ordinary numeric suffix — a `-4096` context size, a `-2025` year — cannot be read as a deployment date and aliased onto an unrelated live row's metadata. The eight-digit branch stays a bare digit run on purpose: tightening it to a real calendar date would change which ids fold today, which is not what this fixes. The fold still requires a live relative, so an id with no live counterpart drops as before; the end-to-end test pins that alongside the retained ones. One tradeoff worth stating. A provider that genuinely retires a dated id while keeping its base will now keep that id advertised, where dropping it was right. The fold cannot tell "unadvertised but callable" from "retired". The forward direction already carried the mirror of this risk, and the cost of today's behaviour is concrete and reported: an entitled, verified-callable model disappears with no signal on any API surface. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
✅ Deterministic PR hygiene checks passed. |
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. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe catalog now recognizes dated model variants with either ChangesDated variant catalog handling
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to This change preserves configured model identifiers when live discovery reports a supported dated or undated variant, preventing callable models from disappearing from the catalog. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes in src/codex/catalog/provider-fetch.ts implement the requirements in issue Full details: Out of Scope Changes checkExplanation The changes are limited to the dated-variant matching logic in src/codex/catalog/provider-fetch.ts and related regression tests in tests/codex-catalog.test.ts. These changes directly support issue
✨ Finishing Touches🧪 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 |
리뷰 · 우선순위 70 / 80설명 이 PR은 이슈 #3024를 지금 #3024가 말한 장면은 이렇습니다. Alibaba Token Plan(국제) 계정은 지금 HEAD의 이 PR이 하는 일은 그 함수만 고치는 것입니다. 테스트는 라인 src/codex/catalog/provider-fetch.ts:DATED_VARIANT_SUFFIX - 네 자리는 달/일을 검사하지만, 여덟 자리는 예전처럼 숫자만 봅니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
Closing as superseded by #3100, which landed on The underlying #3024 bug was real, but the merged replacement deliberately combines the calendar-aware suffix matcher with a directional merge rule. This PR's default bidirectional fold would let a live base row resurrect a configured dated snapshot with no evidence that the dated ID remains callable—the blocker already identified during #3041 review. #3100 retains configured base IDs when live discovery supplies a dated relative, without making the unsafe reverse inference. Thank you for the focused report and tests. No additional patch from this branch is needed now that the canonical fix is on |
3 participants
Fixes #3024.
Summary
mergeConfiguredModelsIntoLiveCatalogkeeps a configured id when live discovery returns the same deployment under a date-suffixed id.isDatedVariantIddecided that, and it was wrong in two independent ways:YYYYMMDDsuffix, andconfigured = baseagainstlive = dated.Both halves have to hold for the reported case. Alibaba Token Plan entitles the account to
deepseek-v4-pro-0813while upstreamGET /modelsadvertises justdeepseek-v4-pro, so neither the four-digit suffix nor the reverse direction matched and the configured id was removed from the authoritative live catalog — silently, sinceGET /api/providersstill reporteddiscovery: { "status": "ok" }. The reporter verified the model is callable both directly upstream and through the proxy.Two deliberate limits
The four-digit branch range-checks month and day. A numeric suffix is not automatically a date — a
-4096context size or a-2025year must not be read as a deployment date and aliased onto an unrelated live row's metadata. Covered by a test.The eight-digit branch stays a bare digit run. Tightening it to a real calendar date would change which ids fold today, which is not what this fixes.
The fold still requires a live relative, so an id with no live counterpart drops exactly as before.
Verification
Reverting
isDatedVariantIdto its previous body turns all four new cases red, including the end-to-end one:The end-to-end test reproduces the report's shape — upstream advertises
deepseek-v4-proanddeepseek-v4-flash-0731, config lists both plusdeepseek-v4-pro-0813and adeepseek-retired-9with no live relative — and asserts the first three survive while the last still drops.bun run test— 16628 pass, 14 skip, 0 fail, exit 0; all six serial lanes green.bun run typecheck— passed.bun run privacy:scan— passed.tests/codex-catalog.test.ts— 226 pass / 0 fail.One tradeoff worth stating
A provider that genuinely retires a dated id while keeping its base will now keep that id advertised, where dropping it was the right answer. The fold cannot distinguish "unadvertised but callable" from "retired".
The forward direction already carried the mirror image of this risk and was accepted. The cost of today's behaviour is concrete and reported: an entitled, verified-callable model disappears from the dashboard,
ocx models live, and the Codex model picker, with no signal on any API surface.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
YYYYMMDD) and short-date (MMDD) variant formats.