fix(api): machine-readable code for plan-gated model errors#301
fix(api): machine-readable code for plan-gated model errors#301Evrard-Nil wants to merge 5 commits into
Conversation
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.
There was a problem hiding this comment.
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.
| 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()) | ||
| } |
There was a problem hiding this comment.
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())
}
Review: machine-readable code for plan-gated model errorsFocused, low-risk change. Reviewed for correctness, backward compatibility, and production safety. Findings
Minor (non-blocking)
No critical issues found. ✅ (approved) |
There was a problem hiding this comment.
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::ModelNotAllowedInPlanoccurs, return a 403 JSON body that includes a newcodefield (model_not_allowed_in_plan). - Include the
modelandplanvalues in the response while keeping the existingerrormessage unchanged for backward compatibility.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| StatusCode::FORBIDDEN, | ||
| Json(serde_json::json!({ | ||
| "error": error, | ||
| "code": "model_not_allowed_in_plan", | ||
| "model": model, | ||
| "plan": plan, | ||
| })), |
PierreLeGuen
left a comment
There was a problem hiding this comment.
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_testsonly check status and/or the legacyerrorstring, so deletingcode,model, orplanwould still pass. Please extend at least one blocked allowlist case to assertcode == "model_not_allowed_in_plan"plus the echoedmodelandplanvalues. -
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 exposeserror. 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).
|
Thanks for the review — addressed in the latest commit:
|
PierreLeGuen
left a comment
There was a problem hiding this comment.
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 sharedenforce_model_accesspath now returns a stable plan-gated 403 payload withcode,model, andplan, but the affected endpoint OpenAPI annotations still declare the 403 body asErrorResponse, whose schema only haserror(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 -- --checkpassedcargo clippy -p api --tests --features test -- -D warningspassedcargo test -p api --features test test_responses_endpoint_respects_model_allowlistcompiled, then failed before assertions because local Postgres was unavailable (Connection refusedduring 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.
|
Thanks @PierreLeGuen — both points from your reviews are addressed.
Local checks on the new head: |
PierreLeGuen
left a comment
There was a problem hiding this comment.
Requesting changes for one API-contract issue.
crates/api/src/routes/api.rs:2308(also:3895,:4047,:4242): these annotations now document every 403 asModelNotAllowedErrorResponse, whosecode,model, andplanfields are required. But these same endpoints callensure_user_not_bannedbefore model access, and that path still returnsErrorResponsewith onlyerror(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 asErrorResponseand 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.
|
Good catch @PierreLeGuen — fixed in the latest commit. The 403 is now modeled as a Local: |
PierreLeGuen
left a comment
There was a problem hiding this comment.
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 apipassed.cargo test --lib --bins --features testpassed.cargo test --test model_allowlist_tests --features testcompiled but could not run in this environment because local PostgreSQL was unavailable; all tests failed during migrations atcrates/api/tests/common.rs:120with connection refused.
Code ReviewVerdict: Approve Clean, minimal change. Adding Observations
Test assertions extended correctly ( NitThe |
lloydmak99
left a comment
There was a problem hiding this comment.
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.
Problem
When a user in Private Chat picks a model that isn't in their subscription plan's
allowed_models,enforce_model_accessreturns:The body carries only a human-readable
errorstring, 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(plusmodelandplan) to the 403 body on theModelNotAllowedInPlanpath:{ "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" }erroris byte-for-byte unchanged → fully backward compatible (existing tests assert onlybody["error"]).enforce_model_access), which covers/v1/chat/completions,/v1/responses, and image endpoints.Frontend counterpart
nearai/private-chat#353 consumes
codeto show an "Upgrade to access this model" dialog instead of the generic toast.Testing
cargo check -p api✅model_allowlist_testsassertbody["error"]only → unaffected.