fix(kanban): requeue review changes on same card

hermes_cli/kanban.py

@@ -630,7 +630,7 @@ def build_parser(parent_subparsers: argparse._SubParsersAction) -> argparse.Argu
     p_submit_review.add_argument("--metadata", default=None, help="JSON evidence object")
 
     p_review_changes = sub.add_parser(
-        "review-changes", help="Complete a review and create implementer remediation"
+        "review-changes", help="Record review findings and requeue the same card"
     )
     p_review_changes.add_argument("task_id")
     p_review_changes.add_argument("summary", nargs="+", help="Requested changes")
@@ -2249,21 +2249,16 @@ def _cmd_review_changes(args: argparse.Namespace) -> int:
     with kb.connect_closing() as conn:
         task = kb.get_task(conn, args.task_id)
         run_id = task.current_run_id if task else None
-        remediation = kb.request_review_changes(
+        requeued = kb.request_review_changes(
             conn, args.task_id, summary=" ".join(args.summary), metadata=metadata,
             expected_run_id=run_id,
         )
-    if not remediation:
+    if not requeued:
         print(f"cannot request changes for {args.task_id}", file=sys.stderr)
         return 1
-    remediation_status = "unknown"
-    with kb.connect_closing() as conn:
-        remediation_task = kb.get_task(conn, remediation)
-        if remediation_task is not None:
-            remediation_status = remediation_task.status
     print(
-        "Review changes recorded; original task is done; "
-        f"remediation task: {remediation}; remediation status: {remediation_status}"
+        "Review changes recorded; task requeued for implementer: "
+        f"{requeued}; status: ready"
     )
     return 0
 

hermes_cli/kanban_db.py

@@ -3086,9 +3086,9 @@ def create_task(
     for attempt in range(2):
         task_id = _new_task_id()
         try:
-            # A review changes-requested handoff may create a remediation
-            # while already holding the lifecycle transaction. SQLite has no
-            # nested BEGIN support, so reuse that transaction when present.
+            # A review lifecycle handoff may run while already holding the
+            # lifecycle transaction. SQLite has no nested BEGIN support, so
+            # reuse that transaction when present.
             with (contextlib.nullcontext() if conn.in_transaction else write_txn(conn)):
                 # Determine task status from parent status, unless the caller
                 # parks it directly in blocked for human-ops review or in
@@ -4771,7 +4771,12 @@ def request_review_changes(
     metadata: Optional[dict] = None,
     expected_run_id: Optional[int] = None,
 ) -> Optional[str]:
-    """Close the review card and create one idempotent remediation child."""
+    """Record review findings and requeue the canonical card for its implementer.
+
+    Review remediation is a same-card transition.  The current reviewer run
+    is closed with durable findings, while the task identity and all prior
+    history remain intact for the next immutable-head submission.
+    """
     if not summary or not summary.strip():
         raise ValueError("changes-requested summary is required")
     with write_txn(conn):
@@ -4820,33 +4825,18 @@ def request_review_changes(
         implementer = _canonical_assignee(handoff.get("original_assignee")) or ""
         if not implementer:
             return None
-        remediation_key = f"{_INTERNAL_REVIEW_REMEDIATION_PREFIX}{task_id}:{current_run_id}"
-        remediation_title = f"Address review feedback: {row['title']}"
-        remediation_body = (
-            f"Review task: {task_id}\n\nChanges requested:\n{summary.strip()}"
+        remediation_key = (
+            f"{_INTERNAL_REVIEW_REMEDIATION_PREFIX}{task_id}:{current_run_id}"
         )
-        existing = conn.execute(
-            "SELECT id FROM tasks WHERE idempotency_key = ? LIMIT 1",
+        preseeded = conn.execute(
+            "SELECT 1 FROM tasks WHERE idempotency_key = ? LIMIT 1",
             (remediation_key,),
         ).fetchone()
-        if existing is not None:
-            # This key is an internal authorization binding, not a normal
-            # idempotency shortcut. Never adopt a pre-existing row: even a
-            # row whose visible fields look correct may carry unchecked
-            # execution fields (for example, model_override) or an
-            # attacker-controlled claim state.
+        if preseeded is not None:
+            # The current remediation key is an authorization binding. A
+            # pre-existing row means the request cannot be reconciled safely;
+            # never adopt it and never mutate the canonical review card.
             return None
-        remediation_id = create_task(
-            conn, title=remediation_title, body=remediation_body,
-            assignee=implementer, created_by=row["assignee"] or "reviewer",
-            tenant=row["tenant"], priority=row["priority"],
-            workspace_kind=row["workspace_kind"], workspace_path=row["workspace_path"],
-            branch_name=row["branch_name"], project_id=row["project_id"],
-            skills=json.loads(row["skills"]) if row["skills"] else None,
-            parents=(task_id,),
-            idempotency_key=remediation_key,
-            _allow_internal_idempotency=True,
-        )
         review_metadata = dict(metadata or {})
         review_metadata.update({
             "approved": False,
@@ -4855,37 +4845,24 @@ def request_review_changes(
             "original_implementer": implementer,
             "review_identity": handoff.get("review_identity"),
             "changes_requested": True,
-            "remediation_task_id": remediation_id,
-            "remediation_key": remediation_key,
         })
         cur = conn.execute(
-            "UPDATE tasks SET status='done', result=?, completed_at=?, claim_lock=NULL, "
-            "claim_expires=NULL, worker_pid=NULL WHERE id=? "
+            "UPDATE tasks SET status='ready', assignee=?, result=?, completed_at=NULL, "
+            "consecutive_failures=0, last_failure_error=NULL, "
+            "claim_lock=NULL, claim_expires=NULL, worker_pid=NULL WHERE id=? "
             "AND status='running' AND current_run_id IS NOT NULL",
-            (summary.strip(), int(time.time()), task_id),
+            (implementer, summary.strip(), task_id),
         )
         if cur.rowcount != 1:
-            raise RuntimeError("review task changed while creating remediation")
+            raise RuntimeError("review task changed while requeueing same card")
         run_id = _end_run(
             conn, task_id, outcome="changes_requested", status="done",
             summary=summary.strip(), metadata=review_metadata,
         )
         if run_id is None:
             raise RuntimeError("review run disappeared while requesting changes")
         _append_event(conn, task_id, "review_changes_requested", review_metadata, run_id=run_id)
-        # The remediation child is the one intentional exception to the
-        # unsuccessful-parent dependency guard: its parent is done precisely
-        # because review requested a new implementation cycle. Promote this
-        # child atomically so both public review-changes surfaces can report
-        # the ready handoff without making unrelated dependents ready.
-        promoted = conn.execute(
-            "UPDATE tasks SET status='ready' WHERE id=? AND status='todo'",
-            (remediation_id,),
-        )
-        if promoted.rowcount == 1:
-            _append_event(conn, remediation_id, "promoted", None)
-    recompute_ready(conn)
-    return remediation_id
+    return task_id
 
 
 def heartbeat_claim(

tests/hermes_cli/test_kanban_review_lifecycle.py

@@ -54,7 +54,7 @@ def test_implementation_handoff_is_claimable_by_reviewer(board):
         assert review.assignee == "reviewer"
 
 
-def test_review_changes_creates_one_idempotent_remediation_child(board):
+def test_review_changes_requeues_the_same_card(board):
     with board as conn:
         task_id = kb.create_task(conn, title="implement", assignee="dev")
         implementation = kb.claim_task(conn, task_id, claimer="worker:dev")
@@ -65,18 +65,19 @@ def test_review_changes_creates_one_idempotent_remediation_child(board):
         )
         review = kb.claim_review_task(conn, task_id, claimer="worker:reviewer")
         assert review is not None
-        remediation_id = kb.request_review_changes(
+        requeued_id = kb.request_review_changes(
             conn, task_id, summary="Fix the regression test", expected_run_id=review.current_run_id
         )
-        assert remediation_id != task_id
+        assert requeued_id == task_id
         task = kb.get_task(conn, task_id)
         assert task is not None
-        assert task.assignee == "reviewer"
-        assert task.status == "done"
+        assert task.assignee == "dev"
+        assert task.status == "ready"
+        assert task.current_run_id is None
         rows = conn.execute(
             "SELECT COUNT(*) AS n FROM tasks",
         ).fetchone()
-        assert rows["n"] == 2
+        assert rows["n"] == 1
         run = conn.execute(
             "SELECT status, outcome, ended_at FROM task_runs "
             "WHERE task_id=? ORDER BY id DESC LIMIT 1",
@@ -87,7 +88,7 @@ def test_review_changes_creates_one_idempotent_remediation_child(board):
         assert run["ended_at"] is not None
 
 
-def test_review_changes_remediation_is_ready_after_parent_done(board, capsys):
+def test_review_changes_cli_reports_same_card_ready(board, capsys):
     from hermes_cli import kanban as cli
 
     with board as conn:
@@ -105,17 +106,12 @@ def test_review_changes_remediation_is_ready_after_parent_done(board, capsys):
             task_id=task_id, summary=["Fix", "the", "regression"], metadata=None
         )
         assert cli._cmd_review_changes(args) == 0
-        assert "remediation status: ready" in capsys.readouterr().out
-        remediation = conn.execute(
-            "SELECT child_id FROM task_links WHERE parent_id=?", (task_id,)
-        ).fetchone()
-        assert remediation is not None
-        child = kb.get_task(conn, remediation["child_id"])
-        assert child is not None
-        assert child.status == "ready"
+        assert "task requeued for implementer" in capsys.readouterr().out
+        assert conn.execute("SELECT COUNT(*) AS n FROM task_links").fetchone()["n"] == 0
+        assert kb.get_task(conn, task_id).status == "ready"
 
 
-def test_remediation_child_can_be_reviewed_again(board, monkeypatch):
+def test_same_card_can_be_reviewed_again(board, monkeypatch):
     from hermes_cli import profiles
 
     monkeypatch.setattr(profiles, "profile_exists", lambda _name: True)
@@ -130,26 +126,26 @@ def test_remediation_child_can_be_reviewed_again(board, monkeypatch):
         review = kb.claim_review_task(conn, task_id, claimer="worker:reviewer")
         assert review is not None
 
-        remediation_id = kb.request_review_changes(
+        requeued_id = kb.request_review_changes(
             conn, task_id, summary="Fix the regression test",
             metadata={"approved": False}, expected_run_id=review.current_run_id,
         )
-        assert remediation_id != task_id
+        assert requeued_id == task_id
 
         task = kb.get_task(conn, task_id)
         assert task is not None
-        assert task.status == "done"
-        assert task.assignee == "reviewer"
-        assert conn.execute("SELECT COUNT(*) AS n FROM tasks").fetchone()["n"] == 2
+        assert task.status == "ready"
+        assert task.assignee == "dev"
+        assert conn.execute("SELECT COUNT(*) AS n FROM tasks").fetchone()["n"] == 1
 
-        fix = kb.claim_task(conn, remediation_id, claimer="worker:dev")
+        fix = kb.claim_task(conn, task_id, claimer="worker:dev")
         assert fix is not None
         assert kb.submit_for_review(
-            conn, remediation_id, reviewer="reviewer", summary="fixed",
+            conn, task_id, reviewer="orion", summary="fixed",
             metadata={**REVIEW_METADATA, "head_sha": "b" * 40},
             expected_run_id=fix.current_run_id,
         )
-        assert kb.get_task(conn, remediation_id).status == "review"
+        assert kb.get_task(conn, task_id).status == "review"
 
 
 def test_forged_review_remediation_prefix_cannot_bypass_parent_gate(board):
@@ -395,19 +391,19 @@ def test_dev_implementation_rerun_cannot_use_historical_review_submission(board)
         )
         review = kb.claim_review_task(conn, task_id, claimer="worker:reviewer")
         assert review is not None
-        remediation_id = kb.request_review_changes(
+        requeued_id = kb.request_review_changes(
             conn, task_id, summary="Fix the regression test",
             expected_run_id=review.current_run_id,
         )
-        assert remediation_id != task_id
+        assert requeued_id == task_id
 
         assert kb.request_review_changes(
             conn, task_id, summary="I found another issue",
         ) is None
         task = kb.get_task(conn, task_id)
         assert task is not None
-        assert task.status == "done"
-        assert task.assignee == "reviewer"
+        assert task.status == "ready"
+        assert task.assignee == "dev"
         assert conn.execute(
             "SELECT COUNT(*) AS n FROM task_events "
             "WHERE task_id=? AND kind='review_changes_requested'",
@@ -640,7 +636,7 @@ def test_changes_requested_event_cannot_complete_or_promote_dependency(board):
         )
         review = kb.claim_review_task(conn, parent, claimer="worker:reviewer")
         assert review is not None
-        remediation_id = kb.request_review_changes(
+        requeued_id = kb.request_review_changes(
             conn,
             parent,
             summary="changes required",
@@ -652,10 +648,10 @@ def test_changes_requested_event_cannot_complete_or_promote_dependency(board):
             },
             expected_run_id=review.current_run_id,
         )
-        assert remediation_id != parent
+        assert requeued_id == parent
         assert kb.get_task(conn, child).status == "todo"
 
-        assert kb.get_task(conn, parent).status == "done"
+        assert kb.get_task(conn, parent).status == "ready"
 
 
 def test_changes_requested_run_outcome_does_not_satisfy_dependency(board):