From 32d46f44503df91d0c2493950e298f10f8b35afe Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Sat, 6 Jun 2026 00:19:29 -0700 Subject: [PATCH] =?UTF-8?q?Release=20v0.51.293=20=E2=80=94=20Release=20JI?= =?UTF-8?q?=20(stage-s5=20=E2=80=94=20thinking=20card=20no=20longer=20rend?= =?UTF-8?q?ers=20twice=20#3709)=20(#3715)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#3709): thinking card no longer renders twice (in Activity + below answer) The #3592 inline-render branch (v0.51.258) emitted a thinking card for a thinking-only message even when a sibling tool-message in the same turn already built an Activity group carrying that turn's thinking — so the card showed twice, the second one stranded below the answer + 'Done in …' footer (insertAdjacentHTML 'beforeend' on a segment that already had body+footer). Fix (keeps #3592, does NOT revert it): - A1: precompute turnsWithActivityGroup (turns whose segments have tool cards); the inline branch only renders when the anchor turn is NOT in that set. - A2: when it does render inline, insert 'beforebegin' the .msg-body/.msg-foot so the card sits above the answer, not orphaned below the footer. - B: strip thinking against the TURN's combined visible answer (_turnVisibleTextByRawIdx), so a trailing thinking-only message that echoes the answer gets de-duped even though its own body is empty. Live-verified in browser: #3709 repro (tool+trailing-thinking) → exactly 1 card in Activity, above footer; #3592 repro (thinking-only) → exactly 1 inline card, not buried in a collapsed group. + regression test tests/test_issue3709_*. Supersedes #3708 (which deleted the inline branch outright, re-breaking #3592). * fix(#3709): merge suppressed sibling thinking into the Activity group (Codex re-gate) Codex caught a content-loss edge in the first cut: when A1 suppresses a thinking-only sibling's inline card (its turn has an Activity group), the group only rendered assistantThinking.get(aIdx) for the TOOL message — so a sibling with DISTINCT reasoning was neither inline nor in the group → dropped. Fix: aggregate all of a turn's thinking (turnThinkingParts, de-duped, index order) and render that merged text once per turn in the Activity group (_renderedTurnThinking guard). Live-verified: tool-thinking A + distinct sibling-thinking B → 1 merged node carrying both, no loss. + regression test. * fix(#3709): shared anchor resolver so inline-suppression & group placement agree (Codex re-gate #2) Codex caught a fallback-anchor mismatch: turnsWithActivityGroup was populated only from assistantSegments.get(tcIdx) (direct segment), but the group-render path falls back to a nearby earlier segment when a tool's assistant_msg_idx has no directly rendered segment (legacy/rebased). So a fallback-anchored group's turn wasn't in turnsWithActivityGroup → the sibling rendered inline AND the group rendered → dup again. Fix: one shared _anchorRowForActivityIdx(aIdx) helper (direct-or-fallback) used by the precompute, the inline branch, and the group render — they now agree. Live-verified all three repros still pass. * test(#3709): update test_compact_activity assertion to mergedThinking var The brittle source-scan asserted _thinkingActivityNode(thinkingText, false) — the #3709 fix renders the turn's MERGED thinking via _thinkingActivityNode(mergedThinking, false) into the same Activity body. Intent (settled thinking renders inside the Activity disclosure alongside tools) unchanged; only the source variable. Updated to assert the new variable, kept all intent assertions. --------- Co-authored-by: nesquena-hermes <[email protected]> --- CHANGELOG.md | 5 + static/ui.js | 126 +++++++++++-- .../test_issue3709_thinking_double_render.py | 165 ++++++++++++++++++ tests/test_ui_tool_call_cleanup.py | 8 +- 4 files changed, 290 insertions(+), 14 deletions(-) create mode 100644 tests/test_issue3709_thinking_double_render.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 44f51996..11ccf082 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,11 @@ ## [Unreleased] +## [v0.51.293] — 2026-06-06 — Release JI (stage-s5 — thinking card no longer renders twice) + +### Fixed +- **The "Thinking" card no longer renders twice on a settled turn.** For a turn that had both a tool call and reasoning (e.g. think → call a tool → answer), the thinking card could appear once inside the collapsed **Activity** group at the top of the turn and again as a stranded second card below the answer and the `Done in …` footer. The thinking-only inline render path (added in v0.51.258 for #3592) now only fires when the turn has no Activity group of its own, and when it does render inline it inserts the card **above** the answer body instead of after the footer. Thinking that echoes the visible answer on a trailing reasoning-only message is also de-duplicated against the whole turn's answer text now, not just the same message's body. Genuinely thinking-only turns still show their thinking inline (the #3592 fix is preserved, not reverted). (#3709; supersedes #3708) + ## [v0.51.292] — 2026-06-06 — Release JH (stage-s4 — compression-exhausted turns surface as errors, not fake completions) ### Fixed diff --git a/static/ui.js b/static/ui.js index 2cf60d49..5a071f32 100644 --- a/static/ui.js +++ b/static/ui.js @@ -7162,6 +7162,39 @@ function renderMessages(options){ if(role==='user') lastQuestionRawIdx=entry.rawIdx; else if(role==='assistant') questionRawIdxByAssistantRawIdx.set(entry.rawIdx,lastQuestionRawIdx); } + // #3709 (defect B): build a per-turn combined visible-answer text so the + // thinking echo-strip can de-dupe a thinking-only message (whose own visible + // body is empty) against the answer prose carried by a SIBLING message in the + // same turn. A turn = the run of assistant messages between two user messages. + // Map every assistant rawIdx in a run to the run's combined visible text. + const _turnVisibleTextByRawIdx=new Map(); + { + let _run=[]; let _runText=[]; + const _flush=()=>{ + if(_run.length){ + const combined=_runText.join('\n\n'); + for(const ri of _run) _turnVisibleTextByRawIdx.set(ri, combined); + } + _run=[]; _runText=[]; + }; + for(const entry of renderVisWithIdx){ + const em=entry&&entry.m; const role=em&&em.role; + if(role==='assistant'){ + _run.push(entry.rawIdx); + // Visible prose = content with any leading … /channel-thought + // block stripped (the same blocks the per-message extractor removes below). + let vis=typeof em.content==='string'?em.content:''; + vis=vis.replace(/^\s*[\s\S]*?<\/think>\s*/,'') + .replace(/^\s*<\|channel\|?>thought\n?[\s\S]*?\s*/,'') + .replace(/^\s*<\|turn\|>thinking\n[\s\S]*?\s*/,'').trim(); + if(vis) _runText.push(vis); + }else{ + _flush(); + } + } + _flush(); + } + const assistantSegments=new Map(); const assistantThinking=new Map(); const userRows=new Map(); @@ -7233,6 +7266,13 @@ function renderMessages(options){ const displayContent=isUser?_stripAttachedFilesMarkerForDisplay(_stripWorkspaceDisplayPrefix(content)):content; if(thinkingText&&!isUser){ thinkingText=_stripVisibleAssistantEchoFromThinking(thinkingText, displayContent); + // #3709 (defect B): if this message's own visible body didn't strip the + // echo (e.g. a thinking-only message whose answer prose lives on a sibling + // message in the same turn), also strip against the turn's combined answer. + const turnVisible=_turnVisibleTextByRawIdx.get(rawIdx); + if(thinkingText&&turnVisible&&turnVisible!==displayContent){ + thinkingText=_stripVisibleAssistantEchoFromThinking(thinkingText, turnVisible); + } } const isLastAssistant=!isUser&&vi===renderVisWithIdx.length-1; const nextRendered=renderVisWithIdx[vi+1]; @@ -7545,22 +7585,80 @@ function renderMessages(options){ const assistantIdxs=[...assistantSegments.keys()].sort((a,b)=>a-b); const anchorInsertAfter = new Map(); if(isSimplifiedToolCalling()){ + // Shared anchor resolver: maps an activity index to the assistant segment + // its Activity group will anchor on. The group-render path falls back to a + // nearby earlier segment when an index has no directly-rendered segment + // (legacy/rebased assistant_msg_idx). The inline-suppression precompute MUST + // use the SAME resolution, or a fallback-anchored group's turn won't be in + // turnsWithActivityGroup and the thinking renders twice (Codex re-gate). + const _anchorRowForActivityIdx=(aIdx)=>{ + let row=assistantSegments.get(aIdx)||null; + if(!row&&assistantIdxs.length){ + if(aIdxidx<=aIdx); + row=fallbackIdx!==undefined?assistantSegments.get(fallbackIdx):assistantSegments.get(assistantIdxs[assistantIdxs.length-1]); + } + return row; + }; + // #3709: a turn (one .assistant-turn, spanning every assistant segment + // between two user messages) can contain BOTH a tool-bearing message and a + // trailing thinking-only message. The tool message builds an Activity group + // that already carries the turn's thinking at its top; if the thinking-only + // sibling ALSO renders its thinking inline, the card shows twice. Precompute + // the set of turn nodes that have any tool card so the inline branch can + // skip turns that already own an Activity group. + const turnsWithActivityGroup=new Set(); + // Per-turn merged thinking: a tool-bearing turn's Activity group must carry + // ALL of that turn's thinking — including a trailing thinking-only sibling + // whose inline card we suppress below (#3709 A1). Rendering only the tool + // message's own assistantThinking entry would silently DROP a sibling's + // distinct reasoning. Aggregate every assistantThinking entry by turn, + // de-duped and in index order, and render that once in the group. + const turnThinkingParts=new Map(); // turnNode -> [text,...] + const _addTurnThinking=(idx)=>{ + if(!assistantThinking.has(idx)) return; + const seg=_anchorRowForActivityIdx(idx); + const turn=seg?seg.closest('.assistant-turn'):null; + if(!turn) return; + const txt=String(assistantThinking.get(idx)||'').trim(); + if(!txt) return; + const arr=turnThinkingParts.get(turn)||[]; + if(!arr.includes(txt)) arr.push(txt); + turnThinkingParts.set(turn, arr); + }; + for(const tcIdx of Object.keys(byAssistant).map(k=>parseInt(k))){ + if(!(byAssistant[tcIdx]||[]).length) continue; + const tcSeg=_anchorRowForActivityIdx(tcIdx); + const tcTurn=tcSeg?tcSeg.closest('.assistant-turn'):null; + if(tcTurn) turnsWithActivityGroup.add(tcTurn); + } + // Aggregate thinking for every assistant idx that has it (tool-bearing or not). + for(const tIdx of assistantThinking.keys()) _addTurnThinking(tIdx); + const _renderedTurnThinking=new Set(); const activityIdxs=[...new Set([...Object.keys(byAssistant).map(k=>parseInt(k)), ...assistantThinking.keys()])].sort((a,b)=>a-b); for(const aIdx of activityIdxs){ const cards=byAssistant[aIdx]||[]; if(!cards.length&&assistantThinking.has(aIdx)){ - const anchorRow=assistantSegments.get(aIdx); - if(anchorRow&&window._showThinking!==false){ - anchorRow.insertAdjacentHTML('beforeend',_thinkingCardHtml(assistantThinking.get(aIdx))); + // Thinking-only message. Render its thinking inline ONLY when the turn + // has no Activity group at all (#3592 — a genuinely thinking-only turn + // must not bury its thinking in a collapsed group). If a sibling + // tool-message in the same turn built a group, that group carries the + // turn's thinking (including THIS message's, via turnThinkingParts) — + // don't emit a duplicate inline card here (#3709 A). + const anchorRow=_anchorRowForActivityIdx(aIdx); + const anchorTurn=anchorRow?anchorRow.closest('.assistant-turn'):null; + if(anchorRow&&window._showThinking!==false&&!(anchorTurn&&turnsWithActivityGroup.has(anchorTurn))){ + // Insert the thinking card BEFORE the answer body + msg-foot footer + // (the segment already has them appended), so it reads above the + // answer rather than orphaned below the "Done in …" line (#3709 A2). + const bodyEl=anchorRow.querySelector('.msg-body,.msg-foot'); + const cardHtml=_thinkingCardHtml(assistantThinking.get(aIdx)); + if(bodyEl) bodyEl.insertAdjacentHTML('beforebegin',cardHtml); + else anchorRow.insertAdjacentHTML('beforeend',cardHtml); } continue; } - let anchorRow=assistantSegments.get(aIdx)||null; - if(!anchorRow&&assistantIdxs.length){ - if(aIdxidx<=aIdx); - anchorRow=fallbackIdx!==undefined?assistantSegments.get(fallbackIdx):assistantSegments.get(assistantIdxs[assistantIdxs.length-1]); - } + let anchorRow=_anchorRowForActivityIdx(aIdx); if(!anchorRow) continue; const anchorParent=anchorRow.parentElement; let insertAfterNode = anchorInsertAfter.get(anchorRow) || anchorRow; @@ -7569,9 +7667,13 @@ function renderMessages(options){ if(sourceMsg._turnDuration!==undefined) group.setAttribute('data-turn-duration', String(sourceMsg._turnDuration)); const body=group&&group.querySelector('.tool-call-group-body'); if(!body) continue; - const thinkingText=assistantThinking.get(aIdx); - if(thinkingText){ - body.appendChild(_thinkingActivityNode(thinkingText, false)); + // Render the TURN's merged thinking once (covers this message's own + // thinking + any suppressed thinking-only sibling in the same turn, #3709). + const groupTurn=anchorRow?anchorRow.closest('.assistant-turn'):null; + const mergedThinking=(groupTurn&&turnThinkingParts.get(groupTurn)||[]).join('\n\n').trim(); + if(mergedThinking&&!(groupTurn&&_renderedTurnThinking.has(groupTurn))){ + body.appendChild(_thinkingActivityNode(mergedThinking, false)); + if(groupTurn) _renderedTurnThinking.add(groupTurn); } for(const tc of cards){ body.appendChild(buildToolCard(tc)); diff --git a/tests/test_issue3709_thinking_double_render.py b/tests/test_issue3709_thinking_double_render.py new file mode 100644 index 00000000..d65045c0 --- /dev/null +++ b/tests/test_issue3709_thinking_double_render.py @@ -0,0 +1,165 @@ +"""#3709 -- Thinking card must not render twice (inside Activity AND below the answer). + +Regression coverage for the double-render introduced by #3592's inline branch +(v0.51.258). In a turn that has BOTH a tool-bearing message and a trailing +thinking-only message, two code paths emitted a thinking card from the same +``assistantThinking`` map: + + 1. the Activity-group path (tool-bearing message) put the thinking at the top + of the collapsed Activity group, and + 2. the inline path (thinking-only message) appended a SECOND card via + ``insertAdjacentHTML('beforeend')`` -- which, because the segment already + carried the answer body + ``msg-foot`` footer, stranded the card *below* + the "Done in ..." line. + +The fix keeps the #3592 inline behaviour for genuinely thinking-only turns (so +their thinking is not buried in a collapsed group) but: + + A1. only renders inline when the turn has NO Activity group at all + (``turnsWithActivityGroup`` gate), so a tool-bearing turn's thinking-only + sibling does not emit a duplicate card; + A2. inserts the inline card BEFORE the answer body / footer + (``insertAdjacentHTML('beforebegin')`` on ``.msg-body,.msg-foot``) so it + reads above the answer instead of orphaned below "Done in ..."; + B. strips the thinking against the TURN's combined visible answer + (``_turnVisibleTextByRawIdx``) so a trailing thinking-only message whose + answer prose lives on a sibling message still gets its answer-echo removed. + +These are static source-structure assertions (the render path is DOM-driven and +exercised live); they lock the invariants so the double-render cannot silently +return, and so a future blunt "just delete the inline branch" change (which would +re-break #3592) fails fast here instead. +""" +from __future__ import annotations + +import re +from pathlib import Path + +UI_JS = (Path(__file__).resolve().parent.parent / "static" / "ui.js").read_text(encoding="utf-8") + + +def _render_messages_body() -> str: + """Return the body of renderMessages() (best-effort slice) for scoped asserts.""" + start = UI_JS.find("function renderMessages(") + assert start != -1, "renderMessages() not found" + # Slice a generous window; the activityIdxs loop + footer logic live within. + return UI_JS[start:start + 60000] + + +def test_inline_thinking_branch_still_exists_for_thinking_only_turns(): + """#3592 must NOT be reverted: a thinking-only turn still renders its thinking + inline (not buried in a collapsed Activity group).""" + assert "!cards.length&&assistantThinking.has(aIdx)" in UI_JS, ( + "the thinking-only inline branch (#3592) must remain — deleting it " + "re-buries thinking-only turns in a collapsed Activity group" + ) + assert "_thinkingCardHtml(" in UI_JS + + +def test_inline_branch_gated_on_turn_having_no_activity_group(): + """A1: the inline card must only render when the turn has no Activity group, + so a tool-bearing turn's thinking-only sibling does not duplicate the card.""" + body = _render_messages_body() + assert "turnsWithActivityGroup" in body, ( + "must precompute the set of turns that already own an Activity group (#3709 A1)" + ) + # The inline render must be guarded by a membership check on that set. + assert re.search( + r"turnsWithActivityGroup\.has\(\s*anchorTurn\s*\)", + body, + ), "the inline thinking render must be gated on turnsWithActivityGroup.has(anchorTurn)" + + +def test_turns_with_activity_group_built_from_tool_bearing_segments(): + """The turnsWithActivityGroup set must be populated from tool-bearing message + segments' enclosing .assistant-turn nodes.""" + body = _render_messages_body() + block = re.search( + r"const turnsWithActivityGroup=new Set\(\);(.*?)const activityIdxs=", + body, + re.DOTALL, + ) + assert block, "turnsWithActivityGroup population block not found" + text = block.group(1) + assert "closest('.assistant-turn')" in text, ( + "must map tool-bearing segments to their enclosing .assistant-turn" + ) + assert "turnsWithActivityGroup.add(" in text + + +def test_inline_card_inserted_before_body_and_footer(): + """A2: when the inline render is correct, the card must land BEFORE the answer + body / msg-foot (beforebegin), not appended after the 'Done in ...' footer.""" + body = _render_messages_body() + # The inline branch selects the body/foot element and inserts before it. + assert re.search(r"querySelector\(\s*'\.msg-body,\.msg-foot'\s*\)", body), ( + "inline branch must locate the .msg-body/.msg-foot element to anchor before it" + ) + assert "insertAdjacentHTML('beforebegin'" in body, ( + "the inline thinking card must be inserted 'beforebegin' the answer body/footer " + "(not 'beforeend', which strands it below 'Done in ...') (#3709 A2)" + ) + + +def test_no_unconditional_beforeend_thinking_in_inline_branch(): + """The old orphaning insert ('beforeend' of the raw thinking card on the anchor + row) must be gone from the inline branch.""" + body = _render_messages_body() + # The specific regression pattern: appending the thinking card to the end of + # the anchor row unconditionally. It must no longer be the inline path. + assert "anchorRow.insertAdjacentHTML('beforeend',_thinkingCardHtml(assistantThinking.get(aIdx)))" not in body, ( + "the inline branch must not append the thinking card to the end of the " + "anchor row (that stranded it below the footer — the #3709 bug)" + ) + + +def test_turn_level_echo_strip_exists(): + """B: thinking is stripped against the TURN's combined visible answer, not only + the same message's body — so a trailing thinking-only message that echoes the + answer gets de-duped too.""" + body = _render_messages_body() + assert "_turnVisibleTextByRawIdx" in body, ( + "must build a per-turn combined visible-answer map (#3709 defect B)" + ) + # The strip site must consult the turn-level text in addition to displayContent. + assert re.search( + r"_turnVisibleTextByRawIdx\.get\(\s*rawIdx\s*\)", + body, + ), "the echo-strip must look up the turn's combined visible text" + # And it must feed that into the echo-strip helper. + strip_block = re.search( + r"_turnVisibleTextByRawIdx\.get\(\s*rawIdx\s*\)(.*?)_stripVisibleAssistantEchoFromThinking\(\s*thinkingText\s*,\s*turnVisible\s*\)", + body, + re.DOTALL, + ) + assert strip_block, ( + "the turn-level visible text must be passed to " + "_stripVisibleAssistantEchoFromThinking" + ) + + +def test_suppressed_sibling_thinking_merged_into_group_not_dropped(): + """When the A1 gate suppresses a thinking-only sibling's inline card (because + its turn has an Activity group), that sibling's thinking must NOT be lost — the + group must render the TURN's merged thinking, not only the tool message's own + entry. (Codex re-gate finding: rendering only assistantThinking.get(aIdx) for + the tool index dropped a distinct sibling's reasoning.)""" + body = _render_messages_body() + # A per-turn thinking aggregation must exist... + assert "turnThinkingParts" in body, ( + "must aggregate thinking per turn so a suppressed sibling's reasoning is " + "carried into the Activity group, not dropped (#3709 / Codex re-gate)" + ) + # ...and the Activity group must render the MERGED text, de-duped, once per turn. + assert "mergedThinking" in body, ( + "the Activity group must render the turn's merged thinking" + ) + assert "_renderedTurnThinking" in body, ( + "merged thinking must render once per turn (guard against double-emit when " + "a turn has multiple tool messages)" + ) + # The group node must be built from the merged text, not the single-index entry. + assert re.search( + r"_thinkingActivityNode\(\s*mergedThinking\s*,", + body, + ), "the Activity group thinking node must be built from mergedThinking" diff --git a/tests/test_ui_tool_call_cleanup.py b/tests/test_ui_tool_call_cleanup.py index 215d6713..4d6c8d73 100644 --- a/tests/test_ui_tool_call_cleanup.py +++ b/tests/test_ui_tool_call_cleanup.py @@ -272,10 +272,14 @@ class TestToolCallGroupingStatic: assert "isSimplifiedToolCalling()" in render_fn and "assistantThinking.set(rawIdx, thinkingText)" in render_fn, ( "Compact settled transcript rendering should preserve Thinking cards after switching sessions." ) - assert "_thinkingActivityNode(thinkingText, false)" in render_fn, ( + # #3709: the Activity disclosure now renders the TURN's merged thinking + # (mergedThinking — all of a turn's thinking de-duped, incl. a suppressed + # thinking-only sibling) rather than a single message's entry. Same node, + # same Activity body — only the source variable changed. + assert "_thinkingActivityNode(mergedThinking, false)" in render_fn, ( "Settled Thinking cards should render inside the compact Activity disclosure." ) - assert "body.appendChild(_thinkingActivityNode(thinkingText, false))" in render_fn, ( + assert "body.appendChild(_thinkingActivityNode(mergedThinking, false))" in render_fn, ( "Settled Thinking cards should stay inside the same Activity body as the related tools." ) assert ".agent-activity-thinking:not([data-live-thinking=\"1\"])" in render_fn, (