fix(ui): stop duplicating the final answer and a picker row (#2051)

The interrupt notice, and with it any turn-final assistant message, could be rendered twice through the compact Worklog fold (#6948). _assistantMessageBelongsInWorklog() folded on the _live marker before it reached the rule that protects a turn's final answer, but the caller only consults that predicate once the turn has settled, so a marker seen there is a leftover of the live-snapshot projection rather than an ongoing stream: the answer was hidden inline, echoed into the Worklog as a .wl-reason row, and the turn was left with no visible content, which the #3875 blank-turn fail-safe then revealed as two copies. A turn-final message with visible text is now kept out of the fold; every other _live message folds exactly as before.

Two nearby paths could produce the same duplicate on their own, so both are tightened: the settled Worklog echoes an anchor's prose only when that anchor was actually folded into it (assistant-segment-worklog-source), which is the proof the group construction already used, and the blank-turn fail-safe materializes a deferred Worklog (#5839) before judging it empty, so it no longer un-hides the folded segments and then renders the deferred rows beside them.

The composer picker listed one configured model twice because the provider-qualified spelling of its badge key was not recognised as an alias. _normalizeConfiguredModelKey() strips only one leading slash segment (#3360 keeps vendor_a/x and vendor_b/y/x distinct), so vendor/model and custom/vendor/model normalise to different keys and renderModelDropdown() synthesised a second row. _isEquivalentConfiguredModelEntry() now accepts <provider>/<model> under the same proof the @<provider>: rule uses - the badge declares the provider, the key starts with that provider's prefix, and an existing row from that provider normalises equal to the remainder - so only a duplicate of a row the catalog already produced can be dropped.

Adds tests/test_issue2051_duplicate_interrupt_notice.py and tests/test_issue2051_duplicate_model_picker_entry.py.
This commit is contained in:
Thireus ☠ committed 2026-09-21 10:38:40 +01:00
1 parent 166ca0ec05
commit 9d0c629b55
3 files changed
+441 -2

No files matched your search

+51 -2
View File
@@ -3939,6 +3939,27 @@ function _isEquivalentConfiguredModelEntry(modelId,badge,entries){
// providers (@custom:name:model) without collapsing matching model IDs from
// different providers.
const rawId=String(modelId||'');
// `<provider>/<model>` is another routing spelling of `<model>`, so its badge
// key must not become a second picker row. configured_model_badges holds every
// spelling of a configured model and renderModelDropdown() synthesises a row
// for each key this predicate does not recognise. The provider-qualified
// spelling was missed because _normalizeConfiguredModelKey() strips only one
// leading slash segment (#3360 keeps `vendor_a/x` and `vendor_b/y/x` distinct),
// so `acme/example-model` and `custom/acme/example-model` normalise to
// different keys and the picker lists one model twice.
// Match it the way the `@provider:` rule below does: the badge declares a
// provider, the key starts with that provider's `<provider>/` prefix, and an
// existing row from the same provider normalises equal to the remainder. Two
// different models never satisfy the last clause, so this can only drop a
// duplicate of a row the catalog already produced.
const slashPrefix=provider?`${provider}/`:'';
if(slashPrefix&&rawId.toLowerCase().startsWith(slashPrefix)){
const slashRoutedId=rawId.slice(slashPrefix.length);
if(slashRoutedId&&(entries||[]).some(entry=>
String(entry.providerId||'').toLowerCase()===provider
&&_normalizeConfiguredModelKey(entry.value)===_normalizeConfiguredModelKey(slashRoutedId)
)) return true;
}
const prefix=provider?`@${provider}:`:'';
if(!prefix||!rawId.toLowerCase().startsWith(prefix)) return false;
const routedId=rawId.slice(prefix.length);
@@ -11564,6 +11585,15 @@ function _assistantMessageBelongsInWorklog(m, rawIdx, toolCallAssistantIdxs, vis
const isTurnFinalAssistant=!!(opts&&opts.isTurnFinalAssistant);
const visibleText=String(visibleContent!==undefined?visibleContent:msgContent(m)||'').trim();
const hasVisibleText=!!visibleText&&!_isAssistantEmptyPlaceholderContent(m, visibleText);
// The caller only consults this predicate once the turn has settled (it gates
// on `!S.busy`), so an `_live` marker seen here is a leftover of the
// live-snapshot projection, not an ongoing stream. Folding on it hid the turn's
// final answer inline and echoed it into the Worklog, leaving the turn with no
// visible content until the #3875 blank-turn fail-safe revealed both copies. A
// turn-final assistant message with visible text is the answer, so keep it out
// of the fold; every other `_live` message folds as before. Kept as a separate
// guard so the live rule below keeps its position.
if(m._live&&hasVisibleText&&isTurnFinalAssistant) return false;
if(m._live) return true;
if(hasVisibleText&&m._anchor_activity_scene) return false;
if(hasVisibleText&&isTurnFinalAssistant) return false;
@@ -18317,12 +18347,15 @@ function renderMessages(options){
if(!cards.length&&!anchorReasonHtml&&!thinkingText) continue;
const anchorTurn=anchorRow.closest('.assistant-turn');
if(!anchorTurn) continue;
// Hoisted out of the `if(!state)` block below (same expression, same
// value) so the append path can use the ownership fact the group
// construction already uses.
const anchorIsWorklogSource=anchorRow.classList&&anchorRow.classList.contains('assistant-segment-worklog-source');
let state=activityByTurn.get(anchorTurn);
if(!state){
const includeTurnDuration=!durationAssignedTurns.has(anchorTurn);
if(includeTurnDuration) durationAssignedTurns.add(anchorTurn);
const activityKey=`assistant:${aIdx}`;
const anchorIsWorklogSource=anchorRow.classList&&anchorRow.classList.contains('assistant-segment-worklog-source');
const group=ensureActivityGroup(anchorParent,{
collapsed:true,
anchor:anchorRow,
@@ -18342,7 +18375,14 @@ function renderMessages(options){
state.cards.push(...cards);
_appendWorklogStep(state.group, anchorRow, cards, thinkingText, {
live:false,
includeAnchorReason:!!includeAnchorReason&&!!anchorReasonHtml,
// Echo an anchor's prose as a `.wl-reason` row only when that anchor was
// folded into this Worklog. `assistant-segment-worklog-source` is the
// proof, and its `display:none` is the only reason the echo is not a
// second visible copy. The group construction above already reasons
// that way (`syncAnchorReason`); the append path did not, so an anchor
// that escapes the fold (the turn-final answer, an `_error` message)
// had its text rendered both inline and inside the Worklog.
includeAnchorReason:!!includeAnchorReason&&!!anchorReasonHtml&&!!anchorIsWorklogSource,
thinkingKey:thinkingText?`thinking:${_normalizeThinkingEchoCompare(thinkingText)}`:'',
thinkingDisclosureKey:thinkingText?`thinking:${entry.key}`:'',
seenReasons:state.seenReasons,
@@ -18631,6 +18671,15 @@ function renderMessages(options){
const groups=turn.querySelectorAll('.tool-worklog-group,.tool-call-group');
let revealed=false;
for(const group of groups){
// A settled Worklog whose rows are still deferred (#5839) has an empty
// textContent but is not empty in substance. Judging it empty here drops
// through to the last-resort un-hide below, and the deferred rows then
// materialize the same prose beside the segments it just un-hid.
// Materialize first, then judge.
if(group.getAttribute&&group.getAttribute('data-worklog-rows-deferred')==='1'
&&typeof _materializeDeferredWorklogRows==='function'){
_materializeDeferredWorklogRows(group);
}
if(!(group.textContent||'').trim()) continue; // empty group can't help
if(group.classList.contains('tool-call-group-collapsed')){
group.classList.remove('tool-call-group-collapsed');
@@ -0,0 +1,234 @@
"""Regression: an interrupted turn's notice — and any turn-final answer — is
displayed twice (#6948 / #2051).
A ``hermes chat`` run killed mid tool-call persists a single assistant message
``Operation interrupted.``; the WebUI renders that notice twice. The same
doubling was reported for a long tool run's final answer and for an image reply
carrying a ``MEDIA:`` line. The stored data is clean in every case — one message
in ``state.db``, one in the session sidecar, one in ``/api/session`` — so both
copies are produced client-side.
``renderMessages()`` folds intermediate assistant segments into the collapsed
Worklog card (#3401): the inline segment is classed
``assistant-segment-worklog-source`` (``display:none``) and its prose is
re-rendered as a ``.wl-reason`` row inside the Worklog. Two copies, one hidden —
correct, as long as the fold decision is correct.
``_assistantMessageBelongsInWorklog()`` folded on ``m._live`` before it could
reach the rule that protects the answer (``hasVisibleText && isTurnFinalAssistant
-> false``). Its caller only consults the predicate once the turn has settled (it
gates on ``!S.busy``), so an ``_live`` marker seen there is a leftover of the
live-snapshot projection rather than an ongoing stream. Folding on it hides the
answer inline and echoes it into the Worklog: the turn then holds two copies and
no visible content, which the #3875 blank-turn fail-safe reveals — by expanding
the Worklog, or by un-hiding the source segments — so the user sees the notice or
answer twice.
Three contracts are pinned here:
1. ``_assistantMessageBelongsInWorklog``: a live marker may no longer outrank the
turn-final visible answer. Every other ``_live`` message folds as before, and
the change can only keep a segment visible, never hide or drop one.
2. ``renderMessages``: the Worklog may echo an anchor's prose as a ``.wl-reason``
row only when that anchor was folded into it —
``assistant-segment-worklog-source`` is that proof, and its ``display:none``
is the only reason the echo is not a second visible copy. This also covers
anchors that escape the fold for other reasons, such as ``_error`` messages,
which are never folded.
3. The #3875 fail-safe must materialize a deferred settled Worklog (#5839) before
judging it empty; otherwise it falls through to the last-resort un-hide,
strips the fold off every source segment, and the deferred rows then render
the same prose beside them.
Each contract is a structural/ownership check rather than a rendered-text
comparison, and each fails towards keeping content: the first only un-hides, the
second only suppresses an echo whose original is provably visible, the third only
builds rows earlier.
"""
import shutil
import subprocess
from pathlib import Path
import pytest
from tests.js_source_extract import extract_function
ROOT = Path(__file__).resolve().parents[1]
UI_JS = (ROOT / "static" / "ui.js").read_text(encoding="utf-8")
NODE = shutil.which("node")
def js(*names):
"""Real source of the named ui.js functions — never a re-implementation."""
return "\n".join(extract_function(UI_JS, name) for name in names)
def _function_body(src: str, name: str) -> str:
marker = f"function {name}("
start = src.find(marker)
assert start != -1, f"{name} not found"
brace = src.find("{", start)
assert brace != -1, f"{name} body not found"
depth = 0
for idx in range(brace, len(src)):
ch = src[idx]
if ch == "{":
depth += 1
elif ch == "}":
depth -= 1
if depth == 0:
return src[brace + 1 : idx]
raise AssertionError(f"{name} body not closed")
def run_node(script):
out = subprocess.run([NODE, "-e", script], capture_output=True, text=True)
assert out.returncode == 0, f"node harness failed:\n{out.stderr}\n{out.stdout}"
assert "OK" in out.stdout, out.stdout
return out.stdout
HARNESS = """
const assert=require('assert');
var window={};
function _messageHasReasoningPayload(m){ return !!(m&&(m.reasoning||m.reasoning_content)); }
const assistant=(content,extra)=>Object.assign({role:'assistant',content},extra||{});
"""
def test_live_marker_guard_precedes_the_live_fold_rule():
"""Contract 1, structurally: the guard sits before the live fold rule.
Written as an added guard rather than an edit to `if(m._live) return true;`
so the live rule keeps the exact shape and ordering pinned by
tests/test_live_to_final_anchor_visible_order.py.
"""
belongs = _function_body(UI_JS, "_assistantMessageBelongsInWorklog")
guard = "if(m._live&&hasVisibleText&&isTurnFinalAssistant) return false;"
assert guard in belongs, (
"a settled-render live remnant must not fold the turn's final visible answer"
)
assert belongs.index(guard) < belongs.index("if(m._live) return true;"), (
"the guard must be evaluated before the unconditional live fold"
)
@pytest.mark.skipif(NODE is None, reason="node not on PATH")
def test_stale_live_marker_never_folds_the_turn_final_answer():
"""The fold must not swallow the answer on a settled-render live remnant."""
script = HARNESS + js(
"msgContent",
"_isAssistantEmptyPlaceholderContent",
"_assistantMessageBelongsInWorklog",
) + """
const FINAL={isTurnFinalAssistant:true};
const MID={isTurnFinalAssistant:false};
const owners=new Set([0]);
// The bug: the settled turn-final answer folds on a stale live marker.
assert.strictEqual(
_assistantMessageBelongsInWorklog(assistant('Operation interrupted.',{_live:true}),0,owners,undefined,FINAL),
false,
'a settled turn-final notice must never be folded into the Worklog (#2051)');
assert.strictEqual(
_assistantMessageBelongsInWorklog(
assistant('Here is the image you asked for.\\n\\nMEDIA:/tmp/example.png',{_live:true}),0,owners,undefined,FINAL),
false,
'a MEDIA-bearing turn-final answer must never be folded either');
// Renderer-independent: the decision must not depend on what the text looks like.
assert.strictEqual(
_assistantMessageBelongsInWorklog(
assistant('Here you go:\\n\\n```python\\nprint(1)\\n```',{_live:true}),0,owners,undefined,FINAL),
false,
'a fenced-code turn-final answer must never be folded');
// Everything else about the fold is unchanged.
// A live intermediate segment still folds — that is the Worklog's content.
assert.strictEqual(
_assistantMessageBelongsInWorklog(assistant('Reading the files now.',{_live:true}),0,owners,undefined,MID),
true,
'a live intermediate segment must still fold into the Worklog');
// A live turn-final message with no visible text still folds (an empty tool-call
// anchor is Worklog material, not an answer).
assert.strictEqual(
_assistantMessageBelongsInWorklog(assistant('',{_live:true}),0,owners,undefined,FINAL),
true,
'an empty live anchor must still fold');
// `(empty)` placeholder + reasoning payload is not visible text either.
assert.strictEqual(
_assistantMessageBelongsInWorklog(
assistant('(empty)',{_live:true,reasoning:'thought'}),0,owners,undefined,FINAL),
true,
'an (empty) placeholder anchor must still fold');
// Settled (no marker at all) is untouched in both directions.
assert.strictEqual(
_assistantMessageBelongsInWorklog(assistant('Operation interrupted.'),0,owners,undefined,FINAL),
false, 'settled turn-final answer: unchanged');
// (A settled message with visible text and no live/burst marker never folds —
// `if(hasVisibleText) return false;` precedes the tool-metadata rule. Pinned so a
// future change to the live-marker line cannot quietly alter this ordering.)
assert.strictEqual(
_assistantMessageBelongsInWorklog(assistant('Reading the files now.'),0,owners,undefined,MID),
false, 'settled visible intermediate without a live/burst marker: unchanged');
assert.strictEqual(
_assistantMessageBelongsInWorklog(assistant('',{}),0,owners,undefined,MID),
true, 'settled empty tool-call anchor still folds: unchanged');
assert.strictEqual(
_assistantMessageBelongsInWorklog(assistant('Done.'),0,new Set(),undefined,MID),
false, 'settled intermediate with no tool metadata: unchanged');
// An error message is still never folded.
assert.strictEqual(
_assistantMessageBelongsInWorklog(assistant('**Error:** boom',{_error:true,_live:true}),0,owners,undefined,MID),
false, 'an error message is never folded');
// Live burst/segment markers on a non-final message still fold.
assert.strictEqual(
_assistantMessageBelongsInWorklog(assistant('mid',{_activityBurstId:2}),0,new Set(),undefined,MID),
true, 'activity-burst anchors still fold');
console.log('OK');
"""
run_node(script)
def test_worklog_reason_echo_requires_a_folded_anchor():
"""Contract 2: only a folded anchor may be echoed as a .wl-reason row."""
body = _function_body(UI_JS, "renderMessages")
assert "includeAnchorReason:!!includeAnchorReason&&!!anchorReasonHtml&&!!anchorIsWorklogSource," in body, (
"the settled Worklog step may only re-render an anchor's prose when that "
"anchor was folded into it (assistant-segment-worklog-source); without "
"that proof the row is a second visible copy of a visible segment"
)
# The ownership fact must be computed for every entry, not only for the first
# entry of a turn (where the group is constructed) — otherwise the append path
# would read a stale/undefined value for later anchors in the same turn.
hoisted = body.find(
"const anchorIsWorklogSource=anchorRow.classList"
"&&anchorRow.classList.contains('assistant-segment-worklog-source');"
)
state_lookup = body.find("let state=activityByTurn.get(anchorTurn);")
assert hoisted != -1 and state_lookup != -1, "settled worklog append path not found"
assert hoisted < state_lookup, (
"anchorIsWorklogSource must be computed per activity entry (before the "
"per-turn `state` lookup), not only inside the group-construction branch"
)
def test_blank_turn_failsafe_materializes_deferred_rows_before_judging_empty():
"""Contract 3: a deferred (#5839) Worklog is not empty."""
body = _function_body(UI_JS, "renderMessages")
failsafe = body[body.find("Fail-safe invariant (#3875)"):]
assert failsafe, "the #3875 fail-safe is missing"
materialize = failsafe.find("data-worklog-rows-deferred")
empty_check = failsafe.find("continue; // empty group can't help")
assert materialize != -1, (
"the fail-safe must materialize a deferred settled Worklog before judging "
"it empty — otherwise it un-hides every folded segment and the deferred "
"rows later render the same prose beside them"
)
assert materialize < empty_check, (
"deferred rows must be materialized before the empty-group check"
)
# The last-resort un-hide must still exist: content is never lost.
assert "seg.classList.remove('assistant-segment-worklog-source');" in failsafe, (
"the last-resort un-hide must remain — a duplicate is preferable to a "
"blank turn"
)
@@ -0,0 +1,156 @@
"""Regression: the composer model picker lists one configured model twice (#2051).
The picker showed two rows for a single configured model, ``acme/example-model``
and ``custom/acme/example-model``, while ``GET /api/models`` returned a ``Custom``
group holding exactly one model and ``#modelSelect`` ended up with exactly one
matching ``<option>``. Both rows are produced client-side.
``configured_model_badges`` carries every routing spelling of a configured model.
For one custom-provider model that is three keys::
acme/example-model (bare/canonical)
custom/acme/example-model (provider-qualified)
@custom:acme/example-model (routable)
``renderModelDropdown()`` walks that map and pushes a synthetic ``_modelData``
row for every key that ``_isEquivalentConfiguredModelEntry()`` does not recognise
as an alias of a row the catalog already produced. That predicate recognised two
of the three spellings; it missed ``<provider>/<model>`` because
``_normalizeConfiguredModelKey()`` strips only one leading slash segment —
deliberately, since #3360 requires ``vendor_a/x`` and ``vendor_b/y/x`` to stay
distinct. So::
acme/example-model -> example.model
custom/acme/example-model -> acme/example.model (different key)
No match, so the alias was pushed as a second row: one model, two picker entries.
``_deduplicateModelPickerOptions()`` cannot catch this — it dedupes ``<option>``
elements inside ``<select>``, and this row never becomes an ``<option>``.
The contract pinned here: ``<provider>/<model>`` is recognised as an alias of
``<model>`` under the same proof the ``@<provider>:<model>`` rule already uses.
The badge declares a provider, the key begins with that provider's prefix, and an
existing picker row from that same provider normalises equal to the remainder.
Two genuinely different models never satisfy the last clause, so the change can
only drop a duplicate — never hide a distinct model.
"""
import shutil
import subprocess
from pathlib import Path
import pytest
from tests.js_source_extract import extract_function
ROOT = Path(__file__).resolve().parents[1]
UI_JS = (ROOT / "static" / "ui.js").read_text(encoding="utf-8")
NODE = shutil.which("node")
def js(*names):
"""Real source of the named ui.js functions — never a re-implementation."""
return "\n".join(extract_function(UI_JS, name) for name in names)
def run_node(script):
out = subprocess.run([NODE, "-e", script], capture_output=True, text=True)
assert out.returncode == 0, f"node harness failed:\n{out.stderr}\n{out.stdout}"
assert "OK" in out.stdout, out.stdout
return out.stdout
HARNESS = """
const assert=require('assert');
var window={};
"""
# One Custom group holding one model, with three badge spellings of it.
CATALOG_ROW = "{value:'acme/example-model',providerId:'custom'}"
BADGE = "{role:'primary',label:'Primary',provider:'custom'}"
@pytest.mark.skipif(NODE is None, reason="node not on PATH")
def test_provider_qualified_badge_alias_is_not_a_second_picker_row():
"""All three spellings of one configured model collapse to one row (#2051)."""
script = HARNESS + js(
"_normalizeConfiguredModelKey",
"_isEquivalentConfiguredModelEntry",
) + f"""
const entries=[{CATALOG_ROW}];
const badge={BADGE};
// All three routing spellings name the same model the catalog already rendered.
for(const alias of ['acme/example-model',
'custom/acme/example-model',
'@custom:acme/example-model']){{
assert.strictEqual(_isEquivalentConfiguredModelEntry(alias,badge,entries), true,
'badge alias '+alias+' must not become a second picker row (#2051)');
}}
console.log('OK');
"""
run_node(script)
@pytest.mark.skipif(NODE is None, reason="node not on PATH")
def test_a_genuinely_different_model_is_never_suppressed():
"""The new rule must not hide a model the catalog does not have."""
script = HARNESS + js(
"_normalizeConfiguredModelKey",
"_isEquivalentConfiguredModelEntry",
) + f"""
const entries=[{CATALOG_ROW}];
const badge={BADGE};
// Same provider prefix, different model -> must still render its own row.
assert.strictEqual(
_isEquivalentConfiguredModelEntry('custom/acme/other-model',badge,entries), false,
'a different model under the same provider must keep its own row');
// Right model name, but no catalog row owned by that provider -> not proven
// equivalent, so keep it.
assert.strictEqual(
_isEquivalentConfiguredModelEntry(
'custom/acme/example-model',
{{role:'primary',label:'Primary',provider:'otherprovider'}},
[{{value:'acme/example-model',providerId:'custom'}}]), false,
'a same-named model owned by a different provider must keep its own row');
// A badge with no provider cannot use the prefix proof at all.
assert.strictEqual(
_isEquivalentConfiguredModelEntry('custom/acme/example-model',
{{role:'primary',label:'Primary'}},entries), false,
'a provider-less badge must not be collapsed by the prefix rule');
// The prefix must be a real path segment, not a substring of the first segment.
assert.strictEqual(
_isEquivalentConfiguredModelEntry('customer/acme/example-model',badge,entries), false,
'only a literal `<provider>/` prefix counts');
console.log('OK');
"""
run_node(script)
def test_prefix_rule_mirrors_the_at_provider_rule_structurally():
"""The `<provider>/` proof must require the same three clauses as `@<provider>:`.
Structural, not behavioural: it pins that the new branch is gated on the
badge-declared provider, on a literal prefix match, and on an existing entry
of that same provider — so it cannot grow into a loose label comparison.
"""
src = extract_function(UI_JS, "_isEquivalentConfiguredModelEntry")
assert "const slashPrefix=provider?`${provider}/`:'';" in src, (
"the provider-qualified alias rule must derive its prefix from the "
"badge-declared provider"
)
assert "rawId.toLowerCase().startsWith(slashPrefix)" in src, (
"the rule must require a literal `<provider>/` prefix on the badge key"
)
assert "String(entry.providerId||'').toLowerCase()===provider" in src, (
"the rule must require an existing picker row owned by the same provider"
)
# It must run before the `@provider:` early return, which would otherwise
# short-circuit every non-`@` alias to false.
assert src.index("slashPrefix") < src.index(
"if(!prefix||!rawId.toLowerCase().startsWith(prefix)) return false;"
), "the `<provider>/` rule must be reached before the `@<provider>:` early return"