Release v0.51.100 (Release BX / stage-393 / 3-PR deep-review batch) - #2658
Merged
Merged
Conversation
…rn marker self-heals
When the WebUI process restarts mid-stream and sidecar repair runs while
the run-journal for the dead stream is not yet visible on disk (WSL2 9p
/ DrvFs page-cache loss, un-fsynced journal tail on network FS, …),
`_append_journaled_partial_output()` returns False and the marker is
permanently baked with the "no agent output was recovered" wording even
though the journaled tokens appear on disk shortly afterwards.
This commit reframes the recovery contract so the read side can
self-heal:
* `_interrupted_recovery_marker` gains a `pending_retry=True` mode
that produces a third wording ("Recovering the partial output …
reload this session to retry.") and stamps a
`_pending_journal_recovery` flag.
* `_apply_core_sync_or_error_marker` now writes that pending-retry
marker (with `_journal_retry_stream_id`,
`_journal_retry_attempts`, `_journal_retry_first_seen_ts` meta)
whenever it cannot recover visible output AND the stream id is
known. The legacy "no output" wording is reserved for the
no-stream-id case. The core-sync branch leaves marker emission to
the existing visible-output check (the core transcript itself is the
canonical history in that branch).
* A new `_retry_journal_recovery_in_place(session)` helper re-runs
`_append_journaled_partial_output(…, dedupe_existing=True)` for the
latest pending marker. On success the marker is promoted in place to
the recovered-output wording, the journaled rows are reordered to
sit above the marker (preserving chronological order), and all
retry meta is stripped. On failure attempts is incremented; after
_JOURNAL_RETRY_MAX_ATTEMPTS (12) or _JOURNAL_RETRY_GIVEUP_SECONDS
(24h) the marker is demoted to a neutral "Partial output may have
been lost." wording.
* `get_session()` cheaply short-circuits via
`_session_has_pending_journal_retry()` and invokes the helper on
both cache-hit and cold-load paths when a pending marker is found.
`metadata_only=True` skips the helper to keep sidebar refresh
cheap. The retry call runs OUTSIDE the SESSIONS LOCK to avoid a
deadlock with `session.save()` write paths.
No streaming write path or run_journal fsync behaviour is changed — the
fix is read-side only.
Reproduces the production failure mode: 1. Stage 1 — sidecar repair runs while the run-journal for the dead stream is empty on disk. Assert the marker arms the lazy-retry hook (`_pending_journal_recovery=True`, `_journal_retry_stream_id`, `_journal_retry_attempts=0`, `_journal_retry_first_seen_ts`) and does NOT carry the legacy "no agent output was recovered" wording. Pending sidecar fields are cleared regardless. 2. Stage 2 — journaled token / tool / tool_complete / token events appear on disk. Call `get_session(sid)` and assert the marker self-heals: wording promotes to "recovered from the run journal", journaled assistant rows + tool card land above the marker in chronological order, all retry meta is stripped. Without the lazy-retry path this test fails at the very first assertion (marker still carries the legacy no-output wording).
…etry path
Adds five test classes that together pin down the contract added in the
previous commit and protect pre-fix session shapes:
* `TestInterruptedRecoveryMarker` — pure-function tests for the new
`pending_retry=True` keyword and the mutual-exclusion rule between
`recovered_output=True` and `pending_retry=True`.
* `TestRetryJournalRecoveryInPlace` — promote-on-success,
increment-on-failure, demote-after-max-attempts,
demote-after-giveup-seconds, no-op when no pending marker, and the
`_session_has_pending_journal_retry` short-circuit (which stops at
the most recent normal assistant turn).
* `TestGetSessionLazyRetryHook` — both `get_session()` entry paths
(cache-hit and cold-load) trigger the helper when a pending marker
is present; the short-circuit avoids the helper when nothing is
pending; and `metadata_only=True` skips the helper to keep sidebar
refresh free.
* `TestLazyRetryBackwardsCompat` — pre-fix sessions whose markers
use the legacy "no agent output" wording (no flag) are not touched
by `get_session()`. The four retry-meta keys round-trip cleanly
through `Session.save()` / `Session.load()`.
* `TestWslPageCacheRace` — covers the WSL2 / network-FS shape: a
first `read_run_events` raising IOError followed by a successful
read; a journal that grows visible tokens between sidecar repair
and retry; and two concurrent `get_session(sid)` calls converging
on a single promoted marker with a single recovered body
(deduped by `dedupe_existing=True`).
Two pre-existing assertions had to be relaxed because they encoded the
buggy contract (permanent "no agent output was recovered" / "user
message above was preserved" wording in the journal-empty + stream-id
known case). Both tests now accept either the legacy wording or the
new "Recovering the partial output…" wording with the pending-retry
flag, reflecting the broader fact that the old wording was the bug.
…ponse self-heal CHANGELOG: append an Unreleased / Fixed entry describing the user-visible behaviour change (interrupted-turn marker now self-heals on the next session read; gives up gracefully after 12 retries or 24h). docs/troubleshooting.md: add a 'Symptom → Why → Diagnostic → Fix → Caps → When to file a bug' entry for the 'no agent output was recovered' marker so users who hit the lost-response shape on WSL2 / network FS can recognise it, verify the run-journal on disk, and know that reloading the session is enough.
…am id + tighten docs Four code-review comments from the automated Copilot reviewer on this PR: 1. `_journal_tool_already_present` dedupe was session-wide, so a legitimately-repeated tool (e.g. a second `terminal: ls` in an earlier turn) could cause the retry path to falsely skip materializing the recovered tool card. The helper now takes a keyword `stream_id` argument; when supplied, a tool card whose `_recovered_stream_id` is set AND differs from the candidate is no longer treated as a duplicate. Untagged tool cards (live tools, or tool cards carried over from a pre-tagging core transcript) still match, preserving the existing 'core transcript already has this tool, don't duplicate' invariant. Two new tests in `TestJournalToolDedupeScoping` cover both legs of the rule. 2./3. The troubleshooting FAQ pointed at `~/.hermes/webui/sessions/session_<sid>.json` and `~/.hermes/_run_journal/...`. The actual sidecar filename has no `session_` prefix and the run-journal lives under the WebUI sessions dir (`~/.hermes/webui/sessions/_run_journal/<sid>/<stream>.jsonl`, default). Both paths fixed and an explicit note added about `HERMES_WEBUI_STATE_DIR` overriding the state root. 4. Drop unused `json` / `queue` / `Path` imports from `tests/test_session_lost_response_regression.py` so the file stops carrying noise that future linting would flag.
…and drop unused fixture arg
Two non-functional cleanups from the second Copilot pass:
1. The inline comment in `test_error_marker_no_preserved_as_draft`
said the legacy "user message above was preserved" wording was used
for the post-retry-give-up case. The actual implementation demotes
give-up markers to a different neutral wording ("Partial output may
have been lost."). Comment rewritten to match the contract.
2. The regression test `test_lost_response_recovered_on_second_read`
declared a `monkeypatch` parameter it never used. Dropped.
# Conflicts: # CHANGELOG.md
# Conflicts: # static/sessions.js
…(Opus advisor PR #2637)
SysAdminDoc
pushed a commit
to SysAdminDoc/hermes-webui
that referenced
this pull request
Jun 26, 2026
Release v0.51.100 (Release BX / stage-393 / 3-PR deep-review batch)
bernyforce
pushed a commit
to bernyforce/hermes-webui
that referenced
this pull request
Jul 29, 2026
Release v0.51.100 (Release BX / stage-393 / 3-PR deep-review batch)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release v0.51.100 — Release BX (stage-393) — 3-PR deep-review batch
Three substantial contributor PRs that went through deep parallel agent review + concurrency self-verify. Each had its prior-review notes addressed by the contributor; verdicts captured below.
Constituents
metadata_onlyskip semantics documented). Empirical agent self-verify: 10 concurrent_try_retry_journal_recovery_in_place(sid)calls → exactly 1 executes, 9 no-op; 100-SID stress test cleans up cleanly with zero remaining locks.tests/test_session_events_http_integration.pycovering: pure-bus subscribe/unsubscribe balance, real HTTP handshake + event delivery driven by a side-effect POST, rapid open/close burst that survives a subsequent/api/sessionsGET, source-level guard on the_CLIENT_DISCONNECT_ERRORStuple wiring. All 4 pass locally. Architectural review: bus design is sound (latest-wins drain on bounded Queue, manual cron path gated by_cron_profile_context_depth()to avoid double-publish, finally-block cleanup on all socket failure modes).Conflict resolution during merge
Single conflict on
static/sessions.jsbetween #2633 (FLIP first-render animation) and #2637 (SSE init). Both orthogonal — kept both blocks. Verified withnode -c.Risk class — self-verify per memory rule
Per the agent-side empirical verification rule (PRs touching streaming/profiles/config/upload/routes + concurrency primitives), I ran unmocked production-mechanism tests on:
Both passed empirically. The pattern is in
~/WebUI/docs/agent-memory/agent-side-empirical-verification.md.Pre-Opus gate (all passed)
static/boot.js,static/panels.js,static/sessions.js,static/ui.js.pyfiles**PR TBD**placeholdersStreaming/profiles/routes surface — browser sanity required
Per the standard rule for PRs touching streaming/profiles/routes:
api/models.pysession-loading hot path (everyget_session()call exercises the new lazy-retry probe)api/profiles.py+api/config.py+ boot ordering/api/sessions/events+ cron event publish pathBrowser sanity will exercise:
/health,/api/settings,/api/session/new,/api/session,/api/chat/start,/api/chat/stream/status,/api/sessions/events(handshake),/api/session/delete.Stats
3 contributor PRs + 1 maintainer test-only commit. ~2028 LOC net change, 24 files. Surfaces: session-recovery + profile-switch + cross-tab events bus. No new visible UI chrome.
Pytest + Opus advisor + browser sanity will run in parallel.
Follow-ups (filed for after merge)
Will file 2 follow-up issues post-merge: