From 9870e8f11125fcf93119bb5d29780fe6e5f132ab Mon Sep 17 00:00:00 2001 From: Isla Liu Date: Wed, 20 May 2026 12:18:03 +0800 Subject: [PATCH] =?UTF-8?q?fix(session):=20address=20Copilot=20review=20?= =?UTF-8?q?=E2=80=94=20scope=20tool-card=20dedupe=20by=20stream=20id=20+?= =?UTF-8?q?=20tighten=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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_.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//.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. --- api/models.py | 43 +++++++- docs/troubleshooting.md | 8 +- .../test_session_lost_response_regression.py | 4 - tests/test_session_sidecar_repair.py | 103 ++++++++++++++++++ 4 files changed, 147 insertions(+), 11 deletions(-) diff --git a/api/models.py b/api/models.py index 7bb831e9..c666fc1f 100644 --- a/api/models.py +++ b/api/models.py @@ -883,9 +883,33 @@ def _find_existing_assistant_for_journal_content(session, content: str) -> int | return None -def _journal_tool_already_present(session, name: str, preview: str) -> bool: +def _journal_tool_already_present( + session, + name: str, + preview: str, + *, + stream_id: str | None = None, +) -> bool: + """Return True when an equivalent tool card already exists. + + Matching rule: + + * If the existing tool card carries ``_recovered_stream_id``, that means a + previous journal-recovery run materialized it. The retry can safely + collapse against it only when both stream ids match — otherwise a + legitimately-repeated tool (e.g. a second ``terminal: ls`` in a + different turn) would be dropped. + * If the existing tool card has no ``_recovered_stream_id`` (a live tool + card, or a tool card carried over from a core transcript that pre-dates + stream-id tagging), the legacy name+preview match still wins. This + preserves the "core transcript already has this tool, don't duplicate + it" invariant the original repair path established. + * When ``stream_id`` is omitted, the helper degrades cleanly to its + pre-fix session-wide behaviour. + """ candidate_name = str(name or '') candidate_preview = _normalize_journal_recovery_text(preview) + candidate_stream = str(stream_id) if stream_id else None for tool_call in session.tool_calls or []: if not isinstance(tool_call, dict): continue @@ -894,8 +918,17 @@ def _journal_tool_already_present(session, name: str, preview: str) -> bool: existing_preview = _normalize_journal_recovery_text( tool_call.get('preview') or tool_call.get('snippet') or '' ) - if existing_preview == candidate_preview: - return True + if existing_preview != candidate_preview: + continue + if candidate_stream is not None: + existing_stream = tool_call.get('_recovered_stream_id') + # A tool card explicitly tagged with a recovered_stream_id that + # differs from ours belongs to another retry's turn — don't let + # it pre-empt this retry. Untagged tool cards (live or carried + # over from the core transcript) still match. + if existing_stream and str(existing_stream) != candidate_stream: + continue + return True return False @@ -1038,7 +1071,9 @@ def _append_journaled_partial_output( anchor_idx = ensure_assistant_anchor(created_at) name = str(payload.get('name') or 'tool') preview = str(payload.get('preview') or '') - if dedupe_existing and _journal_tool_already_present(session, name, preview): + if dedupe_existing and _journal_tool_already_present( + session, name, preview, stream_id=stream_id, + ): current_assistant_idx = anchor_idx continue recovered_tool_calls.append({ diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index cb6384bb..6ba7dc8b 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -93,11 +93,13 @@ If after running steps 1-4 the import still fails *and* `pip install -e .` succe **Diagnostic.** -1. Identify the affected session id and stream id from the marker. The marker JSON (under `~/.hermes/webui/sessions/session_.json`) shows them on `_journal_retry_stream_id` after the fix; pre-fix sessions only carry the legacy wording. +The on-disk locations below assume the default `~/.hermes/webui` state directory. If you override it via `HERMES_WEBUI_STATE_DIR`, substitute that path for `~/.hermes/webui` in every step. + +1. Identify the affected session id and stream id from the marker. The marker JSON lives at `~/.hermes/webui/sessions/.json`; after the fix it shows them on the `_journal_retry_stream_id` key. Pre-fix sessions only carry the legacy wording, with no retry meta. 2. Check whether the run-journal contains real events: ```bash - ls -la ~/.hermes/_run_journal//.jsonl - head -2 ~/.hermes/_run_journal//.jsonl + ls -la ~/.hermes/webui/sessions/_run_journal//.jsonl + head -2 ~/.hermes/webui/sessions/_run_journal//.jsonl ``` If the file exists and contains `token` / `tool` events, the lazy-retry path will pick them up the next time the session is opened. diff --git a/tests/test_session_lost_response_regression.py b/tests/test_session_lost_response_regression.py index aad6a17d..365f526f 100644 --- a/tests/test_session_lost_response_regression.py +++ b/tests/test_session_lost_response_regression.py @@ -15,10 +15,6 @@ The scenario this test pins down: assistant text/tools into the transcript in the correct chronological position. """ -import json -import queue -from pathlib import Path - import pytest import api.models as models diff --git a/tests/test_session_sidecar_repair.py b/tests/test_session_sidecar_repair.py index 34377e78..c712ab0a 100644 --- a/tests/test_session_sidecar_repair.py +++ b/tests/test_session_sidecar_repair.py @@ -1475,6 +1475,109 @@ class TestLazyRetryBackwardsCompat: assert last.get("_journal_retry_first_seen_ts") == 1779200000 +class TestJournalToolDedupeScoping: + """`_journal_tool_already_present` must only collapse against tool cards + recovered from the same stream — a repeated tool (e.g. ``terminal: ls``) + in a previous turn must NOT pre-empt this turn's recovery.""" + + def test_repeated_tool_in_earlier_turn_does_not_block_recovery(self, hermes_home, monkeypatch): + sid = "dedupe_scope_sid" + stream_id = "dedupe_scope_stream" + s = _make_session(session_id=sid, messages=[ + {"role": "user", "content": "earlier turn"}, + {"role": "assistant", "content": "earlier reply"}, + {"role": "user", "content": "later turn", "_recovered": True}, + ]) + # Pre-existing tool card from an earlier turn with same (name, preview) + # but a different recovered-stream-id. The retry must NOT see this as a + # hit when dedupe_existing is asked to scope to ``stream_id``. + s.tool_calls = [ + { + "name": "terminal", + "preview": "ls", + "snippet": "ls", + "tid": "old-1", + "_recovered_from_run_journal": True, + "_recovered_stream_id": "earlier_stream", + "done": True, + } + ] + marker = models._interrupted_recovery_marker(pending_retry=True) + marker["_journal_retry_stream_id"] = stream_id + marker["_journal_retry_attempts"] = 0 + marker["_journal_retry_first_seen_ts"] = int(time.time()) + s.messages.append(marker) + s.save() + append_run_event(sid, stream_id, "token", {"text": "Listing."}) + append_run_event( + sid, stream_id, "tool", + {"name": "terminal", "preview": "ls", "args": {"cmd": "ls"}}, + ) + + ok = models._retry_journal_recovery_in_place(s) + assert ok is True + + # Two tool cards now: the old one untouched, plus a new one for this + # stream. If dedupe were session-wide the new one would be dropped. + scoped = [ + tc for tc in s.tool_calls + if tc.get("_recovered_stream_id") == stream_id + ] + assert len(scoped) == 1 + assert scoped[0]["name"] == "terminal" + assert scoped[0]["preview"] == "ls" + # Old tool card is preserved. + assert any( + tc.get("_recovered_stream_id") == "earlier_stream" + for tc in s.tool_calls + ) + + def test_untagged_tool_still_matches_for_core_transcript_invariant(self, hermes_home, monkeypatch): + """Tool cards without ``_recovered_stream_id`` (live tools, or tools + carried over from the core transcript) match regardless of the + ``stream_id`` argument. This preserves the "core transcript already + contains this tool, don't duplicate it" invariant the original repair + path relies on.""" + s = _make_session(messages=[ + {"role": "user", "content": "x"}, + {"role": "assistant", "content": "y"}, + ]) + s.tool_calls = [ + {"name": "terminal", "preview": "ls", "snippet": "ls"}, + ] + # No stream_id → legacy session-wide check returns True. + assert models._journal_tool_already_present(s, "terminal", "ls") is True + # With a stream_id, untagged tool cards still match (different stream + # ids only override the match decision when the existing card itself + # is tagged with a stream id that disagrees). + assert models._journal_tool_already_present( + s, "terminal", "ls", stream_id="some_stream", + ) is True + + def test_tagged_tool_with_different_stream_does_not_match(self, hermes_home, monkeypatch): + """A tool card tagged with a different recovered_stream_id must NOT + be considered a duplicate when the retry is scoped to a different + stream.""" + s = _make_session(messages=[ + {"role": "user", "content": "x"}, + ]) + s.tool_calls = [ + { + "name": "terminal", + "preview": "ls", + "snippet": "ls", + "_recovered_stream_id": "other_stream", + }, + ] + assert models._journal_tool_already_present( + s, "terminal", "ls", stream_id="this_stream", + ) is False + # But scoping to the same stream id matches. + assert models._journal_tool_already_present( + s, "terminal", "ls", stream_id="other_stream", + ) is True + + class TestWslPageCacheRace: """Cover the WSL2 / network-FS shape: read_run_events returns empty / errors first, recovers on a later call."""