fix(catalog): widen the dated-variant fold and guard its direction at the merge loop - #3100
Conversation
`isDatedVariantId` only accepted an 8-digit `YYYYMMDD` suffix, so the
dated-alias fold in `mergeConfiguredModelsIntoLiveCatalog` never fired for
providers that publish shorter release dates. On a real multi-provider
install the `\d{8}` rule matched none of the 26 numeric-suffixed ids
present, dropping DeepSeek, Kimi, Mistral, Qwen and Solar aliases into
`droppedConfiguredIds` even though a live row for the same model existed.
Widen the suffix to the formats upstreams actually publish -- `YYYYMMDD`,
`YYMMDD`, `MMDD` and `YYMM` -- behind a calendar guard so ordinary numeric
suffixes are not read as dates. `-2048`, `-4096` and `-8192` are rejected;
`YY` is `2\d` rather than `\d\d` so `1301` is rejected too. `-1024` is a
valid `MMDD` and is therefore accepted -- an irreducible collision, pinned
by a test so it stays a known cost.
The fold stays one-directional (`configured=base` -> `live=dated`). A
configured id the provider no longer lists must not be retained on the
strength of a format match alone; #1690 is the explicit opt-in for that.
Hyphenated ISO suffixes (`-2024-08-06`, `-05-06`) remain out of scope.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 0a6393d)
(cherry picked from commit 6dd0850)
…t the predicate #3034 widens the dated-suffix matcher and pins it with a predicate test that the fold stays one-way: isDatedVariantId("deepseek-v4-pro", "deepseek-v4-pro-0813") is false. That assertion is true of every implementation, including one whose merge loop calls the predicate a second time with the arguments swapped -- which is exactly what #3041 originally proposed and then withdrew. So the guard is moved to where the resurrection would actually happen. These three drive mergeConfiguredModelsIntoLiveCatalog itself, carried from #3041: - a live base row must not resurrect a configured dated id - a live MMDD dated row still folds onto its configured base - a dated id named in retainConfiguredModelIds survives Both directions were mutation-checked. Adding || isDatedVariantId(candidate.id, live.id) to the merge loop fails only the first test (253 pass / 1 fail); narrowing the suffix back to /^\d{8}$/ fails 13, including the MMDD and YYMM folds. Neither mutation is caught by the predicate test alone. The retention test is labelled for what it actually covers: production fills retainConfiguredModelIds from combo targets, not from providers.*.models, so it pins the OCX-111 path. The operator-facing opt-in is #1690's retainModels, which does not exist yet -- and until it does, the dated id #3024 reports is still dropped. This lands the safe half of #3024 and says so. (cherry picked from commit a909682)
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: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe change expands dated model variant detection to four validated suffix formats. It preserves strict rejection of invalid dates and numeric context-size suffixes. Tests cover matching, merge direction, live catalog folding, and explicit configured-model retention. ChangesDated variant catalog folding
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR broadens dated model alias recognition while preserving calendar validation and one-way matching. Focused tests and type checks pass, and no actionable merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The PR addresses the YYYYMMDD and MMDD dated-suffix defect in Resolution Implement the reverse configured-dated/live-base fold required by Full details: Out of Scope Changes checkExplanation The implementation changes in
✨ Finishing Touches📝 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 |
리뷰 · 우선순위 70 / 80설명 이 PR은 카탈로그 합치기에서 "날짜처럼 생긴 모델 별명"을 너무 좁게만 인정하던 버그를 고칩니다. 지금 고치는 방식은 두 겹입니다. 첫째, #3034에서 가져온 부분으로 날짜 접미사 판별을 현재 라인 939 근처(PR의
경로 #3034/#3041 - 이 PR이 둘을 대체한다고 본문에 적혀 있는데 아직 둘 다 OPEN입니다. 머지 후 경로 #3024/#1690 - #3024의 반대 방향(설정 dated → 라이브 base)은 의도적 drop이고, 남기려면 #1690이 필요합니다. 이 PR 머지 후에도 #3024는 부분 수정으로 남기고, 닫을 조건(#1690 또는 정책 변경)을 이슈에 짧게 적는 편이 좋습니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
Administrator bypass record\n\nOwner authorization was explicit in Codex session 01a05a34-1e3a-73f2-8607-15e517cbec11 on 2026-09-01 KST. Exact head: 7063e3e. Current exact-head check rollup: 0 failing, 0 pending. Independent A-gate review found no unresolved code blocker. Using the maintain/admin pull_request bypass documented in MAINTAINERS.md because the author cannot approve their own PR. This bypass does not waive CI or security review. |
2 participants
Summary
Supersedes #3034 and #3041, which found the same defect from two directions. This carries the better half of each.
isDatedVariantIdonly matched-YYYYMMDD, so every dated alias a provider actually publishes in another shape fell through todroppedConfiguredIdswhile discovery still reportedok(#3024). The matcher now acceptsYYYYMMDD,YYMMDD,MMDDandYYMMwith calendar validation, so-2048,-4096,-8192,-20250229and-0431stay rejected.-1024is October 24th and is the one irreducible collision; the test table pins it rather than pretending otherwise.The first commit is #3034's work, cherry-picked with authorship intact.
The second commit moves the direction guard to where a resurrection would actually happen. #3034 pins one-wayness with a predicate assertion —
isDatedVariantId("deepseek-v4-pro", "deepseek-v4-pro-0813") === false— which is true of every implementation, including one whose merge loop calls the predicate a second time with the arguments swapped. That is exactly what #3041 first proposed and then withdrew, so the guard needs to outlive the proposal. Three tests carried from #3041 drivemergeConfiguredModelsIntoLiveCatalogitself.This does not close #3024. The reported case is a configured dated id against a live base id, and that still drops — deliberately, because a live base row is not callability evidence for a dated snapshot. Retaining it needs #1690's
retainModelsopt-in. Executed on this branch:Verification
Both directions mutation-checked, then restored:
|| isDatedVariantId(candidate.id, live.id)to the merge loopthe merge loop does not infer a configured dated id from a live base id/^\d{8}$/Neither mutation is caught by the predicate test alone, which is the argument for the second commit.
Suffix behavior probed directly over 22 shapes: accepted
2510 2412 2409 1024 2401 2101 0813 2407 1231 2612; rejected2048 4096 8192 2413 2400 3012 1912 0000 0100 1300 1601.Checklist
bun x tsc --noEmitcleanTriaged in the 2026-08-31 non-priority-70 bug round.
Summary by CodeRabbit