feat(kanban): release active-PR respawn guard on reviewer feedback (#2)

[aliaadil]

Closes hermes-agent#2 (extends #1).

Problem

The 24h active_pr respawn guard in hermes_cli/kanban_db.py:check_respawn_guard silently swallowed reviewer feedback on an open PR. Reproduced 2026-08-20 on t_de993dac (alias aliaadil/alerthq#174, PR aliaadil/alerthq#178): the operator left a 19:10:04 UTC reviewer comment on the PR, but the task sat in active_pr for 45 minutes because the guard has no exception for "reviewer feedback exists after that PR URL."

Combined with the mirror bug (PR-thread comments never reached the kanban — fixed separately in /opt/data/scripts/github_issues_mirror.py, no PR per operator clarification), the reviewer-feedback loop was silently broken.

Fix

Part B — guard release triggers

In check_respawn_guard, before returning "active_pr", check whether any comment whose created_at is after the most recent PR-URL comment satisfies any of:

• non-default author AND len(body) >= 80 (rules out auto-mirrored status pings — they're default-authored and short)
• distinct _content_key from the prior PR-URL comment (idempotent re-push does NOT release)
• body matches reviewer-feedback patterns: contains please update, fix in this pr, also address, needs to, this pr, not sufficient, all actions, all clicks, please add, please include, OR references the PR number directly (#178 / PR #178 / pull/178)
• linked PR has reviewDecision == 'CHANGES_REQUESTED' (via gh pr view <url> --json reviewDecision)

If any trigger fires, return None (allow the respawn). Otherwise keep "active_pr".

Word-boundary tightening

this pr was matching inside priority / this private method, and all actions inside hall actions / small action. The legacy alternation compiled each phrase with re.escape but didn't anchor on word boundaries. New _compile_word_boundary_re tokenizes each phrase on whitespace and wraps each token in \b — multi-word phrases still match their original prose (please update the doc → match), but substrings inside longer words don't. Verified against the four existing pattern-match tests + 3 new substring-false-positive guards + 1 phrase-still-matches regression test.

Branch-routing override

When the guard releases due to reviewer feedback, the dispatched Builder must use the same branch and head SHA as the most recent PR-URL comment — not a fresh feat/<task-id>-<new-slug> branch. Implementation:

• TTL-cached (5 min) gh pr view <url> --json headRefName,headRefOid lookup, gated to the worktree workspace_kind
• Pass (branch, head_sha) through the new pr_head_sha spawn-kwarg (no parent-process env mutation → no SHA leak between tasks in the same tick)
• Worker sees HERMES_KANBAN_PR_HEAD_SHA env var and can git reset --hard to that exact commit

Tests

tests/hermes_cli/test_kanban_review_lifecycle.py:

• 6 trigger conditions (substantive non-default author + length, body pattern, PR-number reference, distinct content_key, reviewDecision CHANGES_REQUESTED, short author + pattern match)
• 4 non-trigger conditions (auto-mirrored default pings, short default comment, APPROVED reviewDecision, feedback before PR URL)
• 2 substring false-positives (priority/private, hall actions)
• 1 phrase-still-matches regression guard
• 3 branch-override tests (override populated, override NOT populated, dispatch consumes + branch rename + env-leak regression)

tests/scripts/test_github_issues_mirror.py (NEW):

• GH_REF_RE matches both /issues/<N> and /pull/<N> URLs
• _pick_ref recency tie-break (freshest across body+title+comments wins, full-URL beats short-ref on ties)
• _secondary_ref_for_task dual-ref pairing (issue ↔ PR)
• mirror_pull appends from both refs
• Dedup by (owner, repo, N, comment_id) across endpoints
• Sidecar persistence (state survives across calls)

Sweep: scripts/run_tests.sh tests/hermes_cli/{test_kanban_db,test_kanban_review_lifecycle,test_kanban_review_lifecycle_complete,test_kanban_review_surfaces,test_kanban_db_init,test_kanban_db_repair,test_kanban_block_kinds,test_kanban_reclaim_claim_lock_guard,test_kanban_blocked_sticky,test_kanban_dispatch_lock,test_kanban_write_txn_busy_retry} tests/scripts/test_github_issues_mirror.py → 128 passed, 0 failed, 1 skipped (windows_only).

Out of scope

• Changes to _RESPAWN_GUARD_PR_WINDOW (24h stays)
• Adding a separate reviewer-feedback lane
• Switching to repos/{o}/{r}/pulls/{n}/comments (issue-style endpoint covers threaded discussion; review-submission comments remain a follow-up if data shows that's the dominant feedback channel)

Part A (mirror) landed in /opt/data/scripts/github_issues_mirror.py directly per operator clarification (deployment path, no PR).

Risk

• gh pr view adds ~100ms per guard-release check (cached for 5min); tick budget stays well under the 60s dispatcher tick
• Worker sees HERMES_KANBAN_PR_HEAD_SHA as the ONLY way to receive the head SHA — no env leakage
• The TTL cache for _REVIEW_DECISION_CACHE and _BRANCH_OVERRIDE_CACHE is process-local and TTL-bounded; reviewer feedback that arrives during a 5min cache window won't be re-detected until the cache expires (acceptable — re-spawn is at most one tick behind the comment)

[Enough1122]

AI code review — automated review for reference; please use your judgment.

Excellent spec discipline (empirical repro timeline, explicit non-trigger criteria) and the implementation mostly honors it — the per-word-boundary phrase compiler kills the real substring false positives ("this priority", "hall actions"), dual-ref mirroring with cross-endpoint dedupe is tested, and routing branch+SHA through a spawn kwarg instead of process env avoids the cross-task leak. Two trigger-design holes and two smaller points:

1. hermes_cli/kanban_db.py (_check_reviewer_feedback_release, trigger 3) — _REVIEWER_FEEDBACK_PR_NUM_RE matches any #\d+/PR #N/pull/N, and unlike trigger 1 it is applied author-agnostic. A default-authored auto-mirrored status ping that happens to say "PR fix(install): ignore commented lines when checking for PATH #178 opened" therefore releases the guard — directly violating the spec's own acceptance criterion "auto-mirrored default-authored status comments do NOT release". Either gate trigger 3 on author != 'default' like trigger 1, or require the referenced number to match the linked PR's number and a human author. The existing tests pass because their default-authored fixtures avoid #N strings — add one where the ping contains the PR number.
2. kanban_db.py (check_respawn_guard release path) — nothing consumes reviewer feedback. Trigger 1 fires on any non-default comment ≥80 chars after the breadcrumb (including the builder's own mirrored follow-ups if they're authored under a bot id), and after a released respawn fails without a new PR-URL comment, the same stale feedback re-releases the guard on every subsequent tick — a respawn loop bounded only by the failure circuit breaker. Suggest appending a respawn_released (reason=reviewer_feedback) event at release time and requiring feedback newer than the last release, which also gives you the audit trail the current silent return None lacks.
3. kanban_db.py:_query_pr_review_decision — a blocking subprocess.run(timeout=10) now sits inside check_respawn_guard. If that call runs on the gateway event loop thread (rather than via the dispatcher's to_thread path), one cold gh invocation per PR per 5-minute window stalls all dispatching for up to 10s. Worth confirming the caller context or moving the gh probe out of the synchronous guard.
4. kanban_db.py:_any_spawnable_review (~10613) — the branch override is popped and then applied only for workspace_kind == "worktree"; for dir/scratch tasks it disappears without a trace and the run proceeds from whatever base it has. A debug log (or refusing release for non-worktree tasks up front) would make that path diagnosable. (nit)
5. _REVIEW_DECISION_CACHE / _BRANCH_OVERRIDE_CACHE grow without eviction (entries expire logically but are never deleted); trivial at kanban scale, just noting. (nit)

No blocking issues found beyond items 1–2, which together decide whether this ships as a fix or as a new silent-respawn generator.

[aliaadil]

Pushed commit dade612 addressing both blocking findings from the AI code review.

Issue #1 — author-agnostic PR-number regex. trigger 3 could fire on a default-authored auto-mirrored ping like "📌 PR #178 opened", violating the spec's "auto-mirrored default-authored status comments do NOT release" criterion and creating a respawn loop. Fix: _has_reviewer_feedback now takes author=; the PR-number regex and pr_url match are skipped when author == "default". Phrase matches (e.g. "please update", "fix in this pr") stay author-agnostic because they're concrete reviewer signals that don't appear in mirrored status pings.

Issue #2 — respawn loop on stale feedback. After a release, the same stale feedback would re-fire trigger 1/3 on every subsequent tick — bounded only by the failure circuit breaker. Fix: each release now emits a respawn_released event with feedback_at and trigger=a/b/c/d. Subsequent ticks read MAX(feedback_at) as a watermark and skip comments at or before it. Trigger 4 (CHANGES_REQUESTED) gated on the same watermark, with now() so a follow-up comment after the decision still re-releases.

Audit trail. The new respawn_released event carries reason, feedback_at, trigger so operators can trace why a spawn was allowed from hermes kanban tail without grepping logs.

Polish. _dispatch_once_locked branch-override pop now logs a debug message when the override is dropped for non-worktree tasks (was previously silent).

Tests. +17 new tests (4 default-author PR-number ping forms don't release; stale-feedback watermark prevents the respawn loop; newer feedback re-releases; trigger-4 dedupe after first release; default ping doesn't advance the watermark; release event audit fields; same-timestamp re-mirror doesn't re-release). 46/46 review-lifecycle tests pass. Ruff clean. git diff --check clean.

PR is ready for re-review.