From 80b202f53aa719eebb73ae1af596fa806f7768d3 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Mon, 24 Aug 2026 06:16:29 +0530 Subject: [PATCH] =?UTF-8?q?harden(adoption):=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20exact-id=20donors=20only,=20divergence=20guard,=20h?= =?UTF-8?q?onest=20donor=5Fretired?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review batch (3 reviewers) on the final diff surfaced: - H1: title-based donor matching could adopt AND non-recoverably retire an UNRELATED default-store conversation (bot titles collide by design; get_session_by_title has no archived filter/ordering). Donor probe is now exact-id only — the stranded repro always has the id. - H2: re-adoption after a partial run could retire a donor that had accumulated NEWER messages than the profile copy (skip-based idempotency never merges). New divergence guard compares message counts and refuses retirement when the donor is ahead (still adopts). - M1: donor_retired reported True even when every retirement step failed under suppress. Now per-segment tracked + warn-logged; True only when all applied. - M3: adopted=False (e.g. import validation limits) was silent — now warn-logged with import errors. - M4: archived donors are never re-adopted (no cross-profile cloning). - Dead 'from pathlib import Path' dropped; contextlib no longer needed. 5 new red-first-verified regressions (title-collision immunity, archived-donor immunity, non-vacuous owns_db gating with a real donor seeded, divergent-donor retirement refusal, donor_retired truthfulness). tests/tui_gateway: 578 passed. ruff clean. --- hermes_state_portability.py | 50 ++++++- .../test_stranded_session_adoption.py | 127 ++++++++++++++++++ tui_gateway/methods_session.py | 17 ++- 3 files changed, 186 insertions(+), 8 deletions(-) diff --git a/hermes_state_portability.py b/hermes_state_portability.py index 3f460001e6..f4cdbc6044 100644 --- a/hermes_state_portability.py +++ b/hermes_state_portability.py @@ -9,10 +9,8 @@ module-level constants live in hermes_state_common. """ import logging -import contextlib import json import time -from pathlib import Path from typing import Any, Dict, List, Optional from agent.skill_commands import SKILL_SCAFFOLD_SQL_LIKE @@ -342,7 +340,8 @@ class SessionPortabilityMixin: canonical-lookup resurrection must not undo an adoption. Returns the ``import_sessions`` result dict, plus ``adopted`` (bool) - and ``donor_retired`` (bool). + and ``donor_retired`` (bool — True only when EVERY segment's + retirement actually applied). """ payload = donor_db.export_session_lineage(session_id) if not payload: @@ -354,25 +353,64 @@ class SessionPortabilityMixin: } segments = payload.get("segments") or [payload] + + # Divergence guard: a segment we are about to SKIP (already present + # here) may have kept accumulating messages in the donor store after + # a partial earlier adoption. Retiring it would strand those newer + # messages behind a non-recoverable archive. Compare counts up front + # and refuse to retire (still adopt/import) when the donor is ahead. + donor_ahead = False + for seg in segments: + seg_id = seg.get("id") + if not seg_id or self.get_session(seg_id) is None: + continue + donor_count = len(seg.get("messages") or []) + local_count = len(self.get_messages(seg_id)) + if donor_count > local_count: + donor_ahead = True + logger.warning( + "adoption divergence: donor segment %s has %d messages, " + "local copy has %d — donor will NOT be retired", + seg_id, donor_count, local_count, + ) + result = self.import_sessions([dict(seg) for seg in segments]) imported = int(result.get("imported") or 0) skipped = int(result.get("skipped") or 0) adopted = result.get("ok", False) and (imported + skipped) == len(segments) + if not adopted: + logger.warning( + "adoption of %s did not complete: imported=%s skipped=%s " + "of %s segment(s); errors=%s", + session_id, imported, skipped, len(segments), + result.get("errors"), + ) donor_retired = False - if adopted and retire_donor: + if adopted and retire_donor and not donor_ahead: + retire_ok = True for seg in segments: seg_id = seg.get("id") if not seg_id: continue - with contextlib.suppress(Exception): + try: # First end_reason wins in end_session(); reopen first so # the adoption boundary is stamped even on ended segments # (e.g. 'compression' parents). donor_db.reopen_session(seg_id) donor_db.end_session(seg_id, "adopted_by_profile") donor_db.set_session_archived(seg_id, True) - donor_retired = True + except Exception: + # Best-effort by design: a retirement failure must not + # fail the adoption (the profile copy is already whole; + # a later resume retries retirement idempotently). But + # never claim success we didn't have. + retire_ok = False + logger.warning( + "failed to retire donor segment %s after adoption", + seg_id, exc_info=True, + ) + donor_retired = retire_ok return {**result, "adopted": adopted, "donor_retired": donor_retired} diff --git a/tests/tui_gateway/test_stranded_session_adoption.py b/tests/tui_gateway/test_stranded_session_adoption.py index d772c1cb24..874673c4cc 100644 --- a/tests/tui_gateway/test_stranded_session_adoption.py +++ b/tests/tui_gateway/test_stranded_session_adoption.py @@ -262,3 +262,130 @@ def test_launch_profile_resume_path_is_untouched(gateway): assert resp.get("error") assert resp["error"]["code"] == 4007 + + +# ------------------------------------------------------------------------- +# Review-hardening regressions (deleg_e8230ed7): title-collision safety, +# divergence guard, donor_retired truthfulness, no re-adoption of retired +# donors, and a non-vacuous launch-profile gating test. +# ------------------------------------------------------------------------- + + +def test_title_lookup_is_never_used_for_adoption(gateway): + """H1: a profile resume by a TITLE (not id) that collides with an + unrelated default-store session must NOT adopt/retire it. Only exact-id + donors qualify.""" + mod, default_db, profile_home = gateway + # Unrelated default-profile conversation titled like every bot chat. + _seed_stranded(default_db, session_id="innocent-default", title="Bot Chat") + + resp = mod.handle_request( + { + "id": "10", + "method": "session.resume", + "params": { + # resolves nothing by id; would have matched by title pre-fix + "session_id": "Bot Chat", + "profile": "developer", + "lazy": True, + }, + } + ) + + assert resp.get("error") and resp["error"]["code"] == 4007 + innocent = default_db.get_session("innocent-default") + assert not innocent["archived"], "unrelated session must never be retired" + pdb = SessionDB(db_path=profile_home / "state.db") + try: + assert pdb.get_session("innocent-default") is None + finally: + pdb.close() + + +def test_archived_donor_is_not_readopted(gateway): + """M4: after profile A adopts (donor archived), a second profile resuming + the same id must NOT clone the conversation from the archived donor.""" + mod, default_db, profile_home = gateway + _seed_stranded(default_db) + # Simulate a prior completed adoption's retirement stamp. + default_db.reopen_session(STRANDED_ID) + default_db.end_session(STRANDED_ID, "adopted_by_profile") + default_db.set_session_archived(STRANDED_ID, True) + + resp = mod.handle_request( + { + "id": "11", + "method": "session.resume", + "params": { + "session_id": STRANDED_ID, + "profile": "developer", + "lazy": True, + }, + } + ) + + assert resp.get("error") and resp["error"]["code"] == 4007 + pdb = SessionDB(db_path=profile_home / "state.db") + try: + assert pdb.get_session(STRANDED_ID) is None + finally: + pdb.close() + + +def test_launch_profile_resume_never_adopts_even_when_donor_exists(gateway): + """Non-vacuous owns_db gating (reviewer 3): seed a REAL donor in the + default store, resume WITHOUT profile scope under an unknown id — the + fallback must not run, and the donor must stay untouched.""" + mod, default_db, _profile_home = gateway + _seed_stranded(default_db) + + resp = mod.handle_request( + { + "id": "12", + "method": "session.resume", + "params": {"session_id": "unknown-launch-id", "lazy": True}, + } + ) + + assert resp.get("error") and resp["error"]["code"] == 4007 + donor = default_db.get_session(STRANDED_ID) + assert not donor["archived"] + assert donor["end_reason"] is None + + +def test_divergent_donor_is_not_retired(stores): + """H2: donor gained messages after a partial adoption — re-adoption must + NOT retire it (the newer messages would become unreachable).""" + default_db, profile_db = stores + _seed_stranded(default_db) + first = profile_db.adopt_session_lineage_from(default_db, STRANDED_ID) + assert first["adopted"] and first["donor_retired"] + + # Donor keeps living (e.g. user kept chatting there) — un-retire + append. + default_db.set_session_archived(STRANDED_ID, False) + default_db.append_message(STRANDED_ID, "user", "late question") + default_db.append_message(STRANDED_ID, "assistant", "late answer") + + second = profile_db.adopt_session_lineage_from(default_db, STRANDED_ID) + + assert second["adopted"] is True # profile copy still serves + assert second["donor_retired"] is False + donor = default_db.get_session(STRANDED_ID) + assert not donor["archived"], "diverged donor must stay reachable" + assert len(default_db.get_messages(STRANDED_ID)) == 8 + + +def test_donor_retired_reports_false_on_retirement_failure(stores, monkeypatch): + """M1: donor_retired must not lie when retirement fails.""" + default_db, profile_db = stores + _seed_stranded(default_db) + + def _boom(_sid, _reason): + raise RuntimeError("locked") + + monkeypatch.setattr(default_db, "end_session", _boom) + result = profile_db.adopt_session_lineage_from(default_db, STRANDED_ID) + + assert result["adopted"] is True + assert result["donor_retired"] is False + assert not default_db.get_session(STRANDED_ID)["archived"] diff --git a/tui_gateway/methods_session.py b/tui_gateway/methods_session.py index 0413edac7f..a4cffc5310 100644 --- a/tui_gateway/methods_session.py +++ b/tui_gateway/methods_session.py @@ -500,10 +500,23 @@ def _(rid, params: dict) -> dict: if owns_db: try: default_db = _get_db() + # Exact-id match ONLY. Title lookup (get_session_by_title) + # has no archived filter, no ordering, and bot titles + # collide by design ("Bot Chat") — a title-matched donor + # could adopt and non-recoverably retire an UNRELATED + # default-profile conversation. The stranded-session + # repro always has the exact id (the desktop routes by + # id), so nothing real is lost. donor_row = ( default_db.get_session(target) - or default_db.get_session_by_title(target) - ) if default_db is not None else None + if default_db is not None + else None + ) + # Never re-adopt an already-retired donor: a second + # profile resuming the same id would otherwise clone + # the conversation into two "canonical" stores. + if donor_row and donor_row.get("archived"): + donor_row = None if donor_row: adoption = db.adopt_session_lineage_from( default_db, donor_row["id"]