fix(kanban): worker-initiated block must not be auto-promoted (#28712)

hermes_cli/kanban_db.py

@@ -861,12 +861,13 @@ class Event:
     -- inside an agent loop that propagated ``HERMES_SESSION_ID``. NULL
     -- for tasks created from the CLI, dashboard, or any path that doesn't
     -- set the env var. Indexed so per-session list queries stay cheap on
-    -- larger boards.
+    -- larger boards.  The accompanying index is created later in
+    -- ``_migrate_add_optional_columns`` so legacy DBs (where
+    -- ``CREATE TABLE IF NOT EXISTS`` is a no-op) still get the column
+    -- added *before* the index is asserted — see #28712.
     session_id           TEXT
 );
 
-CREATE INDEX IF NOT EXISTS idx_tasks_session_id ON tasks(session_id);
-
 CREATE TABLE IF NOT EXISTS task_links (
     parent_id  TEXT NOT NULL,
     child_id   TEXT NOT NULL,
@@ -937,8 +938,15 @@ class Event:
 
 CREATE INDEX IF NOT EXISTS idx_tasks_assignee_status ON tasks(assignee, status);
 CREATE INDEX IF NOT EXISTS idx_tasks_status          ON tasks(status);
-CREATE INDEX IF NOT EXISTS idx_tasks_tenant          ON tasks(tenant);
-CREATE INDEX IF NOT EXISTS idx_tasks_idempotency     ON tasks(idempotency_key);
+-- ``idx_tasks_tenant``, ``idx_tasks_idempotency`` and
+-- ``idx_tasks_session_id`` are created in
+-- ``_migrate_add_optional_columns`` instead of here.  Those columns
+-- were added after v1, so on a legacy DB ``CREATE TABLE IF NOT EXISTS``
+-- above is a no-op and the columns don't exist yet — running the
+-- index DDL before the additive migration crashed ``init_db`` with
+-- ``no such column`` on real-world upgrades (#28712).  Deferring the
+-- indexes lets the migration ALTER the columns in first, then assert
+-- the indexes idempotently.
 CREATE INDEX IF NOT EXISTS idx_links_child           ON task_links(child_id);
 CREATE INDEX IF NOT EXISTS idx_links_parent          ON task_links(parent_id);
 CREATE INDEX IF NOT EXISTS idx_comments_task         ON task_comments(task_id, created_at);
@@ -1083,10 +1091,18 @@ def _migrate_add_optional_columns(conn: sqlite3.Connection) -> None:
         _add_column_if_missing(
             conn, "tasks", "idempotency_key", "idempotency_key TEXT"
         )
-        conn.execute(
-            "CREATE INDEX IF NOT EXISTS idx_tasks_idempotency "
-            "ON tasks(idempotency_key)"
-        )
+    # Indexes for late-added columns live here (not in SCHEMA_SQL) so
+    # legacy DBs don't crash on the ``CREATE INDEX`` before the
+    # additive migrations had a chance to run — see #28712.  Both
+    # statements use ``IF NOT EXISTS`` so they're cheap no-ops on
+    # fresh DBs where the columns and indexes were created together.
+    conn.execute(
+        "CREATE INDEX IF NOT EXISTS idx_tasks_tenant ON tasks(tenant)"
+    )
+    conn.execute(
+        "CREATE INDEX IF NOT EXISTS idx_tasks_idempotency "
+        "ON tasks(idempotency_key)"
+    )
 
     # Refresh after early additive migrations above. Some existing DBs were
     # partially migrated in older releases and can already contain the later
@@ -1170,14 +1186,21 @@ def _migrate_add_optional_columns(conn: sqlite3.Connection) -> None:
         # created from within an agent loop that propagated
         # ``HERMES_SESSION_ID`` (e.g. ACP). NULL on legacy rows and on any
         # creation path that doesn't set the env var (CLI, dashboard).
-        # Index keeps per-session list queries cheap.
         _add_column_if_missing(
             conn, "tasks", "session_id", "session_id TEXT"
         )
-        conn.execute(
-            "CREATE INDEX IF NOT EXISTS idx_tasks_session_id "
-            "ON tasks(session_id)"
-        )
+    # Index is always (re-)asserted here rather than in SCHEMA_SQL so
+    # legacy DBs — where ``CREATE TABLE IF NOT EXISTS`` is a no-op and
+    # the ``session_id`` column does not yet exist — get the migration
+    # ALTER first, then the index.  Otherwise ``init_db`` blew up with
+    # ``no such column: session_id`` mid-script and never reached this
+    # migration at all (#28712).  ``IF NOT EXISTS`` makes the call a
+    # cheap no-op on fresh DBs where the column was already in the
+    # ``CREATE TABLE``.
+    conn.execute(
+        "CREATE INDEX IF NOT EXISTS idx_tasks_session_id "
+        "ON tasks(session_id)"
+    )
 
     # task_events gained a run_id column; back-fill it as NULL for
     # historical events (they predate runs and can't be attributed).
@@ -1994,11 +2017,58 @@ def _synthesize_ended_run(
 # Dependency resolution (todo -> ready)
 # ---------------------------------------------------------------------------
 
+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).
+
+    A ``blocked`` status can come from two very different sources:
+
+    * **Worker- or operator-initiated** — a worker called
+      ``kanban_block(reason="review-required: ...")`` (or somebody ran
+      ``hermes kanban block <id>``).  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``.
+
+    * **Circuit-breaker** — ``_record_task_failure`` tripped after
+      repeated crashes / spawn failures / timeouts.  This emits
+      ``"gave_up"``, *not* ``"blocked"``, and is meant to recover
+      automatically once the underlying conditions change (e.g. parents
+      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.
+
+    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
+    DB manipulation) — preserves the pre-#28712 auto-recover semantics
+    for that path.
+    """
+    row = conn.execute(
+        "SELECT kind FROM task_events "
+        "WHERE task_id = ? AND kind IN ('blocked', 'unblocked') "
+        "ORDER BY id DESC LIMIT 1",
+        (task_id,),
+    ).fetchone()
+    return bool(row) and row["kind"] == "blocked"
+
+
 def recompute_ready(conn: sqlite3.Connection) -> int:
     """Promote ``todo`` tasks to ``ready`` when all parents are ``done`` or ``archived``.
 
     Returns the number of tasks promoted.  Safe to call inside or outside
     an existing transaction; it opens its own IMMEDIATE txn.
+
+    ``blocked`` tasks are also considered for promotion (so a task
+    blocked purely by a parent dependency unblocks itself when the
+    parent completes), *except* when the most recent block event was a
+    worker-initiated ``kanban_block`` — those stay blocked until an
+    explicit ``kanban_unblock`` (#28712).  Without that guard, a
+    ``review-required`` handoff would auto-respawn, the fresh worker
+    would find nothing to do, exit cleanly, get recorded as a protocol
+    violation, and the cycle would repeat indefinitely.
     """
     promoted = 0
     with write_txn(conn):
@@ -2008,6 +2078,12 @@ def recompute_ready(conn: sqlite3.Connection) -> int:
         for row in todo_rows:
             task_id = row["id"]
             cur_status = row["status"]
+            if cur_status == "blocked" and _has_sticky_block(conn, task_id):
+                # Worker / operator asked for human review — do not
+                # silently auto-recover.  ``unblock_task`` is the only
+                # legitimate exit (it emits ``"unblocked"`` which flips
+                # this predicate back).
+                continue
             parents = conn.execute(
                 "SELECT t.status FROM tasks t "
                 "JOIN task_links l ON l.parent_id = t.id "
@@ -2016,7 +2092,8 @@ def recompute_ready(conn: sqlite3.Connection) -> int:
             ).fetchall()
             if all(p["status"] in ("done", "archived") for p in parents):
                 # Blocked tasks also get their failure counters reset —
-                # this is effectively an auto-unblock.
+                # this is effectively an auto-unblock (circuit-breaker
+                # recovery; worker-initiated blocks are skipped above).
                 if cur_status == "blocked":
                     conn.execute(
                         "UPDATE tasks SET status = 'ready', "