Repository navigation
feat(core): per-target context limits for model routes - #811
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 48 minutes. View limit details
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a48eb2dab1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| else if (isOverLimit) unavailable.set(index, "over-limit") | ||
| return until <= now && (limited <= now || kept) && !isSkipped && !isOverBudget && !isOverLimit |
There was a problem hiding this comment.
When every target is unavailable, the initial resolution (allowCooling: true) restores all indexes as eligible at line 504, including targets rejected by this new context check. For example, a request of 130,000 tokens against a route capped at 120,000 still resolves to a target instead of returning RouteUnavailableError. The same bypass occurs when the only target large enough is cooling down or soft-excluded. Filter over-limit targets out of the last-resort candidates as well, so relaxing soft exclusions cannot bypass a hard context ceiling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disposition (Lane A, task 2913): already fixed in head 2bfb0a6 — the last-resort path filters over-limit targets: packages/core/src/model-resolver.ts:508 computes eligible as indexes.filter((index) => !overLimit.has(index)) when allowCooling, with the hard-ceiling comment at :504-507, so relaxing soft exclusions cannot restore a target whose context ceiling the request exceeds.
| if ( | ||
| ceiling !== undefined && | ||
| (result[existing].contextLimit === undefined || ceiling < result[existing].contextLimit) | ||
| ) | ||
| result[existing] = { ...result[existing], contextLimit: ceiling } |
There was a problem hiding this comment.
When two targets in the same route resolve to the same leaf, the earlier if (attached.has(key)) continue bypasses this ceiling merge entirely. For example, targets: ["p/*", { model: "p/expensive", limit: { context: 100000 } }] leaves p/expensive uncapped, whereas reaching that same leaf through separate nested routes correctly retains the smallest ceiling. This also leaves the presented route limit uncapped, allowing compaction to exceed the explicitly configured ceiling. Apply the ceiling merge before the same-node duplicate check while still deduplicating the tree attachment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disposition (Lane A, task 2913): already fixed in head 2bfb0a6 — the same-node duplicate branch in packages/core/src/config/plugin/model-routes.ts:451-462 merges the ceiling into the already-attached leaf (tightening contextLimit when the duplicate declares a smaller one) before continue, so a cap declared on a duplicate target is retained.
| limit: Schema.Struct({ | ||
| context: PositiveInt.annotate({ | ||
| description: | ||
| "Caps the context this target serves within the route, below its own limit — e.g. 100000 keeps this target on requests inside a pricing tier that ends at 100k context. A request that does not fit this ceiling and the route's own cap skips this target like any other ineligible target.", | ||
| }), |
There was a problem hiding this comment.
The public config contract and OpenAPI documents now expose target-level limit, but packages/client/src/promise/generated/types.ts still defines ConfigModelRouteTarget without that property. Since ConfigGetOutput uses these generated config types, Promise-client consumers cannot inspect the newly returned ceilings through the typed API without casts. Run bun run generate from packages/client and commit the generated changes, as required by the root AGENTS.md for public Protocol changes.
AGENTS.md reference: AGENTS.md:L1-L1
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disposition (Lane A, task 2913): already done in head 2bfb0a6 — the client was regenerated during the conflict resolution (bun run generate from packages/client) and packages/client/src/promise/generated/types.ts:763 now defines limit?: { context: number } on ConfigModelRouteTarget (also :2211), so Promise-client consumers see target ceilings through the typed API.
|
The following comment was made by an LLM, it may be inaccurate: |
- model-routes-expand: the flat mixed-ceiling test expanded route id
"small" but the route() helper keys its map as "r", so expand
returned no leaves.
- model-resolver: decision-row labels use the target ref id
(test-model for these leaves), not the modelID (luna); the
over-limit decision assertion now matches the recorded labels.
- model-route-review: configured() crashed on a shorthand string
target ("providerID" in a string); handle string targets the way
targetSpec does in model-routes.ts.
… reaches - model-resolver: the last-resort (allowCooling) fallback no longer restores targets excluded as over-limit; context ceilings are hard limits, unlike soft budgets/skips. Regression test: a single over-limit target now fails resolution instead of serving the request. - model-routes expand: a duplicate target attached to the same node now still tightens the leaf ceiling (smallest ceiling wins), matching the cross-reference merge. Regression test included. - client: add limit to the generated ConfigModelRouteTarget type (surgical one-line update; a full regeneration on this base produces unrelated type-relocation and session-interrupt churn, so it is left to a base-wide regen).
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
…rget-limits # Conflicts: # packages/client/src/promise/generated/types.ts
Issue for this PR
Closes #710
Type of change
What does this PR do?
Implements per-element (per-target) context limits for model routes. Closes #710.
Route targets can now declare
limit: { context }in config. The limit composes down the reach path (smallest ceiling wins, including through nested-route references), the route presents min(route cap, leaf ceilings) so compaction cannot bypass a cap, and selection skips over-limit targets with the reason attributed asover-limit— checked last so existing exclusions (cooldown, provider-limit, user-skip, soft-budget) keep their precedence and reasons.route_statssurfaces declared per-target ceilings (configured.limits,configuredLimit,ceiling Nin text) and the skip reason.The flat motivating case works:
smallas one route with 100k/100k/200k per-element limits presents 100k with per-leaf ceilings in the dump — no nestedsmall-100ksub-route needed. Nested routes carrying their own limits are unchanged.Wiring note:
ModelResolver.resolveModeltakes an optionalrequestTokenssize (unknown sizes never filter, preserving current behavior and precedence). Threading live session prompt sizes through the runner is follow-up work; the eligibility mechanism is complete and covered by tests.Tests: schema parsing/expansion, mixed-ceiling flat pool, nested regression, ceiling composition, resolver-level skip + dump attribution, route_stats ceilings, location-layer presented-limit fixture. CI runs them (never run in the work environment per task discipline).
Follow-up commit addressing automated review findings: the resolver's last-resort fallback no longer restores over-limit targets (context ceilings are hard limits, unlike soft budgets/skips); a duplicate target attached to the same route node now still tightens the leaf ceiling; and the generated client type
ConfigModelRouteTargetnow includeslimit(surgical update — a full regeneration on this base produces unrelated churn, left to a base-wide regen).How did you verify your code works?
bun testover model-route, model-route-review, model-routes-expand and model-resolver suites — 157 pass, 0 fail, including new regression tests for the last-resort ceiling and duplicate-target ceiling merge.tsgo --noEmit) pass.Screenshots / recordings
Not a UI change.
Checklist
Need help on this PR? Tag
@codesmith-botwith what you need.