🐛 fix(kanban): refuse complete_task -> done when a required PR artifact is absent

PATCHES.md

@@ -85,3 +85,4 @@ default; do **not** retire such a row on a PR-merge signal. See the #44338 row.
 | fork PR (TBD — no upstream PR; fork-internal review-lane semantics) | Provide a sanctioned recovery for a card whose `block_recurrences` loop counter is ALREADY inflated (follow-up to the block-loop-breaker row above). The breaker resets `block_recurrences` ONLY inside `complete_task`, so a card whose counter was inflated by prior buggy-code runs or an operator's repeated block→unblock while diagnosing had NO sanctioned way back to the normal flow — every `block`→`unblock` re-tripped straight to `triage` (live symptom 2026-07-05 on card `t_0d57d36d`, counter stuck at 5; the only escape was the non-obvious `move_card`-to-`blocked`-then-`unblock` workaround that bypasses `block_task`'s counter logic). Two sanctioned paths, both in `hermes_cli/kanban_db.py` + CLI, both keyed on the SAME PR #48 `_review_bounce_finding` classifier so the breaker is never weakened: (1) new core API `reset_block_recurrences(conn, id, *, actor, reason)` zeroes the counter and emits a `block_recurrences_reset` audit event (does NOT touch status/block_kind/claim state — a pure counter reset), surfaced as `hermes kanban unblock --reset-loop <id>` which resets FIRST (so a card already escalated to `triage`, where `unblock_task` does not apply, is still recovered) then attempts the normal unblock; (2) `auto_route_review_bounce` now resets the counter to 0 when a genuine author-rework transition routes the card back — i.e. when the current bounce's finding MATERIALLY DIFFERS from the prior round's (a fresh cycle), while a bounce repeating the IDENTICAL finding LEAVES the counter intact so a real same-unfixed-finding loop still escalates to triage. New helper `_prior_block_reason` returns the SECOND-most-recent `blocked` event reason (the router reads a card already re-blocked, so the newest `blocked` event is the CURRENT round; the prior round is the one before it — distinct from `block_task`'s use of `_last_block_reason`, which fires BEFORE the new block event is written). A plain `unblock` (no `--reset-loop`) is byte-for-byte unchanged: the counter deliberately survives an ordinary unblock (the existing anti-amnesia design). Tests: `tests/hermes_cli/test_kanban_reset_block_loop.py` (10 — reset zeroes + audits; missing-task no-op; already-zero still audits; inflated card recovers to `blocked` not `triage` after reset; same-finding loop STILL escalates; auto-route resets on different finding / preserves on same; CLI `--reset-loop` recovers a blocked card, recovers a triage card, and plain unblock leaves the counter). Real `kanban_db`, temp board, no mocks. **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, dropped only when the fork's block-loop-breaker / review-lane dispatch is retired. | permanent-local | main@9be292f1e |
 | fork PR (TBD — no upstream PR; fork-internal dispatcher semantics) | Fix the two SIBLING crash-classification paths #47 missed, so the kanban dispatcher stops false-flagging cleanly-finished and transiently-failed workers as `crashed`/`gave_up`. #47 (`426a8a9c7`) carved out the provably-done case for a reap-registry `clean_exit` (rc=0 captured), but two same-class paths in `detect_crashed_workers` (`hermes_cli/kanban_db.py`) still counted a false failure: **(A) transient endpoint-unreachable failures** — a run that died on `APIConnectionError` / connection-refused / provider 5xx (classified `timeout`/`overloaded`/`server_error`/`upstream_rate_limit` after in-process retries) exited plain `1`, which the reaper read as a real crash toward `failure_limit`; a proxy blip thus `gave_up` a card that would otherwise succeed. Fix: new `KANBAN_TRANSIENT_EXIT_CODE` (69, EX_UNAVAILABLE) sentinel + a single-source-of-truth `kanban_worker_exit_code(failure_reason)` helper the CLI kanban-worker exit path delegates to (replacing the inline rate-limit-only mapping); `_classify_worker_exit` maps 69 → a new `transient` kind; `detect_crashed_workers` treats it exactly like the rate-limit carve-out (requeue to `ready`/`review`, NO failure counted, distinct `transient` run outcome + event, stamped `last_failure_error` so the respawn-guard cooldown spaces the retry), surfaced via `DispatchResult.transient` + the `_last_transient` side-channel; `check_respawn_guard`'s cooldown gate now recognizes both `rate_limited` and `transient` outcomes. **(B) clean exit misread as a crash on the `pid not alive`/`unknown` reap path** — #47's provably-done carve-out was gated on `kind == "clean_exit"` only, so a worker that finished its lane cleanly (draft-PR handoff / edit-in-place card) but whose exit was NOT captured in the reap registry (reaped by init, or gone between the reap tick and the liveness check → `_classify_worker_exit` returns `unknown`) fell into the generic `crashed` branch and counted a failure. Fix: extend the `_lane_work_provably_done` carve-out from `kind == "clean_exit"` to `kind in ("clean_exit", "unknown")`, using the SAME durable proof signals (recent_success / active_pr) #47 trusts. Non-regression preserved: a genuine mid-work death with NO proof still `crashed`→`gave_up` after the limit, and a real non-zero crash (not the sentinel) still counts. Two-file change (`hermes_cli/kanban_db.py` + `cli.py`) + 8 behavior-contract tests in `tests/hermes_cli/test_kanban_db.py` (transient-sentinel classify; transient requeue-without-failure across 6 hits; transient cooldown defer/allow; unknown-exit-after-completed-run and unknown-exit-after-draft-PR not counted; unknown-exit-without-proof still crashes; real-crash non-regression; exit-code mapping). **Retire trigger:** open the upstream PR, then auto-retire when it merges in a tagged release ≥ base; until then carry as upstream-pending. | upstream-pending | main@9be292f1e |
 | fork PR (TBD — no upstream PR; fork-internal review-lane semantics) | Make `unblock_task` also transition a card out of `triage`, so the OUTER feedback loop (Casey-feedback -> author on a card carrying an open PR) self-heals the `active_pr` respawn-guard wedge that only the INNER review-bounce loop was covered for (composes with rows 84/85 -- the block-loop breaker + reset-recovery -- extending them to the non-review author-bounce case). Live symptom 2026-07-11: a writing card (`t_f6126dcd`, author orwell) carrying an OPEN PR needed to bounce back to its author for a Casey-feedback revision; the hand-move `blocked->todo`+author did not clear the `active_pr` guard (a raw status flip emits no `unblocked` cutoff event), so `hermes kanban dispatch` refused to spawn the author every tick (`respawn_guarded {active_pr}`). Repeated churn inflated `block_recurrences` past `BLOCK_RECURRENCE_LIMIT`, escalating the card to `triage`, where the standard block->unblock cutoff SILENTLY no-oped -- `unblock_task` (`hermes_cli/kanban_db.py`) matched only `status IN ('blocked','scheduled')`, so a triage card matched zero rows, returned False, emitted no `unblocked` event, and the guard stayed tripped forever (only a hand `reset_block_recurrences` + status=blocked + `unblock_task` rescued it). Root cause: the INNER review loop self-heals via `auto_route_review_bounce` (reassigns + `unblock_task`, emitting the `unblocked` cutoff that `check_respawn_guard` honors), but the outer feedback loop has no `review-changes-requested` reason so it is never routed, and the only skill-layer primitive (`onecard_common.move_card`) is a raw status flip that emits no cutoff. Fix (minimal, well-scoped): add `'triage'` to both `WHERE status IN (...)` clauses in `unblock_task` -- the stale-run-pointer SELECT and the status UPDATE -- so a card escalated to `triage` transitions back to `ready`/`todo` (parent-gate re-checked) and emits the `unblocked` cutoff, clearing `active_pr` the same way a normal block->unblock does. Preserves the loop breaker: `block_recurrences` is deliberately NOT reset (counter still survives the unblock; reset only on completion or the explicit `reset_block_recurrences` API), so a genuine same-finding loop still escalates. This supersedes row 85's parenthetical about a card already escalated to `triage` where `unblock_task` did not apply -- with this patch `unblock --reset-loop` fully recovers a triaged card (reset + lane flip). Single-file change in `hermes_cli/kanban_db.py` (+ stale-comment sync in `hermes_cli/kanban.py`) + 5 behavior-contract regression tests in `tests/hermes_cli/test_kanban_reset_block_loop.py` (RED-then-GREEN reproducing the exact triage-with-open-PR wedge; parent-gate re-check; counter preserved; the pre-fix wedge documented). Clean upstream candidate -- a sanctioned unblock that no-ops from triage is a real bug -- but tracked fork-internal for now. **Retire trigger:** open the upstream PR, then auto-retire when it merges in a tagged release >= base; until then carry as permanent-local. | permanent-local | main@9be292f1e |
+| fork PR (TBD — no upstream PR; fork-internal completion-lane semantics) | Add a **required-artifact completion guard** to `complete_task` (`hermes_cli/kanban_db.py`) — the third false-`done` guard, beside the phantom-`created_cards` and acceptance-lane guards, in the same before-write-txn shape. Root cause (live 2026-07-08, card `t_e3e5bc16`): a worker can build its deliverable in a git worktree but exit WITHOUT committing / pushing / opening a PR, and its `kanban_complete` still flips the card to `done`; that card wrote three research files into its worktree, opened no PR, completed anyway, and its gated children auto-promoted onto a foundation that did not exist (one began running before it was caught). `done` MUST mean "the declared reviewable artifact exists"; a `done` card with no PR is a false state, and no guard covered "the card's own contract required a PR and none exists." Fix (opt-in per card, derived from the workspace — zero schema footprint, no `requires_pr` column, no migration, existing cards classified correctly with no backfill): a new `_card_requires_pr(workspace_kind, workspace_path)` returns True only for `workspace_kind == 'worktree'` (the sole kind that materializes an isolated linked git worktree on a `topic/*` branch via `_ensure_git_worktree` — precisely the implementer→PR shape; `branch_name` is worktree-only) whose `workspace_path` is NOT anchored under any `~/.hermes` tree (`get_default_hermes_root()` and `$HOME/.hermes` both excluded, so edit-in-place config / live-install worktrees stay completable); `scratch` (throwaway tmp dir) and `dir` (plain shared directory — persistent build dirs, `cwest/hermes-config` edits) are NOT PR-requiring and complete exactly as before. The artifact check `_card_has_pr_artifact(conn, id)` reuses the existing PR→card linkage — a resolvable `pull/<n>` URL in any task comment (the implementer's ready-for-review handoff), matched by the SAME `_RESPAWN_GUARD_PR_URL_RE` the `active_pr` respawn guard and `_lane_work_provably_done` already trust (extend, not duplicate). For a guarded card with no PR, `complete_task` is a clean no-op refusal (returns `False`, no task-state mutation) that emits an auditable `completion_refused_missing_pr` event with a `summary_preview`, mirroring `completion_refused_acceptance`. Casey's merge path (`allow_acceptance_complete=True`, the `github-pr-closed` webhook) BYPASSES the guard exactly as it bypasses the acceptance guard. The guard lives inside `complete_task` — the single chokepoint every completion path calls (worker `kanban_complete`, `hermes kanban complete`, the swarm root helper, the dashboard) — so the whole class is covered by construction. Tests: `tests/hermes_cli/test_kanban_complete_missing_pr_guard.py` (7 behavior-contract cases against a real temp kanban DB — worktree+no-PR refused, refusal emits the audit event + no `completed` event, `dir`/`scratch` cards complete with no PR, worktree+PR-comment completes, `~/.hermes` worktree completes, merge-override completes without a PR). Full `test_kanban_db.py` (271) + the kanban surface green; no regressions. **Retire trigger:** never auto-retires on an upstream PR-merge signal (no upstream PR — fork-internal completion-lane invariant); remove only if the fork's one-card completion discipline is retired. | permanent-local | main@9be292f1e |

hermes_cli/kanban_db.py

@@ -4857,6 +4857,93 @@ def _scan_prose_for_phantom_ids(
     return [m for m in unique if m not in existing]
 
 
+# The workspace kind that means "this card builds in an isolated git worktree"
+# (a branch cut for feature work) and therefore owes a reviewable PR before it
+# can be ``done``. ONLY ``worktree`` qualifies: it materializes a real linked
+# git worktree on a ``topic/*`` branch (see ``_ensure_git_worktree`` /
+# ``branch_name`` is worktree-only) — the exact shape the implementer→PR flow
+# uses. ``scratch`` (throwaway tmp dir) and ``dir`` (a plain shared directory —
+# e.g. a persistent build dir, or a ``~/.hermes`` config / live-install edit)
+# are NOT PR-backed and legitimately complete with no PR, so they are excluded.
+_PR_REQUIRING_WORKSPACE_KINDS = frozenset({"worktree"})
+
+
+def _card_requires_pr(workspace_kind: Optional[str], workspace_path: Optional[str]) -> bool:
+    """Return True when a card's own contract requires a reviewable PR artifact.
+
+    The signal is derived from the card's workspace (zero schema footprint,
+    no per-card flag): a card requires a PR when it builds in an isolated git
+    worktree — ``workspace_kind == 'worktree'`` with a ``workspace_path`` that
+    is NOT under ``~/.hermes``. That is precisely the implementer→PR shape (a
+    linked git worktree on a ``topic/*`` branch). Edit-in-place / shared-dir
+    cards (``scratch`` and ``dir`` kinds — throwaway build dirs, or a
+    ``~/.hermes`` workdir for config / live-install edits, e.g.
+    ``cwest/hermes-config`` homestead work) are NOT PR-requiring and complete
+    exactly as before.
+
+    Keyed on the workspace rather than a stored ``requires_pr`` column so the
+    guard reads the semantic that already exists ("this card has a git
+    worktree to open a PR from") without a migration, and so existing cards
+    are classified correctly with no backfill. The ``~/.hermes`` path carve-out
+    is belt-and-suspenders: a ``worktree`` card is never anchored under
+    ``~/.hermes`` in practice, but excluding it keeps any future edit-in-place
+    worktree completable.
+    """
+    if (workspace_kind or "scratch") not in _PR_REQUIRING_WORKSPACE_KINDS:
+        return False
+    path = (workspace_path or "").strip()
+    if not path:
+        # A worktree/dir card with no resolved path is not a real repo
+        # worktree we can hold to a PR — treat as not-PR-requiring rather
+        # than trapping it uncompletable.
+        return False
+    try:
+        resolved = Path(path).expanduser()
+    except Exception:
+        return False
+    # Exclude edit-in-place workspaces anchored under a ~/.hermes tree
+    # (config / live-install edits — ``cwest/hermes-config`` homestead work
+    # — correctly complete with no PR). Check both the shared Hermes root
+    # (``get_default_hermes_root()``, which honours HERMES_HOME for Docker /
+    # custom deployments) and the canonical ``~/.hermes`` under $HOME, so a
+    # profile-scoped or non-default home still classifies correctly.
+    hermes_roots: list[Path] = []
+    try:
+        from hermes_constants import get_default_hermes_root
+        hermes_roots.append(Path(get_default_hermes_root()))
+    except Exception:
+        pass
+    try:
+        hermes_roots.append(Path.home() / ".hermes")
+    except Exception:
+        pass
+    for root in hermes_roots:
+        try:
+            resolved.relative_to(root)
+            return False
+        except ValueError:
+            continue
+    return True
+
+
+def _card_has_pr_artifact(conn: sqlite3.Connection, task_id: str) -> bool:
+    """Return True when the card carries a resolvable GitHub PR reference.
+
+    Reuses the existing PR->card linkage idiom: a ``pull/<n>`` URL in any task
+    comment (the ready-for-review handoff the implementer lane posts on PR
+    open), matched by the same :data:`_RESPAWN_GUARD_PR_URL_RE` the
+    ``active_pr`` respawn guard and :func:`_lane_work_provably_done` already
+    trust as durable proof of a landed lane. Extend, don't duplicate — the
+    guard and the respawn logic agree on what "has a PR" means.
+    """
+    for row in conn.execute(
+        "SELECT body FROM task_comments WHERE task_id = ?", (task_id,)
+    ).fetchall():
+        if row["body"] and _RESPAWN_GUARD_PR_URL_RE.search(row["body"]):
+            return True
+    return False
+
+
 class HallucinatedCardsError(ValueError):
     """Raised by ``complete_task`` when ``created_cards`` contains ids
     that don't exist or weren't created by the completing worker.
@@ -4994,6 +5081,50 @@ def complete_task(
                     )
                 return False
 
+    # Required-artifact guard: refuse to complete a card whose own contract
+    # required a reviewable PR when no PR artifact exists. ``done`` must mean
+    # "the declared reviewable artifact exists" — a worker that builds in a git
+    # worktree but exits WITHOUT committing / pushing / opening a PR (then calls
+    # kanban_complete) would otherwise flip the card to ``done`` on a PR that was
+    # never opened, and gated children auto-promote onto a foundation that does
+    # not exist (the live 2026-07-08 failure). Opt-in per card: only cards that
+    # build in a real git repo worktree (``_card_requires_pr``) are guarded, so
+    # edit-in-place / no-PR cards (scratch, or a ~/.hermes workdir) complete
+    # exactly as before. The artifact is a resolvable ``pull/<n>`` URL in a task
+    # comment (``_card_has_pr_artifact``, reusing the active_pr linkage). Runs
+    # before the write txn — a rejected completion never mutates task state,
+    # exactly like the two guards above — and emits an auditable
+    # ``completion_refused_missing_pr`` event. Casey's merge path
+    # (``allow_acceptance_complete=True``) bypasses this guard, as it does the
+    # acceptance guard.
+    #
+    # ORDER: this guard runs BEFORE the author-lane→review redirect below. A
+    # PR-requiring card in the author lane with no PR must be REFUSED, not
+    # silently shunted into review — the missing-PR refusal is the stricter gate
+    # and takes precedence over the redirect.
+    if not allow_acceptance_complete:
+        _ws_row = conn.execute(
+            "SELECT workspace_kind, workspace_path FROM tasks WHERE id = ?",
+            (task_id,),
+        ).fetchone()
+        if _ws_row is not None and _card_requires_pr(
+            _ws_row["workspace_kind"], _ws_row["workspace_path"]
+        ) and not _card_has_pr_artifact(conn, task_id):
+            with write_txn(conn):
+                _append_event(
+                    conn, task_id, "completion_refused_missing_pr",
+                    {
+                        "workspace_kind": _ws_row["workspace_kind"],
+                        "workspace_path": _ws_row["workspace_path"],
+                        "summary_preview": (
+                            (summary or result or "").strip().splitlines()[0][:200]
+                            if (summary or result)
+                            else None
+                        ),
+                    },
+                )
+            return False
+
     # Author-lane redirect: an AUTHOR finishing their lane MOVES the card to the
     # REVIEW lane, it does NOT go straight to ``done``. ``done`` means "Casey
     # merged/accepted"; an author's end-of-lane completion is not that. Code cards