http: flatten content-part arrays before chat template render (#126)

[tlkahn]

Summary

Fixes #126: /v1/chat/completions no longer returns 400 template render failed when message content is a content-part array ([{"type":"text","text":"..."}]), the OpenAI SDK default.

Root cause

handle_chat_completions re-serialized the raw request messages subtree via yyjson_val_write and passed that into Jinja. Most HF chat templates treat message.content as a string; an array makes string ops (e.g. .strip()) fail.

parse_content already accepted both shapes into message_content_t, but the template path ignored the parsed form.

Fix

Build template messages_json from the parsed chat_completion_request_t:

1. message_content_text (core/types) - flatten CONTENT_STRING / CONTENT_PARTS / CONTENT_NONE to owned string or NULL
2. messages_template_serialize (core/openai) - yyjson mut serializer that always emits string-or-null content, plus role, optional name / tool_call_id / tool_calls
3. handle_chat_completions - use the serializer instead of raw yyjson_val_write on messages (tools_json unchanged)

Tests

Layer	Coverage
unit	flatten: string, none, single/multi parts, image skip, empty parts
unit	serialize: parts->string, null+tool_calls, name/tool_call_id, empty array
integration	HTTP POST parts array + multi-part + plain string with strip() template
gen	parts-normalized gen_build_chat_prompt token ids == plain string

Known limitation

Unparsed message keys the parser never retained (reasoning_content, etc.) are not passed through. The old raw path was the only pass-through; extending message_t is out of scope here. Vision/image parts are dropped (text-only flatten); vision input remains deferred v2.

Test plan

• ./tests/test_openai
• ./tests/test_http_generate
• ./tests/test_http_gen
• make test (44 passed, 0 failed)

[lgsunnyvale]

Code review: PR 127 - flatten content-part arrays before chat template render

Solid, correctly-scoped fix for a real SDK-compat break (#126). Root cause analysis matches the code: parse_content already normalized both shapes into message_content_t, but the template path bypassed that via raw yyjson_val_write. Building messages_json from the parsed DTO is the right long-term seam (not an ad-hoc walk of the request DOM).

Layering is clean:

• message_content_text (types) - pure flatten
• messages_template_serialize (openai) - template-facing DTO -> JSON
• handle_chat_completions - one call-site switch; tools_json correctly left on the raw path

Tests are above average for this class of bug: unit flatten + serialize, HTTP regression with a .strip() template (exactly the failure mode TRIVIAL_TMPL would mask), and gen-level token-id equivalence for multi-part concat. Empty CONTENT_PARTS -> "" vs CONTENT_NONE -> JSON null is the right Jinja distinction.

---

Medium - OOM on flatten is silently turned into content: null

In messages_template_serialize:

char *text = message_content_text(&msg->content);
if (text) {
    yyjson_mut_obj_add_strcpy(doc, o, "content", text);
    free(text);
} else {
    /* CONTENT_NONE, NULL string, or OOM on flatten: emit JSON null.
     * Serialize callers that need OOM distinction check kind first. */
    yyjson_mut_obj_add_null(doc, o, "content");
}

message_content_text returns NULL for both intentional absence (CONTENT_NONE, null string) and allocation failure (strdup / malloc). The comment tells callers to disambiguate by kind, but the only production caller (handle_chat_completions) does not: any serialize success is treated as OK, and OOM becomes a 200 with a wrong prompt rather than a 500.

Contract today makes OOM distinguishable without an out-param:

kind	return	meaning
CONTENT_NONE	NULL	intentional null
CONTENT_STRING, string == NULL	NULL	intentional null
CONTENT_STRING, string != NULL	NULL	OOM
CONTENT_PARTS	NULL	OOM (success is always non-NULL, including "")
CONTENT_PARTS	non-NULL	ok

Please fail the serialize (return NULL -> existing handler 500 path) on the OOM rows instead of emitting null. Something like:

char *text = message_content_text(&msg->content);
if (!text) {
    bool want_null = msg->content.kind == CONTENT_NONE ||
                     (msg->content.kind == CONTENT_STRING && !msg->content.string);
    if (!want_null)
        return NULL; /* OOM on flatten */
    yyjson_mut_obj_add_null(doc, o, "content");
} else {
    ...
}

Alternatively, change message_content_text to int message_content_text(..., char **out) with explicit -1 on OOM so the footgun cannot recur. Either way, the "callers must check kind" API note is a trap as long as the sole caller does not.

---

Medium (residual risk, acknowledged) - passthrough field regression

Moving off raw yyjson_val_write drops any message key the parser never retained (reasoning_content, vendor extensions, etc.). The PR body calls this out; calling it out here so it is not lost in merge:

• Old path: opaque passthrough of the client messages subtree into Jinja.
• New path: allowlisted fields only (role, flattened content, name, tool_call_id, tool_calls).

That is the correct default for a typed DTO boundary, but it is a behavioral change for multi-turn thinking / reasoning models whose templates read message.reasoning_content (jinja_cpp even has supports_preserve_reasoning). A client that round-trips assistant turns with reasoning will silently lose it post-fix.

Ask before merge:

1. Confirm no current supported chat template in the wild for mlxd depends on request-side reasoning_content (or similar) today.
2. File a follow-up to extend message_t (or an explicit passthrough bag) if thinking-model multi-turn is on the near-term roadmap - do not let this become an invisible footgun when someone enables reasoning round-trip later.

Not a blocker for #126 if (1) is true; still want the follow-up tracked.

---

Low - flatten ignores part.type and joins every non-NULL .text

for (int i = 0; i < c->part_count; i++) {
    if (c->parts[i].text)
        total += strlen(c->parts[i].text);
}

Today image_url parts have text == NULL, so behavior is fine. Safer contract for template input: only concatenate parts whose type is "text" (and non-NULL text). That matches the OpenAI content-part union and avoids accidentally folding ancillary text from future / unknown part types into the prompt. Cheap guard; worth doing while this helper is greenfield.

---

Low - test gaps

Coverage is good on the happy path; a few locks would harden the new API:

1. Empty string vs null: CONTENT_STRING + "" must serialize as JSON "", not null (templates that call .strip() accept "" and blow up on null). Not currently asserted in test_messages_template_serialize.
2. OOM / intentional-null distinction once the Medium above is fixed - even a white-box test that CONTENT_PARTS with a forced failure path returns NULL from serialize is enough.
3. HTTP multi-turn smoke with assistant tool_calls + tool role: unit serialize covers shape, but the handler path that actually feeds Jinja is only exercised for single user turns. Optional; unit coverage of tool_calls is already decent.

---

Nits (non-blocking)

1. Double copy on the string path: message_content_text strdups, then yyjson_mut_obj_add_strcpy copies again. For CONTENT_STRING, serialize can yyjson_mut_obj_add_strcpy(doc, o, "content", msg->content.string) directly and only heap-allocate for CONTENT_PARTS. Minor, but this sits on every chat completion.

2. tool_call_template_obj duplicates the tool_call object shape already inlined in chat_completion_response_serialize (~lines 518-528). Consider one static helper used by both; not required for this PR.

3. strdup in types.c: fine given the NULL guard; rest of openai.c parse path uses local dup_str. No functional issue - just noting the inconsistency if you ever unify alloc helpers.

4. Pre-existing, not introduced here: parse_content does not fail the request when json_str_dup OOMs on a string content (kind = CONTENT_STRING, string = NULL, return 0). That feeds the same null-content path. Fixing serialize OOM handling still leaves parse-time OOM soft-success; worth a separate hygiene pass.

---

What looks good (explicitly)

• Fix placement is at the right layer; no mlx/chat contamination.
• Handler error paths free msg_doc / messages_json / creq / doc consistently on the new branches.
• count == 0 and count > 0 && !msgs guards on serialize are sane.
• PARTS_TMPL "{{ messages[0].content.strip() }}" is the right regression probe - good comment explaining why TRIVIAL_TMPL alone is insufficient.
• Multi-part concat with no separator matches how SDKs split a single logical string across parts.
• Vision drop on flatten is consistent with deferred v2; returning "" for image-only parts is preferable to null for .strip()-style templates.
• PR description is accurate about tradeoffs; test plan claims match the added tests.

---

Verdict

Approve once the OOM-collapse-to-null issue is fixed (or a short justification why silent null is acceptable here, which I would push back on). The passthrough/reasoning_content residual risk should be confirmed + tracked, not necessarily fixed in this PR. Remaining items are low/nit.

This unblocks OpenAI-SDK clients (including pi) against string-content HF templates - thank you for the tight scope and the strip()-based regression test.

[tlkahn]

Thanks for the careful review. Landed fixes for the merge-blocking / hardening items on this branch (014bca9).

M1 - OOM on flatten silently becomes content: null (real; fixed)

Agreed this was a real contract bug at the sole production caller.

Refined null rule (slightly different from the proposed want_null patch):

kind	condition	serialize
CONTENT_NONE	-	JSON null (success)
CONTENT_STRING	string != NULL (incl. "")	JSON string
CONTENT_STRING	string == NULL	return NULL (parse-OOM shape)
CONTENT_PARTS	flatten non-NULL (incl. "")	JSON string
CONTENT_PARTS	flatten NULL	return NULL (OOM)

Intentional JSON null / absent content is parsed as CONTENT_NONE, not CONTENT_STRING + NULL. Treating STRING+NULL as intentional null would preserve the parse-OOM soft-success footgun on the template path. Handler already maps serialize NULL -> HTTP 500 "out of memory". No handler change needed.

Also dropped the intermediate strdup on the string path (N1) by switching on kind and add_strcpy directly.

M2 - passthrough / reasoning_content (confirm + track)

(1) Confirmed: no current supported request-side dependency.

• message_t has no reasoning_content field.
• Thinking control is request-level enable_thinking -> extra_json, not per-message reasoning payload.
• supports_preserve_reasoning is only jinja_cpp capability probing; nothing in mlxd populates that field on inbound messages.
• No fixture/template exercises request-side reasoning_content round-trip.

Safe for #126 merge from that angle.

(2) Follow-up (not this PR): when multi-turn thinking / reasoning round-trip lands, extend message_t (or an explicit allowlisted bag) so template serialize can emit reasoning_content (and any other template-facing fields). Do not re-open raw yyjson_val_write of the full messages subtree - that re-breaks #126. Keep the typed DTO boundary.

L1 - join only type == "text" (fixed as defense-in-depth)

Agreed as hardening. Today's well-formed image_url path was already safe because parse leaves .text == NULL when the field is absent; the filter still blocks unknown types / spurious text on non-text parts. Local content_part_is_text gates both length and copy passes.

L2 tests

• L2.1 locked: CONTENT_STRING + "" serializes to JSON "" (not null); flatten returns non-NULL "".
• L2.2 locked: CONTENT_STRING + string == NULL makes messages_template_serialize return NULL; CONTENT_NONE still succeeds with JSON null; multi-message with a trailing OOM-shape message fails the whole serialize.
• L2.3 (HTTP multi-turn tool_calls smoke): declining as required work. test_messages_template_serialize already locks assistant tool_calls + tool role + tool_call_id + name on the exact serializer the handler calls. Handler change is "call serializer, write JSON, free" - not a second encoding. Happy to revisit if a later handler change re-introduces a second messages path.

Nits

• N1: folded into the M1 serialize switch (direct add_strcpy).
• N2: tool_call_template_obj vs response serialize dedup - deferred (cleanup; shapes match today; risk of wire-format drift if rushed).
• N3: strdup in types.c vs local dup_str in openai.c - non-defect; different modules, no functional issue.
• N4: parse-layer string OOM soft-success is pre-existing; out of scope here. M1 partially mitigates the template path (500 instead of wrong null content). Not claiming full parse-layer OOM hygiene is done.

Coverage / proof

• Extended test_message_content_text + test_messages_template_serialize in tests/test_openai.c.
• ./tests/test_openai, ./tests/test_http_generate, and full make test green (44 passed).
• ASan/UBSan clean on test_openai.