fix(catalog): routed models never inherit the native template's context window (#992) - #1028
Open
lidge-jun wants to merge 1 commit into
Open
fix(catalog): routed models never inherit the native template's context window (#992)#1028lidge-jun wants to merge 1 commit into
lidge-jun wants to merge 1 commit into
Conversation
…xt window (#992) A routed model is not the native template. When /models omits context metadata, the entry now falls to the existing conservative 128k/128k/ 115.2k triple; known live/configured/Jawcode metadata still restores exact values, and a provider context cap never invents capacity. Two tests that pinned inheritance as a feature are updated to the #992 contract.
📝 WalkthroughWalkthroughRouted catalog entries no longer inherit context metadata from native templates. The sync path clears inherited values before normalization. Tests verify conservative defaults, provider-cap handling, and preservation of explicit routed metadata. ChangesRouted catalog context handling
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/codex-catalog.test.ts`:
- Around line 2318-2333: Extend both routed-capacity tests around
buildCatalogEntries to assert the complete context contract: context_window,
max_context_window, and auto_compact_token_limit. For the oversized contextCap
case, verify all three values remain capped at the expected routed capacity; for
explicit contextWindow metadata, verify max_context_window matches 256,000 and
auto_compact_token_limit remains floor(256,000 × 0.9), alongside the existing
context_window assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e432b224-91a0-4b80-80c2-0390848724cf
📒 Files selected for processing (2)
src/codex/catalog/sync.tstests/codex-catalog.test.ts
Comment on lines
+2318
to
+2333
| test("a provider context cap never invents routed capacity (#992)", () => { | ||
| const entries = buildCatalogEntries({ context_window: 372_000 }, [], [ | ||
| { provider: "relay", id: "relay-model", contextCap: 950_000 }, | ||
| ]); | ||
| const routed = entries.find(e => e.slug === "relay/relay-model"); | ||
| expect(routed?.context_window).toBe(128_000); | ||
| }); | ||
|
|
||
| test("known routed metadata still restores the exact context window (#992)", () => { | ||
| const entries = buildCatalogEntries({ context_window: 372_000 }, [], [ | ||
| { provider: "relay", id: "relay-model", contextWindow: 256_000 }, | ||
| ]); | ||
| const routed = entries.find(e => e.slug === "relay/relay-model"); | ||
| expect(routed?.context_window).toBe(256_000); | ||
| expect(routed?.auto_compact_token_limit).toBe(Math.floor(256_000 * 0.9)); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the complete routed context contract.
Lines 2318-2324 assert only context_window for an oversized contextCap. A regression can advertise capacity through max_context_window or auto_compact_token_limit and still pass this test.
Lines 2326-2333 also omit max_context_window for explicit contextWindow metadata. Assert the full expected triples in both tests.
Proposed test additions
expect(routed?.context_window).toBe(128_000);
+ expect(routed?.max_context_window).toBe(128_000);
+ expect(routed?.auto_compact_token_limit).toBe(115_200);
@@
expect(routed?.context_window).toBe(256_000);
+ expect(routed?.max_context_window).toBe(256_000);
expect(routed?.auto_compact_token_limit).toBe(Math.floor(256_000 * 0.9));As per path instructions, “A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("a provider context cap never invents routed capacity (#992)", () => { | |
| const entries = buildCatalogEntries({ context_window: 372_000 }, [], [ | |
| { provider: "relay", id: "relay-model", contextCap: 950_000 }, | |
| ]); | |
| const routed = entries.find(e => e.slug === "relay/relay-model"); | |
| expect(routed?.context_window).toBe(128_000); | |
| }); | |
| test("known routed metadata still restores the exact context window (#992)", () => { | |
| const entries = buildCatalogEntries({ context_window: 372_000 }, [], [ | |
| { provider: "relay", id: "relay-model", contextWindow: 256_000 }, | |
| ]); | |
| const routed = entries.find(e => e.slug === "relay/relay-model"); | |
| expect(routed?.context_window).toBe(256_000); | |
| expect(routed?.auto_compact_token_limit).toBe(Math.floor(256_000 * 0.9)); | |
| }); | |
| test("a provider context cap never invents routed capacity (`#992`)", () => { | |
| const entries = buildCatalogEntries({ context_window: 372_000 }, [], [ | |
| { provider: "relay", id: "relay-model", contextCap: 950_000 }, | |
| ]); | |
| const routed = entries.find(e => e.slug === "relay/relay-model"); | |
| expect(routed?.context_window).toBe(128_000); | |
| expect(routed?.max_context_window).toBe(128_000); | |
| expect(routed?.auto_compact_token_limit).toBe(115_200); | |
| }); | |
| test("known routed metadata still restores the exact context window (`#992`)", () => { | |
| const entries = buildCatalogEntries({ context_window: 372_000 }, [], [ | |
| { provider: "relay", id: "relay-model", contextWindow: 256_000 }, | |
| ]); | |
| const routed = entries.find(e => e.slug === "relay/relay-model"); | |
| expect(routed?.context_window).toBe(256_000); | |
| expect(routed?.max_context_window).toBe(256_000); | |
| expect(routed?.auto_compact_token_limit).toBe(Math.floor(256_000 * 0.9)); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/codex-catalog.test.ts` around lines 2318 - 2333, Extend both
routed-capacity tests around buildCatalogEntries to assert the complete context
contract: context_window, max_context_window, and auto_compact_token_limit. For
the oversized contextCap case, verify all three values remain capped at the
expected routed capacity; for explicit contextWindow metadata, verify
max_context_window matches 256,000 and auto_compact_token_limit remains
floor(256,000 × 0.9), alongside the existing context_window assertions.
Source: Path instructions
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
1 participant
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.
Bug-stack campaign lane (
devlog/_plan/260805_bug_stack_campaign/100). Fixes #992.A routed model is not the native template: when /models omits context metadata, the entry now falls to the existing conservative
128000/128000/115200triple instead of advertising the template's window. Known live/configured/Jawcode metadata still restores exact values; a provider context cap never invents capacity (pinned by tests).bun run typecheck0 errors;bun run privacy:scanpass; tests/codex-catalog 118/118 on this branch; full suite on ssh lidge 8314/0 (campaign tree).Summary by CodeRabbit