diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 086be0afcef01..359c88805b52f 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -6364,6 +6364,10 @@ def request_changes( reviewer = None new_status = _landing_status_after_parents(conn, task_id) + # NOTE: consecutive_failures is deliberately PRESERVED (neither + # reset nor incremented). Review transitions are not evidence the + # pathology cleared — only complete_task's success path resets the + # breaker counter (mirrors unblock_task, #35072). cur = conn.execute( """ UPDATE tasks @@ -6371,9 +6375,7 @@ def request_changes( assignee = COALESCE(?, assignee), claim_lock = NULL, claim_expires = NULL, - worker_pid = NULL, - consecutive_failures = 0, - last_failure_error = NULL + worker_pid = NULL WHERE id = ? AND status = 'running' AND current_run_id = ? """, (new_status, implementer, task_id, int(current_run_id)), @@ -6588,7 +6590,7 @@ def reopen_review_task(conn: sqlite3.Connection, task_id: str) -> bool: The "changes requested" counterpart of :func:`request_review`: sends the task back out of the review lane so the dispatcher re-runs the implementer on the new comments. Mirrors :func:`unblock_task` (parent re-gating, - defensive stale-run close, ``consecutive_failures`` reset) and emits a + defensive stale-run close, ``consecutive_failures`` preserved) and emits a ``review_reopened`` event. Deliberately does NOT touch ``block_recurrences``/``block_kind``: review is @@ -6628,8 +6630,10 @@ def reopen_review_task(conn: sqlite3.Connection, task_id: str) -> bool: ) cur = conn.execute( "UPDATE tasks SET status = ?, current_run_id = NULL, " - "claim_lock = NULL, claim_expires = NULL, worker_pid = NULL, " - "consecutive_failures = 0, last_failure_error = NULL " + "claim_lock = NULL, claim_expires = NULL, worker_pid = NULL " + # consecutive_failures deliberately PRESERVED: review reopen is + # not a success signal; only complete_task resets the breaker + # counter (mirrors unblock_task, #35072). + assignee_sql + " WHERE id = ? AND status = 'review'", params, diff --git a/tests/hermes_cli/test_kanban_review_lifecycle_complete.py b/tests/hermes_cli/test_kanban_review_lifecycle_complete.py index 72684c04520b9..4dc65d0f9ceb6 100644 --- a/tests/hermes_cli/test_kanban_review_lifecycle_complete.py +++ b/tests/hermes_cli/test_kanban_review_lifecycle_complete.py @@ -636,3 +636,74 @@ def test_hard_block_with_waiting_child_is_not_mislabeled_as_review_deadlock(conn }, ) assert not any(d.kind == "review_dependency_deadlock" for d in diagnostics) + + +def _failures(conn, task_id: str) -> int: + return int(conn.execute( + "SELECT consecutive_failures FROM tasks WHERE id = ?", (task_id,) + ).fetchone()[0]) + + +def test_review_transitions_preserve_consecutive_failures(conn) -> None: + """M2 regression: review transitions neither reset nor increment the + circuit-breaker counter. + + A task with consecutive_failures=1 that cycles through + request_review -> request_changes -> re-request keeps the counter at 1; + a crash after request_changes increments it to 2 and trips a + failure_limit=2 breaker. Only complete_task's success path resets it. + """ + task_id = kb.create_task(conn, title="flaky feature", assignee="builder") + with kb.write_txn(conn): + conn.execute( + "UPDATE tasks SET consecutive_failures = 1 WHERE id = ?", + (task_id,), + ) + + implementation = kb.claim_task(conn, task_id, claimer="builder:1") + assert implementation is not None + assert kb.request_review( + conn, task_id, summary="v1", reviewer="reviewer", + expected_run_id=implementation.current_run_id, + ) + assert _failures(conn, task_id) == 1 # request_review preserved it + + review = kb.claim_review_task(conn, task_id) + assert review is not None + assert kb.request_changes( + conn, task_id, reason="needs fixes", + expected_run_id=review.current_run_id, + ) == (True, "builder") + assert _failures(conn, task_id) == 1 # request_changes preserved it + + retry = kb.claim_task(conn, task_id, claimer="builder:2") + assert retry is not None + assert kb.request_review( + conn, task_id, summary="v2", + expected_run_id=retry.current_run_id, + ) + assert _failures(conn, task_id) == 1 # full re-review cycle: still 1 + + # reopen_review_task (manual changes-requested) also preserves it. + assert kb.reopen_review_task(conn, task_id) + assert _failures(conn, task_id) == 1 + + # A crash now increments 1 -> 2 and trips a failure_limit=2 breaker — + # the counter accumulated across the review cycle instead of being + # amnesia-reset back to 0. + tripped = kb._record_task_failure( + conn, task_id, "worker crashed", outcome="crashed", failure_limit=2, + ) + assert tripped is True + assert _failures(conn, task_id) == 2 + assert kb.get_task(conn, task_id).status == "blocked" + + # Sanity: complete_task's success path still clears the counter. + ok_id = kb.create_task(conn, title="healthy", assignee="builder") + with kb.write_txn(conn): + conn.execute( + "UPDATE tasks SET consecutive_failures = 1 WHERE id = ?", + (ok_id,), + ) + assert kb.complete_task(conn, ok_id, summary="done") + assert _failures(conn, ok_id) == 0