fix(tui): propagate verbose flag from thinking.delta to recordReasoningDelta - #30446
AhmetArif0 wants to merge 1 commit into
Conversation
…ngDelta thinking_callback emitted thinking.delta without a verbose field, while the sibling reasoning_callback already conditionally sets verbose:True when the session is in verbose mode. On the client side, thinking.delta called recordReasoningDelta(value) without the force flag, so thinking blocks were silently dropped when showReasoning=false — even in /verbose verbose where the user explicitly expects to see them. Fix: mirror reasoning_callback in server.py so thinking.delta also carries verbose:True under _session_verbose(), and read ev.payload.verbose in the thinking.delta case of createGatewayEventHandler.ts so the force flag reaches recordReasoningDelta. Regression test added: "shows verbose thinking.delta even when normal reasoning display is off", symmetric with the existing reasoning.delta test. Closes the parity gap introduced by NousResearch#30225.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused parity investigation. The current implementation does not support the proposed event semantics.
Problems
thinking_callbackis the quiet-mode spinner path:agent/conversation_loop.py:1073-1079emits a generated spinner label and clears it later. Model reasoning usesrun_agent.py:4745-4750and is already emitted with verbose support bytui_gateway/server.py:3890-3894. Recording verbosethinking.deltawould therefore turn spinner text into persisted reasoning.- The changed handler reads
ev.payload?.verbose, butui-tui/src/gatewayTypes.ts:620definesthinking.deltaas{ text?: string };ui-tui/tsconfig.jsonhasstrict: true. The added test casts its event toany, so it does not exercise that protocol typing mismatch.
Suggested changes
- Confirm a current producer routes model reasoning through
thinking.deltabefore changing this status event; otherwise keep the existing verbose-awarereasoning.deltapath. - If extending the event intentionally, update
ui-tui/src/gatewayTypes.ts,ui-tui/README.md, and add a server_agent_cbspayload test.
Automated hermes-sweeper review.
| @@ -320,7 +320,7 @@ export function createGatewayEventHandler(ctx: GatewayEventHandlerContext): (ev: | |||
| scheduleThinkingStatus(value || statusFromBusy()) | |||
There was a problem hiding this comment.
GatewayEvent currently types thinking.delta payload as { text?: string } in ui-tui/src/gatewayTypes.ts:620. Add verbose?: boolean to that protocol variant (and its documented wire payload) before reading this field; the test's as any cast bypasses the strict TypeScript error.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address the reported behavior with identical diffs, adding a verbose flag to thinking.delta and forcing those events into recorded reasoning. The diff does not establish the reported cause, however: the contributor review identifies thinking.delta as the quiet-mode spinner/status path, while model reasoning already uses the verbose-aware reasoning.delta path.
Related pull requests
- #30446
related— (+25/-2) — keep open for revision, do not merge as-is: the diff would persist spinner/status text as reasoning and accesses an undeclared verbose field on the strictly typed thinking.delta payload; its any-cast test masks that protocol mismatch. This follows the contributor keep_open review on #30446, which requires evidence of a current model-reasoning producer for thinking.delta or, if the protocol is intentionally extended, corresponding gateway type, documentation, and server coverage. - #30501 [closed]
duplicate— (+25/-2) — closed duplicate, still relevant because its diff is identical to #30446 across the server emitter, client handler, and any-cast test, so it has the same semantic and typing defects. It was correctly superseded by #30446 and provides no independent fix to consolidate.
Duplicates
#30501 is an exact duplicate of #30446: both apply the same +25/-2 changes to the same three files.
Suggested consolidation
Do not merge either PR as currently written. Keep #30446 as the sole revision point, requiring producer-level evidence that model reasoning traverses thinking.delta and a protocol-complete implementation if that event is intentionally extended; keep duplicate #30501 closed.
Complex graph
flowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
subgraph Dup30446 ["PRs duplicating each other"]
P30446["PR #30446 (open)"]
P30501["PR #30501 (closed)"]
end
class P30446 open
class P30501 closed
class P30446 target
click P30446 "https://github.com/NousResearch/hermes-agent/pull/30446"
click P30501 "https://github.com/NousResearch/hermes-agent/pull/30501"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 5 kB of PR diffs, 3 kB of issue/PR text, 2 kB of discussion (3 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
thinking.delta both scheduled the status line and recorded its text as reasoning. On a current gateway thinking_callback only ever carries "<face> <verb>..." or "" (agent/conversation_loop.py), so the spinner caption — "(=^..^=) pondering..." — was pasted into the reasoning block. Removing the record outright is wrong: 7d68ea9 ("stream legacy thinking deltas visibly") added it deliberately so a LEGACY gateway, from before reasoning.delta existed, still shows its reasoning. Deleting it would strip reasoning from those gateways entirely, and it is what makes this patch collide with NousResearch#30446. Instead the current gateway marks its emission, and the client suppresses the reasoning record only for marked events: - tui_gateway/server.py: thinking_callback emits status_only: True. - createGatewayEventHandler: records reasoning only when the marker is absent — i.e. exactly the legacy case 7d68ea9 was written for. Both behaviours are pinned: the existing "streams legacy thinking.delta into visible reasoning state" test sends no marker and still passes, and a new test asserts a marked event records nothing while still reaching the status line. This also removes the event-contract blocker. NousResearch#30446 wants thinking.delta's verbose flag forwarded to recordReasoningDelta; that now applies cleanly to the unmarked legacy path, which is the only path where thinking.delta carries reasoning at all — so the two changes compose instead of conflicting. Rebuilt on current main rather than rebased (the branch was ~2900 commits behind). The scripts/release.py addition to the frozen LEGACY_AUTHOR_MAP is dropped. Fixes NousResearch#68600 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Problem
thinking_callbackintui_gateway/server.pyemitsthinking.deltawithout averbosefield, while the siblingreasoning_callback(added in #30225) already setsverbose: Truewhen the session is in verbose mode (/verbose verbose).On the client side, the
thinking.deltacase increateGatewayEventHandler.tscallsrecordReasoningDelta(value)without the force flag, whilereasoning.deltacorrectly passesBoolean(ev.payload.verbose):recordReasoningDeltabails early whenforce=falseandshowReasoning=false:Result: in
/verbose verbosemode, extended-thinking blocks from the model are silently dropped when the user has reasoning display off (the default), even though the user explicitly enabled verbose output to see them. Thereasoning.deltapath works correctly for the same scenario.Fix
Server (
tui_gateway/server.py): mirrorreasoning_callbackinthinking_callback— emitverbose: Trueunder_session_verbose().Client (
createGatewayEventHandler.ts): readev.payload?.verbosein thethinking.deltacase and pass it as the force flag torecordReasoningDelta.Tests
Added
"shows verbose thinking.delta even when normal reasoning display is off"— symmetric with the existing"shows verbose reasoning even when normal reasoning display is off"test from #30225.43/43 tests pass in
createGatewayEventHandler.test.ts.Checklist
reasoning_callback/reasoning.delta(already correct since fix(tui): surface verbose tool details #30225)_session_verbose()is false —verbosefield is omitted,forcestaysfalseruff check tui_gateway/server.pypasses