Review — end-to-end ✅ (approve with one substantive non-blocking concern on #1341)

Release v0.50.246 integrating 5 PRs. #1338 was previously formally approved by nesquena (review-4206679556) and the squash matches that approved code byte-for-byte (verified below). The other four constituents (#1335, #1337, #1339, #1341) are reviewed here for the first time.

Squash audit (byte-identical to open PRs)

e2d33ff #1341  12+/0  across 2 files       ✅ matches open PR
09e12e3 #1339  18+/12-  across 1 file       ✅ matches open PR
fbe84d2 #1337  140+/28- across 10 files     ✅ matches open PR
1fa740d #1335  161+/1-  across 2 files      ✅ matches open PR
d4b055c #1338  417+/5-  across 3 files      ✅ matches prior approved diff (the
                                                2-line gap vs the open PR's 419+/-5
                                                is the CHANGELOG entries that were
                                                relocated under [v0.50.246] in the
                                                release commit, per the PR body)
50418cd  test stabilizer + #1339 regression — release-branch test commit

Confirmed #1338 carries the approved code

d4b055c matches d90a605 (the head of PR #1338) for the actual code surface:

• _liveActivityUserExpanded markers in static/ui.js: 6 occurrences in both
• _onLiveActivityToggle: 2 occurrences in both
• The # ── Preserve the user's typed message before clearing pending state (#1298) ── comment in api/streaming.py: 1 occurrence in both
• tests/test_issue1298_cancel_and_activity.py is byte-identical (diff shows 0 lines)

So my prior end-to-end trace for #1338 stands and I'm not re-tracing here.

End-to-end traces (new content)

PR #1335 — render fenced code blocks in user messages (@bergeouss, fixes #1325)

New helper _renderUserFencedBlocks(text) at static/ui.js:60-87. Pipeline:

1. Match /```([a-zA-Z0-9_+-]*)\n([\s\S]*?)```/g. Each fence becomes <pre><code> (with diff/patch colored-line variant) and is stashed under a \x00UF<idx>\x00 placeholder.
2. The remainder of the string is escaped via esc() and \n → <br>.
3. Placeholders are restored from the stash.

Wire-up at static/ui.js:3087: user-message branch in renderMessages() swaps esc(...).replace(/\n/g,'<br>') for _renderUserFencedBlocks(content). Assistant branch unchanged (still renderMd).

Security: the stashed code is esc(code) — already HTML-safe before stashing. The placeholder pattern uses \x00 null bytes which are extremely unlikely in user input but technically possible via JSON-encoded chat input (�). If a user types literal \x00UF99\x00, the regex matches and stash[99] is undefined → coerces to literal "undefined" text in output. Not an XSS vector — content of any matched stash entry is already escaped at stash time. Worst case is cosmetic UX (literal "undefined" text). Behavioural test (test_1325_user_fenced_code.py) actually runs the JS via node — confirmed pass.

PR #1337 — Mermaid render error cleanup + PWA cache-bust (@dso2ng)

Two related fixes:

1. Mermaid temp DOM cleanup at static/ui.js:4138-4151. When mermaid.render(id, code) fails, the lib leaves a body-level <div id="d<id>">…</div> containing a "Syntax error in text" SVG. The previous code never removed this. Fix: document.getElementById('d'+id) is removed on both success AND failure paths. Failed blocks also drop the mermaid-block class so a later render pass can't retry the same broken content.
2. Tighter mermaid heuristic at static/ui.js:1085-1097: rejects line-numbered tool output (/^\s*\d+\|/.test(firstCodeLine)) and requires the first non-comment line to start with a known mermaid keyword (graph, flowchart, sequenceDiagram, pie, gantt, etc., 30+ keywords).
3. PWA cache-bust at static/index.html:50,954-964: script tags and sw.js registration get ?v=__WEBUI_VERSION__. The token is server-side substituted in api/routes.py:720-723 and api/routes.py:781-786 — uses urllib.parse.quote(WEBUI_VERSION, safe="") to URL-encode (defensive against unusual git-tag formats). WEBUI_VERSION is read from git describe --tags --always --dirty or api/_version.py (api/updates.py:60-92), so it's server-controlled, not user-input.
4. Service worker bypass for itself at static/sw.js:65-67: if (url.pathname.endsWith('/sw.js')) return; — returning from the fetch handler lets the browser do a normal network fetch, ensuring the freshly-versioned sw.js is always retrieved.

Cross-tool: webui-only, no agent surface.

PR #1339 — handle list fallback_providers config (@jimdawdy-hub)

Pre-fix at api/streaming.py:1701: _fallback = _cfg.get('fallback_model') or None — if user had fallback_providers: [{...}, {...}] (the chained-fallback form), this returned None. Even if it had returned the list, _fallback.get('model', '') on a list would raise AttributeError.

Post-fix:

• _fallback = _cfg.get('fallback_model') or _cfg.get('fallback_providers') or None
• isinstance(_fallback, list) → iterate to first valid entry; isinstance(_fallback, dict) → use directly.

Verified against agent CLI behaviour at /tmp/hermes-agent-fresh/cli.py:2055-2059: the CLI does the same dual-key lookup but in opposite order (fallback_providers first, fallback_model fallback). If a user has BOTH keys set (rare), the CLI will use fallback_providers while the WebUI will use fallback_model. Minor inconsistency, not a bug since this is an edge case (users typically have only one).

Behavioural test (test_pr1339_fallback_providers_list.py) is structural-only (asserts that streaming.py contains the dual-key check via grep). The trace above covers the actual behaviour.

PR #1341 — context_length / threshold_tokens / last_prompt_tokens persistence (@fxd-jason, "#1318 split")

This is the constituent that needs the most attention. The PR enables persistence but does not add the writer, so the user-visible bug it claims to fix is NOT actually fixed by this PR alone.

What the PR does:

What the PR does NOT do, and what's needed to actually fix the bug:

$ grep -rn "= .*context_length\|context_length =" api/
api/models.py:347:        self.context_length = context_length    # constructor only
api/streaming.py:2209:                usage['context_length'] = getattr(_cc, 'context_length', 0) or 0
                                       ↑ writes to the SSE usage payload, NOT to session

There is no writer to s.context_length anywhere in the codebase. The PR description claims "These fields are written by streaming.py during context compression" — that's inaccurate. They're written to the SSE usage payload at api/streaming.py:2209-2211, but never to the session itself.

Without a writer, the data flow is:

agent.context_compressor.context_length  (live during streaming)
    ↓ getattr at api/streaming.py:2209
SSE 'done' event → usage payload     (delivered to live client)
    ↓
S.lastUsage in browser memory          (fine while page is open)

  ── page reload here → S.lastUsage cleared ──

GET /api/session → s.context_length    (None on every reload, since no
                                        writer ever populated it)
    ↓
sessions.js:500 _pick(u.context_length, _s.context_length)  → 0 default
    ↓
Context-ring shows 0%                  (the bug, still present)

The CHANGELOG entry at line 16 says "The frontend context-ring indicator was previously losing its percentage on every session load because the Session model silently dropped these fields when reconstructing from disk." That's only true if something was writing to __dict__['context_length'] so save()'s extra dict caught it (see api/models.py:375-377). Without a writer, nothing lands in __dict__ either. The "silently dropped" phrasing implies the data was being written but lost on load — but it was never being written to the Session in the first place.

To actually fix the bug, a 3-line addition is needed near api/streaming.py:2208:

_cc = getattr(agent, 'context_compressor', None)
if _cc:
    usage['context_length'] = getattr(_cc, 'context_length', 0) or 0
    usage['threshold_tokens'] = getattr(_cc, 'threshold_tokens', 0) or 0
    usage['last_prompt_tokens'] = getattr(_cc, 'last_prompt_tokens', 0) or 0
+    # Persist on session so the indicator restores after a page reload (#1318)
+    s.context_length = usage['context_length']
+    s.threshold_tokens = usage['threshold_tokens']
+    s.last_prompt_tokens = usage['last_prompt_tokens']

This sits inside the existing with _agent_lock: block (api/streaming.py:2152) which already owns the session lock during the post-result merge, and the subsequent s.save() would persist them.

I'm not pushing this fix — it's a release-batch PR and the title says "(#1318 split)", explicitly signalling this is a deliberate part-1 component. Adding the writer here would expand scope beyond what fxd-jason / the maintainer chose to land. But the user-visible claim in the CHANGELOG is misleading and a follow-up PR is needed to actually close #1318. Approving as-is because the data-structure changes are safe and load-bearing for whatever follows; the CHANGELOG entry should be tightened to "data-structure scaffolding for #1318 — writer in follow-up".

Cross-PR interactions

Three PRs touch static/ui.js (#1335, #1337, #1338) and one touches api/streaming.py (#1339, #1338). Verified disjoint regions:

• #1335 — adds _renderUserFencedBlocks near line 60 + swap at line ~3087 (renderMessages user branch).
• #1337 — modifies renderMd mermaid heuristic near line 1085 + renderMermaidBlocks near line 4135.
• #1338 — adds _liveActivityUserExpanded tracker near line 2545 + ensureActivityGroup re-create branch near 2562 + finalizeThinkingCard near 4180.
• #1339 — modifies streaming.py:1701-1722 (fallback resolution).
• #1338 — modifies streaming.py:2576-2627 (cancel_stream recovery block).

No overlap. Tests pass.

Cross-tool consistency

• #1339 fallback_providers: agent CLI handles both keys (cli.py:2055-2059). WebUI now consistent.
• #1341 context_length / threshold_tokens / last_prompt_tokens: webui-only fields, surfaced from agent's compressor via SSE; no agent-side persistence implication. Cross-tool clean.
• #1337 cache-bust: webui-only, no agent surface.
• #1335 user fenced code: webui rendering only, no agent surface.
• #1338 cancel recovery: webui session state only, no agent surface (pending_user_message is a WebUI-only field).

Security audit

• ✅ No new endpoints, auth gates, or file-serving code paths.
• ✅ No eval/exec/Function()/shell=True in the diff.
• ✅ No secrets/tokens in the diff.
• ✅ #1335 user fenced code: stashed code is escaped before stashing; null-byte placeholder pattern is XSS-safe (worst case is "undefined" text rendering).
• ✅ #1337 cache-bust token substitution: urllib.parse.quote(WEBUI_VERSION, safe="") defensively URL-encodes; WEBUI_VERSION is server-controlled (git describe / build-time file).
• ✅ #1339 fallback list iteration: isinstance guards on every step prevent type confusion.
• ✅ #1338: re-confirmed against my prior approval.
• ✅ #1341: data-structure-only, no security surface.

Edge-case matrix

Tests

• PR-specific tests across 10 files: 155/155 pass
• Full suite: 3291 passed, 54 skipped, 3 xpassed, 0 failed in 15.75s (3348 collected)
• The PR description claims "3343 passed" — close enough; the discrepancy is consistent with skipped/xpassed/subtests being counted differently across runs.
• test_pr1339_fallback_providers_list.py is structural (regex-based) but the trace covers the runtime behaviour.

Minor observations (non-blocking)

• #1341 is data-structure-only: see #1341 trace above. Approving on the basis that the persistence scaffolding is harmless and load-bearing for whatever follows; CHANGELOG entry should ideally be reworded to make clear the writer is still pending. The user-visible bug ("context-ring shows 0% after reload") is NOT fixed by this PR alone.
• #1339 ordering inconsistency: WebUI tries fallback_model first, CLI tries fallback_providers first. If a user has both set, behaviour differs. Recommend matching the CLI's preference order in a follow-up. Not a bug since both keys aren't supposed to coexist.
• #1335 null-byte placeholder collision: literal \x00UF<N>\x00 in user input could trigger a placeholder restore for an unrelated stash index. Not exploitable (output is escaped either way) but a hardened version could use a Math.random() salt in the placeholder. Cosmetic edge case.
• PR description's test count 3343 passed vs my local 3291 passed — a 52-test gap. Probably environment differences (some tests skip in non-CI runs); 0 failures is what matters.
• CHANGELOG line 16 — "silently dropped these fields when reconstructing from disk" — overstates the fix. Without a writer, nothing was ever written to disk in the first place. See #1341 trace above.

Recommendation

✅ Approved. Five constituent traces clean (security-safe, behaviorally correct, no cross-tool regressions). #1338 squash matches the previously-approved code byte-for-byte. The flagged #1341 gap (no writer) is a documentation/scope issue, not a correctness regression — the persistence scaffolding is safe to merge and unblocks the follow-up writer PR. Approval is conditional on a follow-up landing the writer (3 lines in streaming.py near line 2208) before claiming #1318 fully fixed.

Parked at approval — ready for the release agent's merge/tag pipeline.

Followup Review — both flagged concerns addressed ✅ (clean re-approve)

This is a followup to my prior approval (4206870751). Two new commits landed on the release branch since then, and they directly address the two concerns my earlier trace flagged:

Both fixes traced cleanly. Re-approving.

f328f3b — gate substring guard on pending_started_at timestamp

The concern. PR #1338 added a content-match guard in cancel_stream() to avoid double-appending the user's typed turn when the streaming thread had already merged it. The guard was symmetric:

if _pending_user in _last_content or _last_content in _pending_user:
    _already_persisted = True

The _last_content in _pending_user direction is a false-positive vector. If the prior user turn was a short confirmation ("ok"), and the cancelled turn is a longer follow-up ("ok please continue"), then "ok" in "ok please continue" is True → guard fires → synthesis is skipped → user's "ok please continue" is lost. Exactly the data-loss bug #1298 was supposed to fix.

The fix. Two changes at api/streaming.py:2623-2643:

1. Add a timestamp gate: read _pending_started = _cs.pending_started_at and _last_ts = _last_user.get('timestamp'); only treat the latest user turn as "already merged" if _last_ts >= _pending_started. An earlier turn whose content happens to be a substring of the new pending must NOT short-circuit synthesis.
2. Drop the symmetric in direction: replace _pending_user in _last_content or _last_content in _pending_user with _pending_user == _last_content or _pending_user in _last_content. Keeps equality (exact match) and prefix-tolerance (the streaming thread prepends [Workspace: ...] which makes _last_content a strict superset of _pending_user), drops the false-positive direction.

Race trace under the new guard:

All scenarios safe. The new test test_cancel_synthesizes_when_prior_turn_content_is_substring_of_pending locks the regression. ✅

a5c10d5 — adds the missing writer for #1341

The concern from my prior review: PR #1341 added the Session data structure (constructor params, METADATA_FIELDS, compact(), /api/session route) but did NOT add the writer in streaming.py. Without a writer, s.context_length stayed None forever and the post-reload context-ring still showed 0% — so the user-visible bug was NOT fixed by #1341 alone.

The fix. New 5-line writer block at api/streaming.py:2188-2198, placed BEFORE the existing s.save() at line 2199:

# Persist context window data on the session so the context-ring
# indicator survives a page reload (#1318). Must run BEFORE
# s.save() for the same reason as the reasoning trace above.
_cc_for_save = getattr(agent, 'context_compressor', None)
if _cc_for_save:
    s.context_length = getattr(_cc_for_save, 'context_length', 0) or 0
    s.threshold_tokens = getattr(_cc_for_save, 'threshold_tokens', 0) or 0
    s.last_prompt_tokens = getattr(_cc_for_save, 'last_prompt_tokens', 0) or 0
s.save()

End-to-end trace verified:

1. Block sits inside with _agent_lock: (opened at api/streaming.py:1969). Same lock as all other session mutations. ✅
2. Runs after the result-merge and BEFORE s.save() — so the values are in __dict__ when save serializes. ✅
3. getattr(_cc_for_save, '...', 0) or 0 defends against missing fields and explicit None from older agent builds. ✅
4. The values now flow: agent.context_compressor → s.context_length (in-memory) → s.save() writes to disk via METADATA_FIELDS round-trip → Session.load() reads them back via the named kwargs from #1341 → /api/session GET returns them → frontend _pick(u.X, _s.X) falls back to _s.X after page reload (when S.lastUsage is empty). ✅
5. The SSE usage payload below at line 2207-2211 still captures the same values — live streaming clients still get them in real time. The new writer is the persistence path that survives reload. The two paths are now distinct, with comments updated to make that clear.

Lock safety: identical to other session writers in the same with _agent_lock: block (s.tool_calls, s.active_stream_id = None, s.input_tokens += ...). No concurrent reader can race because both readers (HTTP /api/session handler, cancel_stream) acquire _get_session_agent_lock(sid) for the same lock. Safe. ✅

Round-trip test verified: test_session_round_trip_persists_context_fields actually saves a Session with the fields set, calls Session.load(sid), asserts the values come back identical (200000 / 180000 / 45123). Real round-trip, not regex. ✅

CHANGELOG entry rewritten to remove the misleading "silently dropped these fields" phrasing and credit the writer addition. The new entry honestly reflects that #1341 was scaffolding-only and the writer was added during pre-release review.

Tests after these followup fixes

• tests/test_issue1298_cancel_and_activity.py — 10/10 pass (added test_cancel_synthesizes_when_prior_turn_content_is_substring_of_pending)
• tests/test_pr1341_context_window_persistence.py — 6/6 pass (5 structural + 1 round-trip)
• Full suite: 3298 passed, 54 skipped, 3 xpassed, 0 failed in 15.59s (up from 3291 in my prior run; +7 new tests across these two commits)

Edge-case re-check

Recommendation

✅ Approved (followup). Both new commits trace cleanly, address the exact concerns raised in pre-release review, and have the right shape:

• f328f3b removes the false-positive guard direction AND adds a timestamp gate as belt-and-suspenders. Regression test locks the substring scenario.
• a5c10d5 adds the missing writer in the right place (inside _agent_lock, before s.save()). Round-trip test verifies persistence end-to-end. CHANGELOG honestly reflects the scope of what was actually fixed.

#1318 is now actually fixed (not just scaffolded). #1298 substring guard hardened against the false-positive case.

Parked at approval — ready for the release agent's merge/tag pipeline.