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.
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.
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.
