update json format improvement

[lishixlnx]

some special character parse are missed, json special character should be parsed, only 4 char of \ " \n \r are parsed, but \t \b \f <0x20 char are not parsed correctly previously

[ramkrishna2910]

Review: centralized JSON escaping

The change is correct and a real improvement. Two sites in hunks this PR touches were missed, and they're the same bug class the PR sets out to fix — worth folding in before merge.

What I verified on the core change

• escapeJsonString() handles a strict superset of what each of the six replaced blocks did (\\ " \n \r, plus \t at the multimodal site), and additionally covers \b, \f, \t, and \u00XX for the remaining C0 control bytes.
• Iterating as unsigned char fixes a latent signed-char hazard in the old multimodal escaper for bytes >= 0x80. UTF-8 continuation bytes are still passed through byte-for-byte, which is correct.
• hex[(c >> 4) & 0x0f] is only reachable when c < 0x20, so it always emits a well-formed \u000X / \u001X.
• No double-escaping at the tool-call site: tool_call.arguments.dump() is compact and contains no raw control bytes, so escaping it fully is correct for embedding as a JSON string value.
• No other hand-rolled escaper survives elsewhere in the repo.

1. src/server.cpp:1157 and :1200 — client-controlled model_name interpolated raw (medium)

In handleResponses, model_name reaches the response.created and response.completed events unescaped, even though line 1194 in the same lambda was converted to use the new helper.

The value is fully client-controlled: model_name (1145) <- model (1110) <- request_json.value("model", model_id_).

POST /v1/responses {"model":"C:\\models\\foo","input":"hi","stream":true}
->  "model":"C:\models\foo"

\m is not a valid JSON escape, so the SSE event fails to parse and the stream breaks in the client SDK. A " in the model name additionally lets the caller inject arbitrary keys into the event object.

Suggested fix: escapeJsonString(model_name) at both 1157 and 1200.

2. src/server.cpp:830 — tool_call.name interpolated raw (low-medium)

In the handleChatCompletions streaming tool-call block, escaped_args was correctly routed through the helper on line 824, but tool_call.name two lines below is still concatenated raw:

"...\"function\":{\"name\":\"" + tool_call.name +
"\",\"arguments\":\"" + escaped_args + "\"}}]}..."

tool_call.name is arbitrary model output — src/tool_calls.cpp:35 and :86 assign it from tool_call_obj["name"] parsed out of the model's <tool_call> block, behind only an is_string() check. A hallucinated name containing " or \ emits a malformed SSE chunk that breaks the client's stream parse.

Suggested fix: escapeJsonString(tool_call.name).

Non-blocking cleanup

• Five auto escapeJson = [](const std::string& str) { return escapeJsonString(str); }; lambdas remain (lines 378, 460, 699, 782, 996) and are now pure pass-throughs. The one at 378 is reconstructed on every generated token.
• model_id_ is still interpolated unescaped at 388, 405, 829, 1021 and elsewhere. It's server-side (last path component of the CLI model path), so risk is low, but routing it through the same helper would close the class out entirely.

---

🤖 Generated with Claude Code

[lishixlnx]

the 2 problem in comments:

1. src/server.cpp:1157 and :1200 ---> this one is fixed.
2. src/server.cpp:830 — tool_call.name interpolated raw ---> this one is disappeared by improving the code.

[ramkrishna2910]

Review 2: follow-up commits (754a931..ce392b9)

Thanks for the quick turnaround on the first two items — I verified both:

• model_name (src/server.cpp:1136) — fixed. Escaping at the assignment point covers both call sites (1148, 1191), and the non-streaming branch at 1235 correctly still uses the unescaped model inside a json object. (Minor note: escaping at assignment rather than at interpolation means a future use of model_name inside a real json object would double-escape.)
• tool_call.name — resolved, and better than what I suggested. buildToolCallStreamChunks() builds a json object and .dump()s it, so escaping is structural now. It also fixed two things I hadn't flagged: index was hardcoded to 0 for every call in a multi-call response, and id was the literal "-".

The three commits also added ~540 lines of new parsing/repair logic, so here's a pass over that. Two blocking items, both confirmed by executing the algorithms rather than by inspection alone.

---

Blocking

1. src/tool_calls.cpp:253 — erase offsets are in original-text coordinates but applied to a shrinking string

offset accumulates positions in text, but eraseMatch mutates cleaned_text, which has already shrunk by every prior erase. Any response with 2+ tool calls corrupts. Reproduced:

in:  Sure. <tool_call>{"name":"a",...}</tool_call> and then <tool_call>{"name":"b",...}</tool_call> Done.
out: 'Sure.  and then <tool_call>{"name":"b","arguments":{}}</tool_call>'

Real prose (" Done.") is deleted and the second call's raw markup survives into cleaned_text. src/server.cpp:902 assigns that to content, so a non-streaming client receives tool-call markup as assistant text and loses actual output.

Same defect in the Mistral loop at line 303. The middle loop is safe — ([\s\S]+)$ can only match once.

Also worth noting the new clamp in eraseMatch (line 224) makes this quieter rather than safer: an out-of-range pos previously threw std::out_of_range and surfaced as a 500; it now silently truncates or no-ops.

Suggested fix: track a running removed delta, or build cleaned_text by appending match.prefix() segments instead of erasing in place.

2. src/tool_calls.cpp:19 — isValidJsonEscapeChar accepts u unconditionally, so C:\users\... drops the tool call

\u not followed by 4 hex digits fails strict json::parse. The sanitizer then declines to repair it because 'u' is on the valid list, balanceJsonBrackets can't help, and the call is dropped silently. Confirmed: {"path":"C:\users\me"} → Invalid \uXXXX escape. Affects \users, \utils, etc. — common on a Windows-targeted server.

Suggested fix: treat \u as valid only when followed by 4 hex digits.

One clarification, since it affects what the fix should be: the \t / \n / \b / \f case is a different problem and I would not fix it here. parseToolCallJsonObject:134 tries a strict parse first, and {"path":"C:\temp\notes.txt"} is valid JSON, so it succeeds and yields C:<TAB>emp<LF>otes.txt — the sanitizer never runs, and changing isValidJsonEscapeChar would not help. Making it "work" would mean preferring the sanitized reading over a successful strict parse, which would corrupt legitimate \n in string arguments (file contents, multi-line text). That's genuine ambiguity in what the model emitted; better left alone.

---

Non-blocking

• src/tool_calls.cpp:264 — balanceJsonBrackets can fabricate a structurally valid but semantically wrong call from a truncated generation: ...{"amount":123 becomes amount: 123 when the model was mid-way through emitting 12345. Previously a truncated call produced nothing (a visible failure); now it produces a plausible-looking wrong one.
• tools/test_tool_calls_repair.cpp — never builds. CMakeLists.txt:131-141 is an explicit SOURCES list with a single add_executable(amdgpu-server ...) and no test target, so the file is dead in CI. The documented build command is separately broken: the file #includes <nlohmann/json.hpp>, but the vendored header lives at external/json/json.hpp and is included elsewhere as <json.hpp>. Also, the first case's path (C:\Work\copilot\...) is composed entirely of invalid escapes, so it could not catch item 2 even if it did run — a case asserting on a C:\users\... path would.
• src/tool_calls.cpp:164 — json::parse(raw) is unreachable except for a literal null, since parseToolCallJsonObject already attempted it. Root cause is using is_null() as the failure sentinel, which conflates "parse failed" with "parsed to JSON null"; std::optional<json> would resolve it. Relatedly, the {"raw", raw} fallback rewrites a plain-string argument into an object the tool's declared schema doesn't have.
• src/tool_calls.cpp:173 — toolCallArgumentsString returns a JSON string verbatim instead of dump()ing it, so a double-encoded argument ships as "arguments":"C:/x" and breaks client-side JSON.parse. The is_object() || is_array() branch at 177-179 is also redundant with the fallthrough.

---

Pre-existing (not introduced by this PR)

Listing these separately so they aren't charged to this PR, but together they mean streaming tool calls likely don't work with standard clients today:

• Content chunks set "finish_reason":"stop" before the tool-call chunks are written, and no chunk ever carries "finish_reason":"tool_calls" (tool_calls.cpp:374 sets it to nullptr). OpenAI-compatible clients close the choice on stop and drop the trailing delta.
• src/server.cpp:816 discards extraction.second, so the raw <tool_call> markup has already been streamed to the user as content by the per-token callback.

Happy to open these as separate issues if you'd rather keep this PR scoped.

---

🤖 Generated with Claude Code

[lishixlnx]

2 blocking problems fixed.