test(catalog): pin the custom-provider reasoning-summary opt-in (#1100) - #1274
Conversation
The landed #1100 coverage pins the built-in registry rows. It does not reach the custom-provider route, where an explicit modelSupportsReasoningSummaries opt-in has to survive the routed strip. That survival is ordering-dependent: normalizeRoutedCatalogEntry deletes the flag and applyCatalogModelMetadata restores it, so the opt-in only works because the latter runs second (src/codex/catalog/sync.ts:266-269). Nothing pinned that order for a custom provider, and reversing it would silently stop every opted-in routed provider from receiving reasoning.effort from Codex while the effort ladder still advertised in the picker. Three tests: the opt-in survives the template path, an absent opt-in stays conservative rather than claiming OpenAI-only summary delivery for an arbitrary endpoint, and the no-template fallback reaches the same answer by a different route - kept explicit so unifying the two construction paths is a visible change. Verified by ablation: swapping the two calls fails the first test plus the landed built-in test, and passes all three again once reverted. tests/codex-catalog.test.ts is 132 pass / 0 fail; typecheck clean. Supersedes the test portion of #1119, which asserted the same contract from a branch 1,800+ commits behind dev. Co-authored-by: bitkyc08-arch <bitkyc08@gmail.com>
|
✅ 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 (1)
📝 WalkthroughWalkthroughThe pull request adds three regression tests for reasoning-summary metadata on custom routed models. The tests cover explicit opt-in preservation, conservative disabling without opt-in, and no-template fallback behavior. ChangesRouted reasoning metadata
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Summary
Pins the custom-provider half of the #1100 reasoning-summary contract, so PR #1119 can be closed without losing regression coverage.
The landed #1100 tests cover the built-in registry rows. They do not reach the custom-provider route, where an explicit
modelSupportsReasoningSummariesopt-in has to survive the routed strip. Both existing custom-provider tests callbuildCatalogEntries(null, ...), which takes the fallback branch and never runsnormalizeRoutedCatalogEntry— so an ordering regression in the template path was invisible to every existing test.That survival is ordering-dependent:
normalizeRoutedCatalogEntrydeletes the flag andapplyCatalogModelMetadatarestores it. Reverse them and every opted-in routed provider silently stops receivingreasoning.effortfrom Codex while the picker keeps advertising the ladder — the original #1100 symptom.Three tests: the opt-in survives the template path, an absent opt-in stays conservative rather than claiming OpenAI-only summary delivery for an arbitrary endpoint, and the no-template fallback reaches the same answer by a different route (kept explicit so unifying the two construction paths is a visible change).
Supersedes the test portion of #1119, whose branch is ~1,800 commits behind
dev. Authored originally there; preserved viaCo-authored-by.Verification
bun test tests/codex-catalog.test.ts— 132 pass / 0 failbun run typecheck— cleannormalizeRoutedCatalogEntryandapplyCatalogModelMetadatafails the new opt-in test plus the landed built-in test (6 pass / 2 fail); reverting restores 8 pass / 0 failChecklist
Summary by CodeRabbit