From 31c0e0fe67ccf8fc8056293733e15492f4e9ee4c Mon Sep 17 00:00:00 2001 From: Jakub Wolniewicz <4850809+frizikk@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:46:44 +0200 Subject: [PATCH] fix(kanban): preserve reviewer across re-review --- hermes_cli/kanban_db.py | 37 +++++++++++- .../test_kanban_review_lifecycle_complete.py | 58 ++++++++++++++++++- 2 files changed, 91 insertions(+), 4 deletions(-) diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 3d940e1a4912b..2ab0b8a1c409f 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -6116,10 +6116,11 @@ def request_review( """Transition implementation work into the first-class review phase. Unlike :func:`block_task`, this transition never touches block recurrence - accounting. The current implementer and optional reviewer are recorded on + accounting. The current implementer and resolved reviewer are recorded on the event so an autonomous reviewer can route requested changes back to the - right profile. Supplying ``reviewer`` also reassigns the task before it is - exposed to the review dispatcher. + right profile. Supplying ``reviewer`` reassigns the task before it is + exposed to the review dispatcher. On re-review, omitting it reuses the + reviewer provenance persisted by the latest ``changes_requested`` event. """ summary = redact_review_value(summary) metadata = redact_review_value(metadata) @@ -6132,6 +6133,30 @@ def request_review( if trow is None: return False implementer = trow["assignee"] + if reviewer is None: + changes_event = conn.execute( + "SELECT payload FROM task_events " + "WHERE task_id = ? AND kind = 'changes_requested' " + "ORDER BY id DESC LIMIT 1", + (task_id,), + ).fetchone() + try: + changes_payload = ( + json.loads(changes_event["payload"]) + if changes_event and changes_event["payload"] + else {} + ) + except (json.JSONDecodeError, TypeError): + changes_payload = {} + prior_reviewer = ( + changes_payload.get("reviewer") + if isinstance(changes_payload, dict) + else None + ) + if changes_event is not None: + if not isinstance(prior_reviewer, str) or not prior_reviewer.strip(): + return False + reviewer = prior_reviewer reviewer = _canonical_assignee(reviewer) if reviewer is not None else None assignee_sql = ", assignee = ?" if reviewer is not None else "" params: tuple[Any, ...] @@ -6264,6 +6289,11 @@ def request_changes( implementer = requested_payload.get("implementer") if not isinstance(implementer, str) or not implementer.strip(): return False, "review handoff has no valid implementer provenance" + reviewer = task_row["assignee"] + if isinstance(reviewer, str) and reviewer.strip(): + reviewer = _canonical_assignee(reviewer) + else: + reviewer = None new_status = _landing_status_after_parents(conn, task_id) cur = conn.execute( @@ -6296,6 +6326,7 @@ def request_changes( { "reason": reason, "implementer": implementer, + "reviewer": reviewer, "status": new_status, }, run_id=run_id, diff --git a/tests/hermes_cli/test_kanban_review_lifecycle_complete.py b/tests/hermes_cli/test_kanban_review_lifecycle_complete.py index 750dc5ff9d106..949da113100dd 100644 --- a/tests/hermes_cli/test_kanban_review_lifecycle_complete.py +++ b/tests/hermes_cli/test_kanban_review_lifecycle_complete.py @@ -112,8 +112,10 @@ def test_same_card_review_supports_changes_and_approval_without_block_loop(conn) assert rework.assignee == "builder" assert rework.current_run_id is None changes = _event(kb.list_events(conn, task_id), "changes_requested") + assert changes.payload is not None assert changes.payload["reason"] == "Add a regression for the fallback branch." assert changes.payload["implementer"] == "builder" + assert changes.payload["reviewer"] == "reviewer" _run(kb.list_runs(conn, task_id), "changes_requested") implementation_2 = kb.claim_task(conn, task_id, claimer="builder:2") @@ -121,12 +123,19 @@ def test_same_card_review_supports_changes_and_approval_without_block_loop(conn) assert kb.request_review( conn, task_id, - reviewer="reviewer", summary="Fallback regression added.", expected_run_id=implementation_2.current_run_id, ) + awaiting_rereview = kb.get_task(conn, task_id) + assert awaiting_rereview is not None + assert awaiting_rereview.status == "review" + assert awaiting_rereview.assignee == "reviewer" review_2 = kb.claim_review_task(conn, task_id, claimer="reviewer:2") assert review_2 is not None + assert review_2.assignee == "reviewer" + review_run = kb.latest_run(conn, task_id) + assert review_run is not None + assert review_run.profile == "reviewer" assert kb.complete_task( conn, task_id, @@ -140,6 +149,53 @@ def test_same_card_review_supports_changes_and_approval_without_block_loop(conn) assert completed.block_recurrences == 0 +@pytest.mark.parametrize("bad_payload", ["{not-json", "{}"]) +def test_rereview_requires_explicit_reviewer_when_provenance_is_invalid( + conn, + bad_payload: str, +) -> None: + task_id, review = _claimed_review(conn, "Malformed reviewer provenance") + assert kb.request_changes( + conn, + task_id, + reason="Correct the implementation.", + expected_run_id=review.current_run_id, + ) == (True, "builder") + with kb.write_txn(conn): + conn.execute( + "UPDATE task_events SET payload = ? " + "WHERE id = (SELECT id FROM task_events " + "WHERE task_id = ? AND kind = 'changes_requested' " + "ORDER BY id DESC LIMIT 1)", + (bad_payload, task_id), + ) + + implementation = kb.claim_task(conn, task_id, claimer="builder:retry") + assert implementation is not None + assert not kb.request_review( + conn, + task_id, + summary="Corrected implementation.", + expected_run_id=implementation.current_run_id, + ) + unchanged = kb.get_task(conn, task_id) + assert unchanged is not None + assert unchanged.status == "running" + assert unchanged.assignee == "builder" + + assert kb.request_review( + conn, + task_id, + reviewer="reviewer", + summary="Corrected implementation.", + expected_run_id=implementation.current_run_id, + ) + restored = kb.get_task(conn, task_id) + assert restored is not None + assert restored.status == "review" + assert restored.assignee == "reviewer" + + def test_review_changes_reapply_parent_gate(conn): parent_id = kb.create_task(conn, title="Upstream prerequisite", assignee="planner") task_id = kb.create_task(