🐛 fix(kanban): route a review bounce via the card's owner map, not just event history

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.

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[<lane>]`` 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
 # ---------------------------------------------------------------------------