Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueNo actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change makes registry and operator model-key merging case-insensitive. Explicit operator keys and values remain unchanged. Provider parity tests cover enrichment, routing, and renamed-provider fallback behavior. ChangesCase-insensitive model policy merging
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant UserConfig
participant enrichProviderFromRegistry
participant mapFill
participant routeModel
UserConfig->>enrichProviderFromRegistry: provide case-varied model override
enrichProviderFromRegistry->>mapFill: merge operator and registry maps
mapFill-->>enrichProviderFromRegistry: preserve operator key and value
enrichProviderFromRegistry-->>routeModel: return enriched provider configuration
routeModel-->>UserConfig: route using the case-varied override
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. (2 skipped: 2 unsupported.) ✨ 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 |
|
✅ Deterministic PR hygiene checks passed. |
Ingwannu
left a comment
There was a problem hiding this comment.
The direction is correct, but the case-folded claim is still incomplete on this exact head.
resolveModelPolicy merges modelReasoningEffortMap through nestedMapFill, not mapFill. That helper still preserves the registry key and then appends a case-varied operator key. legacyModelValue subsequently returns the exact registry spelling first, so an operator override such as { "K3": { xhigh: "custom" } } is still shadowed by a registry k3 row. This is the same failure mode this PR fixes for modelReasoningEfforts.
Please make the outer model-key merge in nestedMapFill case-insensitive while preserving the existing inner-map fill semantics, and add a regression covering a case-varied modelReasoningEffortMap override through the resolved routing policy. The hosted workflows are authorized, but approval should wait for that replacement head and green exact-head CI.
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
리뷰 · 우선순위 52 / 80이 PR은 모델 이름 대소문자가 달라도, 운영자가 넣은 effort 설정이 레지스트리 기본값에 가려지지 않게 고칩니다. 레지스트리에는 src/providers/resolved-model-policy-merge.ts nestedMapFill - 같은 파일의 src/providers/resolved-model-policy-merge.ts positiveCapMap - 메인테이너의 판단이 필요한 지점
너의 추천
이 댓글은 grok-bot이 작성했습니다 |
A case-varied operator key in modelReasoningEffortMap claimed nothing: nestedMapFill kept the registry-spelled row beside it, and the case-folded runtime lookup hit the registry row first. The outer key now claims case-insensitively while the claimed row's inner entries still fill underneath the operator's inner map.
|
Applied the nestedMapFill recommendation on 77af7fb: the outer model-key merge now claims registry rows case-insensitively (same pattern as mapFill), while the claimed row's inner entries still fill underneath the operator's inner map. Added a regression test covering a case-varied modelReasoningEffortMap override through resolveModelPolicy plus the folded runtime lookup. bun test on the touched files passes and tsc --noEmit is clean. |
|
Reopening: the fix was never merged and the underlying bug (case-varied operator effort keys shadowed by registry rows) is still present on dev. Rebased onto current dev and re-verified: bun test tests/providers/provider-registry-parity.test.ts tests/providers/resolved-model-policy.test.ts — 97 pass. |
|
For the record: this PR was closed without a rationale comment, and reopening was blocked after the head branch moved. The change is carried forward in #5466 (open), which supersedes this PR. |
Summary
Claude-Opus-5against the registry'sclaude-opus-5) no longer leave both keys in merged model maps.mapFillinsrc/providers/resolved-model-policy-merge.tsandfillRecordOfArraysinsrc/providers/derive.tsnow claim registry keys case-insensitively, matching the case-foldedlegacyModelValuelookup and the already case-insensitive per-model direct-effort path.Verification
bun test tests/providers/provider-registry-parity.test.ts tests/providers/resolved-model-policy.test.ts— 96 pass; the new regression test fails against the unpatched merge.bun teston adjacent provider policy/migration files (model-rename-migration, context-window-seed-repair, vision-classification-seed-repair, model-presets, new-model-policy) — 60 pass.bun x tsc --noEmit— clean.bun run structure:check— pass;structure/catalog.mdandstructure/config.mdupdated for the case-insensitive claim.Checklist
Summary by CodeRabbit
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.