diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index b64d54e53ab7..15d959fd1e0b 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -4096,15 +4096,17 @@ def _synthesize_ended_run( def _has_sticky_block(conn: sqlite3.Connection, task_id: str) -> bool: """Return True when ``task_id`` is sticky-blocked by an explicit - worker/operator ``kanban_block`` call (#28712). + worker/operator action (#28712). A ``blocked`` status can come from two very different sources: - * **Worker- or operator-initiated** — a worker called + * **Worker- or operator-initiated** — a task was created with + ``initial_status="blocked"``, a worker called ``kanban_block(reason="review-required: ...")`` (or somebody ran ``hermes kanban block ``). This is a deliberate handoff that - should stay blocked until an operator unblocks it. The block tool - emits a ``"blocked"`` event row in ``task_events``. + should stay blocked until an operator unblocks or manually promotes it. + Creation records ``status="blocked"`` in the ``"created"`` event; + the block tool emits a ``"blocked"`` event. * **Circuit-breaker** — ``_record_task_failure`` tripped after repeated crashes / spawn failures / timeouts. This emits @@ -4113,10 +4115,10 @@ def _has_sticky_block(conn: sqlite3.Connection, task_id: str) -> bool: finish, transient infra error clears). The cheapest signal that distinguishes the two is the most recent - ``"blocked"`` / ``"unblocked"`` event for the task. If the most - recent one is ``"blocked"`` (or there is a ``"blocked"`` event and - no ``"unblocked"`` event has fired since), the task is sticky and - ``recompute_ready`` must *not* auto-promote it. + provenance event. ``"blocked"`` and a creation-time blocked status set + the sticky state; ``"unblocked"`` and ``"promoted_manual"`` explicitly + clear it. Automatic ``"promoted"`` and circuit-breaker ``"gave_up"`` + events deliberately do neither. Returns ``False`` when there is no such event at all (e.g. the task was set to ``status='blocked'`` by the circuit breaker or by direct @@ -4124,12 +4126,35 @@ def _has_sticky_block(conn: sqlite3.Connection, task_id: str) -> bool: for that path. """ row = conn.execute( - "SELECT kind FROM task_events " - "WHERE task_id = ? AND kind IN ('blocked', 'unblocked') " + "SELECT kind, payload FROM task_events " + "WHERE task_id = ? " + "AND kind IN ('created', 'blocked', 'unblocked', 'promoted_manual') " "ORDER BY id DESC LIMIT 1", (task_id,), ).fetchone() - return bool(row) and row["kind"] == "blocked" + if not row: + return False + if row["kind"] == "blocked": + return True + if row["kind"] != "created": + return False + raw_payload = row["payload"] + if not isinstance(raw_payload, str) or not raw_payload.strip(): + return True + try: + payload = json.loads(raw_payload) + except (TypeError, ValueError, RecursionError): + # Provenance corruption must not crash the whole dispatcher or turn a + # human-held blocked card into runnable work. The caller only asks + # about tasks whose current status is already ``blocked``, so preserve + # that safe state when the creation record cannot be interpreted. + return True + if not isinstance(payload, dict): + return True + created_status = payload.get("status") + if not isinstance(created_status, str) or created_status not in VALID_STATUSES: + return True + return created_status == "blocked" def recompute_ready( diff --git a/tests/hermes_cli/test_kanban_blocked_sticky.py b/tests/hermes_cli/test_kanban_blocked_sticky.py index 9c3e4f4b89f2..7ee0d8df6904 100644 --- a/tests/hermes_cli/test_kanban_blocked_sticky.py +++ b/tests/hermes_cli/test_kanban_blocked_sticky.py @@ -76,6 +76,201 @@ def test_worker_block_is_not_auto_promoted_by_recompute_ready(kanban_home: Path) assert kb.get_task(conn, tid).status == "blocked" +@pytest.mark.parametrize("kind", ["needs_input", "capability"]) +def test_human_block_kinds_remain_sticky( + kanban_home: Path, + kind: str, +) -> None: + with kb.connect() as conn: + tid = kb.create_task(conn, title=f"human block: {kind}") + kb.claim_task(conn, tid) + assert kb.block_task( + conn, + tid, + reason="operator action required", + kind=kind, + expected_run_id=kb.get_task(conn, tid).current_run_id, + ) + + assert kb.recompute_ready(conn) == 0 + assert kb.get_task(conn, tid).status == "blocked" + + +@pytest.mark.parametrize("completed_parent", [False, True]) +def test_initially_blocked_task_is_sticky( + kanban_home: Path, + completed_parent: bool, +) -> None: + """Creation-time human-ops blocks are deliberate, not dependency waits.""" + with kb.connect() as conn: + parents: list[str] = [] + if completed_parent: + parent = kb.create_task(conn, title="completed parent") + conn.execute( + "UPDATE tasks SET status = 'done' WHERE id = ?", + (parent,), + ) + parents.append(parent) + + tid = kb.create_task( + conn, + title="awaiting human ops", + parents=parents, + initial_status="blocked", + ) + + assert kb.recompute_ready(conn) == 0 + assert kb.get_task(conn, tid).status == "blocked" + + +@pytest.mark.parametrize("completed_parent", [False, True]) +def test_dispatch_does_not_claim_or_spawn_initially_blocked_task( + kanban_home: Path, + monkeypatch: pytest.MonkeyPatch, + completed_parent: bool, +) -> None: + """The full dispatch tick must preserve a creation-time blocked card.""" + with kb.connect() as conn: + parents: list[str] = [] + if completed_parent: + parent = kb.create_task(conn, title="completed parent") + conn.execute( + "UPDATE tasks SET status = 'done' WHERE id = ?", + (parent,), + ) + parents.append(parent) + + tid = kb.create_task( + conn, + title="awaiting human ops", + assignee="worker", + parents=parents, + initial_status="blocked", + ) + claim_calls: list[str] = [] + spawn_calls: list[str] = [] + real_claim_task = kb.claim_task + + def spy_claim_task(connection, task_id, **kwargs): + claim_calls.append(task_id) + return real_claim_task(connection, task_id, **kwargs) + + def fake_spawn(task, _workspace): + spawn_calls.append(task.id) + return 12345 + + monkeypatch.setattr("hermes_cli.profiles.profile_exists", lambda _name: True) + monkeypatch.setattr(kb, "claim_task", spy_claim_task) + + result = kb.dispatch_once(conn, spawn_fn=fake_spawn) + + assert result.promoted == 0 + assert result.spawned == [] + assert claim_calls == [] + assert spawn_calls == [] + assert kb.get_task(conn, tid).status == "blocked" + + +@pytest.mark.parametrize( + "payload", + [ + None, + "", + "{}", + '{"assignee":"worker"}', + '{"status":"unknown"}', + '{"status":null}', + '{"status":[]}', + '{"status":{}}', + "{not-json", + "[]", + '"blocked"', + "1", + "true", + ], +) +def test_unreadable_creation_provenance_fails_closed( + kanban_home: Path, + payload: str | None, +) -> None: + """A corrupt legacy event cannot crash dispatch or release blocked work.""" + with kb.connect() as conn: + tid = kb.create_task( + conn, + title="blocked with corrupt provenance", + initial_status="blocked", + ) + conn.execute( + "UPDATE task_events SET payload = ? " + "WHERE task_id = ? AND kind = 'created'", + (payload, tid), + ) + + assert kb.recompute_ready(conn) == 0 + assert kb.get_task(conn, tid).status == "blocked" + + +def test_deep_creation_provenance_does_not_abort_recompute_ready( + kanban_home: Path, +) -> None: + """Recursive JSON corruption must preserve the blocked card.""" + with kb.connect() as conn: + tid = kb.create_task( + conn, + title="blocked with deeply nested provenance", + initial_status="blocked", + ) + conn.execute( + "UPDATE task_events SET payload = ? " + "WHERE task_id = ? AND kind = 'created'", + ("[" * 2000 + "]" * 2000, tid), + ) + + assert kb.recompute_ready(conn) == 0 + assert kb.get_task(conn, tid).status == "blocked" + + +def test_dispatch_does_not_claim_or_spawn_deep_creation_provenance( + kanban_home: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """One corrupt created event must not abort or release dispatch work.""" + with kb.connect() as conn: + tid = kb.create_task( + conn, + title="blocked with deeply nested provenance", + assignee="worker", + initial_status="blocked", + ) + conn.execute( + "UPDATE task_events SET payload = ? " + "WHERE task_id = ? AND kind = 'created'", + ("[" * 2000 + "]" * 2000, tid), + ) + claim_calls: list[str] = [] + spawn_calls: list[str] = [] + real_claim_task = kb.claim_task + + def spy_claim_task(connection, task_id, **kwargs): + claim_calls.append(task_id) + return real_claim_task(connection, task_id, **kwargs) + + def fake_spawn(task, _workspace): + spawn_calls.append(task.id) + return 12345 + + monkeypatch.setattr("hermes_cli.profiles.profile_exists", lambda _name: True) + monkeypatch.setattr(kb, "claim_task", spy_claim_task) + + result = kb.dispatch_once(conn, spawn_fn=fake_spawn) + + assert result.promoted == 0 + assert result.spawned == [] + assert claim_calls == [] + assert spawn_calls == [] + assert kb.get_task(conn, tid).status == "blocked" + + # --------------------------------------------------------------------------- @@ -90,6 +285,52 @@ def test_worker_block_is_not_auto_promoted_by_recompute_ready(kanban_home: Path) # --------------------------------------------------------------------------- +@pytest.mark.parametrize("clear_with", ["unblock", "promote"]) +@pytest.mark.parametrize( + ("failures", "expected_promoted"), + [(0, 1), (2, 0)], +) +def test_explicit_clear_restores_circuit_breaker_retry_policy( + kanban_home: Path, + clear_with: str, + failures: int, + expected_promoted: int, +) -> None: + """Old creation provenance must not make a later ``gave_up`` sticky.""" + with kb.connect() as conn: + tid = kb.create_task( + conn, + title="operator parked", + initial_status="blocked", + ) + + if clear_with == "unblock": + assert kb.unblock_task(conn, tid) + expected_event = "unblocked" + else: + ok, error = kb.promote_task(conn, tid, actor="operator") + assert ok and error is None + expected_event = "promoted_manual" + + events = kb.list_events(conn, tid) + assert events[-1].kind == expected_event + + # A later circuit-breaker block has no explicit ``blocked`` event and + # keeps the existing retry-policy semantics: below the limit it may + # recover; at the limit it remains blocked. + with kb.write_txn(conn): + conn.execute( + "UPDATE tasks SET status = 'blocked', consecutive_failures = ? " + "WHERE id = ?", + (failures, tid), + ) + kb._append_event(conn, tid, "gave_up", {"failures": failures}) + + assert kb.recompute_ready(conn, failure_limit=2) == expected_promoted + expected_status = "ready" if expected_promoted else "blocked" + assert kb.get_task(conn, tid).status == expected_status + + # --------------------------------------------------------------------------- # Full bug-shaped loop: block → promote → crash → gave_up → next tick # ---------------------------------------------------------------------------