Skip to content

fix(api): machine-readable code for plan-gated model errors#301

Open
Evrard-Nil wants to merge 5 commits into
mainfrom
fix/model-not-allowed-error-code
Open

fix(api): machine-readable code for plan-gated model errors#301
Evrard-Nil wants to merge 5 commits into
mainfrom
fix/model-not-allowed-error-code

Conversation

@Evrard-Nil

@Evrard-Nil Evrard-Nil commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Problem

When a user in Private Chat picks a model that isn't in their subscription plan's allowed_models, enforce_model_access returns:

403 {"error":"Model 'deepseek-ai/DeepSeek-V4-Flash' is not available in your plan 'starter'"}

The body carries only a human-readable error string, so clients can't distinguish a gated-model rejection from a generic upstream failure. The private-chat frontend therefore renders a misleading "Model … failed to respond" toast (it should prompt the user to upgrade).

Change

Add a stable, machine-readable code (plus model and plan) to the 403 body on the ModelNotAllowedInPlan path:

{
  "error": "Model 'deepseek-ai/DeepSeek-V4-Flash' is not available in your plan 'starter'",
  "code": "model_not_allowed_in_plan",
  "model": "deepseek-ai/DeepSeek-V4-Flash",
  "plan": "starter"
}
  • error is byte-for-byte unchanged → fully backward compatible (existing tests assert only body["error"]).
  • One call site (enforce_model_access), which covers /v1/chat/completions, /v1/responses, and image endpoints.

Frontend counterpart

nearai/private-chat#353 consumes code to show an "Upgrade to access this model" dialog instead of the generic toast.

Testing

  • cargo check -p api
  • Existing model_allowlist_tests assert body["error"] only → unaffected.

When a user requests a model not in their subscription plan's
allowed_models, enforce_model_access returned a 403 with only a
human-readable `error` string. Clients (private-chat) cannot distinguish
this from a generic upstream failure, so they render a misleading
"model failed to respond" message.

Add a stable `code: "model_not_allowed_in_plan"` (plus `model` and
`plan`) to the 403 body so clients can detect the gated-model case and
prompt the user to upgrade. The `error` string is unchanged for
backward compatibility.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the enforce_model_access route to return a structured JSON response containing a stable error code, model, and plan when a model is not allowed in a user's plan. The reviewer suggested using Rust's subpattern binding (ref err @ ...) to avoid unnecessary clones of the model and plan fields when generating the error string and serializing the response.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread crates/api/src/routes/api.rs Outdated
Comment on lines +3339 to +3360
Err(SubscriptionError::ModelNotAllowedInPlan { model, plan }) => {
// Return a stable, machine-readable `code` alongside the message so
// clients can distinguish "model is gated behind a higher plan"
// from a generic upstream failure, and prompt the user to upgrade
// instead of showing a misleading "model failed to respond" error.
// `error` is kept byte-for-byte identical for backward compatibility.
let error = SubscriptionError::ModelNotAllowedInPlan {
model: model.clone(),
plan: plan.clone(),
}
.to_string();
Err((
StatusCode::FORBIDDEN,
Json(serde_json::json!({
"error": error,
"code": "model_not_allowed_in_plan",
"model": model,
"plan": plan,
})),
)
.into_response())
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Instead of cloning model and plan to reconstruct a temporary SubscriptionError::ModelNotAllowedInPlan variant just to call .to_string(), you can use Rust's subpattern binding (ref err @ ...) to bind the error by reference. This allows you to call err.to_string() directly and serialize the borrowed model and plan fields in the JSON macro, completely avoiding any unnecessary allocations and clones.

        Err(ref err @ SubscriptionError::ModelNotAllowedInPlan { ref model, ref plan }) => {
            // Return a stable, machine-readable `code` alongside the message so
            // clients can distinguish "model is gated behind a higher plan"
            // from a generic upstream failure, and prompt the user to upgrade
            // instead of showing a misleading "model failed to respond" error.
            // `error` is kept byte-for-byte identical for backward compatibility.
            let error = err.to_string();
            Err((
                StatusCode::FORBIDDEN,
                Json(serde_json::json!({
                    "error": error,
                    "code": "model_not_allowed_in_plan",
                    "model": model,
                    "plan": plan,
                })),
            )
                .into_response())
        }

@claude

claude Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review: machine-readable code for plan-gated model errors

Focused, low-risk change. Reviewed for correctness, backward compatibility, and production safety.

Findings

  • Backward compatibility — good. The error field is preserved byte-for-byte (the enum is rebuilt and to_string() is re-invoked, guaranteeing the message matches the Display impl in subscription/ports.rs). The status code stays 403 FORBIDDEN. New fields (code, model, plan) are purely additive, so existing clients that only read error are unaffected. This is safe during rolling updates.
  • No privacy concern. Model/plan identifiers are not customer content, and nothing new is logged.
  • Logic is correct — the response shape change is isolated to the ModelNotAllowedInPlan arm; the generic error arm is untouched.

Minor (non-blocking)

  • The two .clone() calls plus rebuilding the enum just to call to_string() could be replaced with a direct format!("Model '{model}' is not available in your plan '{plan}'", ...), avoiding the allocations. However, the current approach intentionally reuses the Display impl so the message can't drift from the canonical one — a reasonable defensive trade-off on a cold error path. Leaving as-is is fine.

No critical issues found.

✅ (approved)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the API’s plan-based model access enforcement to return a stable, machine-readable error code when a requested model is not allowed in the caller’s subscription plan, enabling clients to reliably detect “upgrade required” scenarios.

Changes:

  • When SubscriptionError::ModelNotAllowedInPlan occurs, return a 403 JSON body that includes a new code field (model_not_allowed_in_plan).
  • Include the model and plan values in the response while keeping the existing error message unchanged for backward compatibility.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/api/src/routes/api.rs Outdated
Comment on lines +3351 to +3357
StatusCode::FORBIDDEN,
Json(serde_json::json!({
"error": error,
"code": "model_not_allowed_in_plan",
"model": model,
"plan": plan,
})),

@PierreLeGuen PierreLeGuen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two issues to address before this is ready:

  • crates/api/src/routes/api.rs:3352 adds the new machine-readable payload, but no test asserts the actual new contract. Existing model_allowlist_tests only check status and/or the legacy error string, so deleting code, model, or plan would still pass. Please extend at least one blocked allowlist case to assert code == "model_not_allowed_in_plan" plus the echoed model and plan values.

  • crates/api/src/routes/api.rs:2289, crates/api/src/routes/api.rs:3875, crates/api/src/routes/api.rs:4027, and crates/api/src/routes/api.rs:4222 still document the relevant 403s as ErrorResponse, which only exposes error. Since this PR is creating a stable client-facing contract consumed by another frontend PR, the OpenAPI schema/docs should describe the extra fields for these plan-gated 403 responses.

Local checks: cargo check -p api passed. Focused cargo test -p api --test model_allowlist_tests test_subscription_plan_allowed_models_blocks_unlisted_model -- --exact --nocapture built but could not run locally because the test DB connection was refused during migrations. GitHub Test Suite was still pending when reviewed.

- Bind the error by reference (ref err @ ...) instead of cloning model/plan
  to rebuild the variant just for to_string() (gemini-code-assist).
- Extend test_responses_endpoint_respects_model_allowlist to assert the new
  stable contract: code == "model_not_allowed_in_plan", and model/plan
  present (Copilot).
@Evrard-Nil

Copy link
Copy Markdown
Contributor Author

Thanks for the review — addressed in the latest commit:

  • gemini (avoid clones): switched to a ref err @ SubscriptionError::ModelNotAllowedInPlan { ref model, ref plan } subpattern; error = err.to_string() and the borrowed model/plan are serialized directly — no clones.
  • Copilot (test coverage): extended test_responses_endpoint_respects_model_allowlist to assert the new contract — code == "model_not_allowed_in_plan", plus model and plan present.

cargo clippy -p api --tests clean.

@Evrard-Nil Evrard-Nil requested a review from PierreLeGuen June 8, 2026 15:28

@PierreLeGuen PierreLeGuen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for one API contract gap.

  • crates/api/src/routes/api.rs:2289, crates/api/src/routes/api.rs:3871, crates/api/src/routes/api.rs:4023, crates/api/src/routes/api.rs:4218: the shared enforce_model_access path now returns a stable plan-gated 403 payload with code, model, and plan, but the affected endpoint OpenAPI annotations still declare the 403 body as ErrorResponse, whose schema only has error (crates/api/src/routes/api.rs:234). Generated clients and Swagger docs will not see the new machine-readable contract. Please add/document a schema for this response shape, or use an appropriate alternative/oneOf response for the plan-gated 403s, so the published API contract matches runtime behavior.

Checks run locally on PR head 4ac2438:

  • cargo fmt --all -- --check passed
  • cargo clippy -p api --tests --features test -- -D warnings passed
  • cargo test -p api --features test test_responses_endpoint_respects_model_allowlist compiled, then failed before assertions because local Postgres was unavailable (Connection refused during migrations)

PierreLeGuen: the shared enforce_model_access 403 returned an ad-hoc JSON
body with code/model/plan, but the endpoint annotations still declared the
403 as ErrorResponse (error-only), so generated clients/Swagger didn't see
the new machine-readable contract.

- Add ModelNotAllowedErrorResponse (ToSchema): error + code + model + plan,
  documented as the plan-gated 403 shape (superset of ErrorResponse).
- Return it (typed) from enforce_model_access instead of serde_json::json!.
- Register it in the OpenAPI components and point the 403 on /v1/responses,
  /v1/chat/completions, /v1/images/generations, /v1/images/edits at it.
@Evrard-Nil

Copy link
Copy Markdown
Contributor Author

Thanks @PierreLeGuen — both points from your reviews are addressed.

  1. Test contract (first review): test_responses_endpoint_respects_model_allowlist now asserts code == "model_not_allowed_in_plan" plus the echoed model and plan, so dropping any of those fields fails the suite.

  2. OpenAPI contract (latest review): replaced the ad-hoc serde_json::json! body with a typed ModelNotAllowedErrorResponse (error + code + model + plan), registered it in the OpenAPI components, and pointed the plan-gated 403 on /v1/responses, /v1/chat/completions, /v1/images/generations, and /v1/images/edits at it (description notes the code=model_not_allowed_in_plan discriminator; other 403s on these routes populate only error). So the published schema now matches runtime.

Local checks on the new head: cargo fmt --all -- --check, cargo clippy -p api --tests --features test -- -D warnings, build all clean.

@Evrard-Nil Evrard-Nil requested a review from PierreLeGuen June 8, 2026 18:50

@PierreLeGuen PierreLeGuen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for one API-contract issue.

  • crates/api/src/routes/api.rs:2308 (also :3895, :4047, :4242): these annotations now document every 403 as ModelNotAllowedErrorResponse, whose code, model, and plan fields are required. But these same endpoints call ensure_user_not_banned before model access, and that path still returns ErrorResponse with only error (crates/api/src/routes/api.rs:2830). Generated clients/Swagger will now reject or misrepresent valid banned-user 403s. Please model the 403 response as a union/oneOf, or keep the endpoint 403 as ErrorResponse and separately document the plan-gated shape, so both actual 403 bodies are in the contract.

Checks: cargo fmt --check passed. cargo test -p api test_responses_endpoint_respects_model_allowlist --test model_allowlist_tests compiled but could not run locally because migrations could not connect to a database (Connection refused). GitHub Test Suite was still pending when reviewed.

PierreLeGuen: declaring every 403 as ModelNotAllowedErrorResponse (with
required code/model/plan) misrepresented the banned-user 403 from
ensure_user_not_banned, which returns ErrorResponse (error only). Generated
clients would reject/misrepresent valid banned 403s.

Introduce ForbiddenErrorResponse, an untagged oneOf of
ModelNotAllowedErrorResponse | ErrorResponse, and point the 403 on
/v1/responses, /v1/chat/completions, /v1/images/generations,
/v1/images/edits at it. Both real 403 bodies are now in the contract.
@Evrard-Nil

Copy link
Copy Markdown
Contributor Author

Good catch @PierreLeGuen — fixed in the latest commit.

The 403 is now modeled as a oneOf via a new ForbiddenErrorResponse (untagged enum of ModelNotAllowedErrorResponse | ErrorResponse), and the 403 on /v1/responses, /v1/chat/completions, /v1/images/generations, and /v1/images/edits points at it. So both real bodies are in the contract: the plan-gated shape (error + code + model + plan) and the generic/banned-user shape (error only). Description updated to spell out both.

Local: cargo fmt --all -- --check + cargo clippy -p api --tests --features test -- -D warnings clean.

@Evrard-Nil Evrard-Nil requested a review from PierreLeGuen June 8, 2026 19:06

@PierreLeGuen PierreLeGuen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the runtime change in enforce_model_access, the updated OpenAPI 403 schema, and the affected callers for /v1/responses, /v1/chat/completions, /v1/images/generations, and /v1/images/edits. I did not find substantive issues.

Local checks:

  • cargo check -p api passed.
  • cargo test --lib --bins --features test passed.
  • cargo test --test model_allowlist_tests --features test compiled but could not run in this environment because local PostgreSQL was unavailable; all tests failed during migrations at crates/api/tests/common.rs:120 with connection refused.

@lloydmak99

Copy link
Copy Markdown

Code Review

Verdict: Approve

Clean, minimal change. Adding code, model, and plan to the 403 body gives clients a stable, machine-readable discriminator without breaking existing consumers (the error string is byte-for-byte identical).

Observations

ref err @ binding is correct Rust — binding the whole SubscriptionError::ModelNotAllowedInPlan to err while also destructuring model and plan avoids cloning for the to_string() call. Good.

ForbiddenErrorResponse as a oneOf union is the right OpenAPI shape — documenting both 403 bodies (gated model vs generic forbidden) as a union type rather than just listing the richer shape lets generated clients handle both correctly. #[allow(dead_code)] is needed since the type is only referenced via macros.

Test assertions extended correctly (model_allowlist_tests.rs:528-548)
The three new assertions (code, model, plan) cover the exact contract the frontend depends on. The plan value "basic" should match whatever the test fixture sets up — confirm this stays in sync if fixture plan names change.

Nit

The _ catch-all in the enforce_model_access match arm still logs tracing::error! for non-gated 403s (banned users etc.) — that's appropriate and unchanged.

@lloydmak99 lloydmak99 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — approve. Clean, backward-compatible addition of code/model/plan fields to the 403 body. Test assertions cover the exact frontend contract. ForbiddenErrorResponse oneOf union is the right OpenAPI shape.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants