Skip to content

fix(admission): resolve the per-model window the way the catalog does - #2085

Merged
lidge-jun merged 1 commit into
lidge-jun:devfrom
ntdatt812:fix/admission-family-context-window
Aug 19, 2026
Merged

fix(admission): resolve the per-model window the way the catalog does#2085
lidge-jun merged 1 commit into
lidge-jun:devfrom
ntdatt812:fix/admission-family-context-window

Conversation

@ntdatt812

@ntdatt812 ntdatt812 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

resolveInputCeiling reads two per-model maps with a bare lookup:

const configured = positive(provider.modelContextWindows?.[modelId]) ?? positive(provider.contextWindow);
...
const configuredMaxInput = positive(provider.modelMaxInputTokens?.[modelId]);

The catalog resolves those same two maps through modelRecordValue (src/codex/catalog/provider-fetch.ts:612), which also accepts a family entry for a tagged id — the documented behaviour for gpt-oss covering gpt-oss:120b.

Why it bites

Config:

{ "contextWindow": 8000, "modelContextWindows": { "gpt-oss": 131072 } }

Request to gpt-oss:120b:

value
what the catalog advertises 131 072
what admission used as the ceiling 8 000

The bare lookup misses gpt-oss:120b, so configured falls through to the provider-wide contextWindow — a window that belongs to a different model — and the gate refuses turns the routed model can plainly hold. A ~50k-token turn is rejected pre-dispatch against a 131k model.

That inverts this module's own stated contract:

Deliberately narrow. This is not a context manager and not a compaction trigger: it catches the pathological case and stays out of the way otherwise. Every uncertainty resolves toward admitting.

modelMaxInputTokens has the mirror of the same bug: a family cap silently never applies to the tagged sibling it was written for. That one fails open, so it is a missed cap rather than a wrong refusal — but it is the same divergence.

Fix

Both lookups go through modelRecordValue. Nothing else changes; an id that already resolved exactly resolves to the same value, and modelRecordValue is pure, so the module stays free of filesystem, catalog and registry reads (the existing "touches no filesystem" test still passes).

Verification

Three new tests in tests/input-admission.test.ts. Reverting only the src change turns exactly those three red and nothing else:

(fail) resolveInputCeiling > a family entry covers its tagged siblings, like the catalog
(fail) resolveInputCeiling > an exact entry still beats the family entry
(fail) resolveInputCeiling > a family modelMaxInputTokens tightens its tagged siblings
 19 pass, 3 fail

with the fix: 22 pass, 0 fail.

The first test asserts modelRecordValue(...) — the catalog's own answer — before asserting the ceiling, so the two cannot drift apart again without the test noticing.

Blast radius, run locally:

tests/input-admission.test.ts tests/codex-catalog.test.ts tests/cli-models.test.ts
tests/routing-compatibility-model-matching.test.ts tests/openai-chat-hardening.test.ts
tests/anthropic-hardening.test.ts
-> 293 pass, 0 fail

npx tsc --noEmit — no errors.

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.

Summary by CodeRabbit

  • Bug Fixes
    • Improved input limit handling for model families and specific model variants.
    • Context-window and maximum-input-token limits are now resolved correctly, including model-specific overrides.
    • Prevents valid requests from being incorrectly rejected or limits from being applied inaccurately.

resolveInputCeiling read modelContextWindows and modelMaxInputTokens with
a bare lookup, while the catalog resolves the same two maps through
modelRecordValue, which also accepts a family entry for a tagged id.

With contextWindow 8_000 and modelContextWindows {"gpt-oss": 131_072}:

  catalog advertises   131_072   provider-fetch.ts:612
  admission ceiling      8_000   before this change

So the gate refused turns the model can plainly hold, using a window that
belongs to a different model. That is the opposite of what this module
documents about itself -- "every uncertainty resolves toward admitting".

modelMaxInputTokens had the mirror of it: a family cap never applied to
the tagged sibling it was written for.

Three tests, all red without the src change and green with it. The first
asserts the catalog's value first so the two can never drift apart again.
No behavior changes for ids that already resolved exactly.
@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 63c4092e-3744-4509-9f83-067d71810a9e

📥 Commits

Reviewing files that changed from the base of the PR and between 7535186 and eceaf0b.

📒 Files selected for processing (2)
  • src/server/responses/input-admission.ts
  • tests/input-admission.test.ts

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


📝 Walkthrough

Walkthrough

resolveInputCeiling now supports family and qualified model IDs for context-window and maximum-input-token limits. Tests verify family fallback, exact model precedence, tightened ceilings, and provider-wide fallback behavior.

Changes

Model limit resolution

Layer / File(s) Summary
Resolve and test model limits
src/server/responses/input-admission.ts, tests/input-admission.test.ts
resolveInputCeiling uses modelRecordValue for context-window and maximum-input-token lookups. Tests cover family entries, exact model overrides, family-level input caps, and untagged model fallback.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to eceaf

This localized change aligns per-model admission limits with catalog resolution for tagged model IDs, with the supplied tests and type checks passing; no actionable merge-blocking risk remains beyond normal checks and review.

Possibly related PRs

Suggested reviewers: lidge-jun

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 identifies the admission fix and its alignment with catalog model-window resolution, which is the main change.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@ntdatt812
ntdatt812 marked this pull request as ready for review August 19, 2026 02:12
@github-actions github-actions Bot added the bug Something isn't working label Aug 19, 2026
@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

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.

4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers: @lidge-jun @Ingwannu @Wibias

@lidge-jun

Copy link
Copy Markdown
Owner

Reviewed as part of a four-PR batch (#2077, #2085, #2086, #2100) applying the same modelRecordValue migration at four call sites. This one looks good to merge.

The "definite wrong answer" framing is verified: a missed modelContextWindows lookup does not degrade to unknown, it falls through to the provider-wide contextWindow (src/server/responses/input-admission.ts:136), so the admission gate refuses turns the model can plainly hold. For modelMaxInputTokens a miss becomes absent instead, which fails open by dropping the intended cap — both directions are worth fixing and both are fixed here.

Both per-model reads in the file are migrated; nothing was missed.

The family assertions are real oracles — they fail against the unfixed code. Two of the surrounding assertions are controls that pass either way (the direct modelRecordValue call, and the exact gpt-oss:20b lookup); that is fine as documentation of intent, just noting it so the red-before-fix property is not overstated.

@lidge-jun

Copy link
Copy Markdown
Owner

Reviewed as part of a four-PR batch with #2077, #2086, and #2100 — same idea (bare map?.[modelId] -> modelRecordValue) at four call sites, so it got one shared verdict on the contract plus per-PR verdicts.

Verdict: merge.

The "definite wrong answer" framing checks out, and it is the part worth stating plainly: a missed modelContextWindows lookup does not degrade to unknown, it falls through to the provider-wide contextWindow (src/server/responses/input-admission.ts:136). So the admission gate refuses turns the model can plainly hold, rather than failing open. The modelMaxInputTokens half is the opposite shape — a miss omits the intended cap — which is worth knowing but is not what this PR is primarily fixing.

Both per-model reads in the file are migrated; nothing was left raw.

One note for future tests rather than a change request: the assertions that call modelRecordValue(...) directly are ground truth, not coverage — they pass before the fix. The test earns its keep on the resolveInputCeiling family assertion.

@lidge-jun
lidge-jun merged commit e0585e5 into lidge-jun:dev Aug 19, 2026
11 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants