Repository navigation
feat(router): support Decisions on the regular HTTP router - #2863
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (11)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughAdds a protected ChangesDecisions API
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant DecisionsClient
participant GatewayServer
participant Gateway
participant HttpRouter
participant SglangDecisionAdapter
participant SGLangWorker
DecisionsClient->>GatewayServer: POST /v1/decisions
GatewayServer->>Gateway: Route validated Decisions request
Gateway->>HttpRouter: Dispatch to selected HTTP router
HttpRouter->>SglangDecisionAdapter: Translate request
SglangDecisionAdapter->>SGLangWorker: Send request to /v1/systemone
SGLangWorker->>SglangDecisionAdapter: Return scoring response
SglangDecisionAdapter->>HttpRouter: Convert Decisions response
HttpRouter->>DecisionsClient: Return response
Merge Risk: ⚪ Minimal · up to This change adds a /v1/decisions endpoint for regular HTTP workers, and SGLang requests are translated and validated. No concrete defect was found that should block merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
grpc
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let code = value | ||
| .as_ref() | ||
| .and_then(|value| value.pointer("/error/code")) | ||
| .and_then(Value::as_str) | ||
| .unwrap_or("upstream_decisions_error"); | ||
| let mut response = error::create_error(parts.status, code, message); |
There was a problem hiding this comment.
🟡 Nit: This passes the backend's error.code into error::create_error, which stores it in the GatewayErrorCode extension and the X-SMG-Error-Code header. Line 276 then copies that extension onto the outgoing response, so record_router_upstream_response in route_typed_request uses a string the worker chose as a Prometheus label. crates/external_router/src/error.rs:27-30 says only gateway-minted codes should get this extension, and the extract_error_code_ignores_header_without_gateway_extension test protects that rule.
What goes wrong:
- A worker that returns many distinct
error.codestrings (for example, codes with request IDs in them) adds a new label value for each one. - Every upstream Decisions error that doesn't have a string code is now labelled as the gateway error
upstream_decisions_error. Other HTTP routes leave the label empty for upstream errors.
Suggested fix: when the body already has /error/message, build the response from the upstream JSON and status without calling create_error. For the fallback, wrap the detail/text message with a fixed code. Never take the code from the worker.
| } | ||
| // Structured text evidence is rendered by System One as ordered JSON, | ||
| // preserving message/part boundaries and roles. Images are rejected above. | ||
| // safety_identifier is consumed at the gateway boundary, not a backend field. |
There was a problem hiding this comment.
🟡 Nit: Nothing in the gateway reads safety_identifier. The only other reference in model_gateway/src is the gRPC Responses path. So on SGLang workers the field is silently dropped, while native workers receive it. That conflicts with how this adapter handles every other unsupported field ("rejects ... instead of dropping them"). Either reword the comment to say the field is intentionally dropped for SGLang because System One has no equivalent, or log/record the drop so it's visible. As written, the comment suggests some gateway-side handling that doesn't exist.
Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
d6ae835 to
299ff5d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
2 participants
Description
Problem
SMG cannot route OpenAI-compatible Decisions requests to local HTTP workers. SGLang 0.5.21 exposes different decision schemas, so callers currently have to bypass the gateway and adapt their requests.
Solution
Expose
/v1/decisionsthrough the regular HTTP router. Native Decisions backends receive the OpenAI request, including explicit extension values; SGLang workers receive a text-only translation through their internal/v1/systemoneendpoint. Convert SGLang answers back into ordered OpenAI answers while preserving names, boolean/string values, score labels, and usage fields.Builds on merged #2862 and targets
main; the diff contains only routing and tests. Refs #2849. This implements the agreed OpenAI-compatible endpoint; it does not expose a separate public/v1/systemoneroute.Changes
routers/http/router.rsdispatch, server/trait wiring, and endpoint metrics.e2e_test/router/test_decisions.py.SGLang translation rejects image input and unsupported extensions instead of dropping them. Its confidence retains System One semantics, and unreported cache counters are represented as zero; numerical equivalence to OpenAI is not claimed. Prefill scoring has zero output/reasoning tokens. SGLang System One selects its own DP rank, so rank-specific cache affinity is unavailable. Other backends must implement native
/v1/decisionsthemselves.Test Plan
cargo test -p smg --test decisions_http_test— 17 passed, including native extension preservation, aliases, SGLang conversion/order/typed values, unsupported inputs/transports, upstream errors, and malformed successes.cargo test -p smg --lib— 2,247 passed, five ignored during feature validation, including adapter, mixed-fleet, and explicit-field serialization tests.cargo clippy -p openai-protocol -p smg -p smg-external-router --lib --bins --tests -- -D warnings -A clippy::nonminimal_bool— passed during feature validation. The exception is an existing unrelated lint atmodel_gateway/src/routers/common/kv_transfer.rs:389.cargo +nightly fmt --all -- --check,git diff --check, and file hygiene hooks — passed.E2E_ENGINE=sglang E2E_RUNTIME=sglang pytest e2e_test/router/test_decisions.py -von an e2e runner with an SGLang engine.The feature diff is unchanged after updating onto merged #2859/#2860. Protocol and HTTP boundary tests were rerun after the update; both affected engine crate suites now pass (245 tests), and their strict Clippy check passes without suppressions. The subsequent rebase onto merged #2862 preserves the exact tested source tree. Engine inference was not run locally. Full-workspace checks still encounter unrelated Linux-only benchmark,
/proc, and allocator assumptions; no full-workspace pass is claimed.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses (scoped check and existing exception documented above)Release note
The gateway serves OpenAI-compatible
POST /v1/decisionson the regular HTTP router, translating for SGLang workers.Numbers
POST /v1/decisionsthrough the regular HTTP routercargo test -p smg --test decisions_http_test, mock HTTP workerse2e_test/router/test_decisions.pyon the SGLang e2e laneRefs: #2849