Repository navigation
feat(server): report a turn's private thinking on its own channel - #1257
Conversation
answer, but it did so by discarding it, and a turn that thinks for a long time before it speaks then sends nothing at all in the meantime. Measured here, a plain "hello" produced its first visible byte after 280 seconds of complete silence; a client cannot tell that apart from a hung server. Thinking was never the problem — passing it off as an answer was. So it is now reported on the channel that exists for it: `reasoning_content`, beside `content` rather than inside it, both on the streaming delta and on a buffered message. The same turn now streams its first reasoning after seven seconds and its answer a second later, and the answer is byte for byte what it was before. The gate keeps one transcript and asks the channel definitions what each one has revealed, so the two channels cannot disagree about where a token belongs, and a model that declares no private channel still reports no thinking rather than having one invented for it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e
05ea59b to
b5977bf
Compare
The reversal added `reasoning_content` on the buffered (non-streamed) message and a distinct `reasoning_content` delta on the wire, but only the gate's streaming routing was covered by tests. Add end-of-path guards for the two uncovered paths so a future change cannot silently regress either: - buffered_reasoning_tests: mirrors run_chat_completion's composition (atem_reasoning_content for reasoning, parse_assistant_output for the answer), asserting a buffered turn reports its thinking on reasoning_content while keeping it out of content. - reasoning_wire_tests: asserts the serialized SSE shape — a reasoning chunk carries a reasoning_content key and no content key, and an ordinary content chunk omits reasoning_content entirely, so existing OpenAI-compatible clients are unaffected. Both were falsified: each goes red when its underlying behaviour is reverted, and green when restored. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review — verified against #1256 acceptance criteria ✅ mergingReviewed in a dedicated worktree off freshly-fetched Wire format (the "on its own channel" claim)Reasoning rides on a separate
Criteria
Suite / lint
Honest limitationNo model available on this box declares a private channel ( |
Replacement for #1190, rebased onto current `main` (the original could not be squash-merged because #1210 is stacked on its head branch, which triggers GitHub's stacked-PR 403). Content is identical to #1190; only the base is refreshed. ## What this adds A 20-file explanatory wiki under `wiki/` (crate architecture, execution backends, EP contracts, memory model, metadata-driven runtime, request lifecycle, testing/verification, performance playbook, repository map). It repeatedly labels itself non-authoritative and points at `docs/` and source for verification. ## Accuracy review (the bar for docs in this repo) I spot-checked the load-bearing claims against `main` as it stands today (after the memory capability-split revert #1247, session-state unification #1255, `reasoning_content` #1257, CUDA mask-reservation fix #1264): - **Crate architecture** — the crate roster and layering match the actual workspace `Cargo.toml` members and `crates/` tree. The map is explicitly non-exhaustive and says so. - **Memory model (the highest-risk area)** — `wiki/memory/Memory Management for Beginners.md` is marked `status: proposed` and frames the capability split as a *recommendation*, while describing the **current combined `DeviceAllocator`** as the present reality. I verified against `crates/onnx-runtime-memory-governor/src/allocator.rs`: the trait indeed still carries `allocate`, `commit_allocation_range`/`decommit_allocation_range`, `allocation_committed_bytes`, `mapped_bytes`, `create_shared_prefix`, and the successful-no-op default it warns about. Because the doc calls the split a proposal and the combined trait the current state, it **survives** the #1192→#1247 revert rather than being invalidated by it. The proposed `onnx-runtime-memory-api` crate correctly does not exist. `with_memory(...)` — cited as the mechanism that already satisfies issue #513's core goal — exists (`onnx-genai-ort`, `ep-cpu`, `ep-cuda`). - **Execution backends** — the ORT-vs-native-nxrt description and EP contract are accurate and high-level; nothing contradicts the #1255 session-state unification. - **Repository Map** — every code path it points at exists (`decode_loop.rs`, `sampling.rs`, `speculative/`, all crate dirs). - **Links** — all 58 repo-relative links resolve. (The one apparent miss, `docs/path.md`, is inside a fenced note-*template* code block, not a real link.) - **Benchmark-number rule (`docs/benchmarks/2026-08-15-...` §32.2)** — no perf figure is copied without metadata; the only "speedup" mentions are the docs *teaching* that a bare speedup is insufficient, and the GB values are illustrative diagram quantities, not measurements. ## Verification - Nothing under a test, build script, or CI job references the new `wiki/` files (checked with `git grep`), so this cannot break the build or suite. - No in-tree markdown lint / mdbook config exists, so there is no docs build to run here — stated rather than invented. No inaccuracies found inside the diff; no corrections were needed. Closes #1190. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1257 +/- ##
==========================================
+ Coverage 79.69% 80.06% +0.36%
==========================================
Files 358 363 +5
Lines 155271 160240 +4969
Branches 155271 160240 +4969
==========================================
+ Hits 123746 128292 +4546
- Misses 26940 27293 +353
- Partials 4585 4655 +70
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
🔴 Benchmark Regression DetectedComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
Symptom
Driving this model from OpenCode, a plain
helloshows nothing at all for minutes, and no reasoning is ever displayed.Measured against this server:
280 seconds of complete silence on the wire. A client cannot tell that apart from a hung server.
Cause
#1249 stopped this package's private channel being served as an answer, which was right — but it did so by discarding it. The model addresses
to=selfwhile it works and only later opensto=user, so for the whole of the thinking there is nothing to send, and the connection simply sits there.Thinking was never the problem. Passing it off as an answer was.
Fix
The private channel is now reported on the channel that exists for it —
reasoning_content, besidecontentrather than inside it — on the streaming delta and on a buffered message alike. Nothing moves intocontent, so the answer is byte for byte what it was.The gate keeps one transcript and asks the channel definitions what each has revealed since the last emission, so the two channels cannot disagree about where a token belongs. A model that declares no private channel reports no thinking rather than having one invented for it.
Reasoning streams even on the tool-detection path, where the answer must be buffered until the turn is known not to be a tool call — that is exactly the agent case, and exactly where the silence was worst.
Verification
Same prompt, same server, after the change:
--all-targetsclean.