diff --git a/crates/spark-model/src/weight_loader/qwen4_exp/probe.rs b/crates/spark-model/src/weight_loader/qwen4_exp/probe.rs index 4e1e5c110..01740e3b2 100644 --- a/crates/spark-model/src/weight_loader/qwen4_exp/probe.rs +++ b/crates/spark-model/src/weight_loader/qwen4_exp/probe.rs @@ -29,9 +29,12 @@ pub struct NamespaceReport { pub stray_layer_norms: usize, } -/// A projection is present in either the dense/NVFP4 layout (`.weight`) or the EXL3 one (`.trellis`). +/// A projection is present in the dense/NVFP4 layout (`.weight`), the compressed-tensors +/// one (`.weight_packed`), or the EXL3 one (`.trellis`). fn has_linear(store: &WeightStore, stem: &str) -> bool { - store.contains(&format!("{stem}.weight")) || store.contains(&format!("{stem}.trellis")) + store.contains(&format!("{stem}.weight")) + || store.contains(&format!("{stem}.weight_packed")) + || store.contains(&format!("{stem}.trellis")) } /// The n-gram tables are deferred (served from NVMe), which `contains` does not see. diff --git a/crates/spark-model/src/weight_loader/qwen4_exp/probe_tests.rs b/crates/spark-model/src/weight_loader/qwen4_exp/probe_tests.rs index 2e13117a9..7c8273139 100644 --- a/crates/spark-model/src/weight_loader/qwen4_exp/probe_tests.rs +++ b/crates/spark-model/src/weight_loader/qwen4_exp/probe_tests.rs @@ -1,7 +1,15 @@ // SPDX-License-Identifier: AGPL-3.0-only -//! `audit_namespace` must see every projection of an EXL3 checkpoint (`.trellis` -//! instead of `.weight`) and the deferred n-gram tables (never in the store map). +//! `audit_namespace`/`has_linear` must see every projection namespace: the +//! dense/NVFP4 `.weight`, the compressed-tensors `.weight_packed`, and the EXL3 +//! `.trellis` — plus the deferred n-gram tables (never in the store map). +//! +//! `weight_packed_experts_resolve_the_layer` pins the reported production bug: +//! a compressed-tensors NVFP4 Flash-Next checkpoint stores its routed experts +//! as `.weight_packed`, so before the fix it probed 0 routed-expert layers and +//! `ensure_loadable` refused with "0 of 48 layers have routed experts". The +//! test fails (expert_tensors = 0, `ensure_loadable` errs) if +//! the `.weight_packed` arm is dropped from `has_linear`. use super::*; use spark_runtime::gpu::DevicePtr; @@ -126,3 +134,111 @@ fn missing_expert_still_refuses() { assert_eq!(r.expert_tensors, 1); assert!(r.ensure_loadable().is_err()); } + +/// The three projection namespaces are interchangeable to `audit_namespace`: +/// swapping a checkpoint's suffix must not change what the probe reports. +#[test] +fn expert_namespace_suffixes_are_equivalent() { + let c = config(); + let l0 = c.layer_prefix(0); + let l1 = c.layer_prefix(1); + for suffix in [".weight", ".weight_packed", ".trellis"] { + let names = [ + n(format!("{l0}.linear_attn.in_proj_qkv{suffix}")), + n(format!("{l1}.self_attn.q_proj{suffix}")), + n(format!("{l1}.self_attn.indexer.index_qk_proj{suffix}")), + n(format!("{l0}.mlp.experts.0.gate_proj{suffix}")), + n(format!("{l1}.mlp.experts.0.gate_proj{suffix}")), + n(format!("{l0}.attn_hyper_connection.hc_norm.weight")), + n(format!("{l1}.attn_hyper_connection.hc_norm.weight")), + n(format!("{l0}.mlp_hyper_connection.hc_norm.weight")), + n(format!("{l1}.mlp_hyper_connection.hc_norm.weight")), + n("model.language_model.embed_tokens.weight".into()), + n(format!("lm_head{suffix}")), + ]; + let r = audit_namespace(&store(&names, &[]), &c); + assert_eq!(r.gdn_layers, 1, "{suffix}"); + assert_eq!(r.attn_layers, 1, "{suffix}"); + assert_eq!(r.indexer_tensors, 1, "{suffix}"); + assert_eq!(r.expert_tensors, 2, "{suffix}"); + assert!(r.has_lm_head, "{suffix}"); + assert!(r.ensure_loadable().is_ok(), "{suffix}"); + } +} + +/// The reported checkpoint shape: FP8 attention/GDN weights keep a plain +/// `.weight`, only the NVFP4 routed experts are `.weight_packed`. This is the +/// case that regressed to "0 of 48 layers have routed experts". +#[test] +fn weight_packed_experts_resolve_the_layer() { + let c = config(); + let l0 = c.layer_prefix(0); + let l1 = c.layer_prefix(1); + let names = [ + n(format!("{l0}.linear_attn.in_proj_qkv.weight")), + n(format!("{l1}.self_attn.q_proj.weight")), + n(format!("{l1}.self_attn.indexer.index_qk_proj.weight")), + n(format!("{l0}.mlp.experts.0.gate_proj.weight_packed")), + n(format!("{l1}.mlp.experts.0.gate_proj.weight_packed")), + n(format!("{l0}.attn_hyper_connection.hc_norm.weight")), + n(format!("{l1}.attn_hyper_connection.hc_norm.weight")), + n(format!("{l0}.mlp_hyper_connection.hc_norm.weight")), + n(format!("{l1}.mlp_hyper_connection.hc_norm.weight")), + n("model.language_model.embed_tokens.weight".into()), + n("lm_head.weight".into()), + ]; + let r = audit_namespace(&store(&names, &[]), &c); + assert_eq!(r.gdn_layers, 1); + assert_eq!(r.attn_layers, 1); + assert_eq!(r.indexer_tensors, 1); + assert_eq!(r.expert_tensors, 2); + assert!(r.has_lm_head); + assert_eq!(r.ple_shards, 0); + assert!(r.ensure_loadable().is_ok()); +} + +/// `has_linear` must match the three projection namespaces exactly and must not +/// mistake a quantization sidecar or a bare stem for a projection. +#[test] +fn has_linear_matches_only_projection_namespaces() { + let with = |name: &str| store(&[name.to_string()], &[]); + for stem in ["blk.self_attn.q_proj", "blk.mlp.experts.0.gate_proj"] { + assert!(has_linear(&with(&format!("{stem}.weight")), stem), "{stem}"); + assert!( + has_linear(&with(&format!("{stem}.weight_packed")), stem), + "{stem}" + ); + assert!( + has_linear(&with(&format!("{stem}.trellis")), stem), + "{stem}" + ); + // A compressed-tensors sidecar (`weight_scale`) is not the projection. + assert!( + !has_linear(&with(&format!("{stem}.weight_scale")), stem), + "{stem}" + ); + assert!(!has_linear(&with(stem), stem), "{stem}"); + } +} + +/// A checkpoint missing one layer's routed experts — even a `.weight_packed` +/// one — must still refuse rather than load on a bad premise. +#[test] +fn missing_weight_packed_expert_still_refuses() { + let c = config(); + let l0 = c.layer_prefix(0); + let l1 = c.layer_prefix(1); + let names = [ + n(format!("{l0}.linear_attn.in_proj_qkv.weight")), + n(format!("{l1}.self_attn.q_proj.weight")), + n(format!("{l0}.mlp.experts.0.gate_proj.weight_packed")), + n(format!("{l0}.attn_hyper_connection.hc_norm.weight")), + n(format!("{l0}.mlp_hyper_connection.hc_norm.weight")), + n("model.language_model.embed_tokens.weight".into()), + n("lm_head.weight".into()), + ]; + let r = audit_namespace(&store(&names, &[]), &c); + assert_eq!(r.expert_tensors, 1); + let err = r.ensure_loadable().unwrap_err().to_string(); + assert!(err.contains("routed experts"), "{err}"); +} diff --git a/crates/spark-server/src/api/chat_blocking_choice.rs b/crates/spark-server/src/api/chat_blocking_choice.rs index a43e953b1..0ab0b7b87 100644 --- a/crates/spark-server/src/api/chat_blocking_choice.rs +++ b/crates/spark-server/src/api/chat_blocking_choice.rs @@ -159,15 +159,16 @@ pub(super) fn build_choice_message( msg_content = msg_content.map(|c| super::strip::strip_orphan_tool_markup(&c)); } - // Refusal classifier: when the model's assistant text opens with - // a known refusal pattern AND no tool call fired, populate - // `refusal` and null out `content` per the OpenAI spec. + // Refusal classifier: when the model's assistant text opens with a + // known refusal pattern AND no tool call fired, populate `refusal`. + // The full assistant text stays in `content` alongside it — streaming + // emits the whole text plus a `Refusal` delta, and nulling `content` + // here would drop every sentence after the first. if msg_tool_calls.is_none() && let Some(content_text) = msg_content.as_deref() && let Some(refusal_sentence) = crate::refusal::detect(content_text) { msg_refusal = Some(refusal_sentence); - msg_content = None; } // Validated wire tool calls → IR (arguments are serde-normalized diff --git a/crates/spark-server/src/api/chat_stream/done_finalization_tests.rs b/crates/spark-server/src/api/chat_stream/done_finalization_tests.rs index 8e820f72d..492be8e06 100644 --- a/crates/spark-server/src/api/chat_stream/done_finalization_tests.rs +++ b/crates/spark-server/src/api/chat_stream/done_finalization_tests.rs @@ -152,3 +152,21 @@ fn terminal_done_keeps_existing_finish_precedence_and_tool_history() { assert_finish_only(&deltas, expected); } } + +#[test] +fn terminal_done_refusal_annotation_preserves_full_content() { + let _guard = crate::refusal::TEST_ENV_LOCK + .lock() + .unwrap_or_else(|e| e.into_inner()); + let text = "I cannot help with that request. Here is a safe alternative."; + let mut s = state(); + s.tag_scan_buf = text.into(); + s.refusal_scan_buf = text.into(); + let (deltas, reason) = finalize_done(&mut s, "stop", usage(), false, flush_sanitizer); + assert_eq!(reason, "stop"); + assert!(matches!(&deltas[..], [ + StreamDelta::Content { text: content, .. }, + StreamDelta::Refusal { text: refusal }, + StreamDelta::Finish { reason: crate::ir::FinishReason::Stop, .. } + ] if content == text && refusal == "I cannot help with that request.")); +} diff --git a/crates/spark-server/src/api/mod.rs b/crates/spark-server/src/api/mod.rs index 092ade195..9f1942a54 100644 --- a/crates/spark-server/src/api/mod.rs +++ b/crates/spark-server/src/api/mod.rs @@ -95,3 +95,6 @@ pub use stubs::{ audio_stub, batch_get_stub, batch_list_stub, batches_stub, files_stub, images_stub, moderations_stub, }; + +#[cfg(test)] +mod refusal_content_tests; diff --git a/crates/spark-server/src/api/refusal_content_tests.rs b/crates/spark-server/src/api/refusal_content_tests.rs new file mode 100644 index 000000000..377262dba --- /dev/null +++ b/crates/spark-server/src/api/refusal_content_tests.rs @@ -0,0 +1,333 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +//! Blocking-mode refusal classification must PRESERVE the model's full +//! assistant text. Streaming already emits the text AND a `Refusal` +//! metadata delta (`chat_stream/done_finalization.rs`); the blocking path +//! used to null `content` after setting `refusal`, silently dropping every +//! sentence after the first. These tests drive the real production +//! assembly (`chat_blocking_choice::build_choice_message`) and the real +//! OpenAI wire projection (`openai::encode_chat_response`) — no logic is +//! copied, so a regression in either layer is caught here. + +use crate::AppState; +use crate::ir; +use serde_json::json; +use std::sync::Arc; +use std::time::Duration; + +/// A tiny real word-level tokenizer. This suite never decodes logprobs, so +/// its vocabulary only has to be valid for `ChatTokenizer::from_model_dir`. +fn tokenizer(dir: &std::path::Path) -> crate::tokenizer::ChatTokenizer { + let vocab = ["[UNK]", "a", ""] + .iter() + .enumerate() + .map(|(i, w)| ((*w).to_string(), i as u32)) + .collect(); + let model = tokenizers::models::wordlevel::WordLevel::builder() + .vocab(vocab) + .unk_token("[UNK]".into()) + .build() + .unwrap(); + let mut inner = tokenizers::Tokenizer::new(model); + inner.with_decoder(Some(tokenizers::decoders::fuse::Fuse::new())); + inner.save(dir.join("tokenizer.json"), false).unwrap(); + crate::tokenizer::ChatTokenizer::from_model_dir(dir, 0, true, "glm5_next", "", None, false) + .unwrap() +} + +/// A minimal `AppState` — only the fields `build_choice_message` and the +/// OpenAI encoder actually read are set to non-default values; the rest are +/// the same neutral defaults the production server builds for a text-only, +/// tool-free GLM serve. +fn state(dir: &std::path::Path) -> AppState { + let (request_tx, _rx) = tokio::sync::mpsc::channel(1); + AppState { + tokenizer: tokenizer(dir), + model_name: "refusal-fixture".into(), + adapter_name: None, + adapter_names: vec![], + active_adapter: Arc::default(), + max_seq_len: 4096, + request_tx, + rotation_tx: None, + vision_config: None, + vision_max_pixels: None, + remote_image_policy: Default::default(), + video_ffmpeg: Default::default(), + video_fps: 1.0, + default_temperature: 0.0, + default_top_k: 0, + default_top_p: 1.0, + default_top_n_sigma: 0.0, + default_min_p: 0.0, + tool_call_parser: None, + chat: Default::default(), + reasoning_parser: Some(crate::reasoning_parser::ReasoningFormat::Qwen.into_parser()), + think_end_token_id: Some(2), + think_start_token_id: Some(3), + tool_max_tokens: 192, + sampling_presets: Default::default(), + tool_call_start_token_id: None, + glm_tool_boundary: None, + auto_compact_threshold: None, + request_timeout: 0, + effective_context: 0, + behavior: Default::default(), + disable_thinking: false, + default_thinking: Default::default(), + default_reasoning_effort: None, + response_store: crate::response_store::ResponseStore::with_config( + 1, + Duration::from_secs(60), + ), + conversation_store: crate::conversation_store::ConversationStore::with_config( + 1, + Duration::from_secs(60), + ), + rate_limiter: crate::rate_limiter::RateLimiter::with_config( + crate::rate_limiter::RateLimitConfig { + rpm: 0, + tpm: 0, + burst_rpm: 1, + burst_tpm: 1, + }, + ), + dump_writer: None, + lora_stageable: Default::default(), + lora_peer_addr: None, + promotion: None, + promoted_slots: Arc::default(), + lora_disk_stageable: Default::default(), + } +} + +/// A response carrying `finish_reason` only — this suite never decodes +/// tokens, it feeds `output_text` straight into `build_choice_message`. +fn response(finish_reason: &str) -> crate::api::inference_types::InferenceResponse { + crate::api::inference_types::InferenceResponse { + output_tokens: vec![], + finish_reason: finish_reason.into(), + time_to_first_token_ms: 0.0, + decode_time_ms: 0.0, + logprobs: vec![], + reasoning_tokens: 0, + cached_prompt_tokens: 0, + accepted_prediction_tokens: 0, + prompt_logprobs: vec![], + } +} + +/// A tool-free, single-turn chat request. +fn request() -> ir::ChatRequest { + let wire: crate::openai::ChatCompletionRequest = serde_json::from_value(json!({ + "model": "fixture", + "messages": [{"role": "user", "content": "hello"}], + "max_tokens": 192 + })) + .unwrap(); + wire.into() +} + +/// Project one piece of pre-assembled model text through the exact +/// production blocking assembly, then through the OpenAI wire encoder, and +/// return the serialized choice JSON object. +fn project( + output_text: String, + reasoning: Option, + tools_active: bool, + finish_reason: &str, +) -> serde_json::Value { + let dir = tempfile::tempdir().unwrap(); + let state = state(dir.path()); + let req = request(); + let response = response(finish_reason); + let choice = crate::api::chat_blocking_choice::build_choice_message( + &state, + &req, + &response, + reasoning, + output_text, + tools_active, + None, + 0, + ); + let ir_response = ir::ChatResponse { + id: "refusal-fixture".into(), + model: "fixture".into(), + created: 0, + choices: vec![choice], + usage: ir::Usage::default(), + }; + let echo = crate::api::ResponseEcho { + service_tier: None, + metadata: None, + store: false, + include_usage: false, + }; + let resp = crate::openai::encode_chat_response(&state, ir_response, &echo, None); + let bytes = tokio::runtime::Builder::new_current_thread() + .build() + .unwrap() + .block_on(axum::body::to_bytes(resp.into_body(), usize::MAX)) + .unwrap(); + let mut body: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + body["choices"][0].take() +} + +/// Multi-sentence refusal: the message must keep EVERY sentence, not just +/// the first, while also carrying the classifier's `refusal` sentence. +#[test] +fn blocking_multi_sentence_refusal_keeps_full_content_and_refusal() { + let _guard = crate::refusal::TEST_ENV_LOCK + .lock() + .unwrap_or_else(|e| e.into_inner()); + let text = "I cannot help with that request. Here is why: it would be \ + unsafe. Please ask something else."; + let choice = project(text.into(), None, false, "stop"); + let message = &choice["message"]; + assert_eq!( + message["content"].as_str(), + Some(text), + "full content must survive refusal classification: {message}" + ); + assert_eq!( + message["refusal"].as_str(), + Some("I cannot help with that request."), + "refusal metadata must still be set: {message}" + ); +} + +/// A helpful answer can match a broad refusal prefix. Keep all its text. +#[test] +fn blocking_prefix_false_positive_keeps_full_answer() { + let _guard = crate::refusal::TEST_ENV_LOCK + .lock() + .unwrap_or_else(|e| e.into_inner()); + let text = "I cannot see your screen. Please paste the error and I can help."; + let choice = project(text.into(), None, false, "stop"); + assert_eq!(choice["message"]["content"].as_str(), Some(text)); + assert_eq!(choice["message"]["refusal"], "I cannot see your screen."); +} + +/// Sympathy alone is not one of the classifier's refusal prefixes. +#[test] +fn blocking_sympathy_keeps_full_content_without_refusal() { + let _guard = crate::refusal::TEST_ENV_LOCK + .lock() + .unwrap_or_else(|e| e.into_inner()); + let text = "I'm sorry for your loss. Take all the time you need."; + let choice = project(text.into(), None, false, "stop"); + assert_eq!(choice["message"]["content"].as_str(), Some(text)); + assert!(choice["message"].get("refusal").is_none()); +} + +/// Ordinary (non-refusal) text must be untouched: content intact, no +/// spurious refusal. +#[test] +fn blocking_ordinary_text_has_content_and_no_refusal() { + let _guard = crate::refusal::TEST_ENV_LOCK + .lock() + .unwrap_or_else(|e| e.into_inner()); + let text = "Sure — here is how to do that in three steps."; + let choice = project(text.into(), None, false, "stop"); + let message = &choice["message"]; + assert_eq!(message["content"].as_str(), Some(text), "{message}"); + assert!( + message.get("refusal").is_none(), + "ordinary text must not gain a refusal: {message}" + ); +} + +/// Empty text stays an empty string, without refusal metadata. +#[test] +fn blocking_empty_text_has_no_content_and_no_refusal() { + let _guard = crate::refusal::TEST_ENV_LOCK + .lock() + .unwrap_or_else(|e| e.into_inner()); + let choice = project(String::new(), None, false, "stop"); + let message = &choice["message"]; + assert_eq!(message["content"].as_str(), Some(""), "{message}"); + assert!(message.get("refusal").is_none(), "{message}"); +} + +/// A refusal that also carries a reasoning trace: the reasoning survives +/// too, and does not suppress the content-preservation fix. +#[test] +fn blocking_refusal_preserves_reasoning_trace() { + let _guard = crate::refusal::TEST_ENV_LOCK + .lock() + .unwrap_or_else(|e| e.into_inner()); + let text = "I must decline. This request falls outside what I can help \ + with safely."; + let reasoning = "The user is asking for something disallowed."; + let choice = project(text.into(), Some(reasoning.into()), false, "stop"); + let message = &choice["message"]; + assert_eq!(message["content"].as_str(), Some(text), "{message}"); + assert_eq!( + message["reasoning_content"].as_str(), + Some(reasoning), + "{message}" + ); + assert_eq!( + message["refusal"].as_str(), + Some("I must decline."), + "{message}" + ); +} + +/// A valid tool call suppresses refusal classification (the classifier is +/// gated on `tool_calls.is_none()`), and the finish reason stays +/// `tool_calls` — the preservation fix must not disturb that. +#[test] +fn blocking_valid_tool_call_keeps_call_and_finish_reason() { + let _guard = crate::refusal::TEST_ENV_LOCK + .lock() + .unwrap_or_else(|e| e.into_inner()); + let dir = tempfile::tempdir().unwrap(); + let state = state(dir.path()); + let wire: crate::openai::ChatCompletionRequest = serde_json::from_value(json!({ + "model": "fixture", + "messages": [{"role": "user", "content": "weather"}], + "tools": [{"type": "function", "function": {"name": "get_weather", + "parameters": {"type": "object", "properties": {"city": {"type": "string"}}, + "required": ["city"]}}}], + "max_tokens": 192 + })) + .unwrap(); + let req: ir::ChatRequest = wire.into(); + let response = response("tool_calls"); + let call = "I cannot answer directly. get_weathercity\ + Oslo"; + let choice = crate::api::chat_blocking_choice::build_choice_message( + &state, + &req, + &response, + None, + call.into(), + true, + None, + 0, + ); + assert_eq!(choice.tool_calls.len(), 1, "{choice:?}"); + assert_eq!(choice.tool_calls[0].name, "get_weather"); + assert_eq!(choice.tool_calls[0].arguments["city"], "Oslo"); + assert_eq!(choice.finish_reason, ir::FinishReason::ToolCalls); + assert!(choice.refusal.is_none(), "tool turn must not be a refusal"); +} + +/// Finish reasons other than a natural stop are preserved verbatim on a +/// refusal turn: a truncated ("length") or timed-out ("timeout") refusal +/// still keeps its text and its reason. +#[test] +fn blocking_refusal_preserves_length_and_timeout_finish_reasons() { + let _guard = crate::refusal::TEST_ENV_LOCK + .lock() + .unwrap_or_else(|e| e.into_inner()); + for wire_reason in ["stop", "length", ir::FINISH_REASON_TIMEOUT] { + let text = "I cannot provide that. Here is an alternative."; + let choice = project(text.into(), None, false, wire_reason); + assert_eq!(choice["message"]["content"].as_str(), Some(text)); + assert_eq!(choice["message"]["refusal"], "I cannot provide that."); + assert_eq!(choice["finish_reason"], wire_reason); + } +} diff --git a/crates/spark-server/src/ir/response.rs b/crates/spark-server/src/ir/response.rs index 4e03d1512..6d3cea06f 100644 --- a/crates/spark-server/src/ir/response.rs +++ b/crates/spark-server/src/ir/response.rs @@ -30,7 +30,7 @@ pub struct ChatResponse { pub struct Choice { pub index: usize, /// Assistant text (`None` mirrors the wire's `content: null`, e.g. - /// after a refusal strip). + /// for a tool-only response). pub content: Option, /// Reasoning/thinking trace, when the model produced one. pub reasoning: Option, diff --git a/crates/spark-server/src/openai/annotations.rs b/crates/spark-server/src/openai/annotations.rs index a63e92025..0ae5b6adb 100644 --- a/crates/spark-server/src/openai/annotations.rs +++ b/crates/spark-server/src/openai/annotations.rs @@ -23,9 +23,8 @@ pub struct ChatMessage { /// retrieval clients see a familiar shape. Omitted when empty. #[serde(skip_serializing_if = "Option::is_none")] pub annotations: Option>, - /// Assistant refusal message. When set, `content` should be treated - /// as null by the client. Atlas does not currently emit refusals; the - /// field is present so safety-aware clients stay compatible. + /// Heuristic refusal annotation. The full assistant text remains in + /// `content`; this field supplements it and does not redact the answer. #[serde(skip_serializing_if = "Option::is_none")] pub refusal: Option, } diff --git a/crates/spark-server/src/openai/encode.rs b/crates/spark-server/src/openai/encode.rs index cce4b71f0..1efdbd997 100644 --- a/crates/spark-server/src/openai/encode.rs +++ b/crates/spark-server/src/openai/encode.rs @@ -62,7 +62,7 @@ pub(crate) fn encode_chat_response( ) }; // URL-citation annotations are derived from the FINAL - // content (post tool-XML strip / refusal null), so offsets + // content (after tool-XML stripping), so offsets // always index into the text the client actually receives. let annotations = c.content.as_deref().and_then(merged_annotations); ChatChoice { diff --git a/crates/spark-server/src/refusal.rs b/crates/spark-server/src/refusal.rs index 7d5ccbb78..edf620da8 100644 --- a/crates/spark-server/src/refusal.rs +++ b/crates/spark-server/src/refusal.rs @@ -87,40 +87,41 @@ pub fn detect(content: &str) -> Option { Some(sentence.trim().to_string()) } +#[cfg(test)] +pub(crate) static TEST_ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + #[cfg(test)] mod tests { use super::*; - use std::sync::Mutex; // Cargo runs unit tests in parallel threads within a single binary, and // env vars are process-wide. `kill_switch_returns_none` mutates // ATLAS_DISABLE_REFUSAL_DETECTION, so every test that calls `detect()` // must hold this lock to avoid observing a transient kill-switch state. - static ENV_LOCK: Mutex<()> = Mutex::new(()); #[test] fn matches_canonical_refusal() { - let _g = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _g = TEST_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); let r = detect("I cannot help with that request. Here is why…").unwrap(); assert_eq!(r, "I cannot help with that request."); } #[test] fn matches_with_leading_whitespace() { - let _g = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _g = TEST_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); let r = detect(" I'm sorry, but I can't assist with weapons design.").unwrap(); assert_eq!(r, "I'm sorry, but I can't assist with weapons design."); } #[test] fn mixed_case_matches() { - let _g = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _g = TEST_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); assert!(detect("As AN ai, I cannot provide that.").is_some()); } #[test] fn non_refusal_returns_none() { - let _g = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _g = TEST_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); assert!(detect("Sure, here's how to do that.").is_none()); assert!(detect("").is_none()); assert!(detect("I can do that for you.").is_none()); @@ -128,13 +129,13 @@ mod tests { #[test] fn kill_switch_returns_none() { - let _g = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); - // SAFETY: serialized via ENV_LOCK across this module's tests. + let _g = TEST_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + // SAFETY: serialized via TEST_ENV_LOCK across this module's tests. unsafe { std::env::set_var("ATLAS_DISABLE_REFUSAL_DETECTION", "1"); } let got = detect("I cannot help with that."); - // SAFETY: serialized via ENV_LOCK across this module's tests. + // SAFETY: serialized via TEST_ENV_LOCK across this module's tests. unsafe { std::env::remove_var("ATLAS_DISABLE_REFUSAL_DETECTION"); } @@ -143,7 +144,7 @@ mod tests { #[test] fn no_terminator_falls_back_to_line() { - let _g = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _g = TEST_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); let r = detect("I cannot answer that\nnext paragraph").unwrap(); assert_eq!(r, "I cannot answer that"); } diff --git a/tests/test_openai_compat_v2.py b/tests/test_openai_compat_v2.py index 6f7f40142..db114037b 100644 --- a/tests/test_openai_compat_v2.py +++ b/tests/test_openai_compat_v2.py @@ -230,11 +230,11 @@ def section(label: str) -> None: max_completion_tokens=30, ) msg = r.choices[0].message - if msg.refusal is not None and (msg.content in (None, "")): - record("refusal populated + content nulled", PASS, f"refusal={msg.refusal!r}") + if msg.refusal is not None and bool(msg.content): + record("refusal populated + content preserved", PASS, f"refusal={msg.refusal!r}") else: record( - "refusal populated + content nulled", + "refusal populated + content preserved", FAIL, f"content={msg.content!r} refusal={msg.refusal!r}", )