🐛 fix(kanban): restore crashed reviewer to review lane, not build lane

PATCHES.md

@@ -72,3 +72,4 @@ default; do **not** retire such a row on a PR-merge signal. See the #44338 row.
 | fork PR (TBD — upstream PR not yet opened) | Make the kanban crash-detector clear a dead worker's stale claim regardless of lane, so a worker that dies while its card sits in a NON-`running` lane (most commonly `review`, after the implementer opened a PR and the card moved on) no longer wedges that lane for the full 1h stale-claim TTL. `detect_crashed_workers` scanned only `WHERE t.status = 'running'`, so a card already in `review` carrying a dead `worker_pid` + `claim_lock` was invisible to the fast reaper; the review-column dispatch query gates on `claim_lock IS NULL`, so the next worker (the reviewer) could not spawn until `release_stale_claims`' TTL eventually freed it (observed live twice on PR #70 rework rounds; card `t_a29b93c2` had to be hand-cleared). The fix widens the scan to any card with a non-NULL `worker_pid` (status added to the projection) and, after the existing host-local + launch-grace + dead-PID checks pass, branches on status: a non-`running` card gets an **in-place claim clear** (`claim_lock`/`claim_expires`/`worker_pid` = NULL) with a `stale_claim_cleared` event and NO lane change (a dead worker in `review` stays in `review` so the reviewer re-spawns; it is not yanked to `ready`), and NO run open/close, crash event, or failure-counter/breaker tick (those are `running`-crash semantics). The `running` path is byte-for-byte unchanged — its UPDATE stays guarded `status = 'running'` and only fires for `running` cards, since non-`running` ones `continue` out above. Launch-grace (measured from the active `task_runs` row) and the host-local claim check still apply, so a freshly-spawned worker is not reaped mid-init. Single-file change in `hermes_cli/kanban_db.py` + 4 regression tests in `tests/hermes_cli/test_kanban_db.py` (dead worker in `review` → claim cleared, status stays `review`, no failure count; review dispatch then permitted; within-grace claim NOT cleared; foreign-host claim untouched). Clean upstream candidate — a reaper that ignores non-running lanes is a general dispatcher bug, not homestead-specific. **Retire trigger:** carried as upstream-pending with NO upstream PR (deliberate — we do not surface this to NousResearch). Auto-retires if/when an equivalent lane-agnostic stale-claim clear appears in the base tag's `detect_crashed_workers` independently (the rebase will drop or empty the hunk); otherwise it stays as a carried fix. | upstream-pending | v2026.6.19 |
 | fork PR (TBD — upstream PR not yet opened) | Fix the kanban crash-detector reaping a re-claimed worker mid-init. `detect_crashed_workers` grants a freshly-spawned worker a launch-window grace (default 30s, `HERMES_KANBAN_CRASH_GRACE_SECONDS`) so its PID can become visible on `/proc` before liveness is checked — but it measured the grace from `tasks.started_at`, which is pinned to the task's first-ever start (`COALESCE` on every claim) and never refreshed on re-claim. A card re-claimed for its next lane (implement → review) therefore inherited a stale `started_at`, the grace had long since expired, and the new worker was reaped as `crashed (pid not alive)` before it finished initializing (plugin load); with `failure_limit=2` the card then landed in `blocked`/`gave_up` without the work ever being attempted. The fix measures the grace from the active `task_runs` row via `COALESCE(r.started_at, t.started_at)` joined on `current_run_id` — the **exact per-attempt pattern `enforce_max_runtime` already uses** (see `test_max_runtime_uses_current_run_start_after_retry`), so retries/re-claims get a fresh window; falls back to `tasks.started_at` when no run row is present (preserves first-claim behavior). Single-file change in `hermes_cli/kanban_db.py` + regression test `test_detect_crashed_workers_grace_uses_current_run_start_after_reclaim`. Proven live: under the fix the auto-dispatcher spawned a worker on a card with a 3600s-stale `started_at` and it reached its agent loop and completed (run 14, 12s) where pre-fix runs crashed mid-init. **Retire trigger:** open the upstream PR (phase 2), then auto-retire when it merges in a tagged release ≥ base; until then carry as upstream-pending. | upstream-pending | v2026.6.19 |
 | fork-local (no upstream PR) | Homestead-specific expansion of the `skills/github/github-code-review` skill for the kanban PR-review pipeline: the automated/non-interactive (webhook) review context, post-once idempotency guards (head-SHA dedup, blocked-timeout-is-unknown-not-failed, write-payload-to-file-before-POST), the humanizer/de-claude gate, and four reference files (`responding-to-and-resolving-review-threads` — the AUTHOR-resolves-threads loop incl. the Casey-2026-06-24 author-vs-reviewer decision; `webhook-triggered-reviews`; `consolidating-duplicate-reviews`; `editing-a-posted-review`) plus `scripts/commentable_lines.py`. This is the durable git home for material that previously lived ONLY in the `~/.hermes` deploy (silent drift). Deliberate divergence from upstream's generic review skill — homestead/kanban-specific, never sent upstream. **Retire trigger:** never auto-retires (permanent-local); remove only if the homestead PR-review pipeline is retired. | permanent-local | v2026.6.19 |
+| fork PR (TBD — no upstream PR; fork-internal review-lane semantics) | Fix the kanban crash-detector dropping a crashed **reviewer** back into the build lane. A card moved to `review` (a worker opened a PR and parked it) is claimed by `claim_review_task`, which CAS-transitions `review → running` and records a `claimed` event carrying `source_status: "review"` — so while the reviewer works the row status is `running`, indistinguishable from a build run. PR #16 only handled a dead worker on a card parked in a NON-`running` lane; it did NOT cover a reviewer that dies while actively `running`. When `detect_crashed_workers` reaped such a crash it ran the running-crash UPDATE `SET status='ready'`, losing the review lane: (i) the normal `ready` dispatch then re-ran the IMPLEMENTER instead of respawning the reviewer (PR-under-review silently falls back into the build lane), and (ii) `check_respawn_guard` recomputed `is_review=False`, re-tripping the `recent_success`/`active_pr` guards (the original build run is a recent `completed` run + left a PR-URL comment) and deferring respawn for the full window. The fix reads the durable `source_status: "review"` signal off the crashed run's `claimed` event (scoped to `current_run_id`; no new schema column) and, on a genuine crash, restores the card to `review` instead of `ready` — claim still cleared, CAS still guarded on `status='running'`, crash event / run outcome / failure counter / circuit breaker bookkeeping unchanged. To keep a flaky reviewer from looping forever in `review`, the crash-path breaker-trip UPDATE in `_record_task_failure` widens its WHERE-IN from `('ready','running')` to `('ready','running','review')`, so a repeatedly-crashing reviewer still trips to `blocked` via the normal failure-count path. Rate-limited (cooldown defer) and protocol-violation (immediate trip) sub-cases unchanged — only the lane on a genuine crash changes. Single-file change in `hermes_cli/kanban_db.py` + 4 regression tests in `tests/hermes_cli/test_kanban_db.py` (crashed reviewer → `review`; crashed build run → `ready` unchanged; repeatedly-crashing reviewer → `blocked`; restored `review` card is free of the `recent_success`/`active_pr` guards). Fork-internal review-lane semantics (the `claim_review_task` source_status signal is fork machinery) — not surfaced to NousResearch. **Retire trigger:** never auto-retires on an upstream PR-merge signal (no upstream PR); remove only if the fork's review-lane dispatch is retired or upstream adopts equivalent review-lane-aware crash recovery. | permanent-local | v2026.6.19 |

hermes_cli/kanban_db.py

@@ -5094,6 +5094,46 @@ def _classify_worker_exit(pid: int) -> "tuple[str, Optional[int]]":
     return ("unknown", None)
 
 
+def _crashed_run_was_review(
+    conn: sqlite3.Connection,
+    task_id: str,
+    current_run_id: Optional[int],
+) -> bool:
+    """Return True if the task's current (crashed) run was a REVIEW run.
+
+    The durable signal is the ``source_status: "review"`` payload that
+    ``claim_review_task`` writes onto the run's ``claimed`` event. A build run
+    claimed via ``claim_task`` writes a ``claimed`` event WITHOUT that key.
+    Reading the existing event payload avoids inventing a new schema column.
+
+    Scoped to the crashed run's ``current_run_id`` so a card that was built
+    once (build ``claimed`` event) and later moved to review (review ``claimed``
+    event) is classified by its CURRENT run, not its history. Falls back to the
+    latest ``claimed`` event for the task when the run id is unknown.
+    """
+    if current_run_id is not None:
+        ev = conn.execute(
+            "SELECT payload FROM task_events "
+            "WHERE task_id = ? AND kind = 'claimed' AND run_id = ? "
+            "ORDER BY id DESC LIMIT 1",
+            (task_id, current_run_id),
+        ).fetchone()
+    else:
+        ev = conn.execute(
+            "SELECT payload FROM task_events "
+            "WHERE task_id = ? AND kind = 'claimed' "
+            "ORDER BY id DESC LIMIT 1",
+            (task_id,),
+        ).fetchone()
+    if ev is None or not ev["payload"]:
+        return False
+    try:
+        payload = json.loads(ev["payload"])
+    except (ValueError, TypeError):
+        return False
+    return isinstance(payload, dict) and payload.get("source_status") == "review"
+
+
 def reap_worker_zombies() -> "list[int]":
     """Reap all zombie children of this process without blocking.
 
@@ -5659,6 +5699,7 @@ def detect_crashed_workers(conn: sqlite3.Connection) -> list[str]:
         # look. See the per-status branch below.
         rows = conn.execute(
             "SELECT t.id, t.status, t.worker_pid, t.claim_lock, "
+            "       t.current_run_id, "
             "       COALESCE(r.started_at, t.started_at) AS active_started_at "
             "FROM tasks t "
             "LEFT JOIN task_runs r ON r.id = t.current_run_id "
@@ -5776,11 +5817,34 @@ def detect_crashed_workers(conn: sqlite3.Connection) -> list[str]:
                     event_payload["exit_kind"] = kind
                     event_payload["exit_code"] = code
 
+            # A crashed REVIEW run must return to ``review`` (not ``ready``):
+            # the card was claimed via ``claim_review_task`` (review -> running)
+            # so at the row level it's indistinguishable from a build run, but
+            # dropping it to ``ready`` loses the review lane — the normal ready
+            # dispatch would re-run the IMPLEMENTER instead of respawning the
+            # reviewer, and ``check_respawn_guard`` would re-trip its
+            # recent_success / active_pr guards (both carved out for review
+            # status). The durable signal is the ``source_status: review``
+            # payload on this run's ``claimed`` event. Rate-limited and
+            # protocol-violation sub-cases are unaffected here: only the LANE a
+            # genuine crash returns to changes; the breaker still trips a
+            # repeatedly-crashing reviewer via the normal failure-count path
+            # (see ``_record_task_failure``, whose blocked transition matches
+            # ``review`` too).
+            current_run_id = (
+                row["current_run_id"]
+                if "current_run_id" in row.keys()
+                else None
+            )
+            was_review_run = _crashed_run_was_review(
+                conn, row["id"], current_run_id
+            )
+            restore_status = "review" if was_review_run else "ready"
             cur = conn.execute(
-                "UPDATE tasks SET status = 'ready', claim_lock = NULL, "
+                "UPDATE tasks SET status = ?, claim_lock = NULL, "
                 "claim_expires = NULL, worker_pid = NULL "
                 "WHERE id = ? AND status = 'running'",
-                (row["id"],),
+                (restore_status, row["id"]),
             )
             if cur.rowcount == 1:
                 # Rate-limited requeues are a clean release, not a crash —
@@ -5942,13 +6006,16 @@ def _record_task_failure(
                     (failures, error[:500], task_id),
                 )
             else:
-                # Timeout/crash path: task is already at ``ready``
-                # with claim cleared; just flip to blocked + update
-                # counter fields.
+                # Timeout/crash path: task is already at ``ready`` (build
+                # crash) or ``review`` (a crashed REVIEW run restored to its
+                # lane by ``detect_crashed_workers``) with claim cleared; just
+                # flip to blocked + update counter fields. ``review`` is
+                # included so a repeatedly-crashing reviewer still trips the
+                # breaker instead of looping forever in the review lane.
                 conn.execute(
                     "UPDATE tasks SET status = 'blocked', "
                     "consecutive_failures = ?, last_failure_error = ? "
-                    "WHERE id = ? AND status IN ('ready', 'running')",
+                    "WHERE id = ? AND status IN ('ready', 'running', 'review')",
                     (failures, error[:500], task_id),
                 )
             run_id = None