fix(kanban): narrow active PR respawn guard - #35593
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Review
A well-diagnosed fix for the kanban respawn guard being too broad — treats upstream/reference PR URLs as task-owned PRs, wedging tasks in ready state.
✅ Looks Good
- Correct diagnosis: SDD clearly identifies the scope creep (bare PR URL match vs PR URL + ownership language).
- Narrow regex: Explicit ownership/opening keywords required; bare
pr_urlkeys explicitly excluded. - Thorough tests: 6 regression tests — positive (task-owned) and negative (upstream/reference) cases, including dispatcher integration.
- Dialectical review artifacts: SDD, B-Team feedback, outside-family review — process evidence.
- Claude critique incorporated:
pr_url/pull_request_urlkeys removed from ownership hints per review.
Reviewed by Hermes Agent (cron job)
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅ — Narrow the active PR respawn guard in kanban. Clean, focused fix.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying a real respawn-guard false positive. Current main still returns active_pr for any recent PR URL at hermes_cli/kanban_db.py:7105-7112.
Problems
hermes_cli/kanban_db.py:4685-4688independently matches ownership language and a PR URL anywhere in the same comment. A handoff sayingOpened a PR for investigation; upstream reference: https://github.com/acme/project/pull/42would still guard even though its only URL is upstream evidence. This does not meet the stated task-owned-PR condition.- The change leaves the user guide inconsistent:
website/docs/user-guide/features/kanban.md:706and:927still documentactive_pras any recent PR link.
Suggested changes
- Correlate ownership language with the particular URL, such as by parsing a structured task-owned field or requiring both signals in one bounded text segment; add a regression for separated ownership wording and an upstream URL.
- Update both canonical Kanban documentation entries to describe the final predicate.
Current check_respawn_guard has also gained rate-limit and explicit-requeue paths at hermes_cli/kanban_db.py:7039-7103; salvage should preserve those branches. This is an automated hermes-sweeper review.
| return bool( | ||
| body | ||
| and _RESPAWN_GUARD_PR_URL_RE.search(body) | ||
| and _RESPAWN_GUARD_TASK_PR_HINT_RE.search(body) |
There was a problem hiding this comment.
This is a comment-global conjunction, not a relationship between the ownership claim and this URL. A handoff can say Opened a PR for investigation and separately cite an upstream PR URL, which still returns True. Please bind ownership evidence to the matched URL/field and add that mixed-comment regression.
Summary
Fixes the Kanban dispatcher
active_prrespawn guard so it only blocks respawn when a recent task comment actually indicates that this task opened/created/submitted a PR.Before this change, any recent GitHub PR URL in any task comment triggered
active_pr. That caused operator-hostile behavior: a worker handoff could cite an upstream/reference PR underupstream_checked, the dashboard Nudge dispatcher button would wake the dispatcher, and the task would still sit inreadybecause the dispatcher silently skipped it asrespawn_guarded: active_pr.The duplicate-PR guard is still preserved for explicit task-owned PR comments such as:
PR created: https://github.com/org/repo/pull/42Opened https://github.com/org/repo/pull/42created_pr,opened_pr,submitted_pr, ortask_prBut upstream/reference/evidence PR URLs no longer wedge a ready task.
SDD
Included in this PR:
DOCS/planning/SDD_kanban_pr_respawn_guard.mdCore diagnosis from the SDD:
Resolved Code
Changed
hermes_cli/kanban_db.py:_RESPAWN_GUARD_TASK_PR_HINT_REfor explicit ownership/opening language._comment_indicates_task_owned_pr(body).check_respawn_guard(...)to returnactive_pronly when both are true:This keeps the guard from treating
upstream_checked,related PR, or evidence-only PR links as task-owned PRs.Tests
Added regression coverage in
tests/hermes_cli/test_kanban_db.py:test_respawn_guard_upstream_pr_reference_not_guardedtest_respawn_guard_related_pr_reference_not_guardedtest_respawn_guard_upstream_pr_url_key_not_guardedtest_dispatch_respawn_guard_spawns_with_upstream_pr_referencePreserved existing positive coverage:
test_respawn_guard_active_pr_in_commenttest_dispatch_respawn_guard_skips_active_prVerification
RED evidence:
AssertionError: assert 'active_pr' is None.res.respawn_guarded == [(task_id, 'active_pr')].pr_urlnegative test was added after outside-family review and failed before resolving the ambiguous key behavior.GREEN evidence:
Thunderdome / Dialectical Review
Included review artifacts:
DOCS/planning/autocode/B_TEAM_FEEDBACK.mdDOCS/planning/autocode/OUTSIDE_FAMILY_SECONDARY_DIALECTIC_REVIEW.mdDOCS/planning/autocode/DEEPSEEK_SECONDARY_DIALECTIC_REVIEW.mdOutside-family Claude review verdict:
APPROVE.Claude raised one concrete critique: bare
pr_url/pull_request_urlkeys can be upstream evidence and should not count as task ownership. That critique was resolved in this PR by addingtest_respawn_guard_upstream_pr_url_key_not_guardedand removing barepr_url/pull_request_urlfrom the ownership-hint regex.DeepSeek/Pi review was attempted but exceeded the 300-second foreground timeout and produced no usable output; the unavailability is recorded in the artifact per the Thunderdome fallback rule.
Related