From 805c1e73713a1286dd0704760b9aa7b8597a79e4 Mon Sep 17 00:00:00 2001 From: Casey West Date: Thu, 9 Jul 2026 14:41:20 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20fix(kanban):=20route=20an=20auth?= =?UTF-8?q?or=20completion=20to=20the=20review=20lane,=20not=20done?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `complete_task` unconditionally set `status='done'` on any worker completion from the author lane (running/ready). `done` on this board means exactly one thing — the work was merged/accepted — so an author's end-of-lane completion landing there skips the review lane and the acceptance gate entirely. Code cards were only accidentally rescued: opening their PR fires the `github-prs` webhook, which moves the card to review. A board-driven review card (no PR-review webhook on its drafting step) had no such rescue, so its author's completion flipped it straight to `done` past the reviewer and past acceptance — a false-`done` that had to be reconciled by hand every cycle. Make the board-native path correct for every kind with a review lane. Before the `-> done` write, when the merge override is NOT set, the card is in the author lane (running/ready), and the card's own stamped owner map declares a `review` owner, MOVE the card to `status='review'` + that owner and emit a `status_changed` event instead of completing it. - No kind-default fallback: a card with no stamped review lane (legacy / un-stamped / plain task, research swarm root) completes to `done` exactly as before — the redirect never shunts an undeclared card. - Idempotent with the webhook path: once in `review` the card is no longer running/ready, so a second completion is a clean no-op and the `-> done` UPDATE cannot match it. - The acceptance-lane refusal, the merge override (`allow_acceptance_complete=True`), the hallucinated-cards gate, and `expected_run_id` atomicity are all preserved. Add `_review_owner_from_owner_map`, a reader that resolves `state_owners["review"]` from the card's submit-stage audit comment (the owner map lives in the audit trail, not a column), returning None when unstamped so the completion path is unchanged for such cards. --- hermes_cli/kanban_db.py | 139 +++++++++ .../test_kanban_complete_author_to_review.py | 269 ++++++++++++++++++ 2 files changed, 408 insertions(+) create mode 100644 tests/hermes_cli/test_kanban_complete_author_to_review.py diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index a899bfe09a7c..02450041146d 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -3812,6 +3812,49 @@ def auto_route_review_bounce( _ACCEPTANCE_SIGNOFF_REASON_PREFIX = "awaiting-casey-signoff" +# The card's per-lane owner map lives in its ``submit``-stage §9.1 audit comment +# (there is no ``state_owners`` column — the map is stamped in the audit trail by +# design, the same signal ``stage-pr-review`` / ``bounce-review-to-author`` read). +# A ``submit`` comment records it in the free-text ``notes:`` line as e.g. +# ``state_owners={ready: eckert, review: lamport, blocked-acceptance: casey}``. +_OWNER_MAP_RE = re.compile(r"state_owners=\{([^}]*)\}") + + +def _review_owner_from_owner_map( + conn: sqlite3.Connection, task_id: str, +) -> Optional[str]: + """Return a card's ``state_owners["review"]`` owner, or None if unstamped. + + Reads the card's materialized owner map from its ``submit``-stage audit + comment (the map lives in the audit trail, not a column — same reader shape + the one-card lane resolvers use). Returns the ``review``-lane owner when the + card carries a stamped map with that lane, else None. + + Deliberately has NO kind-default fallback: a card with no stamped review lane + (a legacy / un-stamped card, or a plain non-pipeline task) must NOT be shunted + into review by :func:`complete_task` — None keeps its completion on the + ``-> done`` path unchanged. Only a card that explicitly declared a review lane + is redirected. + """ + for comment in list_comments(conn, task_id): + body = comment.body or "" + if not body.lstrip().startswith("[audit]"): + continue + # Only the submit-stage audit comment carries the materialized map. + if "stage=submit" not in body: + continue + m = _OWNER_MAP_RE.search(body) + if not m: + continue + for pair in m.group(1).split(","): + if ":" not in pair: + continue + lane, owner = pair.split(":", 1) + if lane.strip() == "review" and owner.strip(): + return owner.strip() + return None + + def _resolve_stray_acceptance_owner( conn: sqlite3.Connection, task_id: str, *, reviewer: Optional[str] = None, ) -> Optional[str]: @@ -4871,6 +4914,102 @@ def complete_task( ) return False + # Author-lane redirect: an AUTHOR finishing their lane MOVES the card to the + # REVIEW lane, it does NOT go straight to ``done``. ``done`` means "Casey + # merged/accepted"; an author's end-of-lane completion is not that. Code cards + # were only accidentally rescued by the ``github-prs`` webhook (stage-pr-review + # moves them to review on PR-open); a writing card whose review is board-driven + # had no such rescue and landed in ``done`` past the reviewer and past Casey. + # This makes the board-native path correct for BOTH kinds. + # + # The redirect fires only when ALL hold: + # * the merge override is NOT set (Casey's merge is the one legit ``->done``); + # * the card is in the author lane — ``running`` or ``ready`` — so the + # acceptance refusal above and the manual-complete-a-stuck-``blocked``-card + # flow are both untouched; + # * the card's OWN stamped owner map declares a ``review`` owner (no + # kind-default fallback — an un-stamped / no-review-lane card completes to + # ``done`` exactly as before, so research/plain cards are never shunted). + # + # Idempotency with the webhook path is structural: once the card is in + # ``review`` it is no longer ``running``/``ready``, so a second completion here + # is skipped AND the ``-> done`` UPDATE below (``status IN ('running','ready', + # 'blocked')``) does not match — a clean no-op (returns False). A rework + # re-completion from ``ready`` after a review bounce correctly moves back to + # review for re-review. + if not allow_acceptance_complete: + _row = conn.execute( + "SELECT status FROM tasks WHERE id = ?", (task_id,) + ).fetchone() + if _row is not None and _row["status"] in ("running", "ready"): + _review_owner = _review_owner_from_owner_map(conn, task_id) + if _review_owner: + with write_txn(conn): + if expected_run_id is None: + cur = conn.execute( + """ + UPDATE tasks + SET status = 'review', + assignee = ?, + claim_lock = NULL, + claim_expires= NULL, + worker_pid = NULL, + block_kind = NULL, + block_recurrences = 0 + WHERE id = ? + AND status IN ('running', 'ready') + """, + (_review_owner, task_id), + ) + else: + cur = conn.execute( + """ + UPDATE tasks + SET status = 'review', + assignee = ?, + claim_lock = NULL, + claim_expires= NULL, + worker_pid = NULL, + block_kind = NULL, + block_recurrences = 0 + WHERE id = ? + AND status IN ('running', 'ready') + AND current_run_id = ? + """, + (_review_owner, task_id, int(expected_run_id)), + ) + if cur.rowcount != 1: + # Card moved between the status read and the write (a race + # with the webhook or a stale expected_run_id) — no-op. + return False + _prev_status = _row["status"] + # End the author's run as completed: the author DID finish + # their lane; the card advancing to review is the handoff. + run_id = _end_run( + conn, task_id, + outcome="completed", status="review", + summary=summary if summary is not None else result, + metadata=metadata, + ) + if run_id is None and (summary or metadata or result): + run_id = _synthesize_ended_run( + conn, task_id, + outcome="completed", + summary=summary if summary is not None else result, + metadata=metadata, + ) + _append_event( + conn, task_id, "status_changed", + { + "from": _prev_status, + "to": "review", + "assignee": _review_owner, + "by": "onecard:complete-task", + }, + run_id=run_id, + ) + return True + with write_txn(conn): if expected_run_id is None: cur = conn.execute( diff --git a/tests/hermes_cli/test_kanban_complete_author_to_review.py b/tests/hermes_cli/test_kanban_complete_author_to_review.py new file mode 100644 index 000000000000..1bba37669998 --- /dev/null +++ b/tests/hermes_cli/test_kanban_complete_author_to_review.py @@ -0,0 +1,269 @@ +"""Regression tests: an AUTHOR's ``complete_task`` MOVES the card to the review +lane, never straight to ``done``. + +``done`` on the board means exactly one thing — Casey merged/accepted the work. +A recurring defect (hit twice live on one writing card) let an author finish +their lane and the card jump straight to ``done``, SKIPPING the reviewer and the +Casey acceptance gate. Code cards were accidentally rescued because their PR +fires the ``github-prs`` webhook -> ``stage-pr-review`` moves them to review; a +writing card whose review is board-driven (no PR-review webhook on the drafting +step) had no such rescue, so the author's ``complete_task`` landed it in ``done`` +past the reviewer and past Casey. + +The fix: when an author completes a card FROM the author lane (``running`` / +``ready``) AND the card's OWN stamped owner map carries a ``review`` owner, the +completion MOVES the card to ``status='review'`` + that review owner instead of +setting ``done``. This is a general fix — it holds for every kind with a review +lane (code AND writing AND research). Code cards keep the webhook rescue too, so +the two paths must be idempotent: once the card is in ``review``, a second +completion attempt is a clean no-op. + +These tests pin the contract on the real ``complete_task`` path: + +* A writing author's completion MOVES the card to ``review`` + the card's review + owner (perkins for writing), NOT ``done``, and emits a ``status_changed``. +* A code author's completion MOVES the card to ``review`` + lamport, NOT ``done``. +* The redirect fires from a ``ready`` card too (a manual CLI complete of a + never-claimed card). +* Idempotency: once the card is in ``review`` (webhook already moved it), a + second completion is a no-op — it does not re-move and does not land ``done``. +* A card with NO stamped review owner (legacy / un-stamped, or a plain + non-pipeline task) still completes to ``done`` — the redirect never shunts a + card that was not declared to have a review lane. +* Casey's merge (``allow_acceptance_complete=True``) is UNCHANGED — it lands the + card in ``done`` regardless of the owner map. +* A generic ``blocked`` card (needs_input / review-changes-requested) stays + completable to ``done`` — the redirect keys on the author lane, not on the + owner map alone. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from hermes_cli import kanban_db as kb + + +@pytest.fixture +def kanban_home(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """Isolated HERMES_HOME with an empty kanban DB.""" + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + kb.init_db() + return home + + +def _stamp_owner_map(conn, tid: str, owner_map: str) -> None: + """Record the card's submit-stage audit comment carrying ``state_owners``. + + Mirrors the ``[audit] ... stage=submit`` comment ``submit_card`` writes at + creation — the free-text ``notes:`` line holds the materialized owner map. + """ + body = ( + "[audit] actor=hollis stage=submit ts=2026-07-09T18:29:21Z\n" + f"notes: state_owners={{{owner_map}}} triager=hollis team=engineering" + ) + kb.add_comment(conn, tid, author="hollis", body=body) + + +def _author_card(conn, *, owner_map: str | None, assignee: str = "eckert") -> str: + """A running author-lane card, optionally stamped with an owner map.""" + tid = kb.create_task(conn, title="feature work", assignee=assignee) + if owner_map is not None: + _stamp_owner_map(conn, tid, owner_map) + kb.claim_task(conn, tid) + assert kb.get_task(conn, tid).status == "running" + return tid + + +# --------------------------------------------------------------------------- +# RED 1 — a writing author's completion MOVES the card to review + perkins +# --------------------------------------------------------------------------- + + +def test_writing_author_completion_moves_to_review(kanban_home: Path) -> None: + with kb.connect() as conn: + tid = _author_card( + conn, + owner_map="ready: orwell, review: perkins, blocked-acceptance: casey", + assignee="orwell", + ) + + ok = kb.complete_task(conn, tid, summary="draft finished") + + assert ok is True, "the author completion must succeed (as a move)" + task = kb.get_task(conn, tid) + assert task.status == "review", "author completion must MOVE to review, not done" + assert task.assignee == "perkins", "assignee is the card's review owner" + assert task.completed_at is None, "a review move is not a completion timestamp" + + +def test_writing_author_completion_emits_status_changed(kanban_home: Path) -> None: + with kb.connect() as conn: + tid = _author_card( + conn, + owner_map="ready: orwell, review: perkins, blocked-acceptance: casey", + assignee="orwell", + ) + kb.complete_task(conn, tid, summary="draft finished") + + events = kb.list_events(conn, tid) + moves = [ + e for e in events + if e.kind == "status_changed" and (e.payload or {}).get("to") == "review" + ] + assert moves, "the redirect must emit a status_changed -> review event" + # A review move is NOT a completion: no 'done'-completed event landed. + assert not [e for e in events if e.kind == "completed"], \ + "a review move must not emit a 'completed' event" + + +# --------------------------------------------------------------------------- +# RED 2 — a code author's completion MOVES the card to review + lamport +# --------------------------------------------------------------------------- + + +def test_code_author_completion_moves_to_review(kanban_home: Path) -> None: + with kb.connect() as conn: + tid = _author_card( + conn, + owner_map="ready: eckert, review: lamport, blocked-acceptance: casey", + ) + + ok = kb.complete_task(conn, tid, summary="implemented + tests") + + assert ok is True + task = kb.get_task(conn, tid) + assert task.status == "review", "code author completion must MOVE to review" + assert task.assignee == "lamport", "assignee is the card's review owner" + + +def test_ready_lane_author_completion_moves_to_review(kanban_home: Path) -> None: + """A never-claimed ``ready`` card completed via the CLI is still redirected.""" + with kb.connect() as conn: + tid = kb.create_task(conn, title="feature work", assignee="eckert") + _stamp_owner_map( + conn, tid, "ready: eckert, review: lamport, blocked-acceptance: casey" + ) + assert kb.get_task(conn, tid).status == "ready" + + ok = kb.complete_task(conn, tid, summary="done from ready") + + assert ok is True + task = kb.get_task(conn, tid) + assert task.status == "review" + assert task.assignee == "lamport" + + +# --------------------------------------------------------------------------- +# RED 3 — idempotency: a card already in review is a no-op (no double-move) +# --------------------------------------------------------------------------- + + +def test_completion_of_review_card_is_noop(kanban_home: Path) -> None: + """Once the card is in ``review`` (e.g. the webhook moved it), a second + completion attempt does not re-move it and does not land it in ``done``.""" + with kb.connect() as conn: + tid = _author_card( + conn, + owner_map="ready: eckert, review: lamport, blocked-acceptance: casey", + ) + # First completion moves it to review. + assert kb.complete_task(conn, tid, summary="impl") is True + assert kb.get_task(conn, tid).status == "review" + + # Second completion attempt (as if a duplicate webhook / stray call). + ok = kb.complete_task(conn, tid, summary="second attempt") + + assert ok is False, "a review-lane card is not completable — clean no-op" + task = kb.get_task(conn, tid) + assert task.status == "review", "card must stay in review, not flip to done" + assert task.assignee == "lamport" + assert task.completed_at is None + + +# --------------------------------------------------------------------------- +# RED 4 — a card with no stamped review owner still completes to done +# --------------------------------------------------------------------------- + + +def test_unstamped_card_completes_to_done(kanban_home: Path) -> None: + """A card with no submit-stage owner map (legacy / plain task) has no review + lane declared — the redirect must NOT shunt it; it completes to ``done``.""" + with kb.connect() as conn: + tid = _author_card(conn, owner_map=None) + + ok = kb.complete_task(conn, tid, summary="plain task done") + + assert ok is True + task = kb.get_task(conn, tid) + assert task.status == "done", "an un-stamped card completes to done as before" + assert task.completed_at is not None + + +def test_owner_map_without_review_lane_completes_to_done(kanban_home: Path) -> None: + """A stamped card whose owner map has no ``review`` lane completes to done.""" + with kb.connect() as conn: + tid = _author_card(conn, owner_map="ready: eckert, blocked-acceptance: casey") + + ok = kb.complete_task(conn, tid, summary="no review lane") + + assert ok is True + assert kb.get_task(conn, tid).status == "done" + + +# --------------------------------------------------------------------------- +# RED 5 — Casey's merge override is unchanged (regression) +# --------------------------------------------------------------------------- + + +def test_merge_override_still_reaches_done(kanban_home: Path) -> None: + """``allow_acceptance_complete=True`` (Casey's merge) lands the card in + ``done`` regardless of the owner map — the redirect must not intercept it.""" + with kb.connect() as conn: + tid = _author_card( + conn, + owner_map="ready: eckert, review: lamport, blocked-acceptance: casey", + ) + + ok = kb.complete_task( + conn, tid, summary="merged by Casey", allow_acceptance_complete=True + ) + + assert ok is True + task = kb.get_task(conn, tid) + assert task.status == "done", "the merge override must reach done, not review" + assert task.completed_at is not None + + +# --------------------------------------------------------------------------- +# RED 6 — a generic blocked card stays completable (no regression) +# --------------------------------------------------------------------------- + + +def test_generic_blocked_card_still_completes_to_done(kanban_home: Path) -> None: + """A ``blocked`` card (needs_input) with a review owner is NOT redirected — + the redirect fires only from the author lane (running/ready), and a generic + blocked card stays completable to ``done`` (manual-complete-a-stuck-card).""" + with kb.connect() as conn: + tid = kb.create_task(conn, title="genuinely stuck", assignee="eckert") + _stamp_owner_map( + conn, tid, "ready: eckert, review: lamport, blocked-acceptance: casey" + ) + kb.claim_task(conn, tid) + assert kb.block_task( + conn, tid, + reason="review-required: please verify the ACL change", + kind="needs_input", + expected_run_id=kb.get_task(conn, tid).current_run_id, + ) + assert kb.get_task(conn, tid).status == "blocked" + + ok = kb.complete_task(conn, tid, summary="resolved out of band") + + assert ok is True, "a generic blocked card must stay completable" + assert kb.get_task(conn, tid).status == "done"