From d1cf01956141fc3bebeffca64b34809fa045946a Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sun, 13 Sep 2026 12:19:24 -0700 Subject: [PATCH] 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 | 25 +----------- tests/hermes_cli/test_kanban_db.py | 53 +------------------------- tests/hermes_cli/test_kanban_notify.py | 2 +- tools/kanban_tools.py | 2 +- 4 files changed, 5 insertions(+), 77 deletions(-) diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index bd5834543d..cd87c2f4ab 100644 --- a/hermes_cli/kanban_db.py +++ b/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): diff --git a/tests/hermes_cli/test_kanban_db.py b/tests/hermes_cli/test_kanban_db.py index 51c02d14c3..774d545308 100644 --- a/tests/hermes_cli/test_kanban_db.py +++ b/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) # --------------------------------------------------------------------------- diff --git a/tests/hermes_cli/test_kanban_notify.py b/tests/hermes_cli/test_kanban_notify.py index 472739de6e..b3542544c1 100644 --- a/tests/hermes_cli/test_kanban_notify.py +++ b/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) diff --git a/tools/kanban_tools.py b/tools/kanban_tools.py index 8e90184437..6a3eb5aa91 100644 --- a/tools/kanban_tools.py +++ b/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