Repository navigation
Conversation
Follow-up to #3358, which merged before these review-comment fixes could be pushed. Three small robustness fixes; no change to the bulk-block safety guards themselves. ## Changes **Share the bulk block limits instead of duplicating them** (`packages/shared/src/bulk-block.ts`) `MIN_BULK_BLOCK_SEARCH_LENGTH` and `MAX_BULK_BLOCK_ORGANIZATIONS` now live in `@llmgateway/shared`, imported by both the API and the admin dashboard. Previously the UI hard-coded `3` with a comment asking future readers to keep it in sync — if the server value changed, the UI would silently show or hide the bulk action for filters the server disagrees about. The spec imports the shared constant too, so the over-cap test can no longer drift from the real limit. **Don't strand the preview dialog in a loading state** (`bulk-block-orgs-button.tsx`) `handleOpen` awaited the preview server action without a `try`/`finally`. A rejection (network error, server crash) left `previewLoading` stuck at `true` with no error surfaced. Now wrapped, so the loading flag always clears and the error is shown. **Keep the confirmation form usable after a failed block** (`bulk-block-orgs-button.tsx`) `handleConfirm` set `result` unconditionally, which hid the preview and the count input while still rendering an enabled destructive button with a stale `confirmationMatches`. A second click resubmitted the same stale count. Now `result` is set only on success; on failure the dialog stays on the confirmation step and re-resolves the preview, so the admin retypes against current numbers. The most likely failure is exactly the `409` the server returns when the set no longer matches the confirmed count, so re-previewing is what makes a retry meaningful. ## Testing `apps/api/src/routes/admin-bulk-block.spec.ts` — 10/10 passing against merged `main`. `turbo run build --filter=api --filter=admin` green. --- _Generated by [Claude Code](https://claude.ai/code/session_01SVus1fjS6z6xppaEai34Uj)_ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved error handling for bulk organization blocking. * Preview loading now recovers correctly from failures and consistently resets its loading state. * Failed bulk-block actions no longer leave stale results and refresh the preview before retrying. * **Improvements** * Bulk organization actions now consistently enforce minimum search-length and maximum selection limits. * Separate preview and blocking errors provide clearer feedback during bulk actions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude <noreply@anthropic.com>
Fixes #3361. ## The bug `case "inference.net": case "together-ai":` in `packages/actions/src/prepare-request-body.ts` forwarded `response_format`, `temperature`, `max_tokens`, `top_p` and the penalties, then `break`ed before the `default` case's generic `reasoning_effort` forwarder. `reasoning_effort` was therefore **never** sent to Together AI for any model — including the gateway's own auto-routing default and `deepseek-v4-pro`'s declared `reasoningEfforts: ["high", "max"]`, which was dead on arrival. ## What the upstream API actually does Probed live against `api.together.ai` (not from docs). Together is OpenAI-compatible on `reasoning_effort`, but **disabling** thinking is not uniform — reasoning models run on two serving stacks with different switches: | Mapping | `reasoning_effort` | Disable switch | |---|---|---| | `openai/gpt-oss-120b`, `openai/gpt-oss-20b` | validated, 400s outside `low`/`medium`/`high` | n/a (`none` is rejected) | | `google/gemma-4-31b-it` | validated; the 400 names the literals (`'none'`, `'low'`, `'medium'`, `'high'`) | `reasoning_effort: "none"` → 0 reasoning tokens | | `deepseek-ai/DeepSeek-V4-Pro` | accepted unvalidated; `xhigh`/`max` roughly double reasoning tokens, `low`/`medium`/`high` land on the default | `thinking: { type: "disabled" }` | | `MiniMaxAI/MiniMax-M3`, `moonshotai/Kimi-K2.6`, `moonshotai/Kimi-K3` | accepted unvalidated; no tier measurably changes reasoning length | `thinking: { type: "disabled" }` | The two switches are **not** interchangeable: Gemma ignores `thinking` (5 runs, reasoning continued at ~1.5k tokens), and DeepSeek V4 Pro / Kimi K2.6 ignore `reasoning_effort: "none"` (still reasoned). `thinking.type` is validated by the second stack — a bad value 400s with `unknown variant ... expected one of enabled, disabled, adaptive`. ## The fix - Forward `reasoning_effort` verbatim from the `together-ai` case, per the repo's no-downgrade rule. - Add `together-ai` to `handlesNoneNatively` so `none` survives to the switch. - Translate `none` per serving stack, driven by a new per-mapping `requiresDisableThinkingParam` flag (sibling to the existing `requiresEnableThinking`), so the DeepSeek/MiniMax/Kimi mappings emit `thinking: { type: "disabled" }` and Gemma emits `reasoning_effort: "none"`. - Set each mapping's `reasoningEfforts` from measured behaviour rather than assumption, replacing `deepseek-v4-pro`'s unverified `["high", "max"]`. ## Testing - 6 new unit tests in `prepare-request-body.spec.ts` covering both stacks, the `none` translation, and the drop when a mapping does not declare `none`. `pnpm test:unit` for `packages/actions` + `packages/models`: 546 passed. - `pnpm build` and `pnpm format` clean. - E2E against the real Together API (`TEST_MODELS=... FULL_MODE=true pnpm test:e2e`), which expands one case per declared effort tier. All pass: gpt-oss-120b/20b `low`/`medium`/`high`, gemma `none`/`low`/`medium`/`high`, deepseek-v4-pro `none`/`xhigh`/`max`, and `none` for minimax-m3, kimi-k2.6, kimi-k3. ### Pre-existing failures (not from this change) `together-ai/gemma-4-31b-it` times out at 60s intermittently across unrelated suites (JSON output, streaming, tool calls) and on the plain no-parameter baseline when probed directly — its effort tiers each pass in one run and time out in another, in a different combination each time. `together-ai/gpt-oss-120b` fails tool-calling and Responses tool-calling. Neither of those suites sends `reasoning_effort` at all. ## Out of scope `moonshotai/Kimi-K2.5` and `zai-org/GLM-4.7` on Together return `Unable to access non-serverless model` for every request — those mappings need a dedicated endpoint and appear unusable as configured. Left untouched here; worth a separate look. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added model-specific reasoning effort support for Together AI models, including supported `none`, `low`, `medium`, `high`, `xhigh`, and `max` tiers where applicable. * Added support for disabling reasoning on compatible models. * Added Together AI configurations for additional Kimi models. * **Bug Fixes** * Improved request handling so reasoning settings are correctly forwarded, translated, or omitted based on model capabilities. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
## What Surfaces per-model output-token limits in the public `GET /v1/models` catalogue: - each `providers[]` entry carries its mapping's exact `maxOutput` as `max_output` - the model level reports the **minimum** across still-servable mappings that declare one — the largest `max_tokens` value guaranteed to be accepted regardless of which provider serves the request (the gateway validates `max_tokens` against the routed mapping's limit and rejects anything above it with HTTP 400). Deactivated mappings can no longer serve, so their limits are excluded; deprecated mappings remain routable and stay in - mappings that declare no limit accept any `max_tokens`, so they don't constrain the model-level bound; models where no mapping declares one omit the field ## Why Consumers currently have no way to discover the cap — they either guess low and truncate output or guess high and get 400s. The Pi coding-agent integration (earendil-works/pi#7480) reads this field to set per-model output limits (falling back to a conservative 4096 until this ships). ## Testing Extended `models.spec.ts` with a spot check (claude-haiku-4-5 → 64000 per-provider and model-level) plus an exhaustive assertion that every model's `max_output` equals the minimum across its still-servable declaring mappings (or is omitted), and a deepseek-v3.2 case pinning the bound to 65536 rather than its retired Nebius mapping's 32768. All 23 models endpoint tests pass; `tsc` and prettier clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Model API responses now include maximum output limits for individual provider mappings. * Model-level responses show the lowest available limit across active mappings, while retaining deprecated mappings and excluding deactivated ones. * The limit is omitted when no applicable value is defined. * **Tests** * Added coverage to verify maximum output limits are reported and calculated correctly. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
The credentials table showed a bare "inactive" badge for a key the worker had auto-disabled, and numbered every row by its list index — so a key that had burned through its cap read as position #1 while the gateway was silently serving from the one below it. Spend-limit state is now derived in one place and shared by the managed credentials page and the per-org BYOK table: a usage bar with a warning tint from 80% of the cap, a "limit reached" badge that no longer depends on the status flip having landed (enforcement lags by one worker batch, and the key still serves traffic in that window), and rotation positions that count only keys the gateway will actually select, with out-of-rotation rows de-emphasised and marked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
There was a problem hiding this comment.
Pull request overview
This PR improves admin visibility into provider-key spend-limit state (including the worker-lag window and rotation eligibility), and also extends the model catalog/API surface with additional routing/validation metadata.
Changes:
- Add shared spend-limit state helpers and new UI cells/badges to clearly distinguish manual inactive vs spend-cap reached (including “disabling” lag state) and show an 80% warning threshold.
- Fix rotation numbering in the managed credentials UI to count only gateway-selectable keys, and refine bulk-block preview/block error handling.
- Extend model/provider metadata: Together AI reasoning-effort/thinking-switch behavior, and expose
max_outputin/v1/models(per mapping + safe model-level minimum).
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/index.ts | Re-export bulk-block limits from shared package. |
| packages/shared/src/bulk-block.ts | Centralize bulk-block limits (min search length, max bulk size). |
| packages/models/src/models/openai.ts | Declare Together-facing reasoningEfforts for specific OpenAI-family mappings. |
| packages/models/src/models/moonshot.ts | Add reasoningEfforts: ["none"] + thinking-disable flag for Together-served Moonshot mappings. |
| packages/models/src/models/minimax.ts | Add reasoningEfforts: ["none"] + thinking-disable flag for Together-served MiniMax mappings. |
| packages/models/src/models/google.ts | Declare Together-accepted reasoningEfforts literals for Gemma mapping. |
| packages/models/src/models/deepseek.ts | Refine Together DeepSeek reasoning tiers and mark thinking-disable behavior. |
| packages/models/src/models.ts | Add requiresDisableThinkingParam to provider mapping metadata. |
| packages/actions/src/prepare-request-body.ts | Implement Together-specific reasoning disable semantics (reasoning_effort: "none" vs thinking: {type:"disabled"}). |
| packages/actions/src/prepare-request-body.spec.ts | Add tests covering Together reasoning forwarding/disable behavior. |
| ee/admin/src/lib/provider-key-spend.ts | Introduce spend-limit state machine + rotation eligibility helpers. |
| ee/admin/src/lib/provider-key-spend.spec.ts | Add unit tests for spend-limit states, fractions, rotation eligibility, and USD formatting. |
| ee/admin/src/components/provider-key-status-badge.tsx | New badge component for “limit reached” and “disabling” states. |
| ee/admin/src/components/provider-key-spend-cell.tsx | New spend/cap display with warning/destructive coloring and progress bar. |
| ee/admin/src/components/provider-credentials-manager.tsx | Use new spend cell/badge; compute rotation position over selectable keys; de-emphasize out-of-rotation rows. |
| ee/admin/src/components/bulk-block-orgs-button.tsx | Split preview vs block errors and re-preview after failed block attempts. |
| ee/admin/src/app/organizations/page.tsx | Use shared MIN_BULK_BLOCK_SEARCH_LENGTH instead of local constant. |
| ee/admin/src/app/organizations/[orgId]/provider-keys-table.tsx | Use new spend cell/badge for org BYOK provider keys. |
| apps/gateway/src/models/models.ts | Expose max_output in models API and compute safe model-level minimum across still-servable mappings. |
| apps/gateway/src/models/models.spec.ts | Add coverage asserting max_output behavior (per mapping + model-level min, excluding deactivated mappings). |
| apps/api/src/routes/admin.ts | Use shared bulk-block constants in admin bulk-block implementation. |
| apps/api/src/routes/admin-bulk-block.spec.ts | Use shared bulk-block constants in admin bulk-block tests. |
Suppressed comments (1)
ee/admin/src/lib/provider-key-spend.ts:74
isInRotationcurrently returns false forstatus: null/undefined, but other parts of the admin UI treat a null status as "active" (e.g. renderingstatus ?? "active"). If any legacy rows have NULL status, they’ll incorrectly show as out-of-rotation and be excluded from rotation numbering.
export function isInRotation(key: SpendLimited): boolean {
return key.status === "active";
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+15
to
+46
| export function ProviderKeyStatusBadge({ keyRow }: { keyRow: SpendLimited }) { | ||
| const state = getSpendLimitState(keyRow); | ||
| const cap = formatUsd(keyRow.usageLimit ?? "0"); | ||
|
|
||
| if (state === "reached") { | ||
| return ( | ||
| <Badge | ||
| variant="destructive" | ||
| title={`Automatically deactivated: attributed spend reached the ${cap} cap, so the gateway no longer selects this key. Raise or clear the limit to re-enable it.`} | ||
| > | ||
| limit reached | ||
| </Badge> | ||
| ); | ||
| } | ||
|
|
||
| if (state === "reached-pending") { | ||
| return ( | ||
| <Badge | ||
| variant="destructive" | ||
| title={`Attributed spend reached the ${cap} cap. The billing worker deactivates the key on its next batch — until then it can still serve requests.`} | ||
| > | ||
| limit reached · disabling | ||
| </Badge> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <Badge variant={keyRow.status === "active" ? "default" : "secondary"}> | ||
| {keyRow.status ?? "active"} | ||
| </Badge> | ||
| ); | ||
| } |
Comment on lines
121
to
+125
| context_length: z.number().optional(), | ||
| max_output: z.number().optional().openapi({ | ||
| description: | ||
| "Largest max_tokens value guaranteed to be accepted regardless of which provider mapping serves the request (the minimum across still-servable mappings that declare a limit; deactivated mappings are excluded). Omitted when no such mapping declares one.", | ||
| }), |
Comment on lines
+52
to
+65
| export function getSpendLimitState(key: SpendLimited): SpendLimitState { | ||
| const fraction = spendLimitFraction(key); | ||
| if (fraction === null) { | ||
| return "none"; | ||
| } | ||
| if (fraction >= 1) { | ||
| // Deliberately independent of status: the cap is crossed the moment the | ||
| // worker credits the spend, and the status flip lands one batch later. | ||
| // Reporting "active" in that window would tell an operator the key is | ||
| // fine seconds before it turns itself off. | ||
| return key.status === "active" ? "reached-pending" : "reached"; | ||
| } | ||
| return fraction >= WARNING_THRESHOLD ? "warning" : "ok"; | ||
| } |
The branch carried two generated migrations. Reset packages/db/migrations to origin/main and regenerated, per the repo's migration policy, so the branch adds a single migration. Regeneration discards hand-written .sql adaptations, so the out-of-band CREATE INDEX CONCURRENTLY guidance and the IF NOT EXISTS guard on log_provider_key_id_created_at_idx were re-applied — without them the migrator scans all of "log" while holding a lock on it. Verified the squashed file carries an identical set of 27 statements to the two it replaces, and that the full chain applies to a clean database with the expected columns, indexes and constraints. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
steebchen
merged commit Aug 2, 2026
b481ec0
into
claude/provider-credentials-ui-lgai9x
8 checks passed
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
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.
Stacked on #3234 (follows #3370, which merged into the same base).
Problem
The Provider Credentials page could not tell an operator that a credential had burned through its spend cap and was no longer eligible for selection:
inactivebadge — indistinguishable from one an admin switched off by hand.#1in the rotation while the gateway was silently serving from the one below it.Changes
Spend-limit state is derived in one place (
ee/admin/src/lib/provider-key-spend.ts) and shared by the managed-credentials page and the per-organization BYOK table:limit reachedbadge no longer requires the status flip to have landed. Enforcement lags by one worker batch, and the key keeps serving traffic in that window, so an over-cap key that is still active renders aslimit reached · disablingrather than a reassuringactive.status = 'active', so out-of-rotation rows now show—with an explanatory tooltip and a de-emphasised background instead of a misleading rank.Verification
pnpm format,pnpm build— clean.provider-key-spend.spec.ts(13 new, covering the state machine including the lag window and a zero/malformed cap),provider-key-stats.spec.ts, andadmin-provider-credentials.spec.ts.$14.5125matched the database, split acrossFinTech Global $10.75andTest Organization $3.7625. Both bucket granularities (hourly for 24h, daily for 7d) verified. Dev data restored afterwards.🤖 Generated with Claude Code