From 04f0eff168fb28392b745d8a80f25e4cd847e486 Mon Sep 17 00:00:00 2001 From: SSC-ENG <225143396+SSC-ENG@users.noreply.github.com> Date: Thu, 30 Jul 2026 22:57:49 -0700 Subject: [PATCH] fix(kanban): block-recurrence counter + auto-decomposer must not treat dispatcher auto-promotion as a legitimate unblock (t_e2b1f62a) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Kanban card: t_e2b1f62a (rhea-ramos) — second, distinct dispatch-routing defect discovered while verifying PR #23's fix for t_21f59f6d. Linear issue: none exists for this card (kanban-native dispatcher infra bug; no user-facing Linear-tracked feature/fix maps to it — same convention as PR #23/t_21f59f6d). Root cause (a): block_task()'s unblock-loop breaker incremented block_recurrences on any same-kind re-block, with no check on WHAT put the task back in running/ready. A dispatcher-side auto-promotion (recompute_ready's parents_terminal trigger, or any other non- unblock_task exit from blocked) looked identical, at this layer, to a genuine human/cron unblock_task -> worker re-block ping-pong. This inflated t_342c4c9f's counter to 3 (limit=2) purely from residual churn caused by the now-fixed t_21f59f6d bug, tripping block_loop_detected and routing an already-reviewed, correctly-blocked card to triage instead of blocked. Fix: block_task now checks the most recent status-exit event kind before counting a same-block_kind re-block as the same-cause loop signal. Only an explicit unblock_task call (which emits "unblocked") re-arms the counter; a dispatcher "promoted" event resets it to a fresh 1. Root cause (b): decompose_task() (and the gateway's auto-decompose sweep via list_triage_ids()) treated every triage card identically, including ones routed there by block_loop_detected specifically to force a human decision. auto-decomposer picked such a card up, made a cosmetic title/body edit, and immediately promoted it straight back to ready via parents_terminal (with REAL, all-done parent ids this time — a distinct defect from t_21f59f6d) — defeating the loop breaker's whole purpose within ~90 seconds. Fix: decompose_task() now checks whether the triage card's most recent event is block_loop_detected and refuses to specify/promote it (no LLM call made), leaving it for an explicit human/operator action. Ask (c) confirmed structurally: the active_pr respawn guard (_RESPAWN_GUARD_PR_WINDOW = 86400s) is NOT indefinite — without fixes (a)/(b) the wasted-spawn loop would have resumed after 24h at a slower cadence, not stayed blocked forever. This makes (a)/(b) the real fix. ## Tests - test_kanban_blocked_sticky.py: added test_dispatcher_repromotion_does_not_inflate_block_recurrences (reproduces the t_342c4c9f loop across 4 dispatcher-style re-promotion cycles; asserts recurrences stay at 1 and status stays blocked) and test_genuine_unblock_reblock_loop_still_trips_breaker (sanity check the original Dale's-Type-1 loop-breaker behavior is preserved: BLOCK_RECURRENCE_LIMIT=2 still trips on the 2nd genuine same-cause unblock->reblock). - test_kanban_decompose.py: added test_decompose_skips_triage_card_from_block_loop_detected (asserts ok=False, no LLM call, card stays in triage untouched). All new/modified tests pass (12/12). Ran full tests/hermes_cli -k kanban suite on this branch (276 passed, 9 pre-existing failures) and diffed 1:1 against the same suite on unmodified fork/main (273 passed, same 9 failures) — confirmed the 9 failures are pre-existing test-isolation issues unrelated to this change, and this diff adds exactly 3 new passing tests with zero regressions. 🤖 Generated with Hermes Agent (rhea-ramos) --- hermes_cli/kanban_db.py | 63 +++++++++- hermes_cli/kanban_decompose.py | 26 +++- .../hermes_cli/test_kanban_blocked_sticky.py | 112 ++++++++++++++++++ tests/hermes_cli/test_kanban_decompose.py | 37 ++++++ 4 files changed, 228 insertions(+), 10 deletions(-) diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index df32877052d5..910c837df90b 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -5374,6 +5374,25 @@ def _has_sticky_block(conn: sqlite3.Connection, task_id: str) -> bool: return bool(row) and row["kind"] == "blocked" +def _most_recent_event_kind(conn: sqlite3.Connection, task_id: str) -> Optional[str]: + """Return the ``kind`` of the single most recent ``task_events`` row for + ``task_id``, or ``None`` if the task has no events. + + Used to detect that a ``triage`` card landed there via the + ``block_loop_detected`` routing (see ``block_task`` / + ``BLOCK_RECURRENCE_LIMIT``) rather than via normal raw-intake — that + routing exists specifically to force a human decision, so callers such + as the auto-decomposer must not treat it as an ordinary triage card to + sweep and re-promote (t_e2b1f62a). + """ + row = conn.execute( + "SELECT kind FROM task_events WHERE task_id = ? " + "ORDER BY id DESC LIMIT 1", + (task_id,), + ).fetchone() + return row["kind"] if row else None + + def recompute_ready( conn: sqlite3.Connection, failure_limit: int = None, ) -> int: @@ -7125,12 +7144,44 @@ def block_task( return True # Truly-blocked kinds. Increment the unblock-loop counter when this is a - # re-block for the SAME reason after a prior unblock. block_task only - # fires from running/ready (i.e. AFTER an unblock returned the task to - # the work pool), so a stored block_kind that matches the incoming kind - # means: blocked → unblocked → about-to-re-block for the same cause. - # An un-typed (None) block compares as "same" to a prior un-typed block. - same_cause = prev_kind == kind + # re-block for the SAME reason after a prior *legitimate* unblock. + # block_task only fires from running/ready (i.e. AFTER something + # returned the task to the work pool), so a stored block_kind that + # matches the incoming kind COULD mean: blocked -> unblocked -> + # about-to-re-block for the same cause (Dale's genuine cron/human + # unblock <-> worker re-block ping-pong — this is what the counter is + # meant to catch). An un-typed (None) block compares as "same" to a + # prior un-typed block. + # + # But "blocked -> ready/running" can ALSO happen without anyone + # actually unblocking the task for a reason: ``recompute_ready``'s + # dispatcher-side ``parents_terminal`` sweep (or any other + # non-``unblock_task`` transition) can flip a non-sticky + # circuit-breaker ``blocked`` state back to ``ready`` on its own. + # That is a completely different signal from "a human/cron decided + # this was resolved" — treating it the same way conflates "re-promoted + # by dispatcher machinery unrelated to the block reason" with + # "re-blocked for the same cause after a legitimate unblock", and + # inflates ``block_recurrences`` on every dispatcher hiccup until the + # loop breaker trips and routes an otherwise-correctly-blocked, + # already-reviewed card to ``triage`` for no reason (t_e2b1f62a). + # + # Disambiguate by checking what the most recent "exit from blocked" + # event actually was: only an explicit ``unblock_task`` call emits + # ``"unblocked"``; a dispatcher auto-promotion emits ``"promoted"`` + # (trigger ``parents_terminal`` or otherwise). Only count this as a + # same-cause re-block when the intervening exit was a genuine + # ``unblocked`` event. + last_exit_row = conn.execute( + "SELECT kind FROM task_events WHERE task_id = ? " + "AND kind IN ('unblocked', 'promoted') " + "ORDER BY id DESC LIMIT 1", + (task_id,), + ).fetchone() + exited_via_legitimate_unblock = bool( + last_exit_row and last_exit_row["kind"] == "unblocked" + ) + same_cause = prev_kind == kind and exited_via_legitimate_unblock recurrences = prev_recurrences + 1 if same_cause else 1 if recurrences >= BLOCK_RECURRENCE_LIMIT: diff --git a/hermes_cli/kanban_decompose.py b/hermes_cli/kanban_decompose.py index 1eacd630fb43..c96288879459 100644 --- a/hermes_cli/kanban_decompose.py +++ b/hermes_cli/kanban_decompose.py @@ -570,11 +570,29 @@ def decompose_task( """ with kb.connect_closing() as conn: task = kb.get_task(conn, task_id) - if task is None: - return DecomposeOutcome(task_id, False, "unknown task id") - if task.status != "triage": + if task is None: + return DecomposeOutcome(task_id, False, "unknown task id") + if task.status != "triage": + return DecomposeOutcome( + task_id, False, f"task is not in triage (status={task.status!r})" + ) + # A triage card whose most recent event is ``block_loop_detected`` was + # routed here specifically to force a HUMAN decision (see + # ``block_task``/``BLOCK_RECURRENCE_LIMIT`` in kanban_db.py) — the + # unblock<->reblock loop breaker tripped because a worker kept + # re-blocking it for the same cause. Blindly re-"specifying" and + # promoting such a card back to ``todo``/``ready`` hands it straight + # back into the same loop the breaker exists to interrupt (t_e2b1f62a): + # the card's disposition is already correct and signed, it just needs a + # human triage call, not another decomposer pass. Skip it here; a human + # (or an explicit ``kanban specify``/``kanban promote`` call) is the + # only legitimate way out of this state. + most_recent_kind = kb._most_recent_event_kind(conn, task_id) + if most_recent_kind == "block_loop_detected": return DecomposeOutcome( - task_id, False, f"task is not in triage (status={task.status!r})" + task_id, False, + "skipped: most recent event is block_loop_detected — " + "awaiting human triage decision, not auto-decompose", ) cfg = _load_config() diff --git a/tests/hermes_cli/test_kanban_blocked_sticky.py b/tests/hermes_cli/test_kanban_blocked_sticky.py index 1dfaeee09a9e..b5268a10d318 100644 --- a/tests/hermes_cli/test_kanban_blocked_sticky.py +++ b/tests/hermes_cli/test_kanban_blocked_sticky.py @@ -29,6 +29,7 @@ from __future__ import annotations +import json import time from pathlib import Path @@ -228,3 +229,114 @@ def test_force_trip_floors_consecutive_failures_at_effective_limit( "floored failure count still exceeds a lower effective_limit" ) assert kb.get_task(conn, tid).status == "blocked" + + +# --------------------------------------------------------------------------- +# block_task's recurrence counter must not treat a dispatcher-side +# auto-promotion (parents_terminal or any other non-unblock_task exit from +# ``blocked``) the same as a genuine human/cron unblock -> worker re-block +# cycle (t_e2b1f62a). +# --------------------------------------------------------------------------- + + +def test_dispatcher_repromotion_does_not_inflate_block_recurrences( + kanban_home: Path, +) -> None: + """Reproduces the t_342c4c9f loop: a card is correctly re-blocked for + the SAME cause after the dispatcher (not a human/cron) flipped it back + to ready/running via a ``parents_terminal``-triggered ``promoted`` + event. This must NOT count toward ``block_recurrences`` — only an + explicit ``unblock_task`` call re-arms the same-cause counter. + """ + with kb.connect() as conn: + tid = kb.create_task(conn, title="t_342c4c9f style review-required card") + kb.claim_task(conn, tid) + + assert kb.block_task( + conn, tid, kind="needs_input", + reason="REJECTED-INTAKE: awaiting producer resubmission", + expected_run_id=kb.get_task(conn, tid).current_run_id, + ) + task = kb.get_task(conn, tid) + assert task.status == "blocked" + assert task.block_recurrences == 1 + + # Simulate the dispatcher-side bug: something (parents_terminal + # sweep, or any non-unblock_task path) flips the card back to + # ready/running WITHOUT going through unblock_task — so no + # "unblocked" event is emitted, only a "promoted" one. + for cycle in range(4): + conn.execute( + "UPDATE tasks SET status = 'running' WHERE id = ?", (tid,), + ) + conn.execute( + "INSERT INTO task_events (task_id, kind, payload, created_at) " + "VALUES (?, 'promoted', ?, ?)", + ( + tid, + json.dumps({ + "from_status": "blocked", "to_status": "ready", + "trigger": "parents_terminal", "satisfied_parent_ids": [], + }), + int(time.time()) + cycle, + ), + ) + conn.commit() + + assert kb.block_task( + conn, tid, kind="needs_input", + reason="REJECTED-INTAKE: awaiting producer resubmission", + expected_run_id=None, + ) + task = kb.get_task(conn, tid) + # The card must land back in 'blocked' (never 'triage') and the + # recurrence counter must stay at 1 — each re-block after a + # dispatcher auto-promotion is treated as a fresh same-cause + # block (recurrences reset to 1), not an accumulating loop. + assert task.status == "blocked", ( + f"cycle {cycle}: dispatcher-side re-promotion inflated the " + f"loop-breaker into routing to {task.status!r} instead of " + "staying blocked" + ) + assert task.block_recurrences == 1, ( + f"cycle {cycle}: block_recurrences={task.block_recurrences}, " + "expected 1 — a parents_terminal-triggered re-entry must not " + "count toward the same-cause loop-breaker counter" + ) + + +def test_genuine_unblock_reblock_loop_still_trips_breaker( + kanban_home: Path, +) -> None: + """Sanity check that the fix above does not defang the original + loop-breaker: a REAL unblock_task -> re-block cycle for the same cause + must still accumulate recurrences and eventually route to triage. + """ + with kb.connect() as conn: + tid = kb.create_task(conn, title="genuine ping-pong reproducer") + kb.claim_task(conn, tid) + + assert kb.block_task( + conn, tid, kind="needs_input", reason="waiting on X", + expected_run_id=kb.get_task(conn, tid).current_run_id, + ) + assert kb.get_task(conn, tid).block_recurrences == 1 + + assert kb.unblock_task(conn, tid) + assert kb.get_task(conn, tid).status == "ready" + conn.execute("UPDATE tasks SET status = 'running' WHERE id = ?", (tid,)) + conn.commit() + + assert kb.block_task( + conn, tid, kind="needs_input", reason="waiting on X", + expected_run_id=None, + ) + task = kb.get_task(conn, tid) + # BLOCK_RECURRENCE_LIMIT is 2, so the loop breaker trips on THIS + # (second) same-cause re-block, routing straight to triage rather + # than back to blocked — it does not take a third cycle. + assert task.status == "triage", ( + "a genuine repeated unblock -> re-block cycle for the same " + "cause must still trip the loop breaker and route to triage" + ) + assert task.block_recurrences == kb.BLOCK_RECURRENCE_LIMIT == 2 diff --git a/tests/hermes_cli/test_kanban_decompose.py b/tests/hermes_cli/test_kanban_decompose.py index 1e50dc73b269..51d6a593a5da 100644 --- a/tests/hermes_cli/test_kanban_decompose.py +++ b/tests/hermes_cli/test_kanban_decompose.py @@ -282,3 +282,40 @@ def test_decompose_returns_false_when_task_not_triage(kanban_home): assert "not in triage" in outcome.reason +def test_decompose_skips_triage_card_from_block_loop_detected(kanban_home): + """A triage card whose most recent event is ``block_loop_detected`` was + routed there specifically to force a human decision (see + ``block_task``/``BLOCK_RECURRENCE_LIMIT`` in kanban_db.py — t_e2b1f62a). + ``decompose_task`` must refuse to auto-specify/promote it, and must not + invoke the auxiliary LLM at all for such a card. + """ + with kb.connect() as conn: + tid = kb.create_task(conn, title="review-required card", triage=True) + kb._append_event( + conn, tid, "block_loop_detected", + {"reason": "REJECTED-INTAKE", "kind": "needs_input", + "recurrences": 2, "limit": kb.BLOCK_RECURRENCE_LIMIT}, + ) + + patches = _patch_list_profiles(["orchestrator"]) + for p in patches: + p.start() + try: + with _patch_aux_client("{}") as mock_call_llm, _patch_extra_body(): + outcome = decomp.decompose_task(tid, author="me") + finally: + for p in patches: + p.stop() + + assert outcome.ok is False + assert "block_loop_detected" in outcome.reason + mock_call_llm.assert_not_called() + with kb.connect() as conn: + task = kb.get_task(conn, tid) + assert task.status == "triage", ( + "a block_loop_detected card must stay in triage untouched, " + "not be re-specified or promoted" + ) + + +