feat(observability): log the remaining gateway-side rejections by njbrake · Pull Request #472 · mozilla-ai/otari

njbrake · 2026-08-02T17:07:13Z

Description

#449 made one gateway refusal visible: the missing-pricing gate wrote an error row, and the dashboard read that back as a live failure count. Every other gate still raised without writing anything, so during those incidents the count read 0 and the activity log showed nothing while requests were being dropped. That is the situation #317 was filed to end.

Log the remaining gateway-side rejections. A row is now recorded for a model outside a key's allow-list, a user/key mismatch, a blocked or over-budget user (raised inside reserve_budget), and a selector that no longer resolves to a configured provider. Both scaffolds are covered: the chat/messages/responses pipeline and the pass-through routes behind embeddings, images, audio, moderations, and rerank.

All of them go through one writer, log_gateway_rejection, which fixes counts_toward_budget=True. That matters per gate: the dashboard classifies counts_toward_budget=False as imported usage and offers those rows for bulk delete and set-price, and a key flagged exclude_from_budget can reach these gates (unlike the pricing gate, which is guarded by not budget_exempt), so inheriting its flag would have put rejection rows in that set. There is no cost on these rows for the flag to gate. Reservations are untouched: each site refunds exactly as before and the row carries cost=null, so nothing moves spend.

Some refusals stay unlogged, deliberately rather than by omission, for three different reasons. A 401 is refused before any user is known; the column is nullable so a NULL-user row would insert fine, but it could not be attributed, filtered, or acted on, and writing one would let an unauthenticated caller append to the usage table. A 404 for a named nonexistent user is skipped for a harder reason: usage_logs.user_id is a foreign key to users, so that row could not be inserted at all. A 429 is skipped because a throttle is expected, self-limiting behavior rather than dropped traffic, unlike the client-driven gates that do log. All three are pinned by tests. Note this is not an exhaustive list of unlogged refusals: a guardrail block, an unreachable sandbox or web-search backend, and the responses-unsupported 400 also write no row today, all outside the scope #465 listed.

The model/provider split is unified rather than left alone. Rejection rows now name the resolved target (model=gpt-4o, provider=openai) instead of the request selector. #449 logged the full instance:model selector on the chat gate, which split a model's failures from its successes: the activity log's model filter is an exact match on that column, and usage-by-model groups on it. Both scaffolds now write the one form every success row already used. The assertions test_require_pricing.py pinned are updated accordingly.

The three hand-built UsageLog literals in _passthrough.py collapse to one (raised by @khaledosman in review of #449): _usage_row(status, **outcome) for the outcomes of an attempted provider call, and the shared rejection writer for the missing-pricing gate. Adding a column is now a one-line change instead of three sites to keep in sync.

resolve_dispatch_provider becomes async (5 call sites) so the unresolvable-selector 400 can log, and it now refunds, which it never did. I first wrote that off as a formality on the grounds that a selector we cannot resolve has no pricing, so the hold would always be 0.0. That is wrong, and review caught it. The preamble carries an unresolvable selector into the pricing lookup as the bare model with no provider, and find_model_pricing then keys on the model alone, which is exactly the provider:model form stored pricing rows use. So an instance removed from config.yml while its pricing row survives prices normally, reserves a real estimate, and only fails later at dispatch; before this refund the hold stayed on users.reserved until the next budget reset, or forever for a budget with no period. The regression test leaks 0.0102 with the refund removed. This PR therefore fixes a real reservation leak on top of the logging work.

The dashboard side needed nothing new: the count and its drill-down already read status=error scoped to source=gateway. The bundled guide (docs/dashboard.md) is updated to name which refusals are logged and which are not, so the dashboard bundle is rebuilt with it; the 17 renamed asset files are the usual hash cascade off the entry chunk, byte-identical apart from import filenames.

PR Type

Relevant issues

Fixes #465

Checklist

I understand the code I am submitting.
I have added or updated tests that cover my change (tests/unit, tests/integration).
I ran the Definition of Done checks locally (make lint, make typecheck, make test).
Documentation was updated where necessary.
If the API contract changed, I regenerated the OpenAPI spec (uv run python scripts/generate_openapi.py).

Test notes

New tests/integration/test_gateway_rejection_logging.py has 13 tests: one per gate on both scaffolds, the two deliberate omissions, and a regression test pinning users.reserved back to 0 after the unresolvable-selector 400 (verified to fail, leaking 0.0102, with the refund removed). Each rejection test asserts a single error row with no cost and counts_toward_budget=True.

ruff, mypy --strict (273 files), openapi-check, postman-check, dashboard typecheck and its 361 tests all pass. Unit: 1034 passed. Integration: 832 passed, 2 failed. The two failures are test_error_detail_leakage.py::test_provider_error_does_not_leak_details and test_streaming_error_event.py::test_streaming_creation_error_returns_http_error; both fail identically on a clean main worktree in the same environment, which has outbound network and so gets a real provider 404 instead of the expected failure mode. Not related to this change.

AI Usage

No AI was used.
AI was used for drafting/refactoring.
This is fully AI-generated.

AI Model/Tool used:

Claude Opus 5 via Claude Code.

Any additional AI details you'd like to share:

Implemented by Claude through back-and-forth with @njbrake. The decisions this PR had to make explicitly (whether 401 and 429 log, which of the two model forms to standardize on, and pinning counts_toward_budget at the writer instead of per gate) were discussed and settled before implementation, not chosen by the model alone.

I am an AI Agent filling out this form (check box if true)

Summary

Added activity logging for gateway rejections across chat, messages, responses, and pass-through routes.
Recorded model access, user, budget, pricing, and provider resolution failures with resolved model and provider values.
Excluded authentication failures, unknown users, and rate limits from logging.
Preserved reservation refunds and ensured rejected requests have no cost and do not increase spend.
Centralized rejection and usage-row creation.
Updated dashboard documentation and assets.
Added integration and unit test coverage for rejection logging and refund behavior.

Technical notes

Added log_gateway_rejection and unresolvable_model_detail.
Updated resolve_dispatch_provider to accept a format adapter and refund reservations.

#449 made one gateway refusal visible: the missing-pricing gate wrote an error row, and the dashboard read that back as a live failure count. Every other gate still raised without writing anything, so during those incidents the count read 0 and the activity log showed nothing while requests were being dropped. An operator learned about it from a user complaint, which is the situation #317 was filed to end. Record a row for each of the remaining refusals: a model outside a key's allow-list, a user/key mismatch, a blocked or over-budget user (raised inside reserve_budget), and a selector that no longer resolves to a configured provider. Both scaffolds are covered, the chat/messages/responses pipeline and the pass-through routes behind embeddings, images, audio, moderations, and rerank. The rows go through one writer, log_gateway_rejection, which fixes counts_toward_budget=True. The dashboard classifies counts_toward_budget= False as imported usage and offers those rows for bulk delete and set-price, which must never reach a row the gateway wrote itself; a key flagged exclude_from_budget can reach these gates, so inheriting its flag would have put rejection rows in that set. There is no cost on these rows for the flag to gate. Reservations are untouched: each site refunds exactly as before and the row carries cost=null, so nothing moves spend. Two refusals stay unlogged, deliberately rather than by omission. A 401 is refused before any user is known, and usage_logs.user_id is a foreign key to users, so such a row could not be inserted at all. A 429 is an expected, self-limiting throttle rather than dropped traffic, and logging it would let a hot-looping client amplify itself into the usage table. Rejection rows now name the resolved target (model=gpt-4o, provider=openai) instead of the request selector. #449 logged the full instance:model selector on the chat gate, which split a model's failures from its successes: the activity log's model filter is an exact match on that column, and usage-by-model groups on it. Both scaffolds write the one form every success row already used. While in _passthrough.py, the three hand-built eleven-field UsageLog literals collapse to one: _usage_row for the outcomes of an attempted provider call, and the shared rejection writer for the missing-pricing gate. Adding a column is now a one-line change instead of three sites to keep in sync. resolve_dispatch_provider becomes async so the unresolvable-selector 400 can log; it also refunds now, which it never did. The estimate is always 0.0 on that path (a selector we cannot resolve has no pricing), so no reservation was actually leaking. Fixes #465 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-08-02T17:07:18Z

Warning

Review limit reached

@njbrake, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 40 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6f3cd5e2-0902-41dd-97b8-3f912754c781

📥 Commits

Reviewing files that changed from the base of the PR and between 594da82 and 9583f66.

📒 Files selected for processing (5)

src/gateway/api/routes/_passthrough.py
src/gateway/api/routes/_pipeline.py
src/gateway/api/routes/usage.py
tests/integration/test_gateway_rejection_logging.py
tests/integration/test_usage_summary.py

Walkthrough

The gateway now logs attributable rejection paths as zero-cost error usage rows, preserves reservation refunds, and supports adapter-aware provider resolution. Tests cover chat and embedding failures. Dashboard documentation and bundled assets were updated.

Changes

Gateway rejection logging

Layer / File(s)	Summary
Central rejection logging and provider resolution `src/gateway/api/routes/_pipeline.py`	Adds shared rejection logging, consistent model-resolution details, adapter-aware dispatch resolution, reservation refunds, and exclusions for unauthenticated or unknown-user requests.
Pass-through rejection and usage-row integration `src/gateway/api/routes/_passthrough.py`	Logs attributable pass-through refusals and centralizes `UsageLog` construction for provider outcomes and gateway rejections.
Adapter-aware route wiring and rejection validation `src/gateway/api/routes/chat.py`, `src/gateway/api/routes/messages.py`, `src/gateway/api/routes/responses.py`, `tests/integration/test_gateway_rejection_logging.py`, `tests/integration/test_require_pricing.py`, `tests/unit/test_bad_model_name_returns_400.py`, `tests/unit/test_gateway_rejection_logging_best_effort.py`	Updates provider-resolution call sites and tests rejection metadata, reservation refunds, usage-row fields, best-effort logging, and excluded authentication cases.
Dashboard guide and bundled asset refresh `docs/dashboard.md`, `src/gateway/static/dashboard/assets/*`, `src/gateway/static/dashboard/index.html`	Documents logged and unlogged Activity error cases, adds the embedded `DocsPage` bundle, and refreshes hashed asset references.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

mozilla-ai/otari#449: Established the gateway rejection logging path that this change extends.
mozilla-ai/otari#254: Relates to unresolvable-model handling and reservation refunds.
mozilla-ai/otari#318: Relates to API-key model allow-list rejection handling.

Suggested reviewers: khaledosman

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 1.87% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title uses the feat Conventional Commit prefix, imperative mood, clear scope, and remains under 70 characters.
Description check	✅ Passed	The description covers the required sections, linked issue, implementation scope, tests, documentation, checklist, and AI usage.
Linked Issues check	✅ Passed	The changes satisfy issue `#465` by logging the specified rejections, preserving refunds and row fields, deduplicating usage rows, and adding tests.
Out of Scope Changes check	✅ Passed	The changes remain within issue `#465`; documentation, tests, and rebuilt dashboard assets directly support the implementation.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/465-log-rejections

✨ Simplify code

Create PR with simplified code
Commit simplified code in branch fix/465-log-rejections

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.}

…e refund Review of #472 caught that the comment justifying the new refund in resolve_dispatch_provider was wrong, and wrong in the direction that undersells the change. It claimed the estimate is always 0.0 on that path because a selector we cannot resolve has no pricing. It can have pricing. The preamble carries an unresolvable selector into the lookup as the bare model with no provider, and find_model_pricing then keys on the model alone, which is exactly the `provider:model` form stored pricing rows use. An instance removed from config while its pricing row survives prices normally, reserves a real estimate, and only fails later at dispatch. Before the refund, that hold stayed on users.reserved until the next budget reset, or forever for a budget with no period. Verified: the new test leaks 0.0102 with the refund removed. Also from review: - The docstring claimed a 401 goes unlogged because usage_logs.user_id is a foreign key. The column is nullable, so a NULL-user row would insert fine; the real reason is that it could not be attributed, filtered, or acted on, and would let an unauthenticated caller append to the usage table. The foreign-key argument belongs to the 404 for a named nonexistent user, where it does hold. Both are now stated separately, along with why the 429 omission is not in tension with the client-driven gates that do log. - The dashboard guide said two refusals were left out deliberately, which overclaims: a guardrail block, an unreachable sandbox or web-search backend, and the responses-unsupported 400 also write no row. Reworded to stop implying the list is exhaustive. - The user/key mismatch row's comment justified its raw-selector form by saying aliases are user-scoped, which does not apply when the row is already attributed to the key's own user. The real reason is that nothing is resolved that early and resolving purely to shape a log row is not worth it on a refusal path. Tests: a regression test pinning users.reserved back to 0 after the unresolvable-selector 400, plus the two pass-through gates review found uncovered (allow-list, which exercises refund-then-log ordering, and the user/key mismatch). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (4)

src/gateway/api/routes/_passthrough.py (1)
37-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider promoting _elapsed_ms and _raise_for_unresolvable_model to public names.

This import block reaches across modules for two underscore-prefixed members of _pipeline. The PR already promoted unresolvable_model_detail to a public name for exactly this reason, so the remaining two now look like leftovers. If a name has a cross-module consumer, the leading underscore no longer tells a reader anything useful. Renaming them to elapsed_ms and raise_for_unresolvable_model keeps the module boundary honest. This is cosmetic and safe to defer; the behavior is identical either way.
♻️ Proposed rename at the import site
 from gateway.api.routes._pipeline import (
-    _elapsed_ms,
-    _raise_for_unresolvable_model,
+    elapsed_ms,
     log_gateway_rejection,
+    raise_for_unresolvable_model,
     rate_limit_headers,
     unresolvable_model_detail,
 )
Update the definitions in src/gateway/api/routes/_pipeline.py and every other call site if you take this.
🤖 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 `@src/gateway/api/routes/_passthrough.py` around lines 37 - 43, Promote the
cross-module helpers `_elapsed_ms` and `_raise_for_unresolvable_model` to
`elapsed_ms` and `raise_for_unresolvable_model` in `_pipeline`, then update
every import and call site, including `_passthrough`, while preserving behavior.
Source: Coding guidelines
src/gateway/api/routes/responses.py (1)
378-378: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Optional: consider whether the unsupported-provider 400 below should also record a rejection row.

The adapter wiring here is correct, and _ADAPTER.endpoint gives the row the right /v1/responses attribution. One consistency thought while you are in the area: the _ensure_provider_supports_responses guard just below refunds the reservation and raises a 400 without writing a row. That refusal is gateway-side and has a known user, so by the rule this PR establishes it would qualify. It is not one of the four gates the linked issue named, so leaving it for follow-up is a perfectly defensible call. Flagging it only so the dashboard's failure count does not quietly miss one path.

Would you like me to open a follow-up issue for it?
🤖 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 `@src/gateway/api/routes/responses.py` at line 378, The unsupported-provider
path guarded by _ensure_provider_supports_responses currently refunds and raises
400 without recording a rejection row. Optionally update that guard to persist a
rejection using the known user and _ADAPTER.endpoint for /v1/responses
attribution, while preserving the existing refund and error behavior; otherwise
leave it unchanged and track it as follow-up work.
tests/integration/test_require_pricing.py (1)
103-109: 🗄️ Data Integrity & Integration | 🔵 Trivial

Consider how pre-existing rejection rows will group after this representation change.

The new assertion is right and matches what _pipeline.py now writes for the missing-pricing gate. One deployment-side thought: rows written by the #449 behavior already sit in usage_logs with the full openai:gpt-4o selector in the model column and, presumably, a null provider. The comment here notes the model filter is an exact match on that column, so those older rows will keep grouping under their own key and will not merge with new ones.

That is probably acceptable for null-cost error rows, and a backfill may well be more trouble than it is worth. It is worth a deliberate decision rather than a surprise, though: either a small data migration that splits the selector on those rows, or a line in the release notes so an operator is not confused by two entries for the same model. Your call.
🤖 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/integration/test_require_pricing.py` around lines 103 - 109, Decide how
existing usage_logs rejection rows using the legacy full selector format should
coexist with the new resolved model/provider representation. Either add a
migration to split legacy openai:gpt-4o-style values and populate provider, or
document the intentional separate grouping in the release notes; make the chosen
behavior explicit near the related pricing-gate change.
src/gateway/api/routes/messages.py (1)
473-473: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Raise the standalone unresolvable-model 400 through _ADAPTER so /v1/messages keeps Anthropic error formatting.

In standalone mode, create_message catches HTTPException only from resolve_request_context and the runner paths. The new standalone calls to resolve_dispatch_provider at lines 473 and 516 can raise _raise_for_unresolvable_model()’s string-detail HTTPException before those runners, so _ensure_anthropic_error does not run. Use adapter.error(400, unresolvable_model_detail(model_selector), ErrorKind.INVALID_REQUEST) in that dispatch path so Anthropic clients receive the structured error body.
🤖 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 `@src/gateway/api/routes/messages.py` at line 473, Update the standalone
dispatch path in create_message around resolve_dispatch_provider to handle an
unresolvable model through _ADAPTER.error with status 400,
unresolvable_model_detail(model_selector), and ErrorKind.INVALID_REQUEST. Ensure
this path produces the structured Anthropic error response instead of allowing
the raw HTTPException to bypass _ensure_anthropic_error; apply the same handling
to the corresponding dispatch call at the other referenced location.
Source: Coding guidelines

🤖 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 `@src/gateway/api/routes/_pipeline.py`:
- Around line 691-713: The rejection logging path around log_gateway_rejection
must not replace the original HTTPException when logging fails. Make
log_gateway_rejection or its callers catch and log any logging failure, then
ensure the existing raise re-raises the original exception and preserves its
status code.

In `@tests/integration/test_gateway_rejection_logging.py`:
- Around line 192-201: Strengthen the reservation refund test around the _chat
call for “ghostprovider:some-model” by first proving that this priced request
creates a nonzero reservation, following the established control or
pricing-match pattern from
test_budget_exempt_key_writes_no_pricing_rejection_row. Keep the existing
post-request query and assert that the reservation is returned to 0.0.

---

Nitpick comments:
In `@src/gateway/api/routes/_passthrough.py`:
- Around line 37-43: Promote the cross-module helpers `_elapsed_ms` and
`_raise_for_unresolvable_model` to `elapsed_ms` and
`raise_for_unresolvable_model` in `_pipeline`, then update every import and call
site, including `_passthrough`, while preserving behavior.

In `@src/gateway/api/routes/messages.py`:
- Line 473: Update the standalone dispatch path in create_message around
resolve_dispatch_provider to handle an unresolvable model through _ADAPTER.error
with status 400, unresolvable_model_detail(model_selector), and
ErrorKind.INVALID_REQUEST. Ensure this path produces the structured Anthropic
error response instead of allowing the raw HTTPException to bypass
_ensure_anthropic_error; apply the same handling to the corresponding dispatch
call at the other referenced location.

In `@src/gateway/api/routes/responses.py`:
- Line 378: The unsupported-provider path guarded by
_ensure_provider_supports_responses currently refunds and raises 400 without
recording a rejection row. Optionally update that guard to persist a rejection
using the known user and _ADAPTER.endpoint for /v1/responses attribution, while
preserving the existing refund and error behavior; otherwise leave it unchanged
and track it as follow-up work.

In `@tests/integration/test_require_pricing.py`:
- Around line 103-109: Decide how existing usage_logs rejection rows using the
legacy full selector format should coexist with the new resolved model/provider
representation. Either add a migration to split legacy openai:gpt-4o-style
values and populate provider, or document the intentional separate grouping in
the release notes; make the chosen behavior explicit near the related
pricing-gate change.

🪄 Autofix (Beta)

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: CHILL

Plan: Pro Plus

Run ID: 2e5ad622-0ccc-47d7-b39a-33c98dce8d77

📥 Commits

Reviewing files that changed from the base of the PR and between 0b37186 and f56fe6e.

📒 Files selected for processing (27)

docs/dashboard.md
src/gateway/api/routes/_passthrough.py
src/gateway/api/routes/_pipeline.py
src/gateway/api/routes/chat.py
src/gateway/api/routes/messages.py
src/gateway/api/routes/responses.py
src/gateway/static/dashboard/assets/ActivityPage-DfJ8bOeD.js
src/gateway/static/dashboard/assets/AliasesPage-_zQbHDNN.js
src/gateway/static/dashboard/assets/BudgetsPage-BDiXaw4V.js
src/gateway/static/dashboard/assets/ConfirmDialog-qeRbgaKd.js
src/gateway/static/dashboard/assets/DocsPage-Cdyk99dG.js
src/gateway/static/dashboard/assets/FilterChips-rf9fYCee.js
src/gateway/static/dashboard/assets/KeysPage-Cm2AmlbK.js
src/gateway/static/dashboard/assets/ModelScopeControl-Bn348AWz.js
src/gateway/static/dashboard/assets/ModelsPage-54pSPT0o.js
src/gateway/static/dashboard/assets/OverviewPage-DZDxWHD4.js
src/gateway/static/dashboard/assets/ProvidersPage-K53SnI1M.js
src/gateway/static/dashboard/assets/SettingsPage-BKpBN5qv.js
src/gateway/static/dashboard/assets/TablePagination-D4m36f36.js
src/gateway/static/dashboard/assets/ToolsGuardrailsPage-BUqBDjx7.js
src/gateway/static/dashboard/assets/UsagePage-DdQSDCn_.js
src/gateway/static/dashboard/assets/UsersPage-BPXBwdFe.js
src/gateway/static/dashboard/assets/index-Pee-X5QW.js
src/gateway/static/dashboard/index.html
tests/integration/test_gateway_rejection_logging.py
tests/integration/test_require_pricing.py
tests/unit/test_bad_model_name_returns_400.py

Two findings from CodeRabbit on #472. Rejection logging ran before the caller re-raised, unguarded. Every one of the ten call sites logs the drop and then raises the refusal it was already going to return, so an exception escaping log_gateway_rejection would replace a clean 403 or 400 with a 500: an unhealthy log writer would look to the client like a broken gateway. SingleLogWriter absorbs SQLAlchemyError itself, but not session setup or teardown, and log_usage builds the row outside any guard. Swallow and report instead, in the one helper every site shares. Nothing leaks: each site refunds before logging, never after, so a dropped row costs only the row. The reservation-refund test could pass vacuously. It asserted only the end state, users.reserved == 0.0, which also holds if the estimate is 0, exactly the case that would mean the refund guards nothing. Added the A/B control the sibling test in test_require_pricing.py already argues for: the same request under a budget smaller than the estimate is refused at the budget gate instead of reaching dispatch, which is only possible if the raw selector really matched pricing and produced a nonzero estimate. Both are mutation-checked. Removing the refund fails on reserved == 0.0 with 0.0102 stranded; forcing estimate_cost to 0 fails the new control with 400 != 403; narrowing the swallow to ValueError fails the new unit test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR extends the gateway’s observability by ensuring that most gateway-side request rejections (when a user is known) are recorded as UsageLog error rows, so the dashboard’s failure count and activity log reflect dropped traffic. It also fixes a real reservation leak on the unresolvable-selector path by refunding the held estimate before returning the 400.

Changes:

Added a shared best-effort log_gateway_rejection helper and wired it into remaining rejection sites across the chat/messages/responses pipeline and pass-through routes.
Standardized rejection rows to log the resolved (model, provider) form where available, aligning failures with successes in dashboard grouping/filtering.
Made resolve_dispatch_provider async to log and refund on unresolvable selectors; added/updated unit + integration coverage and refreshed dashboard docs/assets.

Reviewed changes

Copilot reviewed 27 out of 28 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`src/gateway/api/routes/_pipeline.py`	Adds shared rejection logger, unresolvable-model detail helper, logs additional rejection gates, and refunds leaked reservations on selector resolution failures.
`src/gateway/api/routes/_passthrough.py`	Deduplicates `UsageLog` construction, logs additional rejection paths via shared helper, and aligns model/provider logging form.
`src/gateway/api/routes/chat.py`	Updates standalone normalize path to await async `resolve_dispatch_provider` with adapter for logging.
`src/gateway/api/routes/messages.py`	Updates standalone streaming and non-streaming normalize paths to await async `resolve_dispatch_provider`.
`src/gateway/api/routes/responses.py`	Updates standalone normalize path to await async `resolve_dispatch_provider`.
`tests/unit/test_gateway_rejection_logging_best_effort.py`	Adds unit tests ensuring rejection logging is best-effort and does not change caller-visible failures.
`tests/unit/test_bad_model_name_returns_400.py`	Updates tests for async `resolve_dispatch_provider` signature and behavior.
`tests/integration/test_require_pricing.py`	Updates assertions to the unified resolved `(model, provider)` logging form.
`tests/integration/test_gateway_rejection_logging.py`	Adds end-to-end coverage across rejection gates (including deliberate omissions) and a regression test for reservation release.
`docs/dashboard.md`	Documents which refusals are logged vs deliberately omitted.
`src/gateway/static/dashboard/**`	Rebuilt dashboard bundle/assets to include updated docs content.

khaledosman

Two things worth resolving before this merges, both inline: the user/key-mismatch gate writes its row before check_rate_limit, making it the one logged rejection a caller can spam unthrottled; and cost=null rejection rows feed unpriced_requests, which the Usage page renders as a pricing-gap signal.

Also flagged: the status_code coordination with #470 (that PR stamps the very rows this one now routes through log_gateway_rejection), and the non-idempotent-refund invariant the new release_reservation call depends on. The A/B structure in test_unresolvable_selector_releases_the_reservation is the right way to write that test — without the tiny-budget control it would pass while guarding nothing.

🤖 Reviewed with Claude Code

The user/key mismatch gate is the one logging gate that fires before check_rate_limit on both request scaffolds, so a valid key could loop mismatched requests and append a usage row per request without ever being throttled (write amplification plus an inflated error count on its own key). It now charges the key's own rate-limit bucket through throttle_early_rejection and skips the row once throttled. The 429 is swallowed rather than raised, so the client still sees the same 403: which error a caller gets must not depend on how the gateway recorded it. Pinned on both scaffolds, including that the refusals really consume the bucket. unpriced_requests in the usage summary is the dashboard's pricing-gap signal ("N unpriced" beside the cost), but every rejection row carries cost=NULL because nothing was spent, so a budget or allow-list incident read as a pricing misconfiguration. Scoped to status="success", which fixes the 402 rows #449 introduced as well as the gates added here. Also: keep the traceback when a rejection row fails to write, since a swallowed exception is the only evidence an operator gets that the writer is unhealthy; and record why release-then-raise in resolve_dispatch_provider is safe, given refund_reservation is not idempotent and the invariant is invisible from that call site. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat(search): add a billed POST /v1/search pass-through Otari exposed web search only as an in-loop tool (otari_web_search), so a caller that wants a direct search request had to hold its own provider key and fell off the gateway's billing path entirely. That is the gap blocking MLPA's move from the LiteLLM proxy onto otari: it proxies an Exa search endpoint today and needs an equivalent surface here to keep search on one gateway. Adds POST /v1/search and POST /v1/search/{search_tool_name}, registered standalone-only alongside the other pass-throughs. Both run the same scaffold as a completion (auth, rate limit, budget reservation, usage log, reconcile or refund) and both log endpoint="/v1/search", so one Activity filter covers every search. Tools are declared under search_tools: in config.yml and validated at startup, so an unsupported provider or a missing API key fails before the first request. Exa is wired end to end. The request and response follow LiteLLM's /v1/search, itself shaped after Perplexity's Search API, so a client migrating off that proxy needs no changes; translation to and from Exa's native shape lives in the backend. Search bills per request rather than per token, so a usage row carries zero tokens and a cost taken from Exa's own reported costDollars.total when present, falling back to a flat per-request rate configured for <provider>:<tool> under the moderations convention. Since the provider meters itself, search is exempt from require_pricing like moderations and audio. find_model_pricing grows a use_defaults flag the route sets to False: the genai-prices fallback also matches on a bare name, so a search tool an operator happened to name after a real model would otherwise inherit that model's per-million-token rate. No dashboard change is needed for search to be visible: the Activity endpoint filter is a free-form pass-through rather than an allowlist, and a zero-token row renders as it should. Credentials come from config.yml only; dashboard CRUD for search-tool credentials is left for a follow-up. Fixes mozilla-ai#400 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(search): close the budget and access-control gaps found in review Four fixes to the new /v1/search surface, all found by an independent review of the PR. reserve_budget was handed the "<provider>:<tool>" pricing key as its `model`. That argument exists only to drive the free-model shortcut, which splits the string through any-llm, so every search logged a swallowed "Failed to determine provider pricing" warning. The latent half is worse: the shortcut calls find_model_pricing *without* the use_defaults=False guard the route sets one frame up, so a future search provider whose name is also an any-llm provider (perplexity and cohere are both plausible) would run the genai-prices bare-name fallback on the tool name and, on a zero-priced match, skip the reservation outright. Pass model=None: the free-model shortcut is a token-pricing concept with no meaning for a per-request-billed tool. The per-key model allow-list was not enforced, so a key restricted to specific models could spend freely on search. Enforcing it needed a second change to be usable at all: model_access._known_prefix accepts only a configured provider instance or an any-llm provider, so writing "exa:exa-search" to a key was rejected with a 400 and a restricted key would have been permanently denied search with no way to grant it. The prefix check now also accepts the configured search tools' providers. build_exa_payload replaced contents.text wholesale when the caller sent max_tokens_per_page, dropping siblings such as a tool's pinned verbosity; it now merges. The test that was supposed to cover the override was vacuous, since 50 tokens times 4 chars equalled the pinned 200. Also constrains `country` to two characters, and stops claiming callers migrating from LiteLLM need no request changes: `query` must be a single string, and the Perplexity filters otari does not model are ignored rather than rejected. Both are now stated in the API reference. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(search): log refused searches, pool the client, and tighten config Six review findings on the new /v1/search surface. A gateway-side refusal wrote no usage row, so an unknown or ambiguous tool name (400) and a tool a key's allowed-models list does not name (403) were invisible in Activity and uncounted by the failure tile. Both now write an error row with a null cost, following the shape the require_pricing 402 rejection already uses in the pass-through scaffold. The row for an unresolved tool carries the requested name (or "unknown") and no provider, since none was resolved. The Exa adapter forced contents.text on every search, so an operator who pinned only highlights still paid Exa's per-page content charge and a caller that wanted ranked URLs had no way to opt out. Pinning contents.text to null (or false) now suppresses page content, and drops an otherwise empty contents block. The opt-out is the operator's, so it holds even for a request carrying max_tokens_per_page. Searches shared no HTTP client: a client per call paid a fresh TCP and TLS handshake and pooled nothing, which is the wrong shape for the sustained traffic this endpoint exists to serve. Provider calls now go through one pooled client for the process, with the per-tool timeout passed on the request; shutdown closes it. search_tools validation accepted a negative timeout, which only blew up inside httpx at request time, and a zero, which was silently swapped for the 30s default when the tool was resolved. Both are now rejected at load. The route docstrings claimed an API key plus a user field bills the named user, and that a key without one bills a shared "default" user. Neither is true: spend always binds to the key's own user, and a mismatch is rejected or ignored per reject_user_mismatch. Corrected, so the OpenAPI description and the Postman collection stop repeating it. Finally, with no flat per-request rate configured for <provider>:<tool> the reservation is $0, so a user just under their cap can overshoot by a search and concurrent searches cannot see each other's holds. Spend stays truthful (the provider's reported charge is reconciled onto the usage row), so this is left as a documented recommendation plus a startup warning naming every unpriced tool, rather than a new 402 that would reject traffic the gateway can bill precisely. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(search): route refused searches through the shared rejection writer mozilla-ai#472 landed a shared writer for gateway-side refusals after this branch built its own rows, and the two disagreed on one column that matters: log_gateway_rejection pins counts_toward_budget True, while these rows carried `not budget_exempt`. A key flagged exclude_from_budget would therefore have written a refusal row the dashboard classifies as imported usage and offers for bulk delete and set-price, which must never happen to a row the gateway wrote itself. The two search refusals (unknown or ambiguous tool, allow-list denial) now go through log_gateway_rejection, which also makes the write best-effort: a sick log writer can no longer turn a clean 400 or 403 into a 500. Row shape is otherwise unchanged (status=error, cost NULL, latency, endpoint /v1/search), and the served rows keep their own builder, since those carry a cost and do honor exclude_from_budget. Tests assert counts_toward_budget on both refusal rows, plus a new case for an exempt key hitting the allow-list gate. They read /v1/usage rather than /v1/users/{id}/usage, which does not expose the column. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

njbrake temporarily deployed to integration-tests August 2, 2026 17:07 — with GitHub Actions Inactive

njbrake temporarily deployed to integration-tests August 2, 2026 17:20 — with GitHub Actions Inactive

njbrake marked this pull request as ready for review August 2, 2026 20:02

coderabbitai Bot requested a review from khaledosman August 2, 2026 20:03

coderabbitai Bot reviewed Aug 2, 2026

View reviewed changes

Comment thread src/gateway/api/routes/_pipeline.py

Comment thread tests/integration/test_gateway_rejection_logging.py

njbrake temporarily deployed to integration-tests August 2, 2026 21:11 — with GitHub Actions Inactive

khaledosman requested a review from Copilot August 3, 2026 08:07

Copilot started reviewing on behalf of khaledosman August 3, 2026 08:07 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Comment thread src/gateway/api/routes/_pipeline.py Outdated

khaledosman mentioned this pull request Aug 3, 2026

feat(usage): record a status code on usage logs so failures can be classified #470

Merged

14 tasks

khaledosman approved these changes Aug 3, 2026

View reviewed changes

Comment thread src/gateway/api/routes/_pipeline.py

Comment thread src/gateway/api/routes/_passthrough.py

Comment thread src/gateway/api/routes/_pipeline.py

Comment thread src/gateway/api/routes/_pipeline.py

khaledosman mentioned this pull request Aug 3, 2026

feat(search): add a billed POST /v1/search pass-through #473

Merged

14 tasks

njbrake temporarily deployed to integration-tests August 3, 2026 10:43 — with GitHub Actions Inactive

njbrake merged commit 1a55d9f into main Aug 3, 2026
7 checks passed

njbrake deleted the fix/465-log-rejections branch August 3, 2026 10:48

njbrake mentioned this pull request Aug 3, 2026

feat(usage): break spend down by session, endpoint, and provider #469

Merged

14 tasks

coderabbitai Bot mentioned this pull request Aug 3, 2026

feat(dashboard): browsable activity log and rebuilt usage analytics #474

Merged

14 tasks

coderabbitai Bot mentioned this pull request Aug 10, 2026

feat(otlp): capture coding-agent behavioral events into agent_telemetry #548

Merged

14 tasks

Navigation Menu

Uh oh!

feat(observability): log the remaining gateway-side rejections - #472

feat(observability): log the remaining gateway-side rejections#472
njbrake merged 4 commits into
mainfrom
fix/465-log-rejections

njbrake commented Aug 2, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 2, 2026 •

edited

Loading

Review limit reached

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

khaledosman left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

njbrake commented Aug 2, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PR Type

Relevant issues

Checklist

Test notes

AI Usage

Summary

Technical notes

Uh oh!

coderabbitai Bot commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

khaledosman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

njbrake commented Aug 2, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 2, 2026 •

edited

Loading