diff --git a/contracts/apr-serve-openai-compat-v1.yaml b/contracts/apr-serve-openai-compat-v1.yaml index 0e67ff9188..07d6707c5b 100644 --- a/contracts/apr-serve-openai-compat-v1.yaml +++ b/contracts/apr-serve-openai-compat-v1.yaml @@ -1,7 +1,7 @@ contract: apr-serve-openai-compat metadata: kind: pattern - version: "1.23.0" + version: "1.24.0" description: > OpenAI-compatible serve layer (/v1/chat/completions, /v1/completions, /v1/embeddings) fidelity invariants. Established by an adversarial audit @@ -788,3 +788,15 @@ falsification_tests: test_harness: 'cargo test -p aprender-serve --lib embed_kind_eg3_4902' expected_output: "test result: ok" if_fails: 'An embedding model is refused by `apr serve` or answers with a vector the CLI does not produce; a generative route on it fails with something other than a 4xx naming the kind; or an image reaches the tokenizer as text and comes back as a 200 embedding of its base64 (media dropped, I-7).' + - id: FALSIFY-CHAT-CONTENT-OPTIONAL-5411 + name: chat_message_content_pmat801 + prediction: 'A chat request whose ASSISTANT message omits `content` or sends `content: null` (the tool-call turn OpenAI SDK loops echo back) parses with content "" and keeps its tool_calls; system, user and tool messages still REQUIRE content (absent or null is refused naming `content`); a non-string content is a type error for every role; ChatMessage serialization bytes are unchanged.' + test_harness: 'cargo test -p aprender-serve --lib chat_message_content' + expected_output: "test result: ok" + if_fails: 'An OpenAI-compatible agent loop gets HTTP 422 "missing field content" on its second turn, or a non-assistant role silently accepts a missing content.' + - id: FALSIFY-MAX-COMPLETION-TOKENS-5411 + name: max_completion_tokens_alias + prediction: '`max_completion_tokens` fills `max_tokens` on /v1/chat/completions; `max_tokens` alone still parses; a body carrying both names is refused with serde `duplicate field` rather than silently preferring one.' + test_harness: 'cargo test -p aprender-serve --lib max_completion_tokens' + expected_output: "test result: ok" + if_fails: 'OpenAI SDK >= 1.40 clients lose their generation cap (the unknown key is ignored) and generation runs to the server default.' diff --git a/crates/apr-cli/src/commands/serve/server.rs b/crates/apr-cli/src/commands/serve/server.rs index c0f1a1b58e..8b7a3c38f1 100644 --- a/crates/apr-cli/src/commands/serve/server.rs +++ b/crates/apr-cli/src/commands/serve/server.rs @@ -423,6 +423,170 @@ mod qwen35_serve_route_tests { } } +/// FALSIFY-AUTH-001 for the GGUF router: `build_gguf_router` mounts realizar's REAL +/// router (`/v1/chat/completions`, `/v1/models`, `/health`, ...) behind the Bearer +/// gate, so these tests drive the same seam `serve_router` serves through. The state +/// is model-free (`AppState::demo_mock`): auth runs before any handler, so no weights +/// are needed to prove that a request without the key never reaches one. +#[cfg(all(test, feature = "inference"))] +mod gguf_router_auth_tests { + #![allow(clippy::unwrap_used, clippy::disallowed_methods)] + + use super::super::auth::AuthGate; + use super::*; + use axum::body::{to_bytes, Body}; + use axum::http::{header, HeaderMap, Method, Request, StatusCode}; + use serde_json::Value; + use tower::ServiceExt as _; + + const CHAT_BODY: &str = + r#"{"model":"x","messages":[{"role":"user","content":"Hi"}],"max_tokens":1}"#; + + fn router(gate: AuthGate) -> axum::Router { + let state = realizar::api::AppState::demo_mock().expect("model-free state"); + build_gguf_router(state, &ServerConfig::default(), gate) + } + + async fn fire( + router: axum::Router, + method: Method, + path: &str, + authorization: Option<&str>, + ) -> (StatusCode, HeaderMap, Value) { + let mut builder = Request::builder().method(method.clone()).uri(path); + let body = if method == Method::POST { + builder = builder.header(header::CONTENT_TYPE, "application/json"); + Body::from(CHAT_BODY) + } else { + Body::empty() + }; + if let Some(value) = authorization { + builder = builder.header(header::AUTHORIZATION, value); + } + let resp = router + .oneshot(builder.body(body).expect("request builds")) + .await + .expect("router answers"); + let status = resp.status(); + let headers = resp.headers().clone(); + let bytes = to_bytes(resp.into_body(), 64 * 1024).await.unwrap(); + let json = serde_json::from_slice(&bytes).unwrap_or(Value::Null); + (status, headers, json) + } + + #[tokio::test] + async fn gguf_router_refuses_without_bearer() { + for (method, path) in [ + (Method::POST, "/v1/chat/completions"), + (Method::GET, "/health"), + (Method::GET, "/v1/models"), + ] { + let (status, headers, body) = fire( + router(AuthGate::from_plain_key("k")), + method.clone(), + path, + None, + ) + .await; + assert_eq!(status, StatusCode::UNAUTHORIZED, "{method} {path}: {body}"); + assert_eq!( + body["error"].as_str(), + Some("unauthorized"), + "{method} {path}: {body}" + ); + assert_eq!( + headers + .get(header::WWW_AUTHENTICATE) + .and_then(|v| v.to_str().ok()), + Some("Bearer"), + "{method} {path}: 401 must carry WWW-Authenticate: Bearer" + ); + } + } + + #[tokio::test] + async fn gguf_router_refuses_a_wrong_bearer() { + let (status, _, body) = fire( + router(AuthGate::from_plain_key("k")), + Method::POST, + "/v1/chat/completions", + Some("Bearer wrong"), + ) + .await; + assert_eq!(status, StatusCode::UNAUTHORIZED, "{body}"); + assert_eq!(body["error"].as_str(), Some("unauthorized"), "{body}"); + } + + #[tokio::test] + async fn gguf_router_admits_the_right_bearer() { + // The model-free state answers the chat route with its own "no model" + // 503 "No model available" (a 200 needs real weights); that exact answer proves the + // request reached the handler, so a missing route (404) or a gate refusal (401) fails. + let (status, _, body) = fire( + router(AuthGate::from_plain_key("k")), + Method::POST, + "/v1/chat/completions", + Some("Bearer k"), + ) + .await; + assert_eq!(status, StatusCode::SERVICE_UNAVAILABLE, "{body}"); + assert!(body["error"].as_str().is_some_and(|e| e.contains("No model available")), "{body}"); + assert_ne!(status, StatusCode::UNAUTHORIZED, "{body}"); + let (status, _, body) = fire( + router(AuthGate::from_plain_key("k")), + Method::GET, + "/health", + Some("Bearer k"), + ) + .await; + // A model-free state reports itself as still loading (503); the point is that + // the health handler answered, not the gate. + assert_eq!(status, StatusCode::SERVICE_UNAVAILABLE, "{body}"); + assert_eq!(body["status"].as_str(), Some("loading"), "{body}"); + } + + /// `AuthGate::from_env()` yields a disabled gate when neither env var is set (this test uses + /// `AuthGate::disabled()` directly; the `serve_router` env wiring itself is a one-line call, untested); + /// the seam must not change behaviour in that case (documented upstream + /// semantics: with no key, `from_env()` warns and serves open). + #[tokio::test] + async fn gguf_router_disabled_gate_is_open() { + let (status, _, body) = + fire(router(AuthGate::disabled()), Method::GET, "/health", None).await; + assert_eq!(status, StatusCode::SERVICE_UNAVAILABLE, "{body}"); + assert_eq!(body["status"].as_str(), Some("loading"), "{body}"); + } +} + +/// Build realizar's full router over `state` behind the Bearer gate. +/// +/// HELIX-IDEA-009 / FALSIFY-AUTH-001: every route on the returned router is gated +/// by `auth_gate` via `super::auth::layer`, `/health` included — the auth contract +/// says health and metrics are NOT exempt. Behind the AWS Lambda +/// Web Adapter that is fine (its readiness check treats any 100-499 +/// status as ready), but an ALB or a k8s httpGet probe counts a 401 as unhealthy: pass +/// the probe the key, or `AuthGate::disabled()` to bypass. +/// +/// The GGUF/Qwen3.5 CPU path (and every other caller of `serve_router`) was built straight from +/// `create_router_with_config`: `APR_API_KEY_HASH` was set and +/// ignored, and the server answered 200 without a key (observed on v0.70.3 and +/// v0.71.4). The GPU, CUDA and APR GPU routers (`start_gguf_server_gpu_batched`, +/// `start_gguf_server_cuda` (past its early serve_router fallback), and the APR/SafeTensors GPU starters in +/// `handlers_include_01.rs`) are still ungated: follow-up. +/// `serve_router` is a thin wrapper that supplies `AuthGate::from_env()`; +/// the seam exists so the wiring is testable without a socket or model weights. +#[cfg(feature = "inference")] +fn build_gguf_router( + state: realizar::api::AppState, + config: &ServerConfig, + auth_gate: super::auth::AuthGate, +) -> axum::Router { + super::auth::layer( + auth_gate, + realizar::api::create_router_with_config(state, config.router_config()), + ) +} + /// Serve realizar's full router over `state` until Ctrl+C. /// /// The half of [`run_cpu_server`] that does not depend on which model the @@ -430,8 +594,6 @@ mod qwen35_serve_route_tests { /// served by the same router, banner and shutdown as every other GGUF. #[cfg(feature = "inference")] fn serve_router(state: realizar::api::AppState, config: &ServerConfig) -> Result<()> { - use realizar::api::create_router_with_config; - // aprender#2376(8): the banner is read from the router's own table, not restated. // The previous hand-written list named 11 of the 31 mounted routes and omitted // /tokenize, /realize/*, /models and the health probes entirely, while a @@ -441,7 +603,8 @@ fn serve_router(state: realizar::api::AppState, config: &ServerConfig) -> Result // Create realizar's full inference router (Ollama-parity endpoints). // --no-cors / --no-metrics must reach the router, not stop at the banner. - let app = create_router_with_config(state, config.router_config()); + // FALSIFY-AUTH-001: gated by APR_API_KEY_HASH / APR_API_KEY like build_apr_cpu_router. + let app = build_gguf_router(state, config, super::auth::AuthGate::from_env()); // Create tokio runtime and run server let runtime = tokio::runtime::Runtime::new() diff --git a/crates/aprender-serve/src/api/mod_create_demo.rs b/crates/aprender-serve/src/api/mod_create_demo.rs index e591a5f0d6..074ae0b90a 100644 --- a/crates/aprender-serve/src/api/mod_create_demo.rs +++ b/crates/aprender-serve/src/api/mod_create_demo.rs @@ -65,8 +65,13 @@ pub struct ChatCompletionRequest { pub model: String, /// Chat messages pub messages: Vec, - /// Maximum tokens to generate - #[serde(default)] + /// Maximum tokens to generate. + /// + /// `max_completion_tokens` is accepted as an alias: it is the newer OpenAI + /// name for the same cap (sent by OpenAI SDK >= 1.40 clients). A body that + /// carries BOTH names is refused by serde with a `duplicate field` error + /// rather than silently preferring one. + #[serde(default, alias = "max_completion_tokens")] pub max_tokens: Option, /// Sampling temperature. /// @@ -182,6 +187,10 @@ pub struct ChatCompletionRequest { pub top_logprobs: Option, } +#[cfg(test)] +#[path = "tests/max_completion_tokens_alias.rs"] +mod max_completion_tokens_alias_tests; + /// OpenAI's `response_format` (#3568 PR 4). #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(tag = "type", rename_all = "snake_case")] @@ -333,18 +342,28 @@ pub struct StreamOptions { } /// Chat message +/// +/// Deserialization goes through [`ChatMessageWire`] (`try_from`) so that the +/// `content` rule can depend on the sibling `role` field; serialization is the +/// plain derive and its bytes are a contract (PMAT-801). #[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(try_from = "ChatMessageWire")] pub struct ChatMessage { /// Role: "system", "user", "assistant", "tool" pub role: String, /// Message content. /// - /// PMAT-801: kept REQUIRED on deserialize (a request message missing - /// `content` is rejected 422, preserving the pre-PMAT-801 contract). The - /// response-side assistant tool-call message sets `content: String::new()` - /// directly in Rust, so it does not need a serde default. (Optional/null - /// content for assistant tool-call history on the REQUEST side is a - /// follow-up — it would require relaxing this field to Option.) + /// PMAT-801 follow-up: on the REQUEST side an ASSISTANT message whose + /// `content` key is absent or `null` reads as the empty string. OpenAI- + /// compatible clients (e.g. the `pmcp-agent` SDK loop) omit `content` on the + /// assistant tool-call turn they echo back, which the OpenAI API itself + /// permits; this used to be answered with HTTP 422. Every other role + /// (system, user, tool) still REQUIRES `content` (refused with serde's + /// "missing field" error as before), and a non-string `content` is still a + /// type error. + /// The response side is unchanged: the assistant tool-call message sets + /// `content: String::new()` directly in Rust and serializes `"content":""` + /// exactly as before, so the field stays a plain `String` for every reader. pub content: String, /// Optional name #[serde(default, skip_serializing_if = "Option::is_none")] @@ -360,6 +379,45 @@ pub struct ChatMessage { pub tool_call_id: Option, } +/// Request-side wire shape of [`ChatMessage`]. +/// +/// `content` is an `Option` under `#[serde(default)]` so an absent key and a +/// JSON `null` both read as `None` (a number or any other non-string is still a +/// type error). The `TryFrom` conversion then decides, per role, whether `None` +/// is acceptable. +#[derive(Debug, Deserialize)] +struct ChatMessageWire { + role: String, + #[serde(default)] + content: Option, + #[serde(default)] + name: Option, + #[serde(default)] + tool_calls: Option>, + #[serde(default)] + tool_call_id: Option, +} + +impl TryFrom for ChatMessage { + type Error = String; + + fn try_from(wire: ChatMessageWire) -> std::result::Result { + let content = match wire.content { + Some(text) => text, + // An assistant tool-call history turn legitimately has no text. + None if wire.role == "assistant" => String::new(), + None => return Err("missing field `content`".to_string()), + }; + Ok(Self { + role: wire.role, + content, + name: wire.name, + tool_calls: wire.tool_calls, + tool_call_id: wire.tool_call_id, + }) + } +} + // ============================================================================ // PMAT-801: OpenAI tool-calling wire types (request in, response out) // ============================================================================ @@ -1214,3 +1272,7 @@ pub(crate) fn default_top_k_features() -> usize { pub(crate) fn default_explain_method() -> String { "shap".to_string() } + +#[cfg(test)] +#[path = "tests/chat_message_content_pmat801.rs"] +mod chat_message_content_tests; diff --git a/crates/aprender-serve/src/api/tests/chat_message_content_pmat801.rs b/crates/aprender-serve/src/api/tests/chat_message_content_pmat801.rs new file mode 100644 index 0000000000..cc73e672dd --- /dev/null +++ b/crates/aprender-serve/src/api/tests/chat_message_content_pmat801.rs @@ -0,0 +1,114 @@ +//! PMAT-801 follow-up: an OpenAI-compatible client (e.g. the `pmcp-agent` SDK loop) +//! omits `content` on the assistant tool-call turn it echoes back in the next +//! request, which the OpenAI API itself permits. The request side used to answer +//! that body with HTTP 422 (`missing field \`content\``). +//! +//! Scope of the tolerance, pinned here from both sides: +//! - an ASSISTANT message with `content` absent or `null` reads as `""`; +//! - every other role (user, system, tool) still REQUIRES `content`; +//! - a non-string `content` is still a type error; +//! - serialization is unchanged (the response-side bytes are a contract). +//! +//! The fixture is request #3 of the spike-029 wire log +//! (`results/wire-0.8b-plain.jsonl` line 4, the turn-2 body that returned 422). +#![allow(clippy::unwrap_used, clippy::disallowed_methods)] + +use super::*; + +fn wire_fixture_request() -> serde_json::Value { + serde_json::json!({ + "model": "Qwen3.5-0.8B-Q4_K_M", + "messages": [ + {"role": "system", "content": "You are a concise, helpful assistant. Use tools when helpful."}, + {"role": "user", "content": "What's the weather in Paris right now?"}, + {"role": "assistant", "tool_calls": [ + {"id": "call_0", "type": "function", "function": { + "name": "get_weather", + "arguments": "{\"city\":\"Paris\",\"unit\":\"celsius\"}" + }} + ]}, + {"role": "tool", "tool_call_id": "call_0", + "content": "{\"city\":\"Paris\",\"temperature_c\":17,\"condition\":\"light rain\"}"} + ], + "max_tokens": 384 + }) +} + +fn parse(v: serde_json::Value) -> Result { + serde_json::from_value::(v) +} + +#[test] +fn assistant_message_without_content_parses_pmat801_followup() { + let req = parse(wire_fixture_request()).expect("turn-2 body must parse"); + assert_eq!(req.messages.len(), 4); + assert_eq!(req.messages[2].role, "assistant"); + assert_eq!(req.messages[2].content, ""); + let calls = req.messages[2].tool_calls.as_ref().expect("tool_calls survive"); + assert_eq!(calls.len(), 1); + assert_eq!(calls[0].function.name, "get_weather"); + assert_eq!(calls[0].function.arguments, "{\"city\":\"Paris\",\"unit\":\"celsius\"}"); + assert_eq!(req.messages[3].role, "tool"); + assert_eq!(req.messages[3].tool_call_id.as_deref(), Some("call_0")); +} + +#[test] +fn assistant_message_with_null_content_parses() { + let mut body = wire_fixture_request(); + body["messages"][2]["content"] = serde_json::Value::Null; + let req = parse(body).expect("null assistant content must parse"); + assert_eq!(req.messages[2].content, ""); + assert!(req.messages[2].tool_calls.is_some()); +} + +#[test] +fn chat_message_serialization_is_byte_identical() { + let assistant = ChatMessage { + role: "assistant".to_string(), + content: String::new(), + name: None, + tool_calls: Some(vec![ResponseToolCall { + id: "call_0".to_string(), + call_type: "function".to_string(), + function: ResponseFunctionCall { + name: "get_weather".to_string(), + arguments: "{\"city\":\"Paris\"}".to_string(), + }, + }]), + tool_call_id: None, + }; + assert_eq!( + serde_json::to_string(&assistant).unwrap(), + r#"{"role":"assistant","content":"","tool_calls":[{"id":"call_0","type":"function","function":{"name":"get_weather","arguments":"{\"city\":\"Paris\"}"}}]}"# + ); + let user = ChatMessage { + role: "user".to_string(), + content: "x".to_string(), + name: None, + tool_calls: None, + tool_call_id: None, + }; + assert_eq!(serde_json::to_string(&user).unwrap(), r#"{"role":"user","content":"x"}"#); +} + +#[test] +fn numeric_content_is_still_refused() { + for role in ["assistant", "user", "system", "tool"] { + let body = serde_json::json!({"model": "m", "messages": [{"role": role, "content": 5}]}); + assert!(parse(body).is_err(), "numeric content must stay a type error for role {role}"); + } +} + +#[test] +fn non_assistant_message_without_content_is_refused() { + for role in ["user", "system", "tool"] { + let absent = serde_json::json!({"model": "m", "messages": [{"role": role}]}); + let err = parse(absent).expect_err("absent content must be refused").to_string(); + assert!(err.contains("content"), "role {role}, absent: got {err}"); + let null = serde_json::json!({"model": "m", "messages": [{"role": role, "content": null}]}); + let err = parse(null).expect_err("null content must be refused").to_string(); + assert!(err.contains("content"), "role {role}, null: got {err}"); + let present = serde_json::json!({"model": "m", "messages": [{"role": role, "content": "x"}]}); + assert_eq!(parse(present).expect("present content parses").messages[0].content, "x"); + } +} diff --git a/crates/aprender-serve/src/api/tests/max_completion_tokens_alias.rs b/crates/aprender-serve/src/api/tests/max_completion_tokens_alias.rs new file mode 100644 index 0000000000..83bab33dc8 --- /dev/null +++ b/crates/aprender-serve/src/api/tests/max_completion_tokens_alias.rs @@ -0,0 +1,52 @@ +//! `max_completion_tokens` is the newer OpenAI name for the generation cap that +//! older clients call `max_tokens` (OpenAI SDK >= 1.40 sends the new name). +//! Before this alias the unknown key was silently ignored, so the cap was lost +//! and generation ran to the server default. +//! +//! Pinned here: the new name fills `max_tokens`, the old name alone still +//! works, and a body carrying BOTH names is refused by serde with a +//! `duplicate field` error (a deterministic 422, never a silently chosen value). +#![allow(clippy::unwrap_used, clippy::disallowed_methods)] + +use super::*; + +fn parse(v: serde_json::Value) -> Result { + serde_json::from_value::(v) +} + +fn base() -> serde_json::Value { + serde_json::json!({ + "model": "m", + "messages": [{"role": "user", "content": "x"}] + }) +} + +#[test] +fn max_completion_tokens_aliases_max_tokens() { + let mut body = base(); + body["max_completion_tokens"] = serde_json::json!(12); + let req = parse(body).expect("max_completion_tokens must parse"); + assert_eq!(req.max_tokens, Some(12)); +} + +#[test] +fn max_tokens_alone_still_parses() { + let mut body = base(); + body["max_tokens"] = serde_json::json!(7); + let req = parse(body).expect("max_tokens must still parse"); + assert_eq!(req.max_tokens, Some(7)); +} + +#[test] +fn both_max_tokens_names_present_is_documented() { + let mut body = base(); + body["max_tokens"] = serde_json::json!(7); + body["max_completion_tokens"] = serde_json::json!(12); + let err = parse(body) + .expect_err("a body carrying both names is refused: serde reports a duplicate field") + .to_string(); + assert!( + err.contains("duplicate field"), + "both names present: serde's derive refuses with `duplicate field` (no value wins), got: {err}" + ); +}