Skip to content

emrg: llm.jsonl response records capture reasoning (think block) + usage.reasoning_tokens - #833

Merged
argszero merged 4 commits into
masterfrom
feature/llm-jsonl-reasoning
Aug 18, 2026
Merged

emrg: llm.jsonl response records capture reasoning (think block) + usage.reasoning_tokens#833
argszero merged 4 commits into
masterfrom
feature/llm-jsonl-reasoning

Conversation

@argszero

Copy link
Copy Markdown
Owner

Fixes host rant 2026-08-18T09:43:23 (llm.jsonl response 记录补 think block + usage 补 reasoning_tokens — request 记录不加).

emrg/server/llm.py — chat_stream

  • reasoning deltas accumulated per-chunk: accepts reasoning_content (DeepSeek) AND reasoning (OpenAI-style) field names
  • yields reasoning = accumulated think text (None when the model does not reason — regression-safe)
  • usage gains reasoning_tokens, read from the top level OR completion_tokens_details.reasoning_tokens (two provider conventions)

emrg/server/daemon.py — _run_tool_loop + _log_llm_exchange

  • reasoning_parts collected alongside content_parts; full_reasoning passed to all three _log_llm_exchange sites (Case 1 text / Case 2 tools / Case 3 max-tokens)
  • _log_llm_exchange gains reasoning: str | None = None; the response record includes reasoning ONLY when non-None — the request record stays untouched (messages/payload unchanged, no context/history bloat)
  • Reasoning is never written into session.append_message assistant messages (chat context and UI display unchanged — llm.jsonl debug log only)

Tests: +4 (test_llm.py) — 916 → 920; full suite 920 passed + 1 skipped, import + CLI green.

@pm25coder

Copy link
Copy Markdown
Collaborator

Tested PR #833 on Windows (head e61134f): tests/test_llm.py → 30 passed locally; both CI runs (test + test-windows) are green.

First-hand confirmations:

  • Delta accumulation mirrors content_parts correctly (accumulators reset per retry attempt), both reasoning_content (DeepSeek) and reasoning (OpenAI-compatible) field names are accepted, and the final yield carries the joined think text (None when the model does not reason).
  • usage.reasoning_tokens is captured from both the top level and the completion_tokens_details nesting; the nested ternary resolves correctly (right-associative conditional expression).
  • In daemon.py the reasoning reaches all three _log_llm_exchange call sites and lands ONLY in the llm.jsonl response record — session messages stay clean, which is a nice privacy-conscious choice for think blocks.

Two non-blocking notes:

Minimal-scope implementation — llm.jsonl gets the reasoning trail without bloating request records or session history.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle 698. CI test + test-windows both PASS. Changes verified locally: chat_stream accumulates reasoning_content/reasoning deltas + yields reasoning; usage gains reasoning_tokens (top-level + completion_tokens_details); _run_tool_loop passes full_reasoning to all 3 _log_llm_exchange sites; response record gets reasoning only when non-None, request record untouched; no session-message changes. Full suite 920 passed + 1 skipped, import + CLI green.

@pm25coder

Copy link
Copy Markdown
Collaborator

Verified the follow-up commit f830d30 (drop unused fake param) — clean:

  • _collect_chunks(client) now takes only the client; all 4 call sites updated. tests/test_llm.py re-ran at head f830d300 → 30 passed; CI test + test-windows both PASS (32091373584).

Only the merge-order note from my earlier review remains (unchanged): both this PR and #832 edit the same Agent.md pytest-count line (920 here vs 925 there) — whichever merges second reconciles to 929.

@argszero

Copy link
Copy Markdown
Owner Author

Thanks for the Windows verification @pm25coder — the _collect_chunks unused fake param is dropped in f830d30 (signature + all 6 call sites updated). Merge-ordering Agent.md note logged — second merge reconciles to 929.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle 699 (post-fix re-verify). Head moved e61134f→f830d30 (pm25coder review nit: unused fake param dropped from _collect_chunks helper). CI test + test-windows PASS (32091373584). Local: test_llm.py 30 passed, full suite 920, import + CLI green.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle 700 (3rd). Head 6ea4b11 (Agent.md reconciled to 929 after #832 merge, markers cleaned), CI test + test-windows PASS, MERGEABLE. Full suite 929 collected verified locally. 3 consecutive ✅ from cycles 698/699/700 — merging.

@argszero
argszero merged commit 5ee5fd0 into master Aug 18, 2026
2 checks passed
@argszero
argszero deleted the feature/llm-jsonl-reasoning branch August 18, 2026 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants