diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index c5267fd0c052..8e2ed86f65c2 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -3198,19 +3198,20 @@ def list_comments(conn: sqlite3.Connection, task_id: str) -> list[Comment]: ] -def _review_owner_from_owner_map(conn: sqlite3.Connection, task_id: str) -> Optional[str]: - """Return the ``review``-lane owner from a card's stamped owner map, if any. +def _owner_from_owner_map( + conn: sqlite3.Connection, task_id: str, lane: str +) -> Optional[str]: + """Return the ``lane``-lane owner from a card's stamped owner map, if any. Reads the ``state_owners={ready: …, review: …, …}`` fragment recorded in the card's ``submit``-stage audit comment (the owner map lives in the audit trail, not a column). Only the ``submit``-stage audit comment is authoritative: this keys off the SAME comment ``stage-pr-review``'s ``resolve_reviewer`` reads - (which filters ``parse_audit(...)["stage"] == "submit"`` before parsing the - map), so the two readers resolve the same owner regardless of comment order. + (which filters ``parse_audit(...)`` for the submit stage before parsing the + map), so every reader resolves the same owner regardless of comment order. A later comment that merely echoes a ``state_owners={…}`` fragment (a - different stage's note, quoted prose) is ignored. Returns the ``review`` owner - when stamped, else ``None`` so the caller can fall back to the card's assignee - / the code-review default. + different stage's note, quoted prose) is ignored. Returns the ``lane`` owner + when stamped, else ``None`` so the caller can fall back. """ for c in list_comments(conn, task_id): body = c.body or "" @@ -3222,12 +3223,37 @@ def _review_owner_from_owner_map(conn: sqlite3.Connection, task_id: str) -> Opti for pair in m.group(1).split(","): if ":" not in pair: continue - lane, owner = pair.split(":", 1) - if lane.strip() == "review" and owner.strip(): + candidate, owner = pair.split(":", 1) + if candidate.strip() == lane and owner.strip(): return owner.strip() return None +def _review_owner_from_owner_map(conn: sqlite3.Connection, task_id: str) -> Optional[str]: + """Return the ``review``-lane owner from a card's stamped owner map, if any. + + Thin wrapper over :func:`_owner_from_owner_map` for the ``review`` lane — see + that function for the authoritative-comment / fallback semantics. Kept as a + named helper because ``review_skills_for_card`` and ``complete_task`` read the + reviewer via this name. + """ + return _owner_from_owner_map(conn, task_id, "review") + + +def _ready_owner_from_owner_map(conn: sqlite3.Connection, task_id: str) -> Optional[str]: + """Return the ``ready``-lane owner (the author) from a card's owner map, if any. + + The ``ready`` lane's owner IS the card's implementing author — the identity a + review bounce must route back to. Reading it from the stamped map is + authoritative and shape-independent, unlike the event-history author + resolution in :func:`_resolve_review_author` (which fails when the review MOVE + recorded the ``assigned {assignee: X}`` event shape instead of ``{from, to}``). + Returns ``None`` when no map is stamped so the caller can fall back to event + history. + """ + return _owner_from_owner_map(conn, task_id, "ready") + + def review_skills_for_card(conn: sqlite3.Connection, task: "Task") -> list[str]: """Return the review skill(s) to force-load for a ``review``-status card. @@ -3822,7 +3848,18 @@ def auto_route_review_bounce( reason = _latest_sticky_block_reason(conn, task_id) if not _is_review_bounce_reason(reason): continue - author = _resolve_review_author(conn, task_id, reviewer=row["assignee"]) + # Resolve the author to route back to. The card's stamped + # ``state_owners[ready]`` owner map is authoritative and independent of + # the ``assigned`` event SHAPE, so prefer it: the event-history resolver + # only matches the ``assigned {from, to}`` shape and returns None when + # the review MOVE recorded the ``assigned {assignee: X}`` shape instead + # (the live t_b1055359 / t_faf0bb66 failure — a needs_input bounce that + # sat blocked forever because the author never resolved). Fall back to + # event history for legacy / un-stamped cards. + author = ( + _ready_owner_from_owner_map(conn, task_id) + or _resolve_review_author(conn, task_id, reviewer=row["assignee"]) + ) if not author or author == row["assignee"]: # Unresolvable author, or the card is already assigned to the author # (nothing to route) — leave it for a human rather than guessing. diff --git a/tests/hermes_cli/test_kanban_auto_route_review_bounce.py b/tests/hermes_cli/test_kanban_auto_route_review_bounce.py index 43499e3167fa..35dc7673b33a 100644 --- a/tests/hermes_cli/test_kanban_auto_route_review_bounce.py +++ b/tests/hermes_cli/test_kanban_auto_route_review_bounce.py @@ -244,6 +244,113 @@ def test_circuit_breaker_block_does_not_route(kanban_home: Path) -> None: assert kb.get_task(conn, tid).status == "blocked" +# --------------------------------------------------------------------------- +# RED 6b — a needs_input-kinded bounce whose author is NOT resolvable from event +# history still routes, via the card's stamped state_owners[ready]. +# --------------------------------------------------------------------------- + + +def _stamp_owner_map(conn, tid: str, *, ready: str = "eckert", + review: str = "lamport", acceptance: str = "casey") -> None: + """Stamp the card's submit-stage owner map, mirroring the live hollis audit. + + The per-lane owner map lives in the ``stage=submit`` §9.1 audit comment + (there is no ``state_owners`` column). The one-card lane resolvers read + ``state_owners[]`` from here. + """ + body = ( + "[audit] actor=hollis stage=submit ts=2026-07-19T18:31:21Z\n" + f"notes: state_owners={{ready: {ready}, review: {review}, " + f"blocked-acceptance: {acceptance}}} triager=hollis team=engineering" + ) + kb.add_comment(conn, tid, author="hollis", body=body) + + +def _stage_needs_input_bounce_unresolvable_author( + conn, *, author: str = "eckert", reviewer: str = "lamport", + reason: str = _BOUNCE_REASON, +) -> str: + """A needs_input review bounce whose event history CANNOT resolve the author. + + Reproduces the live failure (t_b1055359 / t_faf0bb66, 2026-07-19): the card's + most-recent ``assigned`` event carries the ``{"assignee": X}`` shape (emitted + by ``assign_task`` / a non-``move_card`` reassignment), NOT the + ``{"from": author, "to": reviewer}`` shape ``_resolve_review_author`` keys on. + Event-history author resolution therefore returns None -- so the ONLY way to + route this card is the card's stamped ``state_owners[ready]`` owner map. The + reviewer's terminal block is ``kind='needs_input'`` (the live shape). + """ + tid = kb.create_task(conn, title="feature work", assignee=author) + _stamp_owner_map(conn, tid, ready=author, review=reviewer) + kb.claim_task(conn, tid) + kb.complete_task( + conn, tid, + result="PR opened: https://github.com/cwest/hermes-agent/pull/71", + ) + # Review MOVE emitted the {"assignee": reviewer} event shape (not from/to), + # so _resolve_review_author can find no author from event history. + with kb.write_txn(conn): + conn.execute("UPDATE tasks SET status='review', assignee=? WHERE id=?", + (reviewer, tid)) + kb._append_event(conn, tid, "assigned", {"assignee": reviewer}) + kb.claim_review_task(conn, tid) + assert kb.block_task(conn, tid, reason=reason, kind="needs_input", + expected_run_id=kb.get_task(conn, tid).current_run_id) + assert kb.get_task(conn, tid).status == "blocked" + assert kb.get_task(conn, tid).block_kind == "needs_input" + past = int(time.time()) - 300 + with kb.write_txn(conn): + conn.execute("UPDATE task_comments SET created_at=? WHERE task_id=?", + (past, tid)) + conn.execute( + "UPDATE task_runs SET ended_at=? WHERE task_id=? AND ended_at IS NOT NULL", + (past, tid)) + return tid + + +def test_needs_input_bounce_routes_via_owner_map(kanban_home: Path) -> None: + """A ``kind='needs_input'`` review-changes-requested bounce whose author is + UNRESOLVABLE from event history must still auto-route ``blocked -> ready`` + + the author read from the card's stamped ``state_owners[ready]``. Fails today: + the card sits ``blocked`` forever (the live t_b1055359 / t_faf0bb66 symptom).""" + with kb.connect() as conn: + tid = _stage_needs_input_bounce_unresolvable_author(conn) + # Ground-truth the premise: event-history resolution genuinely fails here, + # so the fix MUST come from the owner map. + assert kb._resolve_review_author(conn, tid, reviewer="lamport") is None + + routed = kb.auto_route_review_bounce(conn) + + assert routed == 1, "the needs_input bounce must auto-route via the owner map" + task = kb.get_task(conn, tid) + assert task.status == "ready", "card must be routed back to ready" + assert task.assignee == "eckert", \ + "author must be resolved from state_owners[ready]" + assert any("pull/71" in (c.body or "") for c in kb.list_comments(conn, tid)), \ + "the audit comment must name the PR" + + +def test_needs_input_non_bounce_block_stays_parked(kanban_home: Path) -> None: + """A ``kind='needs_input'`` block whose reason is NOT the review bounce prefix + is a genuine human-input block and must STAY ``blocked`` -- even when the card + carries a stamped owner map (routing keys on the reason prefix, not the kind).""" + with kb.connect() as conn: + tid = kb.create_task(conn, title="needs a human call", assignee="lamport") + _stamp_owner_map(conn, tid, ready="eckert", review="lamport") + kb.claim_task(conn, tid) + kb.block_task( + conn, tid, reason="needs_input: which ACL model should this use?", + kind="needs_input", + expected_run_id=kb.get_task(conn, tid).current_run_id) + + routed = kb.auto_route_review_bounce(conn) + + assert routed == 0, "a genuine needs_input block must not auto-route" + task = kb.get_task(conn, tid) + assert task.status == "blocked", "genuine needs_input block must stay parked" + assert task.assignee == "lamport" + + # --------------------------------------------------------------------------- # RED 6 — config toggle off disables the auto-route # ---------------------------------------------------------------------------