Combined server enhancements: OpenAI API compliance, prompt caching, concurrency

[eloe]

Summary

Combined branch with all server enhancements developed on bastion. Consolidates 13 feature branches and 2 review passes (Claude Code + Copilot) into a single integration PR.

Features (issues → PRs)

Issue	PR	Feature
#1	#11	OpenAI Responses API — structured tool calling compliance
#2	#12	Prompt prefix caching — KV cache reuse, TTL eviction, TurboQuant compat
#3	#13	Concurrency guard — Metal GPU serialization via semaphore
#4	#14	finish_reason=tool_calls — correct finish_reason when tools detected
#5	#15	Stop sequences — support for both endpoints
#6	#16	Tool choice — enforce tool_choice parameter
#7	#17	Logprobs — /chat/completions logprobs support
#8	#18	JSON mode — response_format parameter
#9	#19	Context tracking — --max-context-tokens for OOM prevention
#10	#20	Request cancellation — timeout support for streaming/non-streaming

Additional features (no issue/PR yet)

11. Default max tokens (feature/default-max-tokens) — --default-max-tokens CLI flag
12. Default repetition penalty (feature/default-repetition-penalty) — MoE degeneration prevention
13. Tool template compat (feature/tool-template-compat) — Normalize Responses API tool format for Jinja templates

Production fixes in combined branch

• TurboQuant KV cache trim() for prefix reuse
• Stale KV cache recovery + TTL eviction
• Prompt cache key routing for OpenClaw/Hermes
• Cache state LRU eviction (max 64 entries)
• TOCTOU race fix with setdefault()

Review findings addressed

Claude Code review + security scan:

• Wired up --request-timeout to semaphore acquisition (was dead code)
• Moved semaphore before first yield in streaming (leak prevention)
• Added --verbose flag to gate debug prints (PII leakage)
• Wired up resolve_response_format in /v1/responses (JSON mode was dead)
• Added logprobs to combined branch (was missing)

Copilot review:

• Guarded sem.release() against UnboundLocalError/over-release (all 4 paths)
• Fixed mutable Pydantic defaults with Field(default_factory=list)
• Replaced eval with bash -c in validate_oc.sh
• Switched benchmark_models.py to argparse
• Await cleanup_task on shutdown

Test plan

• 83 unit tests passing (test_server.py + test_responses_api.py)
• Claude Code review (code quality + security)
• Copilot review (2 rounds)
• Verify on bastion with OpenClaw
• Merge to eloe/mlx-vlm main
• Submit individual PRs to Blaizzy/mlx-vlm upstream

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR bundles multiple server-side enhancements to improve OpenAI API compatibility (especially the Responses API), add KV prompt-prefix caching with TTL eviction, and serialize Metal/MLX generation to avoid GPU-state corruption under concurrency.

Changes:

• Implements /v1/responses request/response + SSE event schemas, adds previous_response_id replay via an in-memory LRU store, and expands test coverage for Responses API behavior.
• Adds prompt-prefix KV caching (per model and optional prompt_cache_key) including TTL-based eviction and TurboQuant-compatible trimming/validation.
• Introduces an asyncio semaphore-based concurrency guard for generation, plus stop-sequence handling and tool-choice enforcement; updates chat finish_reason to tool_calls when tool calls are detected.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
scripts/validate_oc.sh	Adds a bastion/OpenClaw end-to-end validation script covering health/models, Responses/Chat, streaming, and OC agent flows.
scripts/benchmark_models.py	Adds a simple benchmarking script for speed + agentic behaviors (tools/instructions/code).
mlx_vlm/tests/test_server.py	Adds stop-sequence unit tests ensuring stop is forwarded as eos_tokens.
mlx_vlm/tests/test_responses_api.py	Adds comprehensive Responses API compliance + caching/concurrency/finish_reason/context/timeout tests.
mlx_vlm/server.py	Core implementation: Responses endpoint + SSE streaming, prompt-cache routing/eviction, semaphore guard, stop/tool_choice helpers, context length checks, new CLI/env wiring.
mlx_vlm/responses_store.py	Adds an LRU ResponseStore to support previous_response_id replay.
mlx_vlm/responses_models.py	Adds Pydantic models for Responses API requests/responses and streaming events.
mlx_vlm/prompt_utils.py	Normalizes flat Responses-API tool definitions into nested “chat template” tool format.
mlx_vlm/generate.py	Extends prompt-cache state (timestamps, invalidation, cached token accounting) and improves cache reuse/trim/validation logic.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

response_format is accepted on ResponsesRequest, but it is never applied to chat_messages before templating. As a result, JSON mode requests ({"type":"json_object"}) won’t actually inject the required system instruction. Call resolve_response_format(chat_messages, request.response_format) (or equivalent) before apply_chat_template (and ideally after tool_choice instruction insertion) so the endpoint behavior matches the exposed API surface and tests.

[Copilot]

get_request_timeout() / REQUEST_TIMEOUT is defined and wired to CLI/env, but it is not used to enforce timeouts around generate() / stream_generate(). This means the advertised request cancellation/timeout feature is currently a no-op. Apply the timeout (e.g., asyncio.wait_for around generation, and/or enforce in the streaming loop) so requests are actually cancelled when exceeding the configured limit.

[Copilot]

In stream_responses_generator(), sem is created/acquired inside the try, but the finally unconditionally calls sem.release(). If an exception occurs before sem is assigned/acquired (e.g., during event construction/yielding), this will raise UnboundLocalError or over-release the semaphore, masking the original error and potentially breaking concurrency limits. Initialize sem=None + an acquired flag (or use an async context manager) and only release when successfully acquired.

[Copilot]

sem.release() is unconditional in the streaming chat_completions_endpoint generator. If await sem.acquire() fails or the generator exits early before acquiring, this can over-release the semaphore and corrupt concurrency accounting. Track acquisition state (or use an async context manager) and only release when acquisition succeeded.

[Copilot]

This Pydantic model uses a mutable default (tool_calls: List = []). Mutable defaults can be shared across instances, causing cross-request/state leakage. Use Field(default_factory=list) (and similarly for other list defaults) to ensure each instance gets its own list.

Suggested change

	tool_calls: List = []
	tool_calls: List = Field(default_factory=list)

[Copilot]

This Pydantic model uses a mutable default (content: List[ContentPartOutputText] = []). Use Field(default_factory=list) to avoid shared mutable defaults across instances.

Suggested change

	content: List[ContentPartOutputText] = []
	content: List[ContentPartOutputText] = Field(default_factory=list)

[Copilot]

eval is used to execute the constructed command string. Since HOST comes from user input and is interpolated into commands, this enables shell injection if a caller passes a malicious host value. Prefer avoiding eval by using arrays/positional args (or at least robust shell-quoting) when invoking ssh/curl.

[Copilot]

eval is also used for the check expression. Even if checks are currently constant strings, using eval here makes the function fragile and harder to reason about; it can also accidentally interpret output as shell syntax. Prefer explicit check commands (e.g., call python3 -c ... directly) without eval, or pass a callable function name instead of code.

Suggested change

	if echo "$output" | eval "$check" > /dev/null 2>&1; then
	if printf '%s\n' "$output" | grep -Fq -- "$check"; then

[Copilot]

Argument parsing assumes sys.argv[2] exists whenever --models is present. Running benchmark_models.py --models without a value will raise IndexError. Consider using argparse (consistent with other scripts) or at least guard len(sys.argv) > 2 and provide a helpful error message.

[Copilot]

The background cleanup_task is cancelled on shutdown but never awaited/joined. Depending on the event loop/uvicorn shutdown behavior, this can produce “Task was destroyed but it is pending” warnings and skip cleanup exception handling. After cleanup_task.cancel(), consider await cleanup_task (suppressing asyncio.CancelledError) to ensure a clean shutdown.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The prompt cache cleanup task is cancelled on shutdown but never awaited/handled. This can leave a pending task and produce "Task was destroyed but it is pending" warnings. Consider cancelling and then awaiting the task (suppressing asyncio.CancelledError) so shutdown is clean.

[Copilot]

REQUEST_TIMEOUT is documented as a per-request generation timeout, but the only enforcement is in acquire_semaphore() while waiting for the GPU semaphore. If the intent is request cancellation for long generations, you’ll need to apply the timeout around generate()/stream_generate iteration as well; otherwise consider renaming/rewording this setting to reflect it only limits queue wait time.

[Copilot]

Once a tool call start tag is detected, the streaming loop sets in_tool_call=True and then stops emitting any further text deltas permanently. If the model emits normal assistant text after the tool-call block, that suffix will be dropped from streaming output and from the final message (since display_text is derived from visible_text). Consider tracking tool_call_end and resuming text streaming after the tool-call region, or computing the final message text from process_tool_calls(full_text)['remaining_text'] to keep streaming/non-streaming behavior consistent.

[Copilot]

top_logprobs is accepted in the request model but never used anywhere in the endpoint implementation. Either implement returning the top-N alternatives per token when top_logprobs is set, or reject/ignore the field explicitly to avoid an API that silently does something different than callers expect.

Suggested change

	ge=0,
	le=20,
	@field_validator("top_logprobs")
	@classmethod
	def validate_top_logprobs_supported(cls, value):
	if value is not None:
	raise ValueError(
	"top_logprobs is not supported by this server and must be omitted."
	)
	return value

[Copilot]

This script evaluates command strings with eval, which makes the script fragile and opens command-injection footguns if variables like HOST or cmd contents ever become user-controlled. Prefer executing commands without eval (e.g., using bash arrays / direct pipelines) so quoting is correct and inputs can’t unexpectedly execute code.

[Copilot]

The check is also executed via eval, which can execute arbitrary code if the check expression is modified or receives unexpected input. Consider making checks explicit commands/functions (or passing a command array) instead of eval'ing a string.

[Copilot]

Mutable list default for annotations can be shared between instances. Prefer Field(default_factory=list) for annotations (and other list fields) to avoid cross-instance mutation bugs.

[Copilot]

ResponseMessageItem.content uses a mutable list default. Use Field(default_factory=list) so each ResponseMessageItem has an independent content list.

[Copilot]

Module docstring says the suite covers sections A–D, but the file now includes additional sections (Prompt Cache, Concurrency Guard, finish_reason, JSON mode, etc.). Update the docstring to reflect the expanded scope so it stays accurate.

Suggested change

	"""Tests for the OpenAI Responses API (/v1/responses) compliance.
	Covers:
	A. Model validation (pure unit tests, no server/mlx needed)
	B. Response store (pure unit tests)
	C. Functional endpoint tests (TestClient, mocked model)
	D. Streaming endpoint tests (TestClient, mocked model)
	"""Tests for OpenAI Responses API (/v1/responses) compliance.
	Covers the core test sections in this module, including:
	A. Model validation (pure unit tests, no server/mlx needed)
	B. Response store (pure unit tests)
	C. Functional endpoint tests (TestClient, mocked model)
	D. Streaming endpoint tests (TestClient, mocked model)
	Also covers additional Responses API behaviors and regressions exercised
	elsewhere in this file, such as prompt cache handling, concurrency guards,
	finish_reason behavior, JSON mode, and related compatibility checks.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

_verbose is computed once at import time. Because main() sets os.environ["VERBOSE"] only after the module has been imported, running the server with --verbose will still leave _verbose as False, so the new debug logging paths won’t activate. Consider either reading verbosity dynamically (call _is_verbose() at use sites) or updating the module-level _verbose after setting the env var in main() (or before any logging decisions are made).

Suggested change

	_verbose = _is_verbose()
	class _VerboseFlag:
	def __bool__(self) -> bool:
	return _is_verbose()
	_verbose = _VerboseFlag()

[Copilot]

top_logprobs is accepted on ChatRequest but never used when constructing the response. As-is, clients can request top_logprobs and get no effect, which is a silent API contract mismatch. Either implement top_logprobs (return top-N alternatives per token) or reject/validate unsupported combinations (e.g., return 400 if top_logprobs is set without full support).

Suggested change

	@field_validator("top_logprobs")
	@classmethod
	def validate_top_logprobs_supported(cls, value):
	if value is not None:
	raise ValueError(
	"`top_logprobs` is not supported by this server and must be omitted."
	)
	return value

[Copilot]

In /responses streaming, once tool_call_start_tag appears you set in_tool_call=True and then continue for all subsequent chunks, but there’s no logic to detect tool_call_end and resume streaming any remaining assistant text after the tool call markup. This can truncate streamed output_text compared to non-streaming mode (which uses process_tool_calls(...).remaining_text). Consider buffering and stripping tool-call spans (start→end) or toggling in_tool_call back to False when the end tag is seen so non-tool text after the call is still emitted.

[Copilot]

display_text = visible_text.strip() changes the model output by removing leading/trailing whitespace, which can make the final response.output_text.done / content_part.done text disagree with the streamed deltas (and with non-streaming output). It’s safer to preserve the exact accumulated text (no .strip()), and only post-process tool markup if needed.

Suggested change

	display_text = visible_text.strip()
	display_text = visible_text

[Copilot]

This unconditional print("Generation finished, cleared cache.") will emit a line per non-streaming /responses request, which can be noisy in production and adds overhead under load. Consider gating this behind the existing verbosity flag (or using structured logging with levels).

[Copilot]

stream_generate() prints cache reuse/validation failures unconditionally. In production these paths can be hit during normal operation (TTL eviction, model reloads, cache mismatches), potentially spamming stdout and making logs hard to use. Consider routing these through the server’s verbosity/logging mechanism (e.g., logger.debug) or making them conditional on an explicit debug flag.