Skip to content

fix(providers): preserve case-varied effort overrides - #5254

Closed
luvs01 wants to merge 2 commits into
lidge-jun:devfrom
luvs01:fix/effort-override-case-merge
Closed

luvs01 wants to merge 2 commits into
lidge-jun:devfrom
luvs01:fix/effort-override-case-merge

Conversation

@luvs01

@luvs01 luvs01 commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Case-varied per-model overrides (for example Claude-Opus-5 against the registry's claude-opus-5) no longer leave both keys in merged model maps. mapFill in src/providers/resolved-model-policy-merge.ts and fillRecordOfArrays in src/providers/derive.ts now claim registry keys case-insensitively, matching the case-folded legacyModelValue lookup and the already case-insensitive per-model direct-effort path.
  • Before this, the surviving registry key shadowed the operator's case-varied entry on every folded lookup, so a configured effort ladder was silently dropped for enrichment, routing, and catalog consumers.

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 test on 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.md and structure/config.md updated for the case-insensitive claim.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

  • Bug Fixes
    • Model-specific settings now match consistently regardless of capitalization.
    • User-provided model overrides are preserved correctly instead of being replaced by registry defaults.
    • Routing and provider enrichment now apply the intended settings for case-varied model names.

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.

@coderabbitai

coderabbitai Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: a89a147c-5906-43a6-9106-c591ac16871a

📥 Commits

Reviewing files that changed from the base of the PR and between 64bad3e and 83e942a.

📒 Files selected for processing (5)
  • src/providers/derive.ts
  • src/providers/resolved-model-policy-merge.ts
  • structure/catalog.md
  • structure/config.md
  • tests/providers/provider-registry-parity.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

Case-insensitive model policy merging

Layer / File(s) Summary
Case-insensitive map filling
src/providers/resolved-model-policy-merge.ts, src/providers/derive.ts, structure/config.md
mapFill and fillRecordOfArrays omit registry rows when an operator key matches after case folding. Operator entries retain their original keys and values.
Enrichment validation and references
tests/providers/provider-registry-parity.test.ts, src/providers/derive.ts, structure/catalog.md
Tests verify case-varied overrides through enrichment, routing, and renamed-provider fallback. The enrichment comment now references mapFill. Catalog text remains unchanged in content.

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
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preserving case-varied provider effort overrides during model-map merging.
Full details: Docstring Coverage

Explanation

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)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 20, 2026

@Ingwannu Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions

github-actions Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (0/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

  • ⬜ 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.

0/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@github-actions
github-actions Bot marked this pull request as draft September 20, 2026 04:40
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 52 / 80

이 PR은 모델 이름 대소문자가 달라도, 운영자가 넣은 effort 설정이 레지스트리 기본값에 가려지지 않게 고칩니다. 레지스트리에는 claude-opus-5처럼 소문자 키가 있고, 설정에는 Claude-Opus-5처럼 다른 글자 크기로 넣을 수 있습니다. 찾는 쪽은 이미 소문자로 맞춰 보는데, 합치는 쪽은 예전에는 두 키를 그대로 남겨 두었습니다. 그러면 먼저 들어간 레지스트리 값이 이기고, 운영자가 비운 사다리([]) 같은 설정이 조용히 무시되었습니다. 이제 mapFillfillRecordOfArrays는 운영자 키를 소문자로 맞춘 뒤 같은 모델의 레지스트리 행을 빼서, 운영자 키만 남깁니다. 테스트는 Anthropic enrich와 route, 이름 바꾼 제공자 경로를 같이 봅니다. 베이스는 dev입니다. 같은 주제로 닫을 중복 PR은 없습니다. CI 대부분 통과했고 macOS 조각은 아직 돌아가는 중입니다. PR은 초안이고 준비 체크는 아직입니다.

src/providers/resolved-model-policy-merge.ts nestedMapFill - 같은 파일의 mapFill은 고쳤는데, modelReasoningEffortMap을 합치는 nestedMapFill은 예전처럼 키를 그대로 붙입니다. 이 맵도 effort 쪽입니다. Claude-Opus-5로 넣으면 레지스트리 claude-opus-5와 둘 다 남고, 소문자로 찾는 legacyModelValue는 먼저 나오는 레지스트리 값을 고릅니다. 이번 테스트는 modelReasoningEfforts만 보고 이 맵은 안 봅니다.

src/providers/resolved-model-policy-merge.ts positiveCapMap - openai-apikeymodelContextWindows / modelMaxInputTokens만 타는 합치기도 대소문자를 안 맞춥니다. mapFill을 쓰는 다른 제공자와 규칙이 갈립니다. 이번 버그 범위 밖일 수 있지만, 같은 종류의 구멍입니다.

메인테이너의 판단이 필요한 지점

nestedMapFill까지 이번 PR에서 같이 고칠지, 아니면 modelReasoningEfforts만 먼저 넣고 맵은 뒤를 잡을지 정해 주세요. direct-effort 모델은 이미 소문자로 맞춰 덮어쓰므로, 그 길만 쓰는 설정은 이번에도 괜찮을 수 있습니다.

너의 추천

nestedMapFill에도 mapFill과 같은 소문자 claim을 넣고, modelReasoningEffortMap용 회귀 테스트를 하나 더 넣은 뒤 머지하세요. positiveCapMap은 이번과 같이 안 해도 됩니다. 초안 체크와 macOS CI만 끝난 뒤 ready로 올리면 됩니다.

이 댓글은 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.
@luvs01

luvs01 commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator Author

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.

@luvs01 luvs01 closed this Sep 20, 2026
@luvs01
luvs01 deleted the fix/effort-override-case-merge branch September 20, 2026 06:42
@luvs01

luvs01 commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator Author

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.

@luvs01

luvs01 commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants