harden(adoption): review findings — exact-id donors only, divergence guard, honest donor_retired
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.
This commit is contained in:
kshitijk4poor
@@ -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}
|
||||
|
||||
|
||||
@@ -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"]
|
||||
|
||||
@@ -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"]
|
||||
|
||||
Reference in New Issue
Block a user