fix(catalog): fold dated model aliases in both directions and accept MMDD - #3041
Draft
ntdatt812 wants to merge 2 commits into
Draft
fix(catalog): fold dated model aliases in both directions and accept MMDD#3041ntdatt812 wants to merge 2 commits into
ntdatt812 wants to merge 2 commits into
Conversation
…MMDD The dated-alias fold keeps a configured model id in the catalog when live discovery returns the same model under a date-suffixed id. It missed two cases, and a model that is on the plan, configured, and callable was dropped from the authoritative catalog while discovery still reported ok. isDatedVariantId only accepted an 8-digit YYYYMMDD suffix, so providers that date with MMDD (deepseek-v4-pro-0813) never matched. Four digits also read as a version number, so the month and day now have to be real ones: 0813 is August 13th, 2025 has no twentieth month, which is what keeps the existing claude-haiku-4-5-2025 case a non-match. The fold also only ran configured=base -> live=dated. The reverse, where the account is configured with the dated id and discovery answers the base one, is the case reported here and was not handled at all. Refs lidge-jun#3024.
Contributor
|
Important Review skippedToo many files! This PR contains 1143 files, which is 843 over the limit of 300. To get a review, reduce the PR to 300 files or fewer by splitting it into smaller PRs or changing its base branch. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (20)
📒 Files selected for processing (1143)
You can disable this status message by setting the 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 |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Contributor
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
Owner
리뷰 · 우선순위 68 / 80이 PR은 이슈 #3024를 고친다. 지금 원인은 두 개다. 첫째, 이 PR은 그 두 구멍을 같이 막는다. 같은 이슈를 겨냥한 형제 PR #3034 가 이미 열려 있다. 같은 작성자(ntdatt812)다. #3034 는 접미사만 넓힌다. YYYYMMDD / YYMMDD / MMDD / YYMM 네 갈래다. 역방향 접기는 일부러 안 넣었다. 이 PR은 접미사는 YYYYMMDD+MMDD 만 받고, 대신 이슈 제목의 반대 방향 접기를 넣었다. 두 PR은 같은 함수와 같은 테스트 파일을 고친다. 둘 다 머지할 수 없다. 이슈 #3024 리뷰는 반대 방향 접기를 이 구멍의 수리라고 했고, #1690 과 섞지 말라고 했다. #3034 리뷰는 반대 방향을 #1690 으로 넘기라고 했다. 이 PR은 이슈 리뷰 쪽이다. 테스트는 라인 938 (src/codex/catalog/provider-fetch.ts, 현재 HEAD) - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
requested changes
Aug 31, 2026
Ingwannu
left a comment
Ingwannu
left a comment
Owner
There was a problem hiding this comment.
Requesting changes on exact head 9340189. The MMDD format gap is real, but the reverse fold is not a safe default inference. A live base row proves that the base id is callable; it does not prove that a configured dated snapshot is still callable. Retaining configured=dated when live=base can therefore resurrect a retired or plan-removed snapshot solely because its name looks related. The reporter manually proved one specific id, but the catalog merge has no equivalent callability signal. Keep the fold directional and widen the accepted suffix formats, or require the explicit retainModels operator opt-in for the reverse case. Also pin the unavoidable 1024 MMDD collision and keep the broader observed YYMMDD and YYMM formats if this supersedes #3034. Finally, this branch is not based on the current dev history: its merge base is c0c9544 and current dev is a8c3a96, so rebase before any runtime CI or Ready transition. Please do not close #3024 as fully fixed unless the reverse case is resolved through an explicit, non-inferred retention contract.
…the -1024 cost The review is right that `isDatedVariantId matches only <alias>-YYYYMMDD` stopped being true once MMDD was accepted. Renamed, and the MMDD case it now covers is asserted in it rather than only elsewhere. Also pins the cost the review asked for: a four-digit suffix that is a valid month and day is read as one, so `model-1024` folds into `model`. Suffixes that cannot be a date -- 2048, 4096, 8192, 0000 -- stay separate, which is what keeps most version and size suffixes safe.
ntdatt812
marked this pull request as ready for review
August 31, 2026 09:44
ntdatt812
marked this pull request as ready for review
August 31, 2026 10:06
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3024.
The dated-alias fold exists to keep a configured model id in the catalog when live discovery returns the same model under a date-suffixed id. It missed two independent cases, and the result was that a model on the account's plan, present in
providers.<name>.models, and verifiably callable was dropped from the authoritative catalog — withGET /api/providersstill reportingdiscovery: { "status": "ok" }, so nothing on the API surface said a model had gone.1. Only
YYYYMMDDmatchedProviders on the Alibaba Token Plan and DeepSeek date with
MMDD(deepseek-v4-pro-0813), so they never folded.Four digits are also how a version reads, and the existing test pins that:
claude-haiku-4-5-2025must not be a dated variant. So accepting any four digits was not an option.isDateSuffixrequires a real month and day instead —0813is August 13th,2025has no twentieth month. That keeps the existing case a non-match on its own merits rather than by accident of digit count.2. Only
configured = base→live = datedfoldedThe reverse — the account configured with
deepseek-v4-pro-0813while discovery answersdeepseek-v4-pro— is the case in the report, and was not handled at all. The call site now tries both directions.isDatedVariantIditself stays directional, so its meaning and its existing tests are unchanged.Tests
Added:
isDatedVariantId accepts an MMDD suffix that is a real date— the two ids from the report plusmodel-1231isDatedVariantId still rejects four digits that are not a date—2025(no month 20),0001(no month 00),1300,1240keeps a configured dated id when discovery answers the base id— the reverse fold, end to end throughmergeConfiguredModelsIntoLiveCatalogMutation-checked. Dropping the reverse direction from the call site:
The existing
isDatedVariantId matches only <alias>-YYYYMMDDtest is untouched and still passes, including its-2025and-latestnegatives.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.
Review follow-up (2026-08-31)
Both requests from the review are in:
isDatedVariantId matches only <alias>-YYYYMMDDwas no longer true. Renamed tomatches <alias>-YYYYMMDD and <alias>-MMDD, nothing else, and the MMDD case it now covers is asserted inside it.-1024cost is pinned. A four-digit suffix that is a valid month and day is read as one, somodel-1024folds intomodel. The new test states that plainly and also pins what stays safe:2048,4096,8192and0000cannot be a date, so they remain separate models. Nothing in an id says which reading was intended, and rejecting MMDD outright is the bug this fold exists to fix.bun test tests/codex-catalog.test.ts→ 194 pass, 0 fail. Branch base is level withdev.One correction to the review: #3034 is by @kaicot, not me. I have no claim on it, so it is not mine to close as superseded — that call is yours and theirs. On the substance I agree the two overlap and only one can land; if you prefer their suffix coverage (YYMMDD/YYMM) over the reverse-direction fold, I am happy for this one to be the closed half instead.
YYMMDD/YYMM are still not accepted here, so
deepseek-v4-pro-250813remains a non-match. Left as follow-up as the review suggested.