Skip to content

🐛 fix(kanban): auto-advance a PR-open code card to review on clean-exit-after-done #76

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cwest merged 1 commit into from
Jul 21, 2026
Merged

🐛 fix(kanban): auto-advance a PR-open code card to review on clean-exit-after-done #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 85 additions & 11 deletions hermes_cli/kanban_db.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8136,10 +8136,24 @@ def detect_crashed_workers(conn: sqlite3.Connection) -> list[str]:
protocol violation. It is the completed-but-un-signposted exit: the
worker finished the lane correctly but exited without the terminal verb.
Counting it would trip a FALSE ``gave_up`` and strand a finished card in
``blocked``. It is released as a benign no-op WITHOUT counting a failure
(mirroring the rate-limit carve-out); the respawn guard's
recent_success / active_pr check then defers a duplicate spawn. The ids
are surfaced via the ``_last_clean_exit_after_done`` function attribute.
``blocked``. It is handled WITHOUT counting a failure (mirroring the
rate-limit carve-out), and the ids are surfaced via the
``_last_clean_exit_after_done`` function attribute. There are two release
shapes:

* **PR-open code card** — the provably-done signal is a PR handoff (a
``pull/<n>`` URL in a comment, via :func:`_card_has_pr_artifact`) AND the
card's own submit-stage owner map declares a ``review`` owner. Such a card
is MOVED ``running -> review`` + that reviewer atomically with the reap —
the exact author-lane handoff the worker's clean exit skipped, mirroring
:func:`complete_task`. Merely releasing it to ``ready`` would wedge it: the
``active_pr`` respawn guard holds an open-PR card out of respawn WITHOUT
advancing it, so it would sit in ``running``/``ready`` until an
orchestrator hand-staged it.
* **Everything else** (a completed-run proof with no PR, a no-PR
edit-in-place card, or a PR card with no resolvable owner-map reviewer) is
released as a benign no-op back to ``ready``/``review``; the respawn
guard's recent_success / active_pr check then defers a duplicate spawn.

When the reap registry shows the worker exited with the rate-limit
sentinel (``KANBAN_RATE_LIMIT_EXIT_CODE``), the worker bailed on a
Expand Down Expand Up @@ -8250,6 +8264,10 @@ def detect_crashed_workers(conn: sqlite3.Connection) -> list[str]:
rate_limited_exit = False
transient_exit = False
clean_exit_after_done = False
# For a PR-open code card exiting clean_exit_after_done, the
# 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"]
):
Expand Down Expand Up @@ -8288,6 +8306,22 @@ 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"]):
advance_review_owner = _review_owner_from_owner_map(
conn, row["id"]
)
elif kind == "clean_exit":
# Worker subprocess returned 0 but its task is still
# ``running`` in the DB — it exited without calling
Expand Down Expand Up @@ -8385,13 +8419,29 @@ def detect_crashed_workers(conn: sqlite3.Connection) -> list[str]:
conn, row["id"], current_run_id
)
restore_status = "review" if was_review_run else "ready"
cur = conn.execute(
"UPDATE tasks SET status = ?, claim_lock = NULL, "
"claim_expires = NULL, worker_pid = NULL "
"WHERE id = ? AND status = 'running' "
" AND worker_pid = ? AND claim_lock IS ?",
(restore_status, row["id"], pid, row["claim_lock"]),
)
# A PR-open code card auto-advances to ``review`` + the owner-map
# reviewer instead of releasing to ``ready`` — the transition the
# worker's clean exit skipped. This is the same MOVE ``complete_task``
# performs on the author-lane handoff (status=review, assignee set,
# claim/pid cleared, block state reset), applied here for the card
# whose worker exited without the terminal verb.
if advance_review_owner:
cur = conn.execute(
"UPDATE tasks SET status = 'review', assignee = ?, "
"claim_lock = NULL, claim_expires = NULL, worker_pid = NULL, "
"block_kind = NULL, block_recurrences = 0 "
"WHERE id = ? AND status = 'running' "
" AND worker_pid = ? AND claim_lock IS ?",
(advance_review_owner, row["id"], pid, row["claim_lock"]),
)
else:
cur = conn.execute(
"UPDATE tasks SET status = ?, claim_lock = NULL, "
"claim_expires = NULL, worker_pid = NULL "
"WHERE id = ? AND status = 'running' "
" AND worker_pid = ? AND claim_lock IS ?",
(restore_status, row["id"], pid, row["claim_lock"]),
)
if cur.rowcount == 1:
# Rate-limited / transient requeues and provably-done clean
# exits are a clean release, not a crash — record the run
Expand All @@ -8402,6 +8452,11 @@ def detect_crashed_workers(conn: sqlite3.Connection) -> list[str]:
_run_outcome = "rate_limited"
elif transient_exit:
_run_outcome = "transient"
elif advance_review_owner:
# The author DID finish their lane (an open PR is the
# artifact); the card advancing to review is the handoff —
# record the run as completed, matching ``complete_task``.
_run_outcome = "completed"
elif clean_exit_after_done:
_run_outcome = "clean_exit_after_done"
else:
Expand All @@ -8412,6 +8467,25 @@ def detect_crashed_workers(conn: sqlite3.Connection) -> list[str]:
error=error_text,
metadata=dict(event_payload),
)
if advance_review_owner:
# Emit the lifecycle transition the orchestrator / dashboard
# read to recognize the card reached the review lane — the
# same ``status_changed`` shape ``complete_task``'s handoff
# emits, so both paths look identical on the board. No
# ``crashed`` event, no failure counted: the reap already
# classified this as a benign completed handoff.
_append_event(
conn, row["id"], "status_changed",
{
"from": "running",
"to": "review",
"assignee": advance_review_owner,
"by": "dispatcher:clean-exit-after-done-advance",
},
run_id=run_id,
)
clean_exit_after_done_ids.append(row["id"])
continue
_append_event(
conn, row["id"], event_kind,
event_payload,
Expand Down
145 changes: 145 additions & 0 deletions tests/hermes_cli/test_kanban_db.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1866,6 +1866,151 @@ def test_pr_requiring_card_handoff_comment_is_not_third_proof(
assert "protocol_violation" in kinds, kinds


# ---------------------------------------------------------------------------
# Auto-advance a PR-open code card to review on clean-exit-after-done.
#
# When a code-author card's worker opens a PR, pushes, and exits rc=0 WITHOUT a
# terminal verb, ``_lane_work_provably_done`` is True (Proof 2: a PR URL in a
# recent comment) so the reap does not count a failure — but the OLD behavior
# merely RELEASED the card to ``ready``. For a PR-open code card that release is
# a dead end: the ``active_pr`` respawn guard HOLDS it out of respawn (an open
# PR ⇒ dup-PR risk) WITHOUT advancing it, so the card wedges in ``running`` →
# ``ready`` until an orchestrator hand-stages it to review. The reap must
# instead MOVE the card ``running -> review`` + the owner-map reviewer atomically
# (mirroring ``complete_task``'s canonical author-lane handoff), emitting
# ``status_changed running->review`` + an assignee. The distinguisher from the
# no-PR edit-in-place shape (which must stay a benign release) is the pair of
# durable signals the rest of the subsystem already trusts: a PR artifact
# (``_card_has_pr_artifact``) AND a resolvable ``review`` owner in the card's
# submit-stage owner map.
# ---------------------------------------------------------------------------


def _stamp_submit_owner_map(conn, task_id, *, ready, review):
"""Record the submit-stage owner-map audit comment core reads for routing.

Mirrors ``submit_card``'s §9.1 audit comment shape (the ONE comment
``_owner_from_owner_map`` treats as authoritative).
"""
kb.add_comment(
conn, task_id, "hollis",
"[audit] actor=hollis stage=submit ts=2026-07-21T22:06:57Z\n"
f"notes: state_owners={{ready: {ready}, review: {review}, "
"blocked-acceptance: casey}} triager=hollis team=engineering",
)


def test_clean_exit_after_done_pr_open_code_card_advances_to_review(
kanban_home, monkeypatch,
):
"""A code-author card whose worker opened a PR and exited rc=0 without a
terminal verb must AUTO-ADVANCE to ``review`` + the owner-map reviewer —
not merely release to ``ready`` (where the active_pr guard would wedge it).

This is the reported okfctl PR #1 incident shape: the author finished,
pushed, and exited ``clean_exit_after_done``, but the card sat
``running``/eckert and had to be hand-staged.
"""
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:
tid = kb.create_task(
conn, title="fix: a bug", assignee="eckert",
workspace_kind="worktree",
workspace_path="/Users/caseywest/src/hermes-agent",
)
_stamp_submit_owner_map(conn, tid, ready="eckert", review="lamport")
# The ready-for-review handoff the implementer lane posts on PR open.
kb.add_comment(
conn, tid, "eckert",
"PR opened (draft): https://github.com/cwest/hermes-agent/pull/501 "
"head=c05e13a",
)

_stage_clean_exit(conn, _kb, tid, 69001)
crashed = kb.detect_crashed_workers(conn)

assert tid not in crashed, (
"a PR-open code card that exited clean_exit_after_done must not be "
"treated as a crash"
)
task = kb.get_task(conn, tid)
assert task.status == "review", (
f"a PR-open code card must auto-advance to review, got {task.status}"
)
assert task.assignee == "lamport", (
f"the card must be assigned to the owner-map reviewer, "
f"got {task.assignee}"
)
# The claim/pid must be cleared so the reviewer can spawn.
assert task.worker_pid is None
assert task.claim_lock is None
assert task.consecutive_failures == 0, (
"auto-advance must not count a failure"
)
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
# The lifecycle transition events the orchestrator/dashboard read.
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_clean_exit_after_done_pr_card_without_owner_map_still_releases_ready(
kanban_home, monkeypatch,
):
"""A card with an open PR URL but NO owner-map reviewer cannot resolve a
review destination, so it must fall back to the benign release-to-``ready``
(the pre-existing #47 draft-PR carve-out), NOT wedge or crash. The
auto-advance is gated on a resolvable owner-map reviewer.
"""
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:
tid = kb.create_task(conn, title="draft-pr-no-map", assignee="a")
kb.add_comment(
conn, tid, "worker",
"PR opened (draft): https://github.com/cwest/hermes-agent/pull/502",
)

_stage_clean_exit(conn, _kb, tid, 69002)
crashed = kb.detect_crashed_workers(conn)

assert tid not in crashed
task = kb.get_task(conn, tid)
assert task.status == "ready", (
f"a PR card with no owner-map reviewer must release ready, "
f"got {task.status}"
)
assert task.consecutive_failures == 0
kinds = [e.kind for e in kb.list_events(conn, tid)]
assert "crashed" not in kinds, kinds
assert "gave_up" not in kinds, kinds
cead = getattr(
_kb.detect_crashed_workers, "_last_clean_exit_after_done", []
)
assert tid in cead, (
"a PR card with no owner-map reviewer should still be surfaced via "
"the clean_exit_after_done side-channel"
)


# ---------------------------------------------------------------------------
# Sibling path (#47 missed): transient API / connection failures.
#
Expand Down
Loading