refactor(kanban): trim review-artifact salvage to metadata path, 2 invariant tests

Drop the new `artifacts=` keyword on `request_review()` and its
`_declare_handoff_artifacts` helper: the tool layer already folds the
model-facing `artifacts` list into `metadata["artifacts"]` via the
existing `_merge_artifacts`, so the DB layer needs only the one input it
already honours. Keep two invariant tests (declared artifact survives the
reviewer's completion; notifier uploads the staged copy on
`review_requested`); the prose-reference and rollback variants are
covered by the same helpers `complete_task` already exercises.

Salvage of #109276 by @yoyodine-industries.

hermes_cli/kanban_db.py

@@ -3012,30 +3012,10 @@ def redact_review_value(value: Any) -> Any:
     return value
 
 
-def _declare_handoff_artifacts(
-    metadata: Optional[dict], artifacts: Optional[Iterable[str]],
-) -> Optional[dict]:
-    """Fold an explicit ``artifacts`` argument into ``metadata["artifacts"]``
-    (order-preserving, deduped). Returns ``metadata`` untouched when there is
-    nothing to add, so callers can pass ``None`` through."""
-    if not artifacts:
-        return metadata
-    items = [str(item).strip() for item in artifacts if item is not None and str(item).strip()]
-    if not items:
-        return metadata
-    updated = dict(metadata) if isinstance(metadata, dict) else {}
-    existing = updated.get("artifacts")
-    merged = list(existing) if isinstance(existing, (list, tuple)) else []
-    merged.extend(items)
-    updated["artifacts"] = list(dict.fromkeys(str(p).strip() for p in merged if str(p).strip()))
-    return updated
-
-
 def request_review(
     conn: sqlite3.Connection, task_id: str, *, summary: Optional[str] = None,
     metadata: Optional[dict] = None, reviewer: Optional[str] = None,
     expected_run_id: Optional[int] = None, force: bool = False, with_reason: bool = False,
-    artifacts: Optional[Iterable[str]] = None,
 ):
     """``running``/``ready`` -> ``review``; never touches block recurrence accounting.
 
@@ -3045,7 +3025,7 @@ def request_review(
     claim is only cleared with proof of ownership (``expected_run_id``) or
     ``force=True``. Returns ``bool``, or ``(ok, reason)`` with ``with_reason``.
 
-    ``artifacts`` (or ``metadata["artifacts"]``) names the handoff's deliverable
+    ``metadata["artifacts"]`` names the handoff's deliverable
     files; a review handoff is the last implementer transition, and the
     *reviewer's* completion is what cleans the managed scratch workspace up, so
     the files are staged into the task's durable attachments dir here and the
@@ -3060,10 +3040,9 @@ def request_review(
 
     summary = redact_review_value(summary)
     metadata = redact_review_value(metadata)
-    # Declared (explicit arg or metadata["artifacts"]) and prose-referenced files
+    # Declared (metadata["artifacts"]) and prose-referenced files
     # must be durable BEFORE anything can clean the scratch workspace up: for a
     # review-bound card the reviewer's completion is the cleanup trigger.
-    metadata = _declare_handoff_artifacts(metadata, artifacts)
     metadata = _merge_completion_prose_artifacts(conn, task_id, metadata, summary=summary, result=None)
     now = int(time.time())
     with write_txn(conn):

tests/hermes_cli/test_kanban_db.py

@@ -632,7 +632,7 @@ def test_review_bound_handoff_preserves_declared_artifacts(kanban_home):
         assert run_id is not None
         assert kb.request_review(
             conn, t, summary="ready for review",
-            artifacts=[str(artifact)], expected_run_id=run_id)
+            metadata={"artifacts": [str(artifact)]}, expected_run_id=run_id)
         handoff = [e for e in kb.list_events(conn, t) if e.kind == "review_requested"][-1]
         assert kb.complete_task(conn, t, summary="approved")
         attachments = kb.list_attachments(conn, t)
@@ -646,57 +646,6 @@ def test_review_bound_handoff_preserves_declared_artifacts(kanban_home):
     ]
 
 
-def test_review_bound_handoff_preserves_prose_referenced_artifacts(kanban_home):
-    """Legacy workers name deliverables only by absolute scratch path in prose;
-    the review handoff must stage those too, before the reviewer completes."""
-    with kbc.connect() as conn:
-        t = kb.create_task(conn, title="review bound prose")
-        task = kb.get_task(conn, t)
-        ws = kbw.resolve_workspace(task)
-        kbw.set_workspace_path(conn, t, ws)
-        artifact = ws / "notes.md"
-        artifact.write_bytes(b"# notes\n")
-        kb.claim_task(conn, t)
-        run_id = kb.get_task(conn, t).current_run_id
-        assert run_id is not None
-        assert kb.request_review(
-            conn, t, summary=f"ready for review, deliverable at {artifact}",
-            expected_run_id=run_id)
-        handoff = [e for e in kb.list_events(conn, t) if e.kind == "review_requested"][-1]
-        assert kb.complete_task(conn, t, summary="approved")
-        attachments = kb.list_attachments(conn, t)
-    persisted = Path(handoff.payload["artifacts"][0])
-    assert not ws.exists(), "scratch workspace should still be cleaned up"
-    assert persisted.exists(), "staged copy must survive scratch cleanup"
-    assert persisted.parent == kb.task_attachments_dir(t)
-    assert persisted.read_bytes() == b"# notes\n"
-    assert [(a.filename, a.stored_path) for a in attachments] == [
-        ("notes.md", str(persisted.resolve()))
-    ]
-
-
-def test_review_bound_handoff_rolls_back_when_declared_artifact_missing(kanban_home):
-    """Fail-closed: an unresolvable declared artifact aborts the whole review
-    transition — task stays running/retryable, nothing staged, no event."""
-    with kbc.connect() as conn:
-        t = kb.create_task(conn, title="review bound broken")
-        task = kb.get_task(conn, t)
-        ws = kbw.resolve_workspace(task)
-        kbw.set_workspace_path(conn, t, ws)
-        missing = ws / "missing.png"
-        kb.claim_task(conn, t)
-        run_id = kb.get_task(conn, t).current_run_id
-        assert run_id is not None
-        with pytest.raises(kb.ArtifactPreservationError):
-            kb.request_review(
-                conn, t, summary="ready for review", artifacts=[str(missing)],
-                expected_run_id=run_id)
-        assert kb.get_task(conn, t).status == "running"
-        assert kb.list_attachments(conn, t) == []
-        assert [e for e in kb.list_events(conn, t) if e.kind == "review_requested"] == []
-    assert not missing.exists(), "declared path must be left untouched"
-
-
 # ---------------------------------------------------------------------------
 # Deferred scratch cleanup for parent/child handoff (#33774)
 # ---------------------------------------------------------------------------

tests/hermes_cli/test_kanban_notify.py

@@ -921,7 +921,7 @@ async def test_notifier_uploads_review_handoff_artifacts(kanban_home, tmp_path,
         kb.claim_task(conn, tid)
         run_id = kb.get_task(conn, tid).current_run_id
         assert kb.request_review(
-            conn, tid, summary="ready for review", artifacts=[str(scratch)],
+            conn, tid, summary="ready for review", metadata={"artifacts": [str(scratch)]},
             expected_run_id=run_id)
         handoff = [e for e in kb.list_events(conn, tid) if e.kind == "review_requested"][-1]
         attachments = kb.list_attachments(conn, tid)

tools/kanban_tools.py

@@ -677,7 +677,7 @@ def _handle_request_review(args: dict, **kw) -> str:
         try:
             ok, fail_reason = kb.request_review(
                 conn, tid, summary=summary, metadata=metadata, reviewer=reviewer,
-                artifacts=artifacts, expected_run_id=_worker_run_id(tid), with_reason=True)
+                expected_run_id=_worker_run_id(tid), with_reason=True)
         except kb.ArtifactPreservationError as artifact_err:
             # Same contract as kanban_complete (#22923): the transition rolled
             # back, the task is untouched and retryable — say so explicitly or