diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index e19ec2013b..da9826f33d 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -1919,10 +1919,15 @@ def _current_run_id(conn: sqlite3.Connection, task_id: str) -> Optional[int]: return int(row["current_run_id"]) if row and row["current_run_id"] else None +# Distinguishes "caller named the acting profile" (which may legitimately be +# None for an unassigned card) from "read the card's current assignee". +_UNSET: Any = object() + + def _end_or_synthesize_run( conn: sqlite3.Connection, task_id: str, *, outcome: str, status: str, summary: Optional[str] = None, metadata: Optional[dict] = None, synthesize: bool, - profile: Optional[str] = None, + profile: Any = _UNSET, ) -> Optional[int]: """:func:`_end_run`; when no run was active and ``synthesize`` holds, record a zero-duration run instead so the handoff fields survive in attempt history. @@ -1938,7 +1943,7 @@ def _end_or_synthesize_run( def _synthesize_ended_run( conn: sqlite3.Connection, task_id: str, *, outcome: str, summary: Optional[str] = None, error: Optional[str] = None, metadata: Optional[dict] = None, - profile: Optional[str] = None, + profile: Any = _UNSET, ) -> int: """Zero-duration closed run for a terminal transition on a never-claimed task, so the handoff fields aren't silently dropped (``_end_run`` is a @@ -1952,7 +1957,7 @@ def _synthesize_ended_run( trow = conn.execute( "SELECT assignee, current_step_key FROM tasks WHERE id = ?", (task_id,), ).fetchone() - if profile is None: + if profile is _UNSET: profile = trow["assignee"] if trow else None step_key = trow["current_step_key"] if trow else None cur = conn.execute( diff --git a/tests/hermes_cli/test_kanban_review_handoff_attribution.py b/tests/hermes_cli/test_kanban_review_handoff_attribution.py deleted file mode 100644 index 84a9c6a019..0000000000 --- a/tests/hermes_cli/test_kanban_review_handoff_attribution.py +++ /dev/null @@ -1,90 +0,0 @@ -"""Regression: synthesized review-handoff run attributed to the implementer (#111064). - -``request_review`` captures the acting profile (the implementer) and then -rewrites ``tasks.assignee`` to the reviewer in the same UPDATE. The -zero-duration run synthesized for a never-claimed card must name the -implementer — the actor who performed the handoff — not the reviewer who -received it. -""" - -from __future__ import annotations - -from pathlib import Path - -import pytest - -from hermes_cli import kanban_db as kb -from hermes_cli import kanban_db_connect as kbc - - -@pytest.fixture -def kanban_home(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> Path: - home = tmp_path / ".hermes" - home.mkdir() - monkeypatch.setenv("HERMES_HOME", str(home)) - monkeypatch.setattr(Path, "home", lambda: tmp_path) - monkeypatch.setenv("HERMES_PROFILE", "builder") - monkeypatch.delenv("HERMES_DELEGATED_CHILD_CONTEXT", raising=False) - # request_review rejects reviewers that are not installed profiles (#106163). - (home / "profiles" / "reviewer").mkdir(parents=True) - kb._INITIALIZED_PATHS.clear() - kb.init_db() - return home - - -def test_synthesized_review_handoff_run_names_implementer_not_reviewer( - kanban_home: Path, -) -> None: - """The review_requested handoff row belongs to the implementer. - - Repro from #111064: an unclaimed card (assignee=builder) is handed off - via request_review(reviewer=reviewer). The task is reassigned, but the - synthesized zero-duration run must still record profile="builder". - """ - with kbc.connect() as conn: - task_id = kb.create_task(conn, title="Unclaimed card", assignee="builder") - ok = kb.request_review( - conn, task_id, summary="ready for review", reviewer="reviewer" - ) - assert ok is True - - task = kb.get_task(conn, task_id) - assert task is not None - assert task.status == "review" - assert task.assignee == "reviewer" - - handoff = kb.latest_run(conn, task_id) - assert handoff is not None - assert handoff.outcome == "review_requested" - assert handoff.profile == "builder" - - -def test_synthesized_review_handoff_run_after_live_implementer_run( - kanban_home: Path, -) -> None: - """Same attribution when the implementer had a live run first. - - The implementer's own run keeps profile="builder"; the handoff row must - too — otherwise per-profile tallies mix the two roles. - """ - with kbc.connect() as conn: - task_id = kb.create_task(conn, title="Claimed card", assignee="builder") - claimed = kb.claim_task(conn, task_id, claimer="builder:1") - assert claimed is not None - - ok = kb.request_review( - conn, - task_id, - summary="done", - reviewer="reviewer", - expected_run_id=claimed.current_run_id, - ) - assert ok is True - - rows = conn.execute( - "SELECT profile, outcome FROM task_runs WHERE task_id = ? ORDER BY id", - (task_id,), - ).fetchall() - handoffs = [r for r in rows if r["outcome"] == "review_requested"] - assert len(handoffs) == 1 - assert handoffs[0]["profile"] == "builder" diff --git a/tests/hermes_cli/test_kanban_review_lifecycle.py b/tests/hermes_cli/test_kanban_review_lifecycle.py index 1222cb77fc..dcfcc6da44 100644 --- a/tests/hermes_cli/test_kanban_review_lifecycle.py +++ b/tests/hermes_cli/test_kanban_review_lifecycle.py @@ -711,3 +711,34 @@ def test_reviewer_reassigns_for_autonomous_dispatch(kanban_home: Path) -> None: ev = _events(conn, tid, kind="review_requested")[0][1] assert ev["reviewer"] == "lead-reviewer" assert ev["implementer"] == "worker" + + +def test_review_handoff_without_live_run_attributes_run_to_implementer(kanban_home: Path) -> None: + """#111064: ``request_review`` reassigns the card to the reviewer in the same + UPDATE that flips the status, so the zero-duration run synthesized for a + never-claimed card must be stamped with the implementer captured before + the rewrite, not the reviewer read back off the mutated row.""" + with kbc.connect() as conn: + tid = kb.create_task(conn, title="handoff attribution", assignee="worker") + assert kb.request_review(conn, tid, summary="ready", reviewer="lead-reviewer") is True + assert kb.get_task(conn, tid).assignee == "lead-reviewer" + run = conn.execute( + "SELECT profile, outcome, step_key FROM task_runs WHERE task_id = ? ORDER BY id DESC LIMIT 1", + (tid,), + ).fetchone() + assert (run["outcome"], run["profile"]) == ("review_requested", "worker") + assert run["step_key"] == kb.get_task(conn, tid).current_step_key + assert _events(conn, tid, kind="review_requested")[0][1]["implementer"] == "worker" + + +def test_synthesized_run_for_unassigned_card_keeps_null_profile(kanban_home: Path) -> None: + """A transition that does not name an actor still reads the card: an + unassigned card's synthesized run carries ``profile=NULL`` (the actor + sentinel must not turn "unassigned" into a re-read of the row).""" + with kbc.connect() as conn: + tid = kb.create_task(conn, title="unassigned handoff") + assert kb.block_task(conn, tid, reason="waiting on upstream") is True + run = conn.execute( + "SELECT profile, outcome FROM task_runs WHERE task_id = ? ORDER BY id DESC LIMIT 1", (tid,), + ).fetchone() + assert (run["outcome"], run["profile"]) == ("blocked", None)