[llm_server] Fix transcript replay and reasoning response handling

[mergennachin]

Fall back to rendered text when the assistant boundary cannot be verified. Compare supplied reasoning against the finalized client-visible response before replaying stored tokens, preserving omitted reasoning while invalidating explicit edits.

Discard whitespace-only Muse Glimmer blocks and reject non-boolean return_reasoning values before generation. Document the response contract and verify complete and streaming turns with full prompt assertions.

Run serving tests for relevant pull requests, including the Muse adapter tests. Make tokenizer paths explicit and cover exact BPE prompt assembly with a portable fixture.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22852

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit cabb4b7 with merge base edad0a7 ():

NEW FAILURE - The following job has failed:

• MLX / backend-tester (operators) / test-mlx-backend-operators (gh)
  ##[error]error: RPC failed; curl 28 Recv failure: Operation timed out

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[github-actions]

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

[Copilot]

🟡 Changes recommended

Four unresolved moderate findings affect CI path matching and reasoning replay/streaming consistency.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR improves LLM-server reasoning handling, transcript replay, Muse Glimmer behavior, and CI coverage.

Changes:

• Adds reasoning validation and response-contract documentation.
• Makes transcript replay reasoning-aware and expands replay/tokenizer tests.
• Updates Muse serving behavior, fixtures, dependencies, and CI workflows.

File summaries

File	Summary
examples/models/muse-glimmer/tests/test_serve.py	Adds Muse reasoning and replay tests.
examples/models/muse-glimmer/serving/serve.py	Discards whitespace-only reasoning blocks.
examples/models/muse-glimmer/README.md	Documents reasoning configuration.
examples/llm_server/spec/README.md	Documents response and replay contracts.
examples/llm_server/python/tests/test_warm_resume_scaffold.py	Adds replay and BPE prompt-fidelity tests.
examples/llm_server/python/tests/test_tool_calls.py	Removes relocated reasoning tests.
examples/llm_server/python/tests/test_contract.py	Tests reasoning response and validation behavior.
examples/llm_server/python/tests/conftest.py	Supports reasoning extractor fixtures.
examples/llm_server/python/serving_chat.py	Validates and records reasoning responses. Moderate finding (2 votes): streaming and non-streaming empty reasoning values are inconsistent.
examples/llm_server/python/README.md	Documents test dependencies and tokenizer fixtures.
examples/llm_server/python/protocol.py	Preserves explicit reasoning fields in requests.
examples/llm_server/python/openai_transcript.py	Adds reasoning-aware replay validation. Moderate finding (1 vote): explicit reasoning_content: null does not invalidate unchanged records without reasoning.
.github/workflows/trunk.yml	Updates trunk LLM-server routing. Moderate finding (1 vote): the Muse Glimmer path uses _ instead of -.
.github/workflows/pull.yml	Adds pull-request LLM-server coverage. Moderate finding (1 vote): the Muse Glimmer path uses _ instead of -.
.github/workflows/_llm_server.yml	Installs tokenizer dependencies and runs expanded tests.

Review details

Suppressed comments (3)

.github/workflows/pull.yml:762

• This path never matches the actual examples/models/muse-glimmer/... directory (the condition uses _ instead of -), so changes to Muse Glimmer files outside serving and tests will not run this LLM-server job. Use the hyphenated path so the intended adapter coverage is triggered for the whole model directory.

        contains(needs.changed-files.outputs.changed-files, 'examples/models/muse_glimmer') ||

.github/workflows/trunk.yml:1001

• This path never matches the actual examples/models/muse-glimmer/... directory (the condition uses _ instead of -), so changes to Muse Glimmer files outside serving and tests will not run this LLM-server job on trunk. Use the hyphenated path so the intended adapter coverage is triggered for the whole model directory.

        contains(needs.changed-files.outputs.changed-files, 'examples/models/muse_glimmer') ||

examples/llm_server/python/openai_transcript.py:298

• An explicit reasoning_content: null is treated as unchanged when the recorded response had no reasoning, because both values fingerprint to None. The documented contract says that supplying the field—including explicit null—must invalidate stored IDs, so this can replay a cached turn after a non-equivalent history edit. Preserve the omitted-field fast path, but make any explicitly supplied field diverge when record["reasoning_fp"] is None (and update the unchanged-null test accordingly).

            if self._assistant_fingerprint(m.content, m.tool_calls) != record["fp"] or (
                "reasoning_content" in m.model_fields_set
                and self._reasoning_fingerprint(m.reasoning_content)
                != record["reasoning_fp"]
            ):

• Files reviewed: 15/15 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[shoumikhin]

Some notes inline.

These are about lines the change does not touch, so they could not be attached to one.

In examples/llm_server/python/openai_transcript.py, around line 377:

When a turn is found stale we delete the tail, but the next answer is appended to the end of the shortened list instead of at its real turn index. After that, stored record number k belongs to a different assistant turn than message number k, so every later comparison fails and the list is truncated again. I ran a four turn session: one mismatch and the session never splices more than the first turn again, even after the client goes back to a history that matches exactly. The docstring says the record is placed at the turn count of the request it answers, which is not true once the list is shorter than that. One way out is to stop splicing on a mismatch without shortening the list, and leave invalidation to the recording side, which already drops records at and after the index it is answering. This shape is older than this change, but the new reasoning check makes it easy to reach without the client editing any content.

[shoumikhin]

Falling back when the boundary cannot be verified is the right call, and the old path really could duplicate template framing. The problem is what it now costs and how quietly. The chat template class defaults the assistant header to the ChatML one, and the generic launcher never overrides it and offers no option to set it. So point that launcher at a Llama 3 or Mistral tokenizer and the header is never found, which means no turn is ever spliced and every request re-prefills the whole conversation. I compared both sides on one such template and the reuse goes from working to off. Nothing logs it, nothing warns, and no test would catch it. Two things would help. Let the launcher configure the assistant header, and warn once at startup if the configured header is not in the template's own render. The preamble helper already renders that probe and already returns an empty string in this case, so the signal is there for free.

[shoumikhin]

The new type check runs for every model, including ones with no reasoning extractor at all, where this flag has no effect. I compared both sides with the same request: before this change those requests returned 200, now they return 400. That is a breaking change for existing clients over a setting that did nothing for them. The spec paragraph you added says the rule is for models with a reasoning extractor, so the text and the code disagree right now. Please either gate the check on the extractor being configured, or move that sentence out of the reasoning paragraph so the rule reads as unconditional. Worth noting the description too, since this is visible on the wire.

[shoumikhin]

The new reasoning check keys on whether the field was present in the request body at all. A client that echoes the assistant message back by dumping its own object will normally send the field as null rather than leave the key out. That now counts as a change, so the stored ids for that turn and every later turn are dropped, and they do not come back even after the client sends a history that matches exactly. Trimming has the same effect, and the Muse Glimmer extractor returns reasoning with its leading and trailing newlines on purpose, which is exactly what a client or a user interface tends to strip. Since the server omits the field entirely when there is no reasoning, a null carries nothing an omission does not. Two suggestions. Treat explicit null the same as omitted, since a client that really wants to change the reasoning sends a string. And compare a normalised form, at least trimmed and with line endings settled, so a client that changed nothing real keeps its reuse.