fix(session): address Copilot review — scope tool-card dedupe by stream 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.
This commit is contained in:
Isla Liu
parent
66b6d8f019
commit
9870e8f111
4 changed files with 147 additions and 11 deletions
|
|
@ -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,7 +918,16 @@ 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:
|
||||
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({
|
||||
|
|
|
|||
|
|
@ -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_<sid>.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/<sid>.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/<sid>/<stream_id>.jsonl
|
||||
head -2 ~/.hermes/_run_journal/<sid>/<stream_id>.jsonl
|
||||
ls -la ~/.hermes/webui/sessions/_run_journal/<sid>/<stream_id>.jsonl
|
||||
head -2 ~/.hermes/webui/sessions/_run_journal/<sid>/<stream_id>.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.
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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."""
|
||||
|
|
|
|||
Loading…
Reference in a new issue