diff --git a/PATCHES.md b/PATCHES.md index cd2d853eb916..b0821c241c09 100644 --- a/PATCHES.md +++ b/PATCHES.md @@ -73,3 +73,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) | 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 | +| fork PR (TBD — no upstream PR; fork-internal review-lane semantics) | Auto-route a reviewer's `review-changes-requested` block back to the original author from the housekeeping tick, closing the reviewer→author hop the GitHub `pull_request_review` webhook cannot close when reviewer and author share one GitHub identity. The reviewer (lamport) runs as the `cwest` identity that OWNS the team PRs, so GitHub rejects his `REQUEST_CHANGES` with HTTP 422 and he falls back to a `COMMENT` event — which is NOT `changes_requested`, so the webhook router never bounces and the card sits `blocked` until a human hand-routes it. The fix is board-internal and lives entirely in the dispatcher, NOT the reviewer (the reviewer's terminal action stays a clean `kanban_block`, preserving the lane-corruption-safe design): `auto_route_review_bounce` scans `blocked` cards on each `dispatch_once` tick (before `recompute_ready`), and for any card whose most-recent sticky `blocked` event carries the `review-changes-requested` reason prefix, reassigns it to the original author (resolved from the `assigned` event history — the `from` of the move whose `to` is the current reviewer — never a literal profile name) and unblocks it via the existing `unblock_task`, which clears the `active_pr`/`recent_success` respawn guards exactly like a manual block→unblock cutoff, plus a `dispatcher`-authored `[audit]` comment naming the PR and verdict gist. Idempotent (the route flips the card off `blocked`, so a later tick won't re-fire; two ticks → one route, one comment). The `awaiting-casey-signoff` PASS/acceptance block is excluded by the prefix match (must stay `blocked`+casey), as are non-review and circuit-breaker (`gave_up`) blocks. Two `check_respawn_guard` carve-outs support the route: the `recent_success` guard now honors a trailing `unblocked` event (the bounced build run is the work being reworked — it must not veto respawn), and the dup-PR scan excludes the dispatcher's own same-second audit comment. Toggle `kanban.auto_route_review_bounce` (default ON) gates the whole path; wired through `dispatch_once`, the `hermes kanban dispatch` CLI, and the gateway dispatcher watcher. Changes in `hermes_cli/kanban_db.py`, `hermes_cli/kanban.py`, `gateway/kanban_watchers.py` + 8 regression tests in `tests/hermes_cli/test_kanban_auto_route_review_bounce.py` (core route; route via real `dispatch_once`; acceptance block does NOT route; idempotency; routed card dispatchable; non-review block; circuit-breaker block; toggle off). Upstream has no equivalent (no `review-changes-requested`/`auto_route`/`bounce` concept in upstream `hermes_cli/*.py`) — cwest-team review-loop tooling. **Retire trigger:** never auto-retires (permanent-local); remove only if the fork's review-lane dispatch is retired. | permanent-local | v2026.6.19 | diff --git a/gateway/kanban_watchers.py b/gateway/kanban_watchers.py index a16007074ab8..3d1b88787d93 100644 --- a/gateway/kanban_watchers.py +++ b/gateway/kanban_watchers.py @@ -834,6 +834,16 @@ async def _kanban_dispatcher_watcher(self) -> None: max_in_progress_per_profile, ) + # Read kanban.auto_route_review_bounce — when a reviewer terminates a + # review with a clean review-changes-requested block, the housekeeping + # tick routes that card back to the original author (closes the + # reviewer→author hop the GitHub webhook can't when reviewer and author + # share one identity). Defaults ON; set false to keep bounce blocks + # parked for a human to route. + auto_route_review_bounce_enabled = bool( + kanban_cfg.get("auto_route_review_bounce", True) + ) + # Initial delay so the gateway finishes wiring adapters before the # dispatcher spawns workers (those workers may hit gateway notify # subscriptions etc.). Matches the notifier watcher's delay. @@ -927,6 +937,7 @@ def _tick_once_for_board(slug: str) -> "Optional[object]": stale_timeout_seconds=stale_timeout_seconds, default_assignee=default_assignee, max_in_progress_per_profile=max_in_progress_per_profile, + auto_route_review_bounce_enabled=auto_route_review_bounce_enabled, ) except sqlite3.DatabaseError as exc: if _is_corrupt_board_db_error(exc): diff --git a/hermes_cli/kanban.py b/hermes_cli/kanban.py index 31c4bf68ae85..c7402cd28a1f 100644 --- a/hermes_cli/kanban.py +++ b/hermes_cli/kanban.py @@ -2126,6 +2126,9 @@ def _coerce_positive_int(value): _kanban_cfg.get("max_in_progress_per_profile") ) max_in_progress = _coerce_positive_int(_kanban_cfg.get("max_in_progress")) + auto_route_review_bounce_enabled = bool( + _kanban_cfg.get("auto_route_review_bounce", True) + ) # CLI --max overrides config kanban.max_spawn when both are present; # CLI is the more explicit signal so it wins. cli_max = getattr(args, "max", None) @@ -2136,6 +2139,7 @@ def _coerce_positive_int(value): default_assignee = None max_in_progress_per_profile = None max_in_progress = None + auto_route_review_bounce_enabled = True max_spawn = getattr(args, "max", None) with kb.connect_closing() as conn: res = kb.dispatch_once( @@ -2146,6 +2150,7 @@ def _coerce_positive_int(value): failure_limit=getattr(args, "failure_limit", kb.DEFAULT_SPAWN_FAILURE_LIMIT), default_assignee=default_assignee, max_in_progress_per_profile=max_in_progress_per_profile, + auto_route_review_bounce_enabled=auto_route_review_bounce_enabled, ) if getattr(args, "json", False): print(json.dumps({ @@ -2155,6 +2160,7 @@ def _coerce_positive_int(value): "stale": res.stale, "auto_blocked": res.auto_blocked, "promoted": res.promoted, + "routed_review_bounce": res.routed_review_bounce, "spawned": [ {"task_id": tid, "assignee": who, "workspace": ws} for (tid, who, ws) in res.spawned diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index e646996d07c6..adb7d4bf2a87 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -2971,6 +2971,186 @@ def _has_sticky_block(conn: sqlite3.Connection, task_id: str) -> bool: return bool(row) and row["kind"] == "blocked" +# The reviewer's clean-block contract for a review-bounce (the sdlc-review skill +# emits ``kanban_block(reason="review-changes-requested: ; see PR . …")`` +# when a review needs rework). The dispatcher routes ONLY this prefix back to the +# author. The acceptance/PASS block (``awaiting-casey-signoff: …``) is parked for +# Casey and must NEVER auto-route — matching on this prefix excludes it by design. +_REVIEW_BOUNCE_REASON_PREFIX = "review-changes-requested" + + +def _latest_sticky_block_reason( + conn: sqlite3.Connection, task_id: str, +) -> Optional[str]: + """Return the reason of the most recent sticky ``blocked`` event, or None. + + Sticky == the most recent ``{blocked, unblocked}`` event is a ``blocked`` + (the same predicate :func:`_has_sticky_block` uses). A circuit-breaker block + emits ``gave_up`` (not ``blocked``) and therefore has no reason here. + """ + if not _has_sticky_block(conn, task_id): + return None + row = conn.execute( + "SELECT payload FROM task_events " + "WHERE task_id = ? AND kind = 'blocked' " + "ORDER BY id DESC LIMIT 1", + (task_id,), + ).fetchone() + if not row or not row["payload"]: + return None + try: + payload = json.loads(row["payload"]) + except (ValueError, TypeError): + return None + reason = payload.get("reason") if isinstance(payload, dict) else None + return reason if isinstance(reason, str) else None + + +def _is_review_bounce_reason(reason: Optional[str]) -> bool: + """True iff ``reason`` is the reviewer's review-changes-requested bounce. + + Matched on the ``review-changes-requested`` prefix (leading whitespace + tolerated). Deliberately does NOT match ``awaiting-casey-signoff`` (the PASS + acceptance block) nor any other block reason. + """ + return bool(reason) and reason.lstrip().startswith(_REVIEW_BOUNCE_REASON_PREFIX) + + +def _resolve_review_author( + conn: sqlite3.Connection, task_id: str, *, reviewer: Optional[str] = None, +) -> Optional[str]: + """Resolve the original author a review-bounce card should route back to. + + The card reached ``review`` via a MOVE that emitted an ``assigned`` event + carrying ``{"from": , "to": }`` (see ``onecard.move_card``). + The author is the ``from`` of the most recent move whose ``to`` is the current + reviewer (the build→review hop) — keying on ``to == reviewer`` ignores this + router's OWN later ``assigned`` event (``from=reviewer, to=author``), so a + re-block→re-route loop can never resolve the author back to the reviewer. + Falls back to the most recent ``from``-bearing move whose ``from`` differs + from ``to`` when the reviewer is unknown. Resolving from event history — never + a literal profile name baked into core — keeps this consistent with the + existing ``bounce_review_to_author`` path. Returns None when unresolvable (so + the caller leaves the card for a human rather than guessing). + """ + rows = conn.execute( + "SELECT payload FROM task_events " + "WHERE task_id = ? AND kind = 'assigned' " + "ORDER BY id DESC", + (task_id,), + ).fetchall() + + def _move(payload_str): + if not payload_str: + return None, None + try: + payload = json.loads(payload_str) + except (ValueError, TypeError): + return None, None + if not isinstance(payload, dict): + return None, None + frm = payload.get("from") + to = payload.get("to") + frm = frm.strip() if isinstance(frm, str) and frm.strip() else None + to = to.strip() if isinstance(to, str) and to.strip() else None + return frm, to + + # Preferred: the move INTO the reviewer's hands (to == current reviewer). + if reviewer: + for row in rows: + frm, to = _move(row["payload"]) + if frm and to == reviewer and frm != reviewer: + return frm + # Fallback: the most recent genuine reassignment (from != to). + for row in rows: + frm, to = _move(row["payload"]) + if frm and frm != to: + return frm + return None + + +def auto_route_review_bounce( + conn: sqlite3.Connection, *, enabled: bool = True, +) -> int: + """Auto-route reviewer ``review-changes-requested`` blocks back to the author. + + Closes the reviewer→author hop the GitHub ``pull_request_review`` webhook + cannot close when reviewer and author share one GitHub identity (the reviewer + falls back to a ``COMMENT`` event, which is not ``changes_requested``, so the + webhook router never bounces). The fix is board-internal: on the housekeeping + tick, a ``blocked`` card whose most-recent sticky block carries the + ``review-changes-requested`` reason is transitioned ``blocked → ready`` + + assignee = the original author, with an audit comment. + + The reviewer's terminal action stays a clean ``kanban_block`` — this function + does the routing in the dispatcher, never the reviewer (which would reintroduce + the lane-corruption risk the clean-block design avoids). + + Idempotency (load-bearing): the route flips the card off ``blocked``, so the + detector naturally won't re-fire for the same block on a later tick — two + consecutive ticks produce exactly one route and one audit comment. + + The transition reuses :func:`unblock_task` so the routed card clears the same + respawn guards (``active_pr`` / ``recent_success``) a manual block→unblock + cutoff clears — the card is genuinely dispatchable, not a ``ready`` card the + guard skips. The audit comment is authored as ``dispatcher`` (a distinct + identity from the reviewer/author) and names the PR + verdict gist. + + Returns the number of cards routed this call. ``enabled=False`` (config + ``kanban.auto_route_review_bounce: false``) makes it a no-op. + """ + if not enabled: + return 0 + routed = 0 + blocked_rows = conn.execute( + "SELECT id, assignee FROM tasks WHERE status = 'blocked'" + ).fetchall() + for row in blocked_rows: + task_id = row["id"] + reason = _latest_sticky_block_reason(conn, task_id) + if not _is_review_bounce_reason(reason): + continue + author = _resolve_review_author(conn, task_id, reviewer=row["assignee"]) + if not author or author == row["assignee"]: + # Unresolvable author, or the card is already assigned to the author + # (nothing to route) — leave it for a human rather than guessing. + continue + # Reassign to the author, then unblock. unblock_task emits the + # ``unblocked`` cutoff event (clears the active_pr guard) and resets + # consecutive_failures, exactly like a manual block→unblock cutoff. + with write_txn(conn): + upd = conn.execute( + "UPDATE tasks SET assignee = ? WHERE id = ? AND status = 'blocked'", + (author, task_id), + ) + if upd.rowcount != 1: + # Card changed status between the scan and here — skip. + continue + _append_event( + conn, task_id, "assigned", + {"from": row["assignee"], "to": author, "by": "dispatcher:auto-route"}, + ) + if not unblock_task(conn, task_id): + continue + # Audit comment recording the auto-route, authored as the dispatcher so + # the trail attributes the action to a distinct identity. Mirrors the + # §9.1 ``[audit]`` shape used by the one-card move helpers. + gist = (reason or "").strip() + pr_match = _RESPAWN_GUARD_PR_URL_RE.search(gist) + pr = pr_match.group(0) if pr_match else None + body_lines = ["[audit] actor=dispatcher stage=rework"] + if pr: + body_lines.append(f"pr={pr}") + body_lines.append(f"notes: auto-routed review-changes-requested bounce back " + f"to author {author}; {gist}") + try: + add_comment(conn, task_id, author="dispatcher", body="\n".join(body_lines)) + except ValueError: + pass + routed += 1 + return routed + + def recompute_ready( conn: sqlite3.Connection, failure_limit: int = None, ) -> int: @@ -4967,6 +5147,9 @@ class DispatchResult: reclaimed: int = 0 promoted: int = 0 + routed_review_bounce: int = 0 + """Count of review-changes-requested blocks auto-routed back to their author + this tick (the reviewer→author hop). See :func:`auto_route_review_bounce`.""" spawned: list[tuple[str, str, str]] = field(default_factory=list) """List of ``(task_id, assignee, workspace_path)`` triples.""" skipped_unassigned: list[str] = field(default_factory=list) @@ -6259,11 +6442,27 @@ def check_respawn_guard(conn: sqlite3.Connection, task_id: str) -> Optional[str] # Skipped for review tasks: the build run that produced the artifact # under review is itself a recent ``completed`` run, which would # otherwise block the reviewer from ever spawning. + # Honors an explicit unblock the same way the active_pr guard does + # (below): when a card is deliberately unblocked to resume work (e.g. a + # review-changes-requested bounce auto-routed back to the author, or any + # operator block→unblock cutoff), the build run that triggered the + # bounce/block is the very work being reworked — it must not veto the + # respawn. An unblock causally FOLLOWS the completion that triggered the + # review (complete → move-to-review → reviewer block → unblock), so a + # completed run is "superseded" by any ``unblocked`` event recorded at or + # after that run's completion (``created_at >= ended_at`` — second-granular, + # so a same-second unblock clears it, matching the causal order). A run + # with no such trailing unblock still guards (the normal post-build case). if not is_review: cutoff = now - _RESPAWN_GUARD_SUCCESS_WINDOW if conn.execute( - "SELECT id FROM task_runs " - "WHERE task_id = ? AND outcome = 'completed' AND ended_at >= ?", + "SELECT r.id FROM task_runs r " + "WHERE r.task_id = ? AND r.outcome = 'completed' AND r.ended_at >= ? " + "AND NOT EXISTS (" + " SELECT 1 FROM task_events e " + " WHERE e.task_id = r.task_id AND e.kind = 'unblocked' " + " AND e.created_at >= r.ended_at" + ")", (task_id, cutoff), ).fetchone(): return "recent_success" @@ -6276,6 +6475,13 @@ def check_respawn_guard(conn: sqlite3.Connection, task_id: str) -> Optional[str] # addressing review feedback) and only guard on PR URLs added at or after # that unblock. Timestamps are second-granular, so a same-second PR URL is # still guarded conservatively. (Carried from upstream PR #46204.) + # The dispatcher's OWN auto-route audit comment (authored ``dispatcher``) + # records the EXISTING PR being reworked — it is posted in the same second + # as the routing unblock, so the same-second-conservative rule above would + # otherwise re-trip the guard on the dispatcher's own audit. The dispatcher + # never opens a PR, so its audit is excluded from the dup-PR scan (analogous + # to the review-status carve-out): a real builder/worker PR comment still + # guards normally. if not is_review: pr_cutoff = now - _RESPAWN_GUARD_PR_WINDOW latest_unblock = conn.execute( @@ -6286,7 +6492,8 @@ def check_respawn_guard(conn: sqlite3.Connection, task_id: str) -> Optional[str] if latest_unblock and latest_unblock["ts"] is not None: pr_cutoff = max(pr_cutoff, int(latest_unblock["ts"])) for c in conn.execute( - "SELECT body FROM task_comments WHERE task_id = ? AND created_at >= ?", + "SELECT body FROM task_comments " + "WHERE task_id = ? AND created_at >= ? AND author != 'dispatcher'", (task_id, pr_cutoff), ).fetchall(): if c["body"] and _RESPAWN_GUARD_PR_URL_RE.search(c["body"]): @@ -6365,6 +6572,7 @@ def dispatch_once( board: Optional[str] = None, default_assignee: Optional[str] = None, max_in_progress_per_profile: Optional[int] = None, + auto_route_review_bounce_enabled: bool = True, ) -> DispatchResult: """Run one dispatcher tick. @@ -6421,6 +6629,13 @@ def dispatch_once( if _crash_rate_limited: result.rate_limited.extend(_crash_rate_limited) result.timed_out = enforce_max_runtime(conn) + # Auto-route reviewer review-changes-requested blocks back to the author + # (close the reviewer→author hop the GitHub webhook can't when reviewer and + # author share one identity). Runs BEFORE recompute_ready so a routed card — + # left in ``ready`` by unblock_task — is eligible to spawn this same tick. + result.routed_review_bounce = auto_route_review_bounce( + conn, enabled=auto_route_review_bounce_enabled, + ) result.promoted = recompute_ready(conn, failure_limit=failure_limit) # Count tasks already running so max_spawn enforces concurrency rather diff --git a/tests/hermes_cli/test_kanban_auto_route_review_bounce.py b/tests/hermes_cli/test_kanban_auto_route_review_bounce.py new file mode 100644 index 000000000000..43499e3167fa --- /dev/null +++ b/tests/hermes_cli/test_kanban_auto_route_review_bounce.py @@ -0,0 +1,258 @@ +"""Auto-route a review-bounce block back to the author (close the reviewer->author hop). + +Card t_cec2251c. The review loop is asymmetric: the author advances his own card +into ``review`` + reviewer, but the reviewer's only sanctioned terminal action on a +bounce is a clean ``kanban_block(reason="review-changes-requested: ...")`` -- he is +forbidden from reassigning (that historically corrupted the lane). The intended +automation that closes the gap (the ``bounce-review-to-author`` GitHub webhook) +cannot fire when reviewer and author share one GitHub identity (a ``COMMENT`` event, +not ``changes_requested``, so the router never bounces). The card then sits +``blocked`` until a human hand-routes it. + +These tests pin the board-internal fix: on the housekeeping tick, a ``blocked`` card +whose most-recent sticky block carries the ``review-changes-requested`` contract is +auto-routed ``blocked -> ready`` + assignee = the original author, with an audit +comment, exactly once, and genuinely dispatchable (respawn guards cleared). The +acceptance block (``awaiting-casey-signoff``), a non-review block, and a card with +the feature toggled off must all stay put. +""" + +from __future__ import annotations + +import time +from pathlib import Path + +import pytest + +from hermes_cli import kanban_db as kb + + +@pytest.fixture +def kanban_home(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """Isolated HERMES_HOME with an empty kanban DB.""" + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + kb.init_db() + return home + + +# Mirrors the live reviewer contract emitted by the sdlc-review skill on a bounce: +# kanban_block(reason="review-changes-requested: ; see PR . ...") +_BOUNCE_REASON = ( + "review-changes-requested: PATCHES.md:75 row bucketed upstream-pending but " + "should be permanent-local; see https://github.com/cwest/hermes-agent/pull/71. " + "Author to rework on the same branch/PR and resolve every open thread before " + "re-review." +) +# The acceptance (PASS) block -- parked for Casey, must NOT route to the author. +_SIGNOFF_REASON = ( + "awaiting-casey-signoff: reviewed PASS — " + "https://github.com/cwest/hermes-agent/pull/71; threads resolved; 240 tests " + "green. Ready to merge." +) + + +def _stage_review_bounce(conn, *, author: str = "eckert", reviewer: str = "lamport", + reason: str = _BOUNCE_REASON) -> str: + """Reproduce the live flow up to (and including) the reviewer's terminal block. + + Author builds + opens PR -> card MOVES to ``review`` + reviewer (the ``assigned`` + event carries ``from=author, to=reviewer``) -> reviewer claims (``source_status: + review``) -> reviewer emits the clean bounce ``kanban_block``. Returns the card id + left in ``blocked`` + reviewer, exactly as the dispatcher would see it. + """ + tid = kb.create_task(conn, title="feature work", assignee=author) + # Author's build run. + kb.claim_task(conn, tid) + kb.complete_task(conn, tid, result="PR opened: https://github.com/cwest/hermes-agent/pull/71") + # Card MOVES to review + reviewer (mirrors onecard move_card on PR-open). + with kb.write_txn(conn): + conn.execute("UPDATE tasks SET status='review', assignee=? WHERE id=?", (reviewer, tid)) + kb._append_event(conn, tid, "status_changed", + {"from": "ready", "to": "review", "by": "onecard:move_card"}) + kb._append_event(conn, tid, "assigned", + {"from": author, "to": reviewer, "by": "onecard:move_card"}) + # Reviewer claims the review card, then bounces with a clean block. + rt = kb.claim_review_task(conn, tid) + assert rt is not None and rt.status == "running" + assert kb.block_task(conn, tid, reason=reason, + expected_run_id=kb.get_task(conn, tid).current_run_id) + assert kb.get_task(conn, tid).status == "blocked" + # In production the build + review happen minutes before the housekeeping + # tick that routes the bounce. Back-date the build's PR-handoff comment and + # completed run so the routing unblock is causally LATER (not the same + # second), modelling the real timeline rather than a compressed-test artifact. + past = int(time.time()) - 300 + with kb.write_txn(conn): + conn.execute("UPDATE task_comments SET created_at=? WHERE task_id=?", (past, tid)) + conn.execute("UPDATE task_runs SET ended_at=? WHERE task_id=? AND ended_at IS NOT NULL", + (past, tid)) + return tid + + +# --------------------------------------------------------------------------- +# RED 1 — the core auto-route +# --------------------------------------------------------------------------- + + +def test_review_bounce_block_auto_routes_to_author(kanban_home: Path) -> None: + """A ``review-changes-requested`` block must auto-route the SAME card + ``blocked -> ready`` + assignee = the original author on the housekeeping tick, + with an audit comment naming the PR. Fails today: the card stays ``blocked``.""" + with kb.connect() as conn: + tid = _stage_review_bounce(conn) + + routed = kb.auto_route_review_bounce(conn) + + assert routed == 1, "exactly one card should auto-route" + task = kb.get_task(conn, tid) + assert task.status == "ready", "card must be routed back to ready" + assert task.assignee == "eckert", "card must be reassigned to the author" + # An audit comment recording the auto-route, naming the PR. + comments = kb.list_comments(conn, tid) + audit = [c for c in comments if "review-changes-requested" in (c.body or "") + or "auto-route" in (c.body or "").lower()] + assert audit, "an audit comment recording the auto-route is required" + assert any("pull/71" in (c.body or "") for c in comments), \ + "the audit comment must name the PR" + + +def test_review_bounce_routed_via_dispatch_once( + kanban_home: Path, all_assignees_spawnable +) -> None: + """The auto-route must fire inside the real housekeeping tick (``dispatch_once``), + not only when called directly.""" + with kb.connect() as conn: + tid = _stage_review_bounce(conn) + kb.dispatch_once(conn, spawn_fn=lambda *_: 4321, dry_run=False) + task = kb.get_task(conn, tid) + # Routed AND dispatchable: the tick reassigns to the author and the card + # leaves the blocked lane (it is claimed to running by the same tick's + # spawn, since the respawn guards are cleared). + assert task.status in ("ready", "running") + assert task.assignee == "eckert" + + +# --------------------------------------------------------------------------- +# RED 2 — acceptance (PASS) block must NOT route +# --------------------------------------------------------------------------- + + +def test_awaiting_casey_signoff_block_does_not_route(kanban_home: Path) -> None: + """The PASS / acceptance block (``awaiting-casey-signoff``) is parked for Casey + and must STAY ``blocked`` -- never routed to the author.""" + with kb.connect() as conn: + tid = _stage_review_bounce(conn, reviewer="lamport", reason=_SIGNOFF_REASON) + # The acceptance lane assigns casey; mirror that the reviewer parked it. + with kb.write_txn(conn): + conn.execute("UPDATE tasks SET assignee='casey' WHERE id=?", (tid,)) + + routed = kb.auto_route_review_bounce(conn) + + assert routed == 0 + task = kb.get_task(conn, tid) + assert task.status == "blocked", "acceptance block must stay blocked" + assert task.assignee == "casey", "acceptance block must stay with casey" + + +# --------------------------------------------------------------------------- +# RED 3 — idempotency: two ticks -> one route, one comment +# --------------------------------------------------------------------------- + + +def test_auto_route_is_idempotent_across_ticks(kanban_home: Path) -> None: + """The housekeeping tick runs repeatedly. The auto-route must fire EXACTLY ONCE + per block: two consecutive ticks produce one route and one audit comment.""" + with kb.connect() as conn: + tid = _stage_review_bounce(conn) + + first = kb.auto_route_review_bounce(conn) + second = kb.auto_route_review_bounce(conn) + + assert first == 1 + assert second == 0, "second tick must not re-route the already-routed card" + # Exactly one auto-route audit comment. + comments = kb.list_comments(conn, tid) + audit = [c for c in comments + if (c.body or "").lstrip().startswith("[audit]") + and "rework" in (c.body or "")] + assert len(audit) == 1, f"exactly one audit comment expected, got {len(audit)}" + + +# --------------------------------------------------------------------------- +# RED 4 — routed card is genuinely dispatchable (respawn guard cleared) +# --------------------------------------------------------------------------- + + +def test_routed_card_is_dispatchable( + kanban_home: Path, all_assignees_spawnable +) -> None: + """Clearing to ``ready`` + author must clear the ``active_pr`` / ``recent_success`` + respawn guards (the card carries a PR-URL comment + a recent completed build run), + so the dispatcher actually spawns the author on the next tick -- not a ``ready`` + card the guard silently skips.""" + with kb.connect() as conn: + tid = _stage_review_bounce(conn) + kb.auto_route_review_bounce(conn) + + # The guard must NOT veto the respawn of the routed card. + assert kb.check_respawn_guard(conn, tid) is None, \ + "routed card must be free of respawn guards" + + # And the dispatcher actually spawns it. + spawned: list[str] = [] + kb.dispatch_once(conn, spawn_fn=lambda task, *a: spawned.append(task.id) or 4321) + assert tid in spawned, "routed card must be spawned by the dispatcher" + + +# --------------------------------------------------------------------------- +# RED 5 — a non-review block (circuit-breaker / arbitrary) must NOT route +# --------------------------------------------------------------------------- + + +def test_non_review_block_does_not_route(kanban_home: Path) -> None: + """A block whose reason is NOT a review-changes-requested bounce (e.g. a generic + operator block) must be left untouched by the auto-router.""" + with kb.connect() as conn: + tid = kb.create_task(conn, title="needs human", assignee="eckert") + kb.claim_task(conn, tid) + kb.block_task(conn, tid, reason="review-required: please verify ACL change", + expected_run_id=kb.get_task(conn, tid).current_run_id) + + routed = kb.auto_route_review_bounce(conn) + + assert routed == 0 + assert kb.get_task(conn, tid).status == "blocked" + + +def test_circuit_breaker_block_does_not_route(kanban_home: Path) -> None: + """A circuit-breaker block (``gave_up`` event, no ``blocked`` event) must not be + treated as a review bounce.""" + with kb.connect() as conn: + tid = kb.create_task(conn, title="flaky", assignee="eckert") + with kb.write_txn(conn): + conn.execute("UPDATE tasks SET status='blocked' WHERE id=?", (tid,)) + conn.execute( + "INSERT INTO task_events (task_id, kind, payload, created_at) " + "VALUES (?, 'gave_up', NULL, ?)", (tid, int(time.time()))) + + routed = kb.auto_route_review_bounce(conn) + + assert routed == 0 + assert kb.get_task(conn, tid).status == "blocked" + + +# --------------------------------------------------------------------------- +# RED 6 — config toggle off disables the auto-route +# --------------------------------------------------------------------------- + + +def test_auto_route_disabled_by_flag(kanban_home: Path) -> None: + """``kanban.auto_route_review_bounce: false`` must leave the bounce block parked.""" + with kb.connect() as conn: + tid = _stage_review_bounce(conn) + routed = kb.auto_route_review_bounce(conn, enabled=False) + assert routed == 0 + assert kb.get_task(conn, tid).status == "blocked"