fix(kanban): report blocking parents from kanban_complete tool, state delegate handoff contract
kanban_complete tool collapsed every complete_task refusal into 'unknown id, stale run, or already terminal'. When a parent reopens mid-run, a worker with genuinely done work is refused for a reason the message hides, and the run burns (then crashes, re-queues, and repeats deterministic work). Name the unsatisfied parents (id, status) from a read-only task_links mirror of _parents_satisfied so the worker and the operator complete the gates instead of re-running; unknown-id and terminal refusals keep the generic text, and the dependency invariant itself is untouched (no force path). Complementary to the CLI-surface PRs #110323/#110330/#110334, which touch hermes_cli/kanban_db.py + hermes_cli/kanban.py; this change is tool-surface only (tools/kanban_tools.py). Also surface the delegate_task handoff contract at spawn time: children cannot close tracked work and must return findings for the parent to apply, so a delegated card no longer burns a full run before learning it can never close (#113373 ask 1). Tests: tests/tools/test_kanban_complete_parents_tool.py (6) — parents named, deterministic multi-parent order, unknown/terminal generic, happy path, description contract. Proven red on unfixed sources.
This commit is contained in:
180
tests/tools/test_kanban_complete_parents_tool.py
Normal file
180
tests/tools/test_kanban_complete_parents_tool.py
Normal file
@@ -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
|
||||
@@ -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."
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user