Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions bindings/python/src/smg/serve.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@
import logging
import os
import random
import shutil
import signal
import tempfile
import socket
import subprocess
import sys
Expand Down Expand Up @@ -550,6 +552,7 @@ def __init__(self, backend: str, args: argparse.Namespace, backend_args: list[st
self.launcher: WorkerLauncher = BACKEND_LAUNCHERS[backend]()
self.workers: list[tuple[subprocess.Popen, int]] = []
self._shutting_down = False
self._prometheus_dir: str | None = None

# -- public API ---------------------------------------------------------

Expand All @@ -571,6 +574,15 @@ def run(self) -> None:
def _launch_workers(self) -> None:
ports = _find_available_ports(self.args.worker_base_port, self.args.data_parallel_size)
host = self.args.worker_host

if getattr(self.args, "connection_mode", "grpc") == "grpc":
self._prometheus_dir = tempfile.mkdtemp(prefix="smg_prometheus_")
os.environ["PROMETHEUS_MULTIPROC_DIR"] = self._prometheus_dir
logger.info(
"Set PROMETHEUS_MULTIPROC_DIR=%s for gRPC metrics collection",
self._prometheus_dir,
)

for dp_rank, port in enumerate(ports):
env = self.launcher.gpu_env(self.args, dp_rank)
proc = self.launcher.launch(self.args, self.backend_args, host, port, env)
Expand Down Expand Up @@ -641,6 +653,23 @@ def _cleanup_workers(self) -> None:
except (ProcessLookupError, OSError):
pass

self._cleanup_prometheus_dir()

def _cleanup_prometheus_dir(self) -> None:
"""Remove the temporary prometheus multiprocess directory and its .db files."""
if self._prometheus_dir is None:
return
try:
shutil.rmtree(self._prometheus_dir)
logger.info("Cleaned up PROMETHEUS_MULTIPROC_DIR=%s", self._prometheus_dir)
except OSError as e:
logger.warning(
"Failed to clean up PROMETHEUS_MULTIPROC_DIR=%s: %s",
self._prometheus_dir,
e,
)
self._prometheus_dir = None


# ---------------------------------------------------------------------------
# Entry point
Expand Down
14 changes: 14 additions & 0 deletions crates/reasoning_parser/src/parsers/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,12 @@ impl ReasoningParser for BaseReasoningParser {
.to_string();

if !processed_text.contains(&self.config.think_end_token) {
// Don't consume tool call markers as reasoning content
if let Some(tool_pos) = processed_text.find("<|tool_calls_section_begin|>") {
let reasoning_text = processed_text[..tool_pos].trim().to_string();
let normal_text = processed_text[tool_pos..].to_string();
return Ok(ParserResult::new(normal_text, reasoning_text));
}
Comment on lines +67 to +71

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The tool call section marker <|tool_calls_section_begin|> is hardcoded in the BaseReasoningParser. This marker is specific to certain models (like Kimi) and its presence in the base parser violates modularity. It should ideally be part of the ParserConfig so it can be customized per model without modifying the base implementation.

// Assume reasoning was truncated before end token
return Ok(ParserResult::reasoning(processed_text));
}
Expand Down Expand Up @@ -133,6 +139,14 @@ impl ReasoningParser for BaseReasoningParser {

// Continue with reasoning content
if self.in_reasoning && self.config.stream_reasoning {
// Some models skip </think> and go straight to tool calls
if let Some(tool_pos) = current_text.find("<|tool_calls_section_begin|>") {
let reasoning_text = current_text[..tool_pos].trim().to_string();
let normal_text = current_text[tool_pos..].to_string();
self.buffer.clear();
self.in_reasoning = false;
return Ok(ParserResult::new(normal_text, reasoning_text));
}
// Stream the content immediately
let reasoning_text = current_text;
self.buffer.clear();
Expand Down
31 changes: 16 additions & 15 deletions crates/tool_parser/src/parsers/kimik2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,16 +57,15 @@ impl KimiK2Parser {
reason = "regex patterns are compile-time string literals"
)]
pub fn new() -> Self {
// Pattern for complete tool calls
let tool_call_pattern = r"<\|tool_call_begin\|>\s*(?P<tool_call_id>[\w\.]+:\d+)\s*<\|tool_call_argument_begin\|>\s*(?P<function_arguments>\{.*?\})\s*<\|tool_call_end\|>";
// Supports alternative delimiters: <|func_start|>/<|func_end|>; (?s) for multi-line JSON
let tool_call_pattern = r"(?s)<\|tool_call_begin\|>\s*(?P<tool_call_id>[\w\.]+:\d+)\s*(?:<\|tool_call_argument_begin\|>\s*|<\|func_start\|>\s*)?(?P<function_arguments>\{.*?\})\s*(?:<\|tool_call_end\|>|<\|func_end\|>)";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The regex pattern \{.*?\} for function_arguments will fail to correctly capture nested JSON structures because the non-greedy match stops at the first encountered closing brace }. For streaming data where the buffer is cleared after each complete item, a greedy regex (.*) should be used to capture all accumulated content for the arguments until the end marker.

Suggested change
let tool_call_pattern = r"(?s)<\|tool_call_begin\|>\s*(?P<tool_call_id>[\w\.]+:\d+)\s*(?:<\|tool_call_argument_begin\|>\s*|<\|func_start\|>\s*)?(?P<function_arguments>\{.*?\})\s*(?:<\|tool_call_end\|>|<\|func_end\|>)";
let tool_call_pattern = r"(?s)<\|tool_call_begin\|>\s*(?P<tool_call_id>[\w\.]+:\d+)\s*(?:<\|tool_call_argument_begin\|>\s*|<\|func_start\|>\s*)?(?P<function_arguments>\{.*)\s*(?:<\|tool_call_end\|>|<\|func_end\|>)";
References
  1. When parsing streaming data where the buffer is cleared after each complete item, a greedy regex (.*) can be intentionally used to capture all accumulated content for the current item's arguments.

let tool_call_extractor = Regex::new(tool_call_pattern).expect("Valid regex pattern");

// Pattern for streaming (partial) tool calls
let stream_pattern = r"<\|tool_call_begin\|>\s*(?P<tool_call_id>[\w\.]+:\d+)\s*<\|tool_call_argument_begin\|>\s*(?P<function_arguments>\{.*)";
let stream_pattern = r"(?s)<\|tool_call_begin\|>\s*(?P<tool_call_id>[\w\.]+:\d+)\s*(?:<\|tool_call_argument_begin\|>\s*|<\|func_start\|>\s*)?(?P<function_arguments>\{.*)";
let stream_tool_call_extractor = Regex::new(stream_pattern).expect("Valid regex pattern");

// Pattern for removing completed tool calls
let end_pattern = r"<\|tool_call_begin\|>.*?<\|tool_call_end\|>";
let end_pattern = r"<\|tool_call_begin\|>.*?(?:<\|tool_call_end\|>|<\|func_end\|>)";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Make Kimi end-token regex handle multiline arguments

The stream parser now supports multiline JSON via (?s) in the start/capture regexes, but this end-pattern still does not use dotall mode. For multiline arguments, tool_call_end_pattern.find(...) can fail to match completed calls, and parse_incremental then clears the entire buffer, which drops any trailing content (including a following tool call) from the same chunk.

Useful? React with 👍 / 👎.

let tool_call_end_pattern = Regex::new(end_pattern).expect("Valid regex pattern");
Comment on lines 67 to 69

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, locate the kimik2.rs file
fd -t f 'kimik2\.rs'

Repository: lightseekorg/smg

Length of output: 147


🏁 Script executed:

# Read the relevant sections of kimik2.rs
cat -n crates/tool_parser/src/parsers/kimik2.rs | sed -n '50,100p'

Repository: lightseekorg/smg

Length of output: 2682


🏁 Script executed:

# Now read the buffer-drain logic around lines 280-300
cat -n crates/tool_parser/src/parsers/kimik2.rs | sed -n '280,310p'

Repository: lightseekorg/smg

Length of output: 1879


🏁 Script executed:

# Check lines 260-275 to verify the completeness check
cat -n crates/tool_parser/src/parsers/kimik2.rs | sed -n '260,275p'

Repository: lightseekorg/smg

Length of output: 1002


🏁 Script executed:

# Let me verify the proposed fix by checking if there are any other usages of tool_call_end_pattern
rg -n "tool_call_end_pattern" crates/tool_parser/src/parsers/kimik2.rs

Repository: lightseekorg/smg

Length of output: 327


🏁 Script executed:

# Check test file to understand expected multi-line scenarios
cat -n crates/tool_parser/tests/tool_parser_kimik2.rs | head -100

Repository: lightseekorg/smg

Length of output: 3994


🏁 Script executed:

# Look for tests with multi-line JSON arguments
rg -n "\\\\n" crates/tool_parser/tests/tool_parser_kimik2.rs | head -20

Repository: lightseekorg/smg

Length of output: 264


🏁 Script executed:

# Search for tests with actual newlines inside JSON payloads
rg -B5 -A5 'tool_call_argument_begin.*?\{' crates/tool_parser/tests/tool_parser_kimik2.rs | head -50

Repository: lightseekorg/smg

Length of output: 2092


🏁 Script executed:

# Verify the Regex behavior by checking if there are any multiline payload tests
rg -n "func_end" crates/tool_parser/tests/tool_parser_kimik2.rs

Repository: lightseekorg/smg

Length of output: 42


🏁 Script executed:

# Check if there are any tests using <|func_start|>/<|func_end|> delimiters
rg -n "func_start" crates/tool_parser/tests/tool_parser_kimik2.rs

Repository: lightseekorg/smg

Length of output: 42


🏁 Script executed:

# Check the is_complete_json logic to see when buffer drain is triggered
cat -n crates/tool_parser/src/parsers/kimik2.rs | sed -n '270,295p'

Repository: lightseekorg/smg

Length of output: 1722


Add (?s) flag to tool_call_end_pattern to match multi-line JSON arguments.

The extractor regexes at lines 61 and 64 use (?s) to accept multi-line arguments, but tool_call_end_pattern at line 68 does not. When a completed tool call spans multiple lines, tool_call_end_pattern.find() at line 292 fails to match (since . doesn't cross newlines), causing line 296 to clear the entire buffer and lose trailing content or subsequent tool calls.

🐛 Fix
-        let end_pattern = r"<\|tool_call_begin\|>.*?(?:<\|tool_call_end\|>|<\|func_end\|>)";
+        let end_pattern = r"(?s)<\|tool_call_begin\|>.*?(?:<\|tool_call_end\|>|<\|func_end\|>)";
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/tool_parser/src/parsers/kimik2.rs` around lines 67 - 69, The regex
stored in end_pattern used to build tool_call_end_pattern lacks the (?s) DOTALL
flag, so Regex::new(end_pattern) in tool_call_end_pattern doesn't match
multi-line tool call bodies; update end_pattern to include the (?s) prefix
(e.g.,
"(?s)<\\|tool_call_begin\\|>.*?(?:<\\|tool_call_end\\|>|<\\|func_end\\|>)") and
rebuild tool_call_end_pattern with Regex::new so tool_call_end_pattern.find(...)
will correctly match tool calls that span multiple lines.


// Robust parser for ids like "functions.search:0" or fallback "search:0"
Expand Down Expand Up @@ -181,7 +180,7 @@ impl ToolParser for KimiK2Parser {
// No tool markers detected - return all buffered content as normal text
let mut normal_text = std::mem::take(&mut self.buffer);
// Remove end tokens if present
for e_token in ["<|tool_calls_section_end|>", "<|tool_call_end|>"] {
for e_token in ["<|tool_calls_section_end|>", "<|tool_call_end|>", "<|func_end|>"] {
normal_text = normal_text.replace(e_token, "");
}
return Ok(StreamingParseResult {
Expand Down Expand Up @@ -242,13 +241,14 @@ impl ToolParser for KimiK2Parser {
function_args
};

// Split by end token before sending (like Python does)
let parsed_args_diff =
if let Some(pos) = argument_diff.find("<|tool_call_end|>") {
&argument_diff[..pos]
} else {
argument_diff
};
// Split by end token before sending
let end_pos = argument_diff.find("<|tool_call_end|>")
.or_else(|| argument_diff.find("<|func_end|>"));
let parsed_args_diff = if let Some(pos) = end_pos {
&argument_diff[..pos]
} else {
argument_diff
};

if !parsed_args_diff.is_empty() {
calls.push(ToolCallItem {
Expand All @@ -265,8 +265,9 @@ impl ToolParser for KimiK2Parser {
}

// Check completeness - split by end token first
let parsed_args = if let Some(pos) = function_args.find("<|tool_call_end|>")
{
let end_pos2 = function_args.find("<|tool_call_end|>")
.or_else(|| function_args.find("<|func_end|>"));
let parsed_args = if let Some(pos) = end_pos2 {
&function_args[..pos]
} else {
function_args
Expand Down
79 changes: 68 additions & 11 deletions model_gateway/src/core/worker_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use tokio::{
sync::{watch, Mutex},
task::JoinHandle,
};
use tracing::{debug, info};
use tracing::{debug, info, warn};

use crate::{
core::{
Expand Down Expand Up @@ -85,6 +85,36 @@ impl IntoResponse for EngineMetricsResult {
}
}

/// Collect gRPC worker metrics by aggregating `PROMETHEUS_MULTIPROC_DIR` via a python3 subprocess.
async fn collect_prometheus_multiproc_metrics() -> Result<String, String> {
let dir = std::env::var("PROMETHEUS_MULTIPROC_DIR").map_err(|_| {
"PROMETHEUS_MULTIPROC_DIR not set; cannot collect metrics from gRPC workers".to_string()
})?;

let output = tokio::process::Command::new("python3")
.args([
"-c",
"import sys\n\
from prometheus_client import CollectorRegistry, generate_latest\n\
from prometheus_client.multiprocess import MultiProcessCollector\n\
registry = CollectorRegistry()\n\
MultiProcessCollector(registry)\n\
sys.stdout.buffer.write(generate_latest(registry))\n",
])
.env("PROMETHEUS_MULTIPROC_DIR", &dir)
.output()
.await
.map_err(|e| format!("failed to run python3 prometheus collector: {e}"))?;

if !output.status.success() {
let stderr = String::from_utf8_lossy(&output.stderr);
return Err(format!("python3 prometheus collector failed: {stderr}"));
}

String::from_utf8(output.stdout)
.map_err(|e| format!("prometheus collector output is not valid UTF-8: {e}"))
}
Comment on lines +89 to +116

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Spawning a python3 subprocess for every metrics scrape is inefficient and introduces a heavy runtime dependency. Consider performing this aggregation natively in Rust. If parsing is required, prefer using a dedicated library like prometheus-parse over manual string manipulation or external scripts to ensure robustness.

References
  1. When parsing Prometheus metrics, prefer using a dedicated library (e.g., prometheus-parse) over fragile string manipulation methods to ensure robustness.

Comment on lines +88 to +116

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Runtime dependency on Python requires documentation.

This function introduces a runtime dependency on python3 being available in PATH and the prometheus_client package being installed. While this aligns with the Python orchestrator in serve.py that sets up PROMETHEUS_MULTIPROC_DIR, the dependency should be documented.

Consider adding a doc comment noting the prerequisites:

📝 Suggested documentation
-/// Collect gRPC worker metrics by aggregating `PROMETHEUS_MULTIPROC_DIR` via a python3 subprocess.
+/// Collect gRPC worker metrics by aggregating `PROMETHEUS_MULTIPROC_DIR` via a python3 subprocess.
+///
+/// # Prerequisites
+/// - `PROMETHEUS_MULTIPROC_DIR` environment variable must be set (typically by the Python orchestrator)
+/// - `python3` must be available in PATH
+/// - `prometheus_client` Python package must be installed
+///
+/// # Errors
+/// Returns an error string if the env var is missing, subprocess fails, or output is not valid UTF-8.
 async fn collect_prometheus_multiproc_metrics() -> Result<String, String> {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// Collect gRPC worker metrics by aggregating `PROMETHEUS_MULTIPROC_DIR` via a python3 subprocess.
async fn collect_prometheus_multiproc_metrics() -> Result<String, String> {
let dir = std::env::var("PROMETHEUS_MULTIPROC_DIR").map_err(|_| {
"PROMETHEUS_MULTIPROC_DIR not set; cannot collect metrics from gRPC workers".to_string()
})?;
let output = tokio::process::Command::new("python3")
.args([
"-c",
"import sys\n\
from prometheus_client import CollectorRegistry, generate_latest\n\
from prometheus_client.multiprocess import MultiProcessCollector\n\
registry = CollectorRegistry()\n\
MultiProcessCollector(registry)\n\
sys.stdout.buffer.write(generate_latest(registry))\n",
])
.env("PROMETHEUS_MULTIPROC_DIR", &dir)
.output()
.await
.map_err(|e| format!("failed to run python3 prometheus collector: {e}"))?;
if !output.status.success() {
let stderr = String::from_utf8_lossy(&output.stderr);
return Err(format!("python3 prometheus collector failed: {stderr}"));
}
String::from_utf8(output.stdout)
.map_err(|e| format!("prometheus collector output is not valid UTF-8: {e}"))
}
/// Collect gRPC worker metrics by aggregating `PROMETHEUS_MULTIPROC_DIR` via a python3 subprocess.
///
/// # Prerequisites
/// - `PROMETHEUS_MULTIPROC_DIR` environment variable must be set (typically by the Python orchestrator)
/// - `python3` must be available in PATH
/// - `prometheus_client` Python package must be installed
///
/// # Errors
/// Returns an error string if the env var is missing, subprocess fails, or output is not valid UTF-8.
async fn collect_prometheus_multiproc_metrics() -> Result<String, String> {
let dir = std::env::var("PROMETHEUS_MULTIPROC_DIR").map_err(|_| {
"PROMETHEUS_MULTIPROC_DIR not set; cannot collect metrics from gRPC workers".to_string()
})?;
let output = tokio::process::Command::new("python3")
.args([
"-c",
"import sys\n\
from prometheus_client import CollectorRegistry, generate_latest\n\
from prometheus_client.multiprocess import MultiProcessCollector\n\
registry = CollectorRegistry()\n\
MultiProcessCollector(registry)\n\
sys.stdout.buffer.write(generate_latest(registry))\n",
])
.env("PROMETHEUS_MULTIPROC_DIR", &dir)
.output()
.await
.map_err(|e| format!("failed to run python3 prometheus collector: {e}"))?;
if !output.status.success() {
let stderr = String::from_utf8_lossy(&output.stderr);
return Err(format!("python3 prometheus collector failed: {stderr}"));
}
String::from_utf8(output.stdout)
.map_err(|e| format!("prometheus collector output is not valid UTF-8: {e}"))
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@model_gateway/src/core/worker_manager.rs` around lines 88 - 116, Add a doc
comment to the collect_prometheus_multiproc_metrics function stating its runtime
prerequisites: that a python3 executable must be available on PATH, the Python
prometheus_client package (with multiprocess support) must be installed, and
PROMETHEUS_MULTIPROC_DIR must be set and pointing to the multiprocess metrics
directory; include a brief note on expected failure modes (missing python3,
missing package, or unset env) so callers know why this function can return an
Err and how to remediate.


Comment on lines +88 to +117

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit: This spawns a python3 subprocess on every /metrics scrape. If Prometheus is polling at a typical 15s interval, that's 4 process spawns per minute (each importing prometheus_client). Probably fine for now, but worth keeping an eye on latency. If it becomes a bottleneck, consider caching the output for a short TTL or using a long-running sidecar.

pub struct WorkerManager;

impl WorkerManager {
Expand Down Expand Up @@ -273,22 +303,49 @@ impl WorkerManager {
return EngineMetricsResult::Err("No available workers".to_string());
}

let responses = fan_out(&workers, client, "metrics", reqwest::Method::GET).await;

let mut metric_packs = Vec::new();
for resp in responses {
if let Ok(r) = resp.result {
if r.status().is_success() {
if let Ok(text) = r.text().await {
metric_packs.push(MetricPack {
labels: vec![("worker_addr".into(), resp.url)],
metrics_text: text,
});

let http_workers: Vec<_> = workers
.iter()
.filter(|w| matches!(w.connection_mode(), ConnectionMode::Http))
.cloned()
.collect();
let has_grpc = workers
.iter()
.any(|w| matches!(w.connection_mode(), ConnectionMode::Grpc));

if !http_workers.is_empty() {
let responses = fan_out(&http_workers, client, "metrics", reqwest::Method::GET).await;
for resp in responses {
if let Ok(r) = resp.result {
if r.status().is_success() {
if let Ok(text) = r.text().await {
metric_packs.push(MetricPack {
labels: vec![("worker_addr".into(), resp.url)],
metrics_text: text,
});
}
}
}
}
}

if has_grpc {
match collect_prometheus_multiproc_metrics().await {
Ok(text) => {
metric_packs.push(MetricPack {
labels: vec![],
metrics_text: text,
});
}
Err(e) => {
warn!(
"Failed to collect gRPC worker metrics from PROMETHEUS_MULTIPROC_DIR: {e}"
);
}
}
}

if metric_packs.is_empty() {
return EngineMetricsResult::Err("All backend requests failed".to_string());
}
Expand Down
93 changes: 89 additions & 4 deletions model_gateway/src/routers/grpc/regular/processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,16 @@ impl ResponseProcessor {
_ => false,
};

if used_json_schema {
if self.configured_tool_parser.is_some() && tool_parser_available {
// Explicitly configured parser takes priority (models may emit native tokens regardless of tool_choice)
(tool_calls, processed_text) = self
.parse_tool_calls(
&processed_text,
&original_request.model,
history_tool_calls_count,
)
.await;
} else if used_json_schema {
(tool_calls, processed_text) = utils::parse_json_schema_response(
&processed_text,
original_request.tool_choice.as_ref(),
Expand Down Expand Up @@ -183,7 +192,58 @@ impl ResponseProcessor {
utils::convert_proto_to_openai_logprobs(proto_logprobs, tokenizer)
});

// Step 5: Build ChatCompletionMessage (proper response message type)
// Strip leaked chatml tokens only when a model-specific parser is configured
if self.configured_tool_parser.is_some() || self.configured_reasoning_parser.is_some() {
for token in [
"<|im_end|>",
"<|im_start|>",
"<|im_user|>",
"<|im_assistant|>",
"<|im_system|>",
"<|im_middle|>",
] {
processed_text = processed_text.replace(token, "");
}
processed_text = processed_text.trim().to_string();
Comment on lines +196 to +207

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit: This chatml-stripping block is now duplicated in 4 places (here, line ~720 in this file, and two sites in streaming.rs) with subtly different token lists — the streaming "regular content emission" path also strips </think> but these don't. Consider extracting a shared strip_leaked_tokens() helper with a single canonical token list to prevent drift between the sites.

}
Comment on lines +196 to +208

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic for stripping leaked ChatML tokens is duplicated across multiple files and functions. This duplication makes the code harder to maintain. Extract this duplicated logic into a shared helper function to improve maintainability and reduce redundancy.

References
  1. Extract duplicated logic into a shared helper function to improve maintainability and reduce redundancy.


let is_json_response = matches!(
&original_request.response_format,
Some(openai_protocol::common::ResponseFormat::JsonObject)
| Some(openai_protocol::common::ResponseFormat::JsonSchema { .. })
);
if is_json_response {
if processed_text.starts_with("```json") || processed_text.starts_with("```JSON") {
if let Some(start) = processed_text.find('\n') {
let inner = &processed_text[start + 1..];
if let Some(end) = inner.rfind("```") {
processed_text = inner[..end].trim().to_string();
}
}
}

if processed_text.starts_with('{') {
let mut depth = 0i32;
let mut in_string = false;
let mut escape = false;
let mut json_end = None;
for (i, ch) in processed_text.char_indices() {
if escape { escape = false; continue; }
if ch == '\\' && in_string { escape = true; continue; }
if ch == '"' { in_string = !in_string; continue; }
if in_string { continue; }
if ch == '{' { depth += 1; } else if ch == '}' {
depth -= 1;
if depth == 0 { json_end = Some(i + 1); break; }
}
}
if let Some(end) = json_end {
processed_text = processed_text[..end].to_string();
}
}
Comment on lines +215 to +243

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit: Two minor edge cases in the JSON cleanup:

  1. rfind("```") (line 219) could match triple-backticks inside a JSON string value, truncating the response. Unlikely but possible.
  2. The depth-counting trimmer only handles { roots — a JSON Schema response that is an array ([...]) would not be trimmed. Most schemas produce objects, but JsonObject format technically allows arrays.

Neither is likely to bite in practice, just flagging for awareness.

}

// Build ChatCompletionMessage (proper response message type)
let chat_message = ChatCompletionMessage {
role: "assistant".to_string(),
content: if processed_text.is_empty() {
Expand Down Expand Up @@ -621,7 +681,17 @@ impl ResponseProcessor {
Some(messages::ToolChoice::Tool { .. } | messages::ToolChoice::Any { .. })
);

if used_json_schema {
if self.configured_tool_parser.is_some() && tool_parser_available {
(tool_calls, processed_text) = self
.parse_tool_calls(
&processed_text,
&messages_request.model,
utils::message_utils::get_history_tool_calls_count_messages(
&messages_request,
),
)
.await;
} else if used_json_schema {
// Bridge Messages ToolChoice to Chat ToolChoice for reuse
let chat_tool_choice = messages_request
.tool_choice
Expand All @@ -647,7 +717,22 @@ impl ResponseProcessor {
}
}

// Step 3: Build content blocks
// Strip leaked chatml tokens only when a model-specific parser is configured
if self.configured_tool_parser.is_some() || self.configured_reasoning_parser.is_some() {
for token in [
"<|im_end|>",
"<|im_start|>",
"<|im_user|>",
"<|im_assistant|>",
"<|im_system|>",
"<|im_middle|>",
] {
processed_text = processed_text.replace(token, "");
}
processed_text = processed_text.trim().to_string();
}

// Build content blocks
let mut content_blocks: Vec<messages::ContentBlock> = Vec::new();

// Thinking block first (if present)
Expand Down
Loading
Loading