Repository navigation
feat(vanity-gateway): support decision model routes /v1/decisions and /v1/systemone - #2983
Conversation
Walkthrough
Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 27.03% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title uses the required Conventional Commits format, includes the required scope for a customer-impacting feature, and accurately describes support for the decision model routes. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 27.03% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 37 functions across 17 files. (4 skipped: 4 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Add v2config.openai.decisions and v2config.typesafeSystemOne as model mapping sections that accept functionType LLM. Request body sampling stays unsupported for them, so promptCacheKey and firstMessageHash are rejected. The values schema, CI values and chart-render checks cover both sections. Relates to #2922 Signed-off-by: Sachin Prabhakar <saprabhakar@nvidia.com>
Register both routes behind the shared request size limit and resolve the vanity model name in each route's own section. The body is forwarded unchanged and stream is ignored, so Accept is not set to event-stream. Gateway-generated errors now use the shape of the route the client called: the OpenAI shape on /v1/decisions and the System One shape on /v1/systemone, with request_id equal to the X-Request-Id response header. One writeNativeError helper covers body errors, unknown model (now JSON instead of plain text), offline and end-of-life, client cancel and proxy failure. The protocol is stored in the request context at route entry because usePexec rewrites the URL path before proxying. Backend errors still pass through unchanged. Relates to #2922 Signed-off-by: Sachin Prabhakar <saprabhakar@nvidia.com>
Include models enabled on /v1/decisions or /v1/systemone in the shared
model list and in /v1/models/{model}, with supported_endpoints naming
the routes each model serves. Expiry and offline state follow the
decision mappings too. Decision models stay out of the Anthropic list.
Span endpoint.type reports decisions and systemone for both the direct
and the LLM gateway path.
Relates to #2922
Signed-off-by: Sachin Prabhakar <saprabhakar@nvidia.com>
Describe the beta /v1/decisions and /v1/systemone routes, their mapping sections, the example mapping, error shapes and the usePexec caveat: a usePexec function reaches only its default path, so a function serving both routes should set usePexec to false. Bodies pass through unchanged. Relates to #2922 Signed-off-by: Sachin Prabhakar <saprabhakar@nvidia.com>
The tooManyRequestsMessage customization rewrote error.message in backend 429 bodies on the decision routes. Skip it there so these bodies pass through byte for byte. Relates to #2922 Signed-off-by: Sachin Prabhakar <saprabhakar@nvidia.com>
088ad15 to
2044b10
Compare
e7bcd4d to
a22b866
Compare
|
@coderabbitai review |
✅ Action performed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments: 1
src/invocation-plane-services/vanity-gateway/gateway/decision_routes_test.go-398-398: 1
398-398:Minor · Maintainability & Code Quality · Quick win
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Use
httptest.NewRequestWithContextto fix thenoctxlint failure.
golangci-lint(noctx) rejectshttptest.NewRequeston this line. This new file can fail the lint step. The file already importscontext, so the fix needs no new import.🔧 Proposed fix
Source: Linters/SAST tools
-
-
- Source mode: Auto
- Repository: NVIDIA/nvcf
- Branch: sachin-prabhakar/decision-routes-3-vanity
- File: .coderabbit.yaml
- Commit: a22b866
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
f86f0a5f-8b83-4eb0-9c5c-79e5e49556c6
-
deploy/helm/vanity-gateway/README.mddeploy/helm/vanity-gateway/helm-nvcf-vanity-gateway/values.schema.jsondeploy/helm/vanity-gateway/tests/chart-render/verify-llm-gateway-routing.shdeploy/helm/vanity-gateway/tests/chart-render/verify-shadow-schema.shsrc/invocation-plane-services/vanity-gateway/README.mdsrc/invocation-plane-services/vanity-gateway/gateway/BUILD.bazelsrc/invocation-plane-services/vanity-gateway/gateway/decision_routes_test.gosrc/invocation-plane-services/vanity-gateway/gateway/h2.gosrc/invocation-plane-services/vanity-gateway/gateway/llm_gateway_director.gosrc/invocation-plane-services/vanity-gateway/gateway/llm_gateway_director_test.gosrc/invocation-plane-services/vanity-gateway/gateway/models_protocol_test.gosrc/invocation-plane-services/vanity-gateway/gateway/native_errors.gosrc/invocation-plane-services/vanity-gateway/gateway/openai_director.gosrc/invocation-plane-services/vanity-gateway/gateway/request_id_test.gosrc/invocation-plane-services/vanity-gateway/gateway/tracing_attrs.gosrc/invocation-plane-services/vanity-gateway/gateway/vanity_director.gosrc/invocation-plane-services/vanity-gateway/gateway_config/gateway_config.gosrc/invocation-plane-services/vanity-gateway/gateway_config/gateway_config_test.gosrc/invocation-plane-services/vanity-gateway/gateway_config/model_info.gosrc/invocation-plane-services/vanity-gateway/gateway_config/model_info_test.gotools/ci/helm-validate-values/vanity-gateway.yaml
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
-
FamousDirector
left a comment
FamousDirector
left a comment
There was a problem hiding this comment.
No blockers found. The routing, error-shape, usePexec, stream guard and 429 passthrough tests exercise the new behavior, and go test ./..., both chart-render scripts and helm lint on the CI values pass at this head. Comments below are minor issues and nits, mostly around README claims and error-code consistency with the gateway in #2981.
|
|
||
| The gateway reads only `model` from the body and never translates between the | ||
| two formats, so the backend must implement each route it is configured for. | ||
| Request and response bodies pass through unchanged, including backend 4xx and |
There was a problem hiding this comment.
Minor: this holds on the direct path but not for functionType: LLM. proxyToLLMGateway rewrites the model through rewriteShadowRequestModel, which decodes the body into a map and re-encodes it. That sorts top-level keys, HTML-escapes strings, compacts whitespace and drops duplicate keys. A quick test shows a body with "input":"a<b && c>d" arriving at the gateway as "a\u003cb \u0026\u0026 c\u003ed" with keys reordered. The JSON is equivalent, but byte or digest comparisons end to end will not match. Suggest narrowing the claim ("unchanged apart from the model rewrite and JSON re-serialization") or rewriting only the model member in place. The gateway in #2981 has the same re-marshal behavior; raised there too.
| // Decision routes are non-streaming: stream is ignored and the body is | ||
| // forwarded as sent. | ||
| if body.Stream && !isDecisionRoute(request) { |
There was a problem hiding this comment.
Minor: the comment here and the README ("reads only model", "stream is ignored") say the body is forwarded as sent, but extractOpenAIJSONBody still decodes stream into a bool. A quick test shows POST /v1/systemone with {"model":"...","stream":"no"} returns 400 invalid_json and never reaches the backend, while the gateway in #2981 binds only model and would pass it. Suggest decoding only model on decision routes (or ignoring a type error on stream) and adding a test with a non-bool stream.
| func writeFunctionStatusError(writer http.ResponseWriter, request *http.Request, offlineMessage string, eol time.Time, name string) bool { | ||
| if offlineMessage != "" { | ||
| markGatewayRejected(writer, request) | ||
| if requestProtocol(request) != nativeProtocolNone { |
There was a problem hiding this comment.
Minor: requestProtocol falls back to URL.Path, and writeFunctionStatusError is also reached from ServeExec for plain vanity host routes. A vanity path entry whose path is /v1/messages, /v1/decisions or /v1/systemone now gets an Anthropic, OpenAI or System One shaped 503 or 410 instead of the previous application/problem+json, and the EOL message loses its date. That is a behavior change on the generic vanity surface not mentioned in the description. Suggest keying this only on the protocol stored by withNativeProtocol, or documenting the change and adding a test.
| if isModelExpired(eol) { | ||
| if requestProtocol(request) != nativeProtocolNone { | ||
| markGatewayRejected(writer, request) | ||
| writeNativeError(writer, request, http.StatusGone, errKindNotFound, "model has reached end of life") |
There was a problem hiding this comment.
Minor: System One codes differ from the gateway in #2981 for the same condition. Here 410 maps to not_found and 499 to internal_error (line 159); the gateway's systemOneErrorCode maps both to invalid_request. A body of {"model":1} gets invalid_json here (asserted in decision_routes_test.go) but invalid_request from the gateway, which treats a type error as invalid_request. The /v1/decisions error code convention also differs between the gateway and Vanity (raised on #2981). Suggest one shared status-to-code mapping for both services, with the tests aligned to it.
| func requestReadFailure(protocol nativeProtocol, err error) (int, string, string) { | ||
| status, kind := http.StatusInternalServerError, errKindInternal |
There was a problem hiding this comment.
Minor: client mistakes on decision routes surface as gateway or upstream failures. Content-Type multipart/form-data without a boundary falls through to 500 internal_error here. A multipart body to an LLM decision model passes extraction, then fails the JSON rewrite in proxyToLLMGateway and returns 502 provider_unavailable, recording the proxy_error outcome and a warn log, which inflates upstream error metrics. A quick test shows both. Suggest rejecting non-JSON content types on decision routes with 400 (or 415) invalid_request.
| func (d *ModelDirector) ServeDecisions(writer http.ResponseWriter, request *http.Request) { | ||
| request = withNativeProtocol(request, nativeProtocolDecisions) |
There was a problem hiding this comment.
Nit: the stored protocol fixes the error shape after the usePexec path rewrite for decisions and System One, but ServeMessages does not call withNativeProtocol. A quick test shows an anthropicMessages model with usePexec: true and a down backend returns 502 application/problem+json rather than the Anthropic shape. This predates the PR, but the fix is one line: request = withNativeProtocol(request, nativeProtocolMessages) in ServeMessages.
| endpointType = traceAttrValueEndpointAnthropic | ||
| } | ||
| span.SetAttributes(traceAttrEndpointType.String(endpointType)) | ||
| span.SetAttributes(traceAttrEndpointType.String(endpointTypeForPath(request.URL.Path, traceAttrValueEndpointLLMGateway))) |
There was a problem hiding this comment.
Nit: for decision traffic routed through the LLM Gateway, endpoint.type is now decisions or systemone instead of llm_gateway, so spans can no longer separate gateway-routed decision traffic from direct traffic. Messages already behaves this way. Consider a separate attribute for the upstream kind so dashboards can split on it.
| - `POST /v1/systemone` serves the TypeSafe System One format. Map models under | ||
| `v2config.typesafeSystemOne`, a top-level sibling of `anthropicMessages`. | ||
|
|
||
| Entries use the same fields and validation as `openai.chatCompletions`. A model |
There was a problem hiding this comment.
Nit: "the same fields and validation as openai.chatCompletions" conflicts with the later paragraph saying promptCacheKey, firstMessageHash and promptCacheKeyHeaders are rejected in the decision sections. Suggest "the same fields as openai.chatCompletions; shadow sampling is limited to random and perBearerKey".
Why
Decision models answer typed questions (predicate or noul, choice, score, box) with probabilities instead of generated text. LiteLLM exposes them on
POST /v1/decisions(OpenAI Decisions format) andPOST /v1/systemone(System One format). The LLM stack rejected both routes. This work adds them as pure passthrough, non-streaming routes routed bymodel, with no translation between the formats and no engine-specific code. It is split into four stacked pull requests, as requested in review of the single pull request: #2965 (now closed).Stack, in merge order: 1 llm-api-gateway (#2981), 2 Stargate, with its test-only mock server (#2982), 3 vanity-gateway (#2983), 4 sample backend, BDD and shared docs (#2984).
This is part 3 of 4: vanity-gateway. Vanity maps a vanity model name to a function and forwards either directly or through the LLM gateway.
Assumptions to confirm
v2config.openai.decisionsor the top-levelv2config.typesafeSystemOne. A model not listed gets a 404 in the route's native error shape.usePexecrewrites the URL path. Codes follow the System One docs table;/v1/decisionsuses the OpenAI shape. An end-of-life 410 maps tonot_foundand a client cancel (499) tointernal_errorfor System One.stream: truedoes not setAccept: text/event-streamfor these routes.usePexec. WithusePexec: truethe invocation service calls the function's default path, so one function serving both routes should useusePexec: false. The docs say so. The example in feat(vanity-gateway): support decision model routes /v1/decisions and /v1/systemone #2922 usedusePexec: truefor both.shadow_guard.go) is unchanged for every route, as it is for/v1/messages.X-Request-Idheader is always Vanity's own (existing policy), so a relayed backend error whose body carries its ownrequest_idhas a different header ID.What changed
/v1/modelsand/v1/models/{model}list decision-enabled models withsupported_endpoints.Customer Release Notes
Added beta support for
/v1/decisionsand/v1/systemonein Vanity Gateway.Plan Summary
Not applicable.
Usage
Testing
Ran, all passing:
go build,go vet,go test -count=1 ./..., race tests for the middleware and gateway packages, the chart-render and shadow-schema scripts, andhelm lintandhelm templateon the CI values. Mutation checks confirm the stream guard and the stored-protocol tests fail when the code is broken.Not run: the Vanity BDD scenario. The published Vanity chart (0.10.0) does not know the new mappings, so it cannot pass against a released chart until a chart containing this schema is released. The direct path through the invocation service is unproven until then.
Notes
References
Relates to #2922
Related Pull Requests
Part 3 of 4. Builds on #2982. The Vanity BDD scenario is in #2984.
Dependencies
None.
Summary by CodeRabbit
POST /v1/decisions) and TypeSafe System One (POST /v1/systemone) routes, with separate model mappings and model discovery.