feat: support OpenAI stop sequences - #535
Conversation
|
Real-world case this PR almost fixes — and a small extension that would close it completely. Hit today while validating Metal fmt=4 + the #488 grid fix under OpenCode (coli serve, GLM-5.2 744B, grouped-int4 g128, TEMP=0): the model answered a 7454-token-prefill request perfectly, closed its turn with <|user|> — one of GLM-5.2's three eos_token_ids — and then generation just kept going, hallucinating a fresh user turn and chatting with itself for the rest of ngen 4096: 1, 2, ..., 25<|user|>I have a list of strings. I want to be able to count how many strings... Why nobody stopped it, per the current code: The engine deliberately filters role markers from the stop set in serve mode (stops_arm_tok, the #401 tool-call-safety block) — my log: [stop] serve mode: filtered 17 non-EOS stop tokens (tool-call safety, #401) → 1 stop tokens: 154820. By design: #401 says the Python server owns these boundaries. So this PR removes blocker (3) and — nicely — actually ends the native slot via STOP instead of just hiding text. But it only acts on stops the client sends, and generic OpenAI clients (OpenCode, Cline, etc.) don't know GLM's role markers exist. Out of the box, the ramble above still happens with this PR merged. Request: when the client sends no stop, have the server inject GLM's role markers (<|user|>, <|observation|>) as default stop sequences. The server already knows them — it builds the prompt with those exact markers — and with this PR's chunk-boundary filtering + STOP machinery in place, it looks like a few lines on top rather than new machinery. It would finally implement the boundary ownership #401 promised, and your x_colibri_ignore_leading_stop already covers the leading-marker edge case that makes naive versions of this risky. Client-sent stop would of course override the defaults, keeping the strict-OpenAI contract intact. Without the default, every serve deployment silently burns up to ngen tokens of compute per turn whenever the model ends with <|user|> rather than <|endoftext|> — on consumer hardware streaming experts from disk, that's real minutes per request. Happy to test on macOS/Metal — that's the platform where this bit me. |
|
Note that this seems also to be the problem behind #549 |
|
Thanks — this is a real gap, and it matches the role-marker overrun we reproduced locally. I added commit 7187c7e: chat requests with no client stop list now use the template-owned |
|
Thanks for adding the GLM role markers. I am trying to get Colibri to be a useable server for the OpenCode client, and this work really helps! |
|
Good PR — real OpenAI stop-sequence support, well-tested, and the C side is a clean mirror of the existing One thing to call out explicitly on the rebase, because it changes default behavior: the PR injects |
|
Field validation on macOS/Metal (GLM-5.2 744B, grouped-int4 g128, coli serve + OpenCode, TEMP=0): applied all three commits onto my working branch. Re-ran the exact scenario from my earlier comment — 7454-token agent-header prefill plus a concurrent session-title request. Both turns now terminate cleanly at the role boundary: title correct, answer correct ("1, 2, … 25."), finish_reason: stop, no <|user|> leakage, no post-turn ramble, engine slot ends instead of burning ngen. Before this patch the same request self-conversed for 4096 tokens. This also resolves the mechanism behind #549 (same [stop] filtered 17 signature). LGTM — and suggest adding "Fixes #549" to the description. |
|
I've been chasing this clean interaction between OpenCode client and Colibri for quite some time, so I had to post a photo of it now that it has finally happened. Server log on the left, OpenCode client session on the right. 
|
7187c7e to
74326b9
Compare
|
Rebased onto current The patient/ignore-leading behavior remains intact: implicit GLM role markers are ignored only before useful content and terminate the native slot at a later boundary. Because Fresh validation passed on Linux and Windows UCRT64, including 64 focused OpenAI server tests, 167 Linux Python tests, 166 Windows Python tests, both native C suites, and two hash-identical |
…gg#597) Two self-contained gaps from mkelcb's serving report (JustVugg#597), both in the Python gateway, both behind explicit opt-ins: - JustVugg#597 item 2 — the DNS-rebinding Host guard accepted only loopback names and the bind address, so a legitimate reverse proxy / Tailscale MagicDNS host in front of the loopback bind was rejected with 403 and had no supported way to be trusted. Add `--allowed-host` (repeatable) and `COLI_ALLOWED_HOSTS` (comma-separated) that extend the allowlist. Default is unchanged (loopback + bind address only); no wildcard, each trusted Host is explicit, and this is orthogonal to CORS and API-key auth. - JustVugg#597 item 5 — the cold-prefill keepalive streamed a visible `reasoning_content: "."` every 10s, painting hundreds of dots in the client's reasoning panel over a minutes-long prefill. Default to an empty delta (still a valid SSE data event, still resets the client idle timer); COLI_VISIBLE_KEEPALIVE=1 restores the old visible dot for diagnosing keepalive delivery. Items 1 and 4 (non-streaming) of the same report are already fixed on dev by JustVugg#535 and JustVugg#568; items 3, 4-streaming and 6 are tracked separately. Tests: 4 new AllowedHostsTest cases (normalisation, trusted host accepted with port/case, untrusted rejected by default and with an allowlist). Full server suite 68/68. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…en 200 stream (JustVugg#597 item 6) A streaming chat request committed its HTTP 200 (SSE headers) before the engine tokenised the prompt and checked it against CTX. An over-long prompt was only then found -- ERROR CONTEXT_EXCEEDED -- far too late to send the 400 the client needs, so the client saw a broken/empty stream instead of a clean context_length_exceeded. (Non-streaming was already correct: it sends its 200 only after generation.) Add an acceptance frame to the engine<->gateway protocol (mkelcb's design in JustVugg#597): - c/colibri.c mux_submit(): after all cheap validation (encode, non-empty, fits CTX) and BEFORE prefill, emit `ACCEPT <id> <ntok>`. Every early failure already returns first with an ERROR, so a request yields exactly one of ACCEPT or an early ERROR. - Engine._dispatch_stdout(): parse ACCEPT -> ("accept", {...}) without popping pending. - Engine.generate(): new on_accept callback; commit exactly once on the first of ACCEPT / DATA / DONE. DATA/DONE before an ACCEPT still commit (an older engine with no ACCEPT frame keeps working); a second ACCEPT is a protocol error. - APIHandler.generation() streaming: defer the SSE 200 + keepalive into start_stream(), fired via on_accept. An ERROR that arrives before ACCEPT now propagates as an APIError with nothing committed, so do_POST returns a normal JSON 400. Tests: 4 Engine-level cases over a fake engine process (ACCEPT-before-DATA fires on_accept first; ERROR-before-ACCEPT raises 400 and never commits; DATA-before-ACCEPT still commits for an old engine; duplicate ACCEPT is a protocol error) + 1 end-to-end HTTP case (streaming context-exceeded returns a clean 400 with context_length_exceeded, not a 200 stream). Full server suite 83/83; the C engine builds clean. Completes the JustVugg#597 series (items 1/2/4/5 already on dev via JustVugg#535/JustVugg#606/JustVugg#607). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3 participants
Summary
stopstrings in chat and completion requestsSTOP <slot>through the native engine mux when a stop matches, preserving the normal DONE, usage, KV, and scheduler lifecyclex_colibri_ignore_leading_stoppatience<|user|>and<|observation|>role boundaries, patiently ignoring only leading markersWhy
Client-only filtering can hide a marker while expensive native generation continues. This change ends the matching native slot as soon as the server accepts a stop.
Generic OpenAI clients also do not know GLM's role markers. Without implicit chat boundaries, a completed GLM turn can emit
<|user|>and continue hallucinating a new conversation untilngenis exhausted.Fixes #549.
Behavior
x_colibri_ignore_leading_stop: trueis explicitly requested.devremain intact.Relationship to #572
PR #572 re-arms tokenizer-derived native stop tokens only when
COLI_SERVE_ALL_STOPS=1. This PR implements request-level OpenAI text stop sequences and context-aware GLM chat boundaries at the API/mux layer. It works with the #572 option disabled, so the two changes are complementary.Rebase
Rebased onto
JustVugg/colibri:devrevision5724dae33cf26191f91fe519a33e8a26b1ad66f9, including the current Anthropic endpoint, Inkling serving, and #572 behavior.Validation
make -C c check: complete native C suite passed; 167 Python tests passed with 13 expected skips.87: complete native C suite passed; 166 Python tests passed with 30 expected platform/feature skipsARCH=nativeWindows builds produced identical SHA-256:6f7b84a94f4f5a1f139d15da5bb308219b95d55deec9b895e010be45f38d4264