🐛 fix(kanban): reconcile a crashed provably-done card to review, not gave_up

hermes_cli/kanban_db.py

@@ -8289,33 +8289,51 @@ def detect_crashed_workers(conn: sqlite3.Connection) -> list[str]:
             # owner-map reviewer to auto-advance the card to (``review``); stays
             # None for every other shape (which release to ``ready`` as before).
             advance_review_owner: Optional[str] = None
-            if kind in ("clean_exit", "unknown") and _lane_work_provably_done(
-                conn, row["id"]
-            ):
+            if kind in (
+                "clean_exit", "unknown", "signaled", "nonzero_exit"
+            ) and _lane_work_provably_done(conn, row["id"]):
                 # Worker's task is still ``running`` but the lane's work is
                 # provably done (a prior completed run, or a PR URL /
-                # ready-for-review handoff in a recent comment). Two reap
+                # ready-for-review handoff in a recent comment). FOUR reap
                 # shapes reach here:
                 #   * ``clean_exit`` — rc=0 captured in the reap registry
-                #     (the exit the worker itself made), and
+                #     (the exit the worker itself made),
                 #   * ``unknown`` — the worker's exit was NOT captured (reaped
                 #     by init, or gone between the reap tick and this liveness
-                #     check), so it would otherwise fall into the generic
-                #     "pid not alive" → ``crashed`` branch below.
-                # Both are the completed-but-un-signposted exit —
-                # indistinguishable at the row level from a real crash /
-                # protocol violation, but NOT a failure: counting either trips
-                # a FALSE ``gave_up`` and strands a finished card (a draft-PR
-                # handoff, an edit-in-place card). Treat it as a benign no-op:
-                # release the task (a later tick / the respawn guard's
-                # recent_success / active_pr check handles it) and do NOT count
-                # a failure — mirroring the rate-limited carve-out below.
+                #     check), and
+                #   * ``signaled`` / ``nonzero_exit`` — a REAL captured worker
+                #     death (SIGKILL / OOM killer / non-zero rc): a silent
+                #     worker crash AFTER the lane work already landed.
+                # All four are indistinguishable at the row level from a
+                # failure the retry budget should count, but the durable proof
+                # says the deliverable IS on disk — so counting any of them
+                # trips a FALSE ``gave_up`` and strands finished work (a
+                # draft-PR handoff, an edit-in-place card). Crash-vs-clean-exit
+                # is orthogonal to whether the work landed: the SAME proof the
+                # clean-exit path trusts must reconcile a crashed card too,
+                # rather than discarding proven work. Treat it as a benign
+                # no-op: release the task (the respawn guard's recent_success /
+                # active_pr check defers a dup spawn) or, for a PR-open code
+                # card, auto-advance to review (below) — and do NOT count a
+                # failure, mirroring the rate-limited carve-out below.
+                #
+                # This is proof-GATED: with no proof, ``signaled`` /
+                # ``nonzero_exit`` fall through to the strict ``crashed`` branch
+                # unchanged, so a genuine crash with no deliverable STILL
+                # ``gave_up``. The ``_lane_work_provably_done`` shape predicate
+                # (``_card_requires_pr``) keeps a PR-requiring card held to a
+                # real PR / completed-run artifact, so the no-PR handoff-comment
+                # carve-out never leaks into the PR-backed crash path.
                 protocol_violation = False
                 clean_exit_after_done = True
-                _how = (
-                    "cleanly (rc=0)" if kind == "clean_exit"
-                    else "without a captured status (pid not alive)"
-                )
+                if kind == "clean_exit":
+                    _how = "cleanly (rc=0)"
+                elif kind == "signaled":
+                    _how = f"after a crash (killed by signal {code})"
+                elif kind == "nonzero_exit":
+                    _how = f"after a crash (exit code {code})"
+                else:
+                    _how = "without a captured status (pid not alive)"
                 error_text = (
                     f"pid {pid} exited {_how} after its lane work was "
                     f"provably done — benign no-op, not counted as a failure"
@@ -8327,19 +8345,38 @@ def detect_crashed_workers(conn: sqlite3.Connection) -> list[str]:
                     "exit_code": code,
                     "exit_kind": kind,
                 }
-                # PR-open code card: the provably-done signal is a PR handoff
-                # (a ``pull/<n>`` URL in a comment) AND the card's own owner map
-                # declares a ``review`` owner. Merely releasing this card to
-                # ``ready`` is a dead end — the ``active_pr`` respawn guard HOLDS
-                # an open-PR card out of respawn WITHOUT advancing it, so it
-                # wedges in ``running`` until an orchestrator hand-stages it. So
-                # MOVE it ``running -> review`` + the owner-map reviewer atomically
-                # with the reap (below), mirroring ``complete_task``'s canonical
-                # author-lane handoff. The no-PR edit-in-place shape (Proof 3) and
-                # a PR card with no resolvable reviewer are NOT auto-advanced:
-                # they fall through to the benign release-to-``ready`` unchanged,
-                # so this narrows to exactly the code-author-opened-a-PR case.
-                if _card_has_pr_artifact(conn, row["id"]):
+                # A provably-done card that would otherwise be DISCARDED
+                # (protocol_violation / crash → ``gave_up``) reconciles FORWARD
+                # to ``review`` + the owner-map reviewer instead of stranding —
+                # the deliverable is real but UNREVIEWED (never ``done``). Two
+                # provably-done shapes auto-advance:
+                #
+                #   * a PR-open code card (ANY reap kind) — the proof is a
+                #     ``pull/<n>`` URL in a comment (Proof 2). Merely releasing
+                #     it to ``ready`` is a dead end: the ``active_pr`` respawn
+                #     guard HOLDS an open-PR card out of respawn WITHOUT
+                #     advancing it, so it wedges in ``running`` → ``ready`` until
+                #     hand-staged. (#76 behavior — unchanged.)
+                #   * a no-PR edit-in-place card with a durable landed-work
+                #     handoff comment (Proof 3), but ONLY when the worker
+                #     CRASHED (``signaled`` / ``nonzero_exit`` / ``unknown``
+                #     pid-not-alive). On the CLEAN-EXIT path such a card releases
+                #     to ``ready`` unchanged (#75 behavior — a fresh dispatch
+                #     re-runs the idempotent edit, no review lane needed). But on
+                #     the CRASH path the retry budget is what strands it: after
+                #     ``failure_limit`` crashes it ``gave_up``s in ``blocked``
+                #     even though the deliverable landed. Advancing it to
+                #     ``review`` is the reconciliation the crash-vs-clean-exit
+                #     asymmetry demands.
+                #
+                # Either way the MOVE is gated on a resolvable owner-map
+                # ``review`` owner; without one there is no review destination,
+                # so the card falls through to the benign release-to-``ready``
+                # (the pre-existing #47 draft-PR carve-out) — never wedged,
+                # never crashed.
+                _has_pr = _card_has_pr_artifact(conn, row["id"])
+                _crashed_kind = kind in ("signaled", "nonzero_exit", "unknown")
+                if _has_pr or _crashed_kind:
                     advance_review_owner = _review_owner_from_owner_map(
                         conn, row["id"]
                     )

tests/hermes_cli/test_kanban_db.py

@@ -1866,6 +1866,196 @@ def test_pr_requiring_card_handoff_comment_is_not_third_proof(
         assert "protocol_violation" in kinds, kinds
 
 
+# ---------------------------------------------------------------------------
+# CRASH path (signaled / nonzero_exit) with landed-work proof.
+#
+# The two carve-outs above only fire for a reap classified ``clean_exit``
+# (rc=0 captured) or ``unknown`` (no reap record). A worker that dies a REAL
+# death whose exit IS captured — ``signaled`` (SIGKILL / OOM killer) or
+# ``nonzero_exit`` — falls into the generic ``crashed`` branch, which counts a
+# failure and, on retry-budget exhaustion, emits ``gave_up`` + strands the card
+# in ``blocked``. That branch NEVER consults ``_lane_work_provably_done``, so a
+# card whose lane work provably landed (a self-verified no-PR edit-in-place
+# handoff, or a draft-PR handoff) but whose worker then crashed is falsely
+# stranded — the exact ``t_e999ef95`` incident. Crash-vs-clean-exit is
+# orthogonal to whether the work landed: the SAME proof must reconcile a
+# crashed card to its owner-map REVIEW lane (deliverable is real but
+# UNREVIEWED — never ``done``). Absence of proof preserves the strict crash →
+# ``gave_up`` behavior exactly, and the no-PR carve-out must not leak into the
+# PR-requiring path.
+# ---------------------------------------------------------------------------
+
+
+def _stage_signaled_exit(conn, kb_mod, tid, pid, signal=9):
+    """Point ``tid`` at a dead host-local pid whose reap record is a SIGNAL.
+
+    ``_classify_worker_exit`` returns ``("signaled", <sig>)`` for a
+    ``WIFSIGNALED`` child — a real crash (SIGKILL, OOM killer). This is the
+    captured-crash shape the two prior carve-outs (clean_exit / unknown) do
+    NOT cover.
+    """
+    host = kb_mod._claimer_id().split(":", 1)[0]
+    kb.claim_task(conn, tid, claimer=f"{host}:w")
+    conn.execute(
+        "UPDATE tasks SET worker_pid = ?, consecutive_failures = 0 WHERE id = ?",
+        (pid, tid),
+    )
+    conn.commit()
+    # WIFSIGNALED raw status: the low 7 bits carry the signal number.
+    kb_mod._record_worker_exit(pid, signal)
+
+
+def test_crash_no_pr_edit_in_place_with_handoff_advances_to_review(
+    kanban_home, monkeypatch,
+):
+    """The reported ``t_e999ef95`` incident: a no-PR edit-in-place card whose
+    worker CRASHES (captured ``signaled`` death — a real silent worker death,
+    not a clean exit) AFTER posting a durable landed-work handoff comment must
+    reconcile to ``review`` + the owner-map reviewer, NOT ``gave_up``. The
+    deliverable is real but unreviewed, so it goes to review — never ``done``.
+    """
+    import hermes_cli.kanban_db as _kb
+
+    monkeypatch.setattr(_kb, "_pid_alive", lambda _pid: False)
+    monkeypatch.setenv("HERMES_KANBAN_CRASH_GRACE_SECONDS", "0")
+
+    with kb.connect() as conn:
+        # Default workspace_kind 'scratch' → a no-PR edit-in-place card.
+        tid = kb.create_task(conn, title="edit-in-place-crash", assignee="eckert")
+        _stamp_submit_owner_map(conn, tid, ready="eckert", review="lamport")
+        kb.add_comment(
+            conn, tid, "eckert",
+            "review-required: applied the config edit; validator green on disk",
+        )
+
+        _stage_signaled_exit(conn, _kb, tid, 70001)
+        crashed = kb.detect_crashed_workers(conn)
+
+        assert tid not in crashed, (
+            "a crashed no-PR card with a landed-work handoff must not be "
+            "treated as a crash"
+        )
+        task = kb.get_task(conn, tid)
+        assert task.status == "review", (
+            f"a crashed provably-done no-PR card must advance to review, "
+            f"got {task.status}"
+        )
+        assert task.assignee == "lamport", (
+            f"the card must be assigned to the owner-map reviewer, "
+            f"got {task.assignee}"
+        )
+        assert task.worker_pid is None
+        assert task.claim_lock is None
+        assert task.consecutive_failures == 0, (
+            "a proven crash reconcile must not increment the failure counter, "
+            f"got {task.consecutive_failures}"
+        )
+        events = kb.list_events(conn, tid)
+        kinds = [e.kind for e in events]
+        assert "crashed" not in kinds, kinds
+        assert "gave_up" not in kinds, kinds
+        sc = [
+            e for e in events
+            if e.kind == "status_changed"
+            and (e.payload or {}).get("to") == "review"
+        ]
+        assert sc, f"expected a status_changed ->review event, got {kinds}"
+        moved = sc[-1].payload or {}
+        assert moved.get("from") == "running", moved
+        assert moved.get("assignee") == "lamport", moved
+
+
+def test_crash_no_pr_card_without_proof_still_gives_up(
+    kanban_home, monkeypatch,
+):
+    """A no-PR card that CRASHES (captured ``signaled`` death) with NO
+    landed-work proof must STILL count a failure and, at the retry limit,
+    ``gave_up``. The crash-path carve-out is proof-gated: a genuine crash with
+    no deliverable cannot be masked.
+    """
+    import hermes_cli.kanban_db as _kb
+
+    monkeypatch.setattr(_kb, "_pid_alive", lambda _pid: False)
+    monkeypatch.setenv("HERMES_KANBAN_CRASH_GRACE_SECONDS", "0")
+
+    with kb.connect() as conn:
+        host = _kb._claimer_id().split(":", 1)[0]
+        tid = kb.create_task(conn, title="crash-no-proof", assignee="eckert")
+        # A chatty comment that is NOT a landed-work handoff must not count.
+        kb.add_comment(conn, tid, "eckert", "starting on this now")
+
+        for i in range(_kb.DEFAULT_FAILURE_LIMIT):
+            pid = 71000 + i
+            kb.claim_task(conn, tid, claimer=f"{host}:w{i}")
+            conn.execute(
+                "UPDATE tasks SET worker_pid = ? WHERE id = ?", (pid, tid),
+            )
+            conn.commit()
+            _kb._record_worker_exit(pid, 9)  # WIFSIGNALED, no proof → real crash
+            crashed = kb.detect_crashed_workers(conn)
+            assert tid in crashed, f"iter {i}: crashed no-proof card must crash"
+
+        task = kb.get_task(conn, tid)
+        assert task.status == "blocked", (
+            f"a repeatedly-crashing no-proof card must gave_up, got {task.status}"
+        )
+        kinds = [e.kind for e in kb.list_events(conn, tid)]
+        assert "crashed" in kinds, kinds
+        assert "gave_up" in kinds, kinds
+
+
+def test_crash_pr_requiring_card_with_handoff_no_pr_still_gives_up(
+    kanban_home, monkeypatch,
+):
+    """A PR-requiring worktree card that CRASHES with a ``review-required:``
+    handoff comment but NO PR URL and NO completed run must STILL ``gave_up`` —
+    the no-PR handoff-comment carve-out (Proof 3) is scoped by
+    ``_card_requires_pr`` and must not leak into the PR-backed crash path. A
+    worktree card is held to a real PR / completed-run artifact.
+    """
+    import hermes_cli.kanban_db as _kb
+
+    monkeypatch.setattr(_kb, "_pid_alive", lambda _pid: False)
+    monkeypatch.setenv("HERMES_KANBAN_CRASH_GRACE_SECONDS", "0")
+
+    with kb.connect() as conn:
+        host = _kb._claimer_id().split(":", 1)[0]
+        tid = kb.create_task(
+            conn, title="worktree-crash-no-pr", assignee="eckert",
+            workspace_kind="worktree",
+            workspace_path="/Users/caseywest/src/hermes-agent",
+        )
+        _stamp_submit_owner_map(conn, tid, ready="eckert", review="lamport")
+        # A handoff comment but NO PR URL — a PR-requiring card is not proven
+        # done by a comment alone.
+        kb.add_comment(
+            conn, tid, "eckert",
+            "review-required: implemented, tests green",
+        )
+
+        for i in range(_kb.DEFAULT_FAILURE_LIMIT):
+            pid = 72000 + i
+            kb.claim_task(conn, tid, claimer=f"{host}:w{i}")
+            conn.execute(
+                "UPDATE tasks SET worker_pid = ? WHERE id = ?", (pid, tid),
+            )
+            conn.commit()
+            _kb._record_worker_exit(pid, 9)  # WIFSIGNALED, no PR → real crash
+            crashed = kb.detect_crashed_workers(conn)
+            assert tid in crashed, (
+                f"iter {i}: PR-requiring card w/o a PR artifact must still crash"
+            )
+
+        task = kb.get_task(conn, tid)
+        assert task.status == "blocked", (
+            f"PR-requiring card w/o PR that crashes must gave_up, "
+            f"got {task.status}"
+        )
+        kinds = [e.kind for e in kb.list_events(conn, tid)]
+        assert "crashed" in kinds, kinds
+        assert "gave_up" in kinds, kinds
+
+
 # ---------------------------------------------------------------------------
 # Auto-advance a PR-open code card to review on clean-exit-after-done.
 #