fix(kanban): active_pr respawn guard yields to deliberate follow-ups and dead PRs

[nikitaBarkov]

What does this PR do?

The Kanban dispatcher's active_pr respawn guard (block "4." of check_respawn_guard in hermes_cli/kanban_db.py) blocked a task's respawn whenever a GitHub PR URL appeared in its comments within the last 24h. It matched the URL by regex only — it never checked the PR's real state (open/closed/merged), and there was no supported way to clear it. So a deliberate follow-up / rework / unblock stayed parked behind the guard until the 24h window expired or someone edited the SQLite DB by hand.

This reworks the guard so it blocks only the unintended duplicate-PR auto-respawn it was built for, while yielding to deliberate continuation and to PRs that are no longer live:

• Pick the newest PR-URL comment in the window (only the latest PR can be duplicated; older links are irrelevant).
• Yield when a fresher deliberate signal exists after that comment — unblocked (from unblock_task), respawn_guard_cleared (from the new clear_respawn_guard), or a review-lane rework verdict: changes_requested (request_changes) and review_reopened (reopen_review_task). Both review verdicts hand the card back to the implementer because of the PR already in its comments, so treating that PR as a duplicate risk parked every review-driven rework until the 24h window expired.
• Yield when the PR is actually closed/merged. Unknown state keeps the guard (safe default — no false "all clear" when gh is unavailable).
• Yield when the task has never run — a task that was never spawned cannot own the PR its comments mention (typically inherited context, e.g. the parent's PR quoted in the briefing), and no unblock/re-queue signal ever arrives to clear it.
• Otherwise still return "active_pr", so ordinary auto-respawn on a live PR is still de-duplicated exactly as before.

It also adds a supported operator override and an opt-in live PR-state check.

Related Issue

Addresses #62418 (guard blocks legitimate rework after unblock, no bypass short of manual claim+spawn) and #29458 (no operator clear-path; ignores closed PRs).

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

• hermes_cli/kanban_db.py — rework block "4." of check_respawn_guard; add helpers _has_fresh_continuation_signal, _resolve_github_pr_state (cached gh lookup, 5-min TTL), _resolve_pr_state_check_enabled; add the public clear_respawn_guard() (emits a respawn_guard_cleared event, no status/claim mutation). New pr_state_resolver arg is keyword-only (back-compat).
• hermes_cli/kanban.py — new unguard verb (parser + dispatch + handler) and /kanban help entry.
• hermes_cli/config.py — opt-in kanban.respawn_guard_check_pr_state (default False) gating the built-in gh-backed live PR-state check.
• website/docs/user-guide/features/kanban.md — rewrote the Respawn-guard section, documented unguard, added the respawn_guard_cleared event.
• tests/hermes_cli/test_kanban_db.py, tests/hermes_cli/test_kanban_cli.py — tests for every branch (see below). The review-verdict tests drive the real lane (claim_task → request_review → claim_review_task → request_changes / reopen_review_task) instead of injecting synthetic events.

How to Test

Run the touched suites:

scripts/run_tests.sh tests/hermes_cli/test_kanban_db.py tests/hermes_cli/test_kanban_cli.py tests/hermes_cli/test_config.py

→ 491 tests pass, 0 failed.

Behavioral coverage:

1. Live PR, no fresh signal → guard still returns "active_pr" (dup protection intact).
2. After unblock / hermes kanban unguard → guard yields (None) on the next dispatcher tick.
3. PR reported closed/merged (via injected pr_state_resolver / opt-in gh) → guard yields; unknown state → guard held.
4. Newest-PR selection; a stale signal before the newest PR still blocks; a same-second tie still blocks.
5. After request_changes / reopen-review on a card whose comments hold an open PR → guard yields, so review-driven rework re-spawns on the next tick.
6. A task with no run history and an inherited PR link → guard yields (its first spawn is not a re-spawn).

Checklist

Code

• My commit messages follow Conventional Commits (fix(kanban):)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix (no unrelated commits)
• I've run the relevant tests and they pass (test_kanban_db, test_kanban_cli, test_config — 491 passed, 0 failed)
• I've added tests for my changes
• I've tested on my platform: macOS (Darwin/arm64)

Documentation & Housekeeping

• I've updated relevant documentation (website/docs/user-guide/features/kanban.md)
• I've added the config key to DEFAULT_CONFIG in hermes_cli/config.py (opt-in, documented in kanban.md)
• N/A — no architecture/workflow changes needing CONTRIBUTING.md / AGENTS.md
• Cross-platform impact considered — the gh lookup degrades safely to "unknown" (guard held) when gh is missing/unauthenticated, and it is off by default
• Updated the CLI surface — new hermes kanban unguard verb + /kanban help entry

[teknium1]

Thanks for tackling a real current-main respawn-guard limitation.

Problems

• hermes_cli/kanban_db.py:7975 requests gh pr view --json state,merged, but the installed CLI rejects merged as an unknown JSON field. The resolver catches that failure and returns None, so configured live-state checks keep active_pr even for closed or merged PRs. gh pr view 72555 --json state,mergedAt succeeds.
• hermes_cli/kanban_db.py:8009 treats an equal integer-second timestamp as a later continuation. Current main writes both comment and event timestamps with int(time.time()) (hermes_cli/kanban_db.py:3525, 3822), in separate tables with no shared ordering key. A prior continuation and a later PR comment in the same second can therefore incorrectly bypass duplicate-PR protection.

Suggested changes

• Use supported gh JSON fields and cover the real CLI response shape.
• Persist or compare a reliable ordering marker, and test both same-second orderings.

Automated hermes-sweeper review.

[teknium1]

gh pr view --json does not expose a merged field: this invocation fails with Unknown JSON field: "merged", so the exception path returns None and a closed/merged PR remains guarded. Query supported fields such as state,mergedAt (or just state) and add coverage for the actual CLI response.

[teknium1]

Equal integer-second timestamps do not prove this event occurred after the PR comment: comments and events are separate tables and both writers use int(time.time()). A prior unblock in the same second as a later PR link can bypass the live-PR guard. Use an ordering marker that establishes causality, or treat equality conservatively and test both same-second orders.

[nikitaBarkov]

Pushed 632a08a — rebased onto current main (conflicts resolved) and both review findings fixed. Both were correct; I verified each against real behavior before changing anything.

1. gh pr view --json state,merged — confirmed, and worse than "the resolver errors out"

$ gh pr view 1 --repo NousResearch/hermes-agent --json state,merged
Unknown JSON field: "merged"

gh (2.87.2) validates --json field names before it ever looks up the PR, so this exited 1 for every PR — _resolve_github_pr_state always returned None, and since unknown state deliberately keeps the guard, the live-PR-state check could never stand the guard down. The opt-in flag was effectively a no-op.

Now state,mergedAt (mergedAt is a timestamp, null while unmerged).

The reason my own tests didn't catch this is worth calling out: they stubbed subprocess.run to return {"state": "MERGED", "merged": true} — a stub that answers a question the real gh refuses to answer. Replaced with a _fake_gh helper that validates the requested field names against gh's actual pr view field set and returns exit 1 + Unknown JSON field otherwise. All five resolver tests now run through it, plus test_resolve_github_pr_state_asks_only_for_fields_gh_has. Every one of them fails against the pre-fix resolver.

2. Same-second ordering — confirmed, fixed with a real ordering marker rather than flipping the comparison

created_at is int(time.time()) in both task_comments and task_events, so an unblocked event and a worker's PR comment can land in the same second and be indistinguishable. My >= resolved that tie as "the unblock is fresher" — which, as you noted, drops duplicate-PR protection in the harmful direction (unblock first, worker's PR comment second).

The two readings aren't symmetric, so I didn't just switch to >:

• Explicit override → order-free. clear_respawn_guard now records cleared_through_comment_id: the task_comments rowid of the newest PR comment at the moment of the call. Rowids are strictly monotonic, so "has this exact PR comment already been acknowledged?" is decidable with no timestamp involved. Bonus correctness: a new PR link posted after the override re-arms the guard, so one unguard no longer disables duplicate-PR protection for the task forever.
• Other kinds (unblocked) → strict >. They're ordinary state transitions with no PR marker, so they still order by timestamp. A tie now keeps guarding, matching the guard's existing safe-default posture (unknown PR state also keeps it). The cost is one deferred tick, and the escape hatch is deterministic (hermes kanban unguard).

New tests cover both orderings inside one second (..._same_second_unblock_before_pr_still_blocks, ..._same_second_unguard_after_pr_clears), the re-arm case, and the persisted marker. The first fails pre-fix; the second passes pre-fix (it was the direction >= happened to get right) and is kept so the fix can't regress it.

Rebase notes

main moved DEFAULT_CONFIG into hermes_cli/config_defaults.py, so kanban.respawn_guard_check_pr_state moved with it. The tests/hermes_cli/test_kanban_cli.py conflict was against the test-pruning waves (6b81590c5, 39975613b) — I took main's side and kept only the two new unguard tests rather than resurrecting the pruned smoke tests.

Suite: scripts/run_tests.sh tests/hermes_cli/ — 4066 passed. The single failure, test_service_manager.py::test_seed_supervise_skeleton_creates_expected_layout, is pre-existing on main (reproduced with my branch stashed) and unrelated.

[nikitaBarkov]

Pushed dbfb4aaf2: the guard no longer blocks a task's first spawn.

What was still broken

This is the third scenario in #29458 (reported by @tuncbahreadingmaterial-ops, v0.18.2), and I verified it is not covered by this PR as it stood, nor by #46204 / #71606 — none of them can be, because all three of us only reason about signals that supersede the PR comment:

A brand-new task with zero task_runs trips active_pr because its briefing comment quotes the parent task's PR URL. Walking check_respawn_guard for that task on the previous head:

• block 1 (rate_limit_cooldown) — needs a run with outcome='rate_limited': no runs, skipped;
• block 2 (blocker_auth) — needs last_failure_error: never failed, skipped;
• block 3 (recent_success) — needs a completed run: skipped;
• block 4 — looked only at task_comments. It matched the URL and returned active_pr.

And nothing ever clears it: an unblocked event or a done → ready transition presupposes a worker that already ran. The task is stuck until the 24h window rolls off — or, if the briefing comment keeps getting re-posted, indefinitely. The reporter says as much: "This case is not fixed solely by treating a later unblocked event as an override."

The fix

Block 4 now stands down when the task has no run history at all. Rationale in one line: the guard exists to stop a re-spawn from duplicating a PR a previous worker opened — with no run history there is no previous worker, so the link is inherited context, not ours.

Two details that are deliberate:

1. Placed inside block 4, not as an early return. A spawn failure can stamp last_failure_error without ever creating a run row, so an early never-ran → None would have silently disabled blocker_auth for exactly the tasks that need it.

2. "Never ran" requires BOTH markers absent: no task_runs row and no spawned event. task_runs alone is not safe. The pre-task_runs migration back-fills a synthetic run only for tasks that were running at upgrade time, so a legacy task that had already opened a PR and gone back to ready reads as zero-runs — and would lose its duplicate-PR protection, i.e. this "fix" would have reintroduced the very duplicate the guard prevents. The event marker is safe against gc_events (30-day retention) because the guard window is 24h: a spawn that produced a PR comment inside the window cannot have been pruned. _has_run_history() short-circuits on the indexed task_runs lookup (idx_runs_task) and only runs for tasks that already matched a PR URL, so the dispatcher hot path is unaffected.

Tests

• ..._never_ran_task_is_not_guarded — never-ran + a resolver reporting open (so the release must come from the missing run history, not from PR state). Fails on the previous head, passes now.
• ..._one_prior_run_still_guards — one finished run and the guard is back. This is the line the release must not cross.
• ..._spawn_event_without_run_row_still_guards — the legacy-DB shape from point 2 above.

Heads-up on a fixture change, since it touches existing tests rather than only adding new ones: the existing active_pr tests described "a prior worker already opened a PR" while creating a task with no runs at all — a state that cannot occur in production. They passed only because block 4 never looked at run history. They now record the finished run their own scenario implies (_add_prior_run, outcome='crashed' so blocks 1 and 3 stay out of the way). Same for test_run_slash_unguard_clears_active_pr_guard in tests/hermes_cli/test_kanban_cli.py. No assertion was weakened — every one of them still asserts active_pr.

scripts/run_tests.sh tests/hermes_cli/test_kanban_db.py tests/hermes_cli/test_kanban_cli.py → 67 passed. Wider tests/hermes_cli/ tests/gateway/ run is clean apart from 7 failures I reproduced on unpatched upstream/main (test_service_manager, test_readiness, test_api_server, test_systemd_notify, test_wecom_callback, test_shutdown_forensics) — pre-existing, unrelated.

Docs updated in the same commit (website/docs/user-guide/features/kanban.md), so the never-ran carve-out is documented alongside the unguard / closed-PR ones.

Coverage of #29458 after this: dead/merged PR ✅ (state,mergedAt), operator "continue anyway" ✅ (unblocked + hermes kanban unguard), never-ran first spawn ✅ (this commit). The remaining ask in that thread is observability (respawn_guarded / skipped_locked in hermes kanban dispatch --json and in gateway logs on zero-spawn ticks) — absent on main today; I'd rather keep it out of this PR and send it as a separate narrow one, since it touches hermes_cli/kanban.py::_cmd_dispatch and gateway/kanban_watchers.py and is asked for as a standalone feature. Happy to do that next if it's wanted.

[GottZ]

This was generated by AI during triage.

Summary

Nine PRs address or reference the active_pr respawn-guard complex across two open issues. Their diffs cover closed/merged PR-state validation, explicit unblock or audited continuation, an operator-clear path, first-spawn false positives, task-owned-link classification, and broad disabling or removal of the URL-derived guard.

Related pull requests

• fix(kanban): check GitHub PR state before applying active_pr respawn guard #29492 [closed] partial — (+34/-0) — n/a: Checks the first matched GitHub PR through the API and releases the guard for closed or merged PRs, but does not cover explicit continuation, operator clearing, or first-spawn false positives. It remains relevant as an early state-check reference implementation whose scope is superseded by fix(kanban): active_pr respawn guard yields to deliberate follow-ups and dead PRs #72555.
• fix(kanban): narrow active PR respawn guard #35593 related — (+250/-3) — n/a: Requires ownership language alongside a PR URL, reducing reference-link false positives, but its contributor keep_open review shows that whole-comment matching can still pair ownership prose with an unrelated URL. Salvage requires URL-local correlation, the requested regression test, and canonical documentation updates.
• kanban: make active_pr respawn guard opt-in (default off) #42003 [closed] partial — (+61/-13) — n/a: Disables active_pr by default through HERMES_KANBAN_RESPAWN_GUARD_ACTIVE_PR, avoiding the wedge while also removing default duplicate-PR protection. It remains relevant as a rejected configuration approach; the automated close verdict cites the repository policy requiring config.yaml rather than a new behavioral environment variable.
• fix(kanban): let PR feedback unblock respawn guard #46204 best fix — (+113/-4) — n/a: Recorded best fix for both issues: uses the latest unblocked event as an inclusive cutoff, allowing deliberate open-PR rework while conservatively guarding same-second and later PR comments. Its high-salvage keep_open verdict supports rebasing the focused active-PR block while preserving the recent_success behavior from 77db9d6bf.
• fix(kanban): stop guarding respawn on PR comment URLs #61196 [closed] partial — (+25/-41) — n/a: Removes URL-derived active_pr state entirely and updates English and Chinese documentation, eliminating the wedge but also the duplicate-PR safeguard. It remains relevant as a tested alternative retained in the author's local fork after being opened upstream by mistake.
• fix(kanban): allow respawn after explicit unblock #62424 partial — (+76/-2) — n/a: Implements the same latest-unblock mechanism as fix(kanban): let PR feedback unblock respawn guard #46204, but its created_at + 1 cutoff can ignore a later PR comment written in the unblock second. Despite its keep_open review, the visible diff retains that reviewed ordering defect, and contributor triage identifies it as a later duplicate of fix(kanban): let PR feedback unblock respawn guard #46204.
• fix(kanban): only guard open pull requests #65948 partial — (+532/-19) — n/a: Queries PR state before guarding, but checks only the first URL in each comment, so a closed URL can hide a later open URL; it also bundles unrelated planning, configuration, workspace, dashboard, and documentation changes. Its contributor keep_open review supports salvaging only a focused split that scans every URL and adds mixed closed-plus-open coverage.
• fix(kanban): let an audited resume clear the active_pr respawn guard #71606 best fix — (+293/-6) — n/a: Recorded best fix for both issues: reads PR comments and audited resume events in one SQLite statement and uses strict ordering so newer unblock, ready-status, or actor-bound manual-promotion events override PR evidence while same-second ambiguity remains guarded. Consistent with its high-salvage keep_open verdict, the visible diff still needs the requested canonical English and Chinese documentation.
• fix(kanban): active_pr respawn guard yields to deliberate follow-ups and dead PRs #72555 best fix — (+1045/-14) — n/a: Recorded best fix for both issues: combines a rowid-bound unguard override, strictly newer unblock handling, supported state,mergedAt lookup with fail-closed unknown state, and run-history checks that release never-run tasks. Despite the existing keep_open review, the current diff addresses both cited blockers with real-field validation and conservative same-second handling; contributor re-review of the corrected head remains the next gate.

Duplicates

#62424 is a direct later duplicate of #46204's latest-unblock mechanism. #29492 overlaps the PR-state portions of #65948 and #72555; #42003 and #61196 are broad guard-disabling alternatives, while #71606 and #72555 extend the unblock family with distinct audited-resume, operator-clear, state-check, or first-spawn handling.

Suggested consolidation

Keep #72555 open with a salvage path: obtain contributor re-review of the corrected state,mergedAt and rowid/strict-order implementation, then split the operator-clear, live-state, and first-spawn corrections if a narrower patch is required. Preserve #46204 through author action to rebase onto main while retaining 77db9d6bf, and keep #71606 open for the requested English and Chinese documentation; close #62424 as duplicate of #46204 despite its keep_open review because the diff retains the contributor-identified +1-second defect. For #35593 and #65948, author action should respectively correlate ownership with each URL and split an all-URL state-check patch from unrelated features; leave #29492, #42003, and #61196 closed as reference or rejected alternatives.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I29458(["issue #29458 (open)"])
    I62418(["issue #62418 (open)"])
    P72555["PR #72555 (open)"]
    P72555 -->|best fix| I29458
    P72555 -->|best fix| I62418
    class I29458 open
    class I62418 open
    class P72555 open
    class P72555 best
    class P72555 best
    class P72555 target
    click I29458 "https://github.com/NousResearch/hermes-agent/issues/29458"
    click I62418 "https://github.com/NousResearch/hermes-agent/issues/62418"
    click P72555 "https://github.com/NousResearch/hermes-agent/pull/72555"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 9 pull requests and 2 issues in this complex. Each diff was read against this issue; Assessment working set: 162 kB of PR diffs, 34 kB of issue/PR text, 27 kB of discussion (24 comments), 23 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[nikitaBarkov]

Re-review requested — corrected head dbfb4aaf2

@teknium1 — I can't set the reviewer field (read-only access here), so this is the re-review request by comment. The triage pass on 2026-08-03 named exactly this as the remaining gate: "contributor re-review of the corrected head remains the next gate." The 2026-07-30 COMMENTED review is still the latest, so mergeStateStatus stays BLOCKED while the PR is MERGEABLE and rebased on current main.

State at head dbfb4aaf2 versus that review:

• Invalid gh field fixed. gh pr view --json state,merged was verified to be broken against a real CLI (gh 2.87.2 → Unknown JSON field: "merged"), so the resolver always raised and returned None and the guard never released. Now state,mergedAt, with unknown state failing closed (guard kept).
• Same-second precedence replaced with a monotonic marker. Ordering no longer relies on whole-second int(time.time()): clear_respawn_guard persists the PR comment rowid it covers (cleared_through_comment_id), and every other continuation signal uses strict >. Tests cover both orderings inside one second.
• New since the review — the never-run case (reported in [Bug]: Kanban active_pr respawn guard has no operator clear path and ignores closed PRs #29458 by @tuncbahreadingmaterial-ops): a task with zero run history was still guarded because its briefing quoted the parent's PR URL, deadlocking its very first spawn. Release requires both markers to be absent (no task_runs row and no spawned event) — the second one matters because the pre-task_runs migration back-fills a synthetic run only for tasks that were running at upgrade time, so a legacy task that already opened a PR would otherwise lose the duplicate-PR protection. The check sits strictly inside block "4.", so a spawn failure without a run row still reaches blocker_auth.

Existing active_pr tests described "a prior worker opened a PR" but created tasks with zero runs — an impossible state; they now seed a finished run (_add_prior_run), with no assertion weakened. scripts/run_tests.sh tests/hermes_cli/test_kanban_db.py tests/hermes_cli/test_kanban_cli.py → 67 passed; the never-run test fails on the previous head.

Deliberately not in this PR: dispatcher observability (respawn_guarded / skipped_locked in hermes kanban dispatch --json and gateway logs). It's a separate file and asked for as a standalone feature — happy to open it as its own narrow PR if you want it.

[nikitaBarkov]

Status refresh, since the sweeper review above is still the last one on this PR and predates the fixes it asked for.

Both review findings were fixed and are still in the head. They were correct; I verified each against real behaviour before changing anything (details in the two comments above):

• gh pr view --json state,merged → state,mergedAt. The old field name made gh exit 1 before it looked the PR up, so _resolve_github_pr_state always returned None and the opt-in live-state check could never stand the guard down — it was a no-op, not just a degraded path. The tests that missed it stubbed an answer the real gh refuses to give; they now go through a _fake_gh that validates the requested field names.
• Same-second ordering → an explicit, order-free marker (cleared_through_comment_id, a task_comments rowid) for the operator override, and strict > for unblocked events so a tie keeps guarding. Both orderings inside one second are covered by tests.

The branch has been rebased twice since (632a08a → dbfb4aaf2 → e99b01e60 → c5b16369e8); the current head is on today's main, checks green, no conflicts.

On overlap with a235d1917e ("skip PR/success respawn guards in review lane"), which landed on main in the meantime: it does not cover this. That commit adds an early return None for lane == "review", placed before the active_pr block; everything in this PR lives inside that block, so the two compose without touching each other, and the review lane never sees the never-ran carve-out. The ready lane — which is what both #62418 and #29458 report — is still the unchanged naive scan on current main:

# 4. GitHub PR URL in a recent comment — prior worker already opened a PR.
pr_cutoff = now - _RESPAWN_GUARD_PR_WINDOW
for c in conn.execute(
    "SELECT body FROM task_comments WHERE task_id = ? AND created_at >= ?",
    (task_id, pr_cutoff),
).fetchall():
    if c["body"] and _RESPAWN_GUARD_PR_URL_RE.search(c["body"]):
        return "active_pr"

No PR state, no clear path, no run history. clear_respawn_guard, respawn_guard_check_pr_state / mergedAt, cleared_through_comment_id and the run-history check exist on this branch only.

One adaptation the rebase needed, called out because it edits an upstream test rather than adding one: test_active_pr_guard_skipped_for_review_lane_but_defers_ready_lane built its ready-lane control task with no run history at all and expected active_pr — i.e. it pinned exactly the first-spawn false positive this PR removes, while its own docstring describes a re-spawn. The control now records a finished prior run (crashed, so the recent_success and rate_limit_cooldown rules stay out of the way), so it asserts the same thing its name and docstring claim. No assertion was weakened: it still expects active_pr on the ready lane and None on the review lane.