fix(kanban): actor sentinel for synthesized runs; two invariant tests
A plain ``profile=None`` default could not tell "caller named the actor" from "read the card" — an unassigned card is a legitimate None actor, and the row re-read would silently kick back in for it. Use a module sentinel so only callers that did not pass an actor fall back to the card row. Trims the salvaged test file to two invariants in the existing review lifecycle suite: the never-claimed handoff names the implementer (red on main) and an unassigned card's synthesized run keeps ``profile=NULL``.
This commit is contained in:
teknium1
@@ -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(
|
||||
|
||||
@@ -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"
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user