From 09592243136e2f0f2c2c58fe62eb8b8117420e53 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Tue, 15 Sep 2026 12:07:49 -0700 Subject: [PATCH] fix(kanban): claim-less complete no longer closes a live worker's run complete_task authorised a terminal transition by task status alone; the `current_run_id = ?` fence only applied when the caller volunteered expected_run_id (derived from HERMES_KANBAN_* env). A human at the CLI, an orchestrator session or any env-less caller therefore marked a `running` card done and _end_run closed the dispatcher worker's run row while that worker kept executing (#111764). Mirror the fence request_review already carries: a `running` task under a live claim needs expected_run_id (worker ownership) or force=True (explicit operator override), otherwise LiveClaimError. `hermes kanban complete --force` and the dashboard's "mark done" (a human action) carry the override; the kanban_complete tool reports a structured refusal. Completing `ready`, `blocked` or `review` cards without a claim is unchanged, so the manual / orchestrator flows PR #73188 pinned keep working. Fixes #111764 --- hermes_cli/kanban.py | 11 +++- hermes_cli/kanban_db.py | 34 +++++++++- hermes_cli/kanban_parser.py | 3 + plugins/kanban/dashboard/plugin_api.py | 5 +- .../test_kanban_complete_live_claim_guard.py | 63 +++++++++++++++++++ tools/kanban_tools.py | 6 ++ website/docs/user-guide/features/kanban.md | 4 +- 7 files changed, 118 insertions(+), 8 deletions(-) create mode 100644 tests/hermes_cli/test_kanban_complete_live_claim_guard.py diff --git a/hermes_cli/kanban.py b/hermes_cli/kanban.py index fb30e2e046..8361a74a37 100644 --- a/hermes_cli/kanban.py +++ b/hermes_cli/kanban.py @@ -885,8 +885,15 @@ def _cmd_complete(args: argparse.Namespace) -> int: fail_msg[tid] = gate_err return False fail_msg[tid] = f"cannot complete {tid} (unknown id or terminal state)" - return kb.complete_task(conn, tid, result=args.result, summary=summary, metadata=metadata, - expected_run_id=_worker_run_id_for(tid)) + try: + return kb.complete_task(conn, tid, result=args.result, summary=summary, metadata=metadata, + expected_run_id=_worker_run_id_for(tid), + force=bool(getattr(args, "force", False))) + except kb.LiveClaimError: + fail_msg[tid] = (f"cannot complete {tid}: a live worker is running it. Wait for the " + f"worker, `hermes kanban reclaim {tid}` to release it, or re-run with " + f"--force to close its run and complete anyway.") + return False return _bulk_apply(ids, op, lambda tid: f"Completed {tid}", fail_msg.__getitem__) diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index c4219fa0b3..c30772e061 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -2599,16 +2599,34 @@ class ArtifactPreservationError(RuntimeError): """Raised when a declared scratch deliverable cannot be preserved.""" +class LiveClaimError(ValueError): + """``complete_task`` refused: the task is ``running`` under a live claim and + the caller neither owns its run (``expected_run_id``) nor passed ``force``. + Completing anyway would close the worker's run row underneath a process + that is still executing. A ``ValueError`` so tool error handlers treat it + as recoverable.""" + + def __init__(self, task_id: str): + super().__init__( + f"{task_id} is running under a live worker claim; pass expected_run_id " + "(worker ownership) or force=True (explicit operator override) instead " + "of closing the live run" + ) + + def complete_task( conn: sqlite3.Connection, task_id: str, *, result: Optional[str] = None, summary: Optional[str] = None, metadata: Optional[dict] = None, created_cards: Optional[Iterable[str]] = None, expected_run_id: Optional[int] = None, - fire_lifecycle_hook: bool = True, + fire_lifecycle_hook: bool = True, force: bool = False, ) -> bool: """``running|ready|blocked|review -> done``; records ``result``. ``ready`` is accepted for manual CLI completion, ``review`` for human - approval; with no active run the handoff fields survive via + approval. A ``running`` task under a live claim is only completed with + proof of ownership (``expected_run_id``) or ``force=True`` (explicit + operator override) — otherwise :class:`LiveClaimError`, the same fence + :func:`request_review` applies. With no active run the handoff fields survive via :func:`_synthesize_ended_run`. ``summary`` (defaults to ``result``) and ``metadata`` land on the closing run for :func:`build_worker_context`. ``created_cards`` are verified first — a phantom id raises @@ -2635,7 +2653,17 @@ def complete_task( return False if acceptance is not None and not record_acceptance(conn, task_id, acceptance): return False - prior_status = _task_status(conn, task_id) + trow = conn.execute("SELECT status, claim_lock FROM tasks WHERE id = ?", (task_id,)).fetchone() + prior_status = trow["status"] if trow else None + # Refuse to close a live worker's run without proof of ownership + # (expected_run_id) or an explicit human override (force=True). + if ( + expected_run_id is None + and not force + and prior_status == "running" + and trow["claim_lock"] is not None + ): + raise LiveClaimError(task_id) sql = """ UPDATE tasks SET status = 'done', diff --git a/hermes_cli/kanban_parser.py b/hermes_cli/kanban_parser.py index e3cb0371d1..9c17b1f0f2 100644 --- a/hermes_cli/kanban_parser.py +++ b/hermes_cli/kanban_parser.py @@ -284,6 +284,9 @@ _SPECS = [ _arg("--metadata", help='JSON dict of structured facts (e.g. \'{"changed_files": [...], ' '"tests_run": 12}\'). Stored on the closing run.'), + _arg("--force", action="store_true", + help="Override the live-claim guard: complete a running, claimed task " + "even without owning its run (closes the worker's run)."), ], help="Mark one or more tasks done"), _cmd("edit", [ _TASK_ID, diff --git a/plugins/kanban/dashboard/plugin_api.py b/plugins/kanban/dashboard/plugin_api.py index 9b09a818cb..f60c280cef 100644 --- a/plugins/kanban/dashboard/plugin_api.py +++ b/plugins/kanban/dashboard/plugin_api.py @@ -547,9 +547,10 @@ def _drag_to(conn, task_id: str, s: str) -> bool: # Status verb dispatch shared by PATCH /tasks/{id} and POST /tasks/bulk: (conn, task_id, # payload) -> ok. ``review`` uses request_review (never a block, so it can't trip unblock-loop -# detection) with ``force=True``: a dashboard action is a human override of a live worker claim. +# detection) and ``done`` pass ``force=True``: a dashboard action is a human override of a live worker claim. _STATUS_HANDLERS: dict[str, Any] = { - "done": lambda conn, tid, p: kanban_db.complete_task(conn, tid, result=p.result, summary=p.summary, metadata=p.metadata), + "done": lambda conn, tid, p: kanban_db.complete_task( + conn, tid, result=p.result, summary=p.summary, metadata=p.metadata, force=True), "blocked": lambda conn, tid, p: kanban_db.block_task(conn, tid, reason=getattr(p, "block_reason", None)), "scheduled": lambda conn, tid, p: kanban_db.schedule_task(conn, tid, reason=getattr(p, "block_reason", None)), "review": lambda conn, tid, p: kanban_db.request_review( diff --git a/tests/hermes_cli/test_kanban_complete_live_claim_guard.py b/tests/hermes_cli/test_kanban_complete_live_claim_guard.py new file mode 100644 index 0000000000..471d482c65 --- /dev/null +++ b/tests/hermes_cli/test_kanban_complete_live_claim_guard.py @@ -0,0 +1,63 @@ +"""Invariant: ``complete_task`` never closes a live worker's run for a caller that +neither owns the run nor asked for an operator override (issue #111764). + +A claim-less completion (a human at the CLI, an orchestrator session — anything +without ``HERMES_KANBAN_*`` env) used to be authorised by task status alone, so it +marked a ``running`` card done and ``_end_run`` closed the dispatcher worker's run +row while that worker kept executing. The guard mirrors ``request_review``'s: a +``running`` task under a live claim needs ``expected_run_id`` or ``force=True``. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from hermes_cli import kanban_db as kb +from hermes_cli import kanban_db_connect as kbc + + +@pytest.fixture +def conn(tmp_path, monkeypatch): + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + db_path = kb.kanban_db_path(board="default") + kb._INITIALIZED_PATHS.discard(str(db_path.resolve())) + kb.init_db() + with kbc.connect() as c: + yield c + + +def _claimed_running_task(conn) -> tuple[str, int]: + tid = kb.create_task(conn, title="live", assignee="coder") + assert kb.claim_task(conn, tid, claimer=kb._claimer_id()) is not None + return tid, kb._current_run_id(conn, tid) + + +def test_claimless_complete_refuses_live_run_until_forced(conn): + tid, run_id = _claimed_running_task(conn) + + with pytest.raises(kb.LiveClaimError): + kb.complete_task(conn, tid, result="someone else says done") + + # Nothing moved: the worker's run is still open and it can still finish its own card. + run = conn.execute("SELECT ended_at FROM task_runs WHERE id = ?", (run_id,)).fetchone() + assert run["ended_at"] is None + assert conn.execute("SELECT status FROM tasks WHERE id = ?", (tid,)).fetchone()["status"] == "running" + assert kb.complete_task(conn, tid, result="worker done", expected_run_id=run_id) is True + + # Explicit operator override still closes a live run. + tid2, run2 = _claimed_running_task(conn) + assert kb.complete_task(conn, tid2, result="operator override", force=True) is True + run = conn.execute("SELECT ended_at, outcome FROM task_runs WHERE id = ?", (run2,)).fetchone() + assert run["ended_at"] is not None and run["outcome"] == "completed" + + +def test_claimless_complete_of_unclaimed_card_unchanged(conn): + """The legitimate manual flow — completing a card nobody is working on — needs no proof.""" + tid = kb.create_task(conn, title="admin", assignee="coder") + assert kb.complete_task(conn, tid, result="done") is True + assert conn.execute("SELECT status FROM tasks WHERE id = ?", (tid,)).fetchone()["status"] == "done" diff --git a/tools/kanban_tools.py b/tools/kanban_tools.py index 233846ea76..298e467dc0 100644 --- a/tools/kanban_tools.py +++ b/tools/kanban_tools.py @@ -598,6 +598,12 @@ def _handle_complete(args: dict, **kw) -> str: f"Your task is still in-flight and its scratch workspace was kept. Fix the " f"artifact path or storage error, then retry kanban_complete with the same " f"handoff.") + except kb.LiveClaimError as claim_err: + # Env-less caller (orchestrator, another session) on a card a dispatcher + # worker is executing: refusing here is what keeps that worker's run open. + return tool_error( + f"kanban_complete refused: {claim_err}. Nothing changed. Wait for the worker " + f"to finish, or an operator can run `hermes kanban complete --force {tid}`.") except kb.HallucinatedCardsError as hall_err: # The gate runs before the write txn, so the task was NOT mutated; # say so explicitly or the model treats the error as terminal and diff --git a/website/docs/user-guide/features/kanban.md b/website/docs/user-guide/features/kanban.md index 85a1e34982..52d49542b9 100644 --- a/website/docs/user-guide/features/kanban.md +++ b/website/docs/user-guide/features/kanban.md @@ -861,7 +861,7 @@ hermes kanban claim [--ttl SECONDS] hermes kanban comment "" [--author NAME] # Bulk verbs — accept multiple ids: -hermes kanban complete ... [--result "..."] +hermes kanban complete ... [--result "..."] [--force] hermes kanban block "" [--ids ...] hermes kanban unblock ... hermes kanban archive ... @@ -1258,6 +1258,8 @@ Runs are exposed on the dashboard (Run History section in the drawer, one colour **Bulk close caveat.** `hermes kanban complete a b c --summary X` is refused — structured handoff is per-run, so copy-pasting the same summary to N tasks is almost always wrong. Bulk close *without* `--summary` / `--metadata` still works for the common "I finished a pile of admin tasks" case. +**Live-claim guard on complete.** A `running` task whose worker holds a live claim is only completed by that worker (`kanban_complete` from inside the run) or by an explicit operator override: `hermes kanban complete --force` and the dashboard's "mark done" action. A claim-less `hermes kanban complete ` or an orchestrator session's `kanban_complete` is refused with a pointer to `--force` / `hermes kanban reclaim`, so a second session can no longer close a live worker's run underneath it. Completing `ready`, `blocked` or `review` cards without a claim is unchanged. + **Reclaimed runs from status changes.** If you drag a running task off `running` in the dashboard (back to `ready`, or straight to `todo`), or archive a task that was still running, the in-flight run closes with `outcome='reclaimed'` rather than being orphaned. The `task_runs` row is always in a terminal state when `tasks.current_run_id` is `NULL`, and vice versa — that invariant holds across CLI, dashboard, dispatcher, and notifier. **Synthetic runs for never-claimed completions.** Completing or blocking a task that was never claimed (e.g. a human closes a `ready` task from the dashboard with a summary, or a CLI user runs `hermes kanban complete --summary X`) would otherwise drop the handoff. Instead the kernel inserts a zero-duration run row (`started_at == ended_at`) carrying the summary / metadata / reason so attempt history stays complete. The `completed` / `blocked` event's `run_id` points at that row.