diff --git a/tests/tools/test_kanban_complete_parents_tool.py b/tests/tools/test_kanban_complete_parents_tool.py new file mode 100644 index 0000000000..04f4feffe6 --- /dev/null +++ b/tests/tools/test_kanban_complete_parents_tool.py @@ -0,0 +1,180 @@ +"""Tool-surface parents reporting for kanban_complete. Regression for #113373. + +``complete_task`` reports every refusal as bare ``False``. On the CLI surface +the open PRs #110323/#110330/#110334 make the parents refusal actionable; +the worker-facing TOOL surface (``tools/kanban_tools._handle_complete`` — the +call a dispatcher-owned worker actually makes) collapsed it into +"unknown id, stale run, or already terminal", sending the worker/operator +chasing stale runs while the real blocker was a reopened or unfinished parent. +""" +import json + +import pytest + + +@pytest.fixture +def running_child_with_parent(monkeypatch, tmp_path): + """A claimed (running) child whose parent is NOT done. Returns ids.""" + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setenv("HERMES_PROFILE", "test-worker") + monkeypatch.delenv("HERMES_SESSION_ID", raising=False) + from pathlib import Path as _Path + monkeypatch.setattr(_Path, "home", lambda: tmp_path) + + from hermes_cli import kanban_db as kb + from hermes_cli import kanban_db_connect as kbc + kb._INITIALIZED_PATHS.clear() + kb.init_db() + conn = kbc.connect() + try: + parent_id = kb.create_task(conn, title="parent gate", assignee="op") + assert kb.complete_task(conn, parent_id, result="parent shipped") + child_id = kb.create_task( + conn, title="child work", assignee="test-worker", parents=[parent_id]) + assert kb.claim_task(conn, child_id) is not None + # Parent reopens mid-run: the #113373 timeline. + with kb.write_txn(conn): + conn.execute( + "UPDATE tasks SET status = 'todo', completed_at = NULL WHERE id = ?", + (parent_id,), + ) + finally: + conn.close() + monkeypatch.setenv("HERMES_KANBAN_TASK", child_id) + return parent_id, child_id + + +def test_complete_names_unsatisfied_parent(running_child_with_parent): + """The refusal names the blocking parent instead of crying stale run.""" + from hermes_cli import kanban_db as kb + from hermes_cli import kanban_db_connect as kbc + from tools import kanban_tools as kt + + parent_id, child_id = running_child_with_parent + out = json.loads(kt._handle_complete( + {"task_id": child_id, "summary": "genuinely done work"})) + assert out.get("error") + assert parent_id in out["error"] + assert "parent" in out["error"].lower() + assert "stale run" not in out["error"] + # Nothing mutated: the run is still open for a retry after the parent. + conn = kbc.connect() + try: + assert kb.get_task(conn, child_id).status == "running" + finally: + conn.close() + + +def test_complete_multiple_parents_deterministic_order(monkeypatch, tmp_path): + """Several unfinished parents are all named, in stable id order.""" + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setenv("HERMES_PROFILE", "test-worker") + monkeypatch.delenv("HERMES_SESSION_ID", raising=False) + from pathlib import Path as _Path + monkeypatch.setattr(_Path, "home", lambda: tmp_path) + + from hermes_cli import kanban_db as kb + from hermes_cli import kanban_db_connect as kbc + from tools import kanban_tools as kt + kb._INITIALIZED_PATHS.clear() + kb.init_db() + conn = kbc.connect() + try: + p1 = kb.create_task(conn, title="gate one", assignee="op") + p2 = kb.create_task(conn, title="gate two", assignee="op") + for p in (p1, p2): + assert kb.complete_task(conn, p, result="x") + child = kb.create_task( + conn, title="child", assignee="test-worker", parents=[p2, p1]) + assert kb.claim_task(conn, child) is not None + # Both gates reopen mid-run. + with kb.write_txn(conn): + conn.execute( + "UPDATE tasks SET status = 'todo', completed_at = NULL " + "WHERE id IN (?, ?)", (p1, p2), + ) + finally: + conn.close() + monkeypatch.setenv("HERMES_KANBAN_TASK", child) + out = json.loads(kt._handle_complete( + {"task_id": child, "summary": "done"})) + assert out.get("error") + # Deterministic id order: ids are random hex, so sort first rather than + # assuming creation order. + first, second = sorted([p1, p2]) + assert out["error"].index(first) < out["error"].index(second) + + +def _isolated_home(monkeypatch, tmp_path): + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + from pathlib import Path as _Path + monkeypatch.setattr(_Path, "home", lambda: tmp_path) + from hermes_cli import kanban_db as kb + kb._INITIALIZED_PATHS.clear() + kb.init_db() + + +def test_complete_unknown_id_stays_generic(monkeypatch, tmp_path): + """Missing tasks keep the generic message (no phantom parent lookup).""" + _isolated_home(monkeypatch, tmp_path) + monkeypatch.delenv("HERMES_KANBAN_TASK", raising=False) + from tools import kanban_tools as kt + out = json.loads(kt._handle_complete( + {"task_id": "t_deadbeefdeadbeef", "summary": "x"})) + assert "unknown id, stale run, or already terminal" in out.get("error", "") + + +def test_complete_terminal_stays_generic(monkeypatch, tmp_path): + """An already-done task keeps the generic message (no parents to name).""" + _isolated_home(monkeypatch, tmp_path) + monkeypatch.setenv("HERMES_PROFILE", "test-worker") + from hermes_cli import kanban_db as kb + from hermes_cli import kanban_db_connect as kbc + from tools import kanban_tools as kt + conn = kbc.connect() + try: + tid = kb.create_task(conn, title="t", assignee="test-worker") + assert kb.claim_task(conn, tid) is not None + assert kb.complete_task(conn, tid, result="done") + finally: + conn.close() + monkeypatch.setenv("HERMES_KANBAN_TASK", tid) + out = json.loads(kt._handle_complete({"task_id": tid, "summary": "again"})) + assert "unknown id, stale run, or already terminal" in out.get("error", "") + + +def test_complete_happy_path_unchanged(monkeypatch, tmp_path): + """Parents done -> completion still succeeds exactly as before.""" + _isolated_home(monkeypatch, tmp_path) + monkeypatch.setenv("HERMES_PROFILE", "test-worker") + from hermes_cli import kanban_db as kb + from hermes_cli import kanban_db_connect as kbc + from tools import kanban_tools as kt + conn = kbc.connect() + try: + parent = kb.create_task(conn, title="p", assignee="op") + assert kb.complete_task(conn, parent, result="p done") + child = kb.create_task( + conn, title="c", assignee="test-worker", parents=[parent]) + assert kb.claim_task(conn, child) is not None + finally: + conn.close() + monkeypatch.setenv("HERMES_KANBAN_TASK", child) + out = json.loads(kt._handle_complete({"task_id": child, "summary": "c done"})) + assert out.get("ok") is True + + +def test_delegate_description_states_child_handoff_contract(): + """Spawn-time surfacing (#113373 ask 1): the delegate_task schema tells the + parent up front that a child cannot close tracked work and must hand back + findings. Keyword-level so rewording does not break CI.""" + from tools.delegate_tool import _build_top_level_description + desc = _build_top_level_description() + assert "tracked work" in desc + assert len(desc) <= 2200 diff --git a/tools/delegate_tool.py b/tools/delegate_tool.py index eaf0261f2a..5634fc5116 100644 --- a/tools/delegate_tool.py +++ b/tools/delegate_tool.py @@ -562,6 +562,8 @@ _DESCRIPTION_HEAD = ( "\"file written\" may be wrong. For external side effects (uploads, remote writes, publishing), require a " "verifiable handle (URL, ID, absolute path) and verify it yourself before telling the user the operation " "succeeded.\n" + "- Children cannot close tracked work: a child asked to close it returns findings instead; " + "the parent applies the transition.\n" ) _DESCRIPTION_TAIL = ( "- Children inherit the parent model unless pinned via delegation.provider / delegation.model in config.yaml." diff --git a/tools/kanban_tools.py b/tools/kanban_tools.py index 21645659c8..24f456c134 100644 --- a/tools/kanban_tools.py +++ b/tools/kanban_tools.py @@ -563,6 +563,25 @@ def _handle_list(args: dict, **kw) -> str: "promoted": promoted}) +def _unsatisfied_parent_blockers(kb, conn, tid: str) -> list[tuple[str, str]]: + """``(parent_id, status)`` for every direct parent not in a terminal state. + + Read-only mirror of the ``task_links`` join in ``kanban_db._parents_satisfied`` + (``done`` / ``archived`` release the child), in deterministic id order, so a + ``complete_task`` refusal that only returns ``False`` can still name the + actionable blockers. Advisory reporting only: queried after the authoritative + write, so a concurrent parent completion may have already cleared it. + """ + rows = conn.execute( + "SELECT p.id, p.status FROM task_links l " + "JOIN tasks p ON p.id = l.parent_id " + "WHERE l.child_id = ? AND p.status NOT IN ('done', 'archived') " + "ORDER BY p.id", + (tid,), + ).fetchall() + return [(row["id"], row["status"]) for row in rows] + + @_kanban_handler("kanban_complete") def _handle_complete(args: dict, **kw) -> str: """Mark the current task done with a structured handoff.""" @@ -619,8 +638,19 @@ def _handle_complete(args: dict, **kw) -> str: f"summary/metadata and either drop these ids from created_cards, or pass " f"created_cards=[] to skip the card-claim check entirely.") task = kb.get_task(conn, tid) - _check(ok, (task.last_failure_error if task else None) or - f"could not complete {tid} (unknown id, stale run, or already terminal)") + if not ok: + # complete_task reports every refusal as bare False; a reopened or + # never-finished parent is the actionable one (#113373: the worker's + # done work was refused as "stale run"). Name the blockers so the + # worker/operator completes the parents instead of re-running. + blockers = _unsatisfied_parent_blockers(kb, conn, tid) + if blockers: + detail = ", ".join(f"{pid} ({status})" for pid, status in blockers) + raise _Reject( + f"could not complete {tid}: unsatisfied parent dependencies: " + f"{detail}; complete the parents first (done or archived)") + _check(False, (task.last_failure_error if task else None) or + f"could not complete {tid} (unknown id, stale run, or already terminal)") run = kb.latest_run(conn, tid) return _ok(task_id=tid, run_id=run.id if run else None)