From 34e69daf5790c3393d977038e5fd37a6b2464b52 Mon Sep 17 00:00:00 2001 From: David Carroll Date: Sat, 30 May 2026 19:04:03 -0500 Subject: [PATCH] fix(kanban): narrow active PR respawn guard --- DOCS/planning/SDD_kanban_pr_respawn_guard.md | 63 +++++++++++++++ DOCS/planning/autocode/B_TEAM_FEEDBACK.md | 39 ++++++++++ .../DEEPSEEK_SECONDARY_DIALECTIC_REVIEW.md | 11 +++ ...TSIDE_FAMILY_SECONDARY_DIALECTIC_REVIEW.md | 30 ++++++++ hermes_cli/kanban_db.py | 34 ++++++++- tests/hermes_cli/test_kanban_db.py | 76 ++++++++++++++++++- 6 files changed, 250 insertions(+), 3 deletions(-) create mode 100644 DOCS/planning/SDD_kanban_pr_respawn_guard.md create mode 100644 DOCS/planning/autocode/B_TEAM_FEEDBACK.md create mode 100644 DOCS/planning/autocode/DEEPSEEK_SECONDARY_DIALECTIC_REVIEW.md create mode 100644 DOCS/planning/autocode/OUTSIDE_FAMILY_SECONDARY_DIALECTIC_REVIEW.md diff --git a/DOCS/planning/SDD_kanban_pr_respawn_guard.md b/DOCS/planning/SDD_kanban_pr_respawn_guard.md new file mode 100644 index 000000000000..706e7a092f57 --- /dev/null +++ b/DOCS/planning/SDD_kanban_pr_respawn_guard.md @@ -0,0 +1,63 @@ +# SDD: Kanban respawn guard should only treat task-owned PRs as active PRs + +## Problem + +The Kanban dispatcher has a respawn guard that returns `active_pr` whenever any recent task comment contains a GitHub pull request URL. The intent is valid: avoid respawning a worker that already opened a PR for the task and might create duplicate PR spam. + +The current predicate is too broad. Worker handoffs often include upstream reconnaissance such as related issues or upstream PRs. A comment that merely records an upstream/reference PR is not evidence that the worker opened a task-owned PR. Treating any PR URL as a task-owned PR leaves the task stuck in `ready`: dashboard nudge wakes the dispatcher, the dispatcher skips with `active_pr`, and the operator experiences the button as doing nothing. + +## Reproduction + +A ready task has a recent handoff comment containing structured evidence like: + +```json +{ + "upstream_checked": [ + "https://github.com/NousResearch/hermes-agent/issues/35542 is OPEN and matches the bug", + "https://github.com/NousResearch/hermes-agent/pull/35544 is OPEN and implements a related narrow fix" + ] +} +``` + +Actual behavior: `check_respawn_guard(...) == "active_pr"` and `dispatch_once(...)` skips the task. + +Expected behavior: upstream/reference PR URLs do not trigger `active_pr`; the dispatcher may spawn the ready task normally. + +## Desired Behavior + +### Guard active PRs when a comment explicitly says this task produced/opened a PR + +Examples that should still guard: + +- `PR created: https://github.com/org/repo/pull/42` +- `Opened https://github.com/org/repo/pull/42` +- structured text using task-owned labels such as `created_pr`, `opened_pr`, `submitted_pr`, or `task_pr` + +Bare keys such as `pr_url` or `pull_request_url` are intentionally not sufficient by themselves because they are frequently used for upstream/reference evidence. + +### Do not guard reference/upstream/evidence PR URLs + +Examples that should not guard: + +- JSON or prose under `upstream_checked` +- `related_prs` +- `references` +- `evidence` +- comments saying a PR was checked, reviewed, referenced, or matched upstream behavior + +## Implementation Plan + +1. Add a small helper in `hermes_cli/kanban_db.py` that classifies whether a comment indicates a task-owned PR. +2. Preserve the existing PR URL regex, but require explicit ownership/opening language rather than any URL match. +3. Update `check_respawn_guard` to call the helper. +4. Add regression tests covering: + - explicit task-owned PR comments still trigger `active_pr`; + - upstream/reference PR comments do not trigger `active_pr`; + - dispatcher spawns ready tasks when the only PR URL is an upstream/reference PR. + +## Acceptance Criteria + +- Existing active-PR duplicate-prevention behavior remains for explicit worker-created PR comments. +- Upstream/reference PR comments no longer block respawn. +- Targeted Kanban tests pass. +- The PR description includes this SDD, the diagnosis, and the resolved code behavior. diff --git a/DOCS/planning/autocode/B_TEAM_FEEDBACK.md b/DOCS/planning/autocode/B_TEAM_FEEDBACK.md new file mode 100644 index 000000000000..3ec7b014f792 --- /dev/null +++ b/DOCS/planning/autocode/B_TEAM_FEEDBACK.md @@ -0,0 +1,39 @@ +# Thunderdome B-Team Feedback: Kanban PR Respawn Guard + +## A-Team / Builder Summary + +Implemented a narrow guard predicate for Kanban dispatcher respawn protection: + +- Previous behavior: any recent GitHub PR URL in task comments triggered `active_pr`. +- New behavior: a recent GitHub PR URL only triggers `active_pr` when the same comment includes task-ownership/opening language such as `PR created`, `Opened `, or explicit task-owned keys such as `created_pr`, `opened_pr`, `submitted_pr`, or `task_pr`. +- Upstream/reference/evidence PR links no longer wedge ready tasks behind `active_pr`. + +## Verification Evidence + +- RED: new upstream/reference PR tests failed under the old predicate. +- GREEN: after implementation, the focused tests passed. +- Full targeted module: `209 passed in 7.50s` for `tests/hermes_cli/test_kanban_db.py`. +- Lint: `ruff check hermes_cli/kanban_db.py tests/hermes_cli/test_kanban_db.py` passed. + +## Outside-Family B-Team Critic (Claude Opus 4.7) + +Claude reviewed the diff and SDD read-only. It approved the direction but identified one concrete ambiguity: bare `pr_url` / `pull_request_url` keys are too noisy because they may appear in upstream evidence metadata. + +Resolution applied by host: + +- Added RED test `test_respawn_guard_upstream_pr_url_key_not_guarded`. +- Verified it failed under the initial implementation. +- Removed bare `pr_url` / `pull_request_url` from the ownership-hint regex. +- Re-ran targeted tests and lint successfully. + +OUTSIDE_FAMILY_B_TEAM_VERDICT: APPROVE + +## DeepSeek B-Team Critic + +DeepSeek/Pi review was attempted with `pi --provider deepseek --model deepseek-v4-pro`, but it exceeded the 300-second foreground timeout and wrote no usable output. Per Thunderdome fallback rules, unavailability is recorded and the loop proceeds with outside-family review plus local verification. + +DEEPSEEK_B_TEAM_VERDICT: UNAVAILABLE + +## Verdict + +VERDICT: APPROVED diff --git a/DOCS/planning/autocode/DEEPSEEK_SECONDARY_DIALECTIC_REVIEW.md b/DOCS/planning/autocode/DEEPSEEK_SECONDARY_DIALECTIC_REVIEW.md new file mode 100644 index 000000000000..c9cbc278c5fa --- /dev/null +++ b/DOCS/planning/autocode/DEEPSEEK_SECONDARY_DIALECTIC_REVIEW.md @@ -0,0 +1,11 @@ +# DeepSeek Secondary Dialectic Review + +DeepSeek/Pi review was attempted with: + +```bash +pi --provider deepseek --model deepseek-v4-pro --no-session --tools read,bash,grep,find,ls --print ... +``` + +The command exceeded the 300-second foreground tool timeout and wrote no usable output before termination. Per the Thunderdome fallback rule, this unavailability is recorded and the loop continues with the outside-family review and local verification evidence. + +DEEPSEEK_SECONDARY_VERDICT: UNAVAILABLE diff --git a/DOCS/planning/autocode/OUTSIDE_FAMILY_SECONDARY_DIALECTIC_REVIEW.md b/DOCS/planning/autocode/OUTSIDE_FAMILY_SECONDARY_DIALECTIC_REVIEW.md new file mode 100644 index 000000000000..1e27a901ae67 --- /dev/null +++ b/DOCS/planning/autocode/OUTSIDE_FAMILY_SECONDARY_DIALECTIC_REVIEW.md @@ -0,0 +1,30 @@ +# Outside-Family Secondary Dialectic Review (Claude Opus 4.7) + +Findings: + +- **M-1 — `pr_url` JSON key is the noisiest signal in the ownership hint alternation.** In real handoff JSON, `pr_url` / `pull_request_url` may describe upstream or evidence PRs, not a task-owned PR. Add a negative test and avoid treating bare `pr_url` as ownership proof. +- **M-2 — Narrowing is English-only and could over-fix.** Legitimate human comments may use phrasings not covered by the regex, but this fails in the less harmful direction: duplicate-PR prevention may be weaker, while dispatcher wedging is avoided. Canonical `PR created: ` remains covered. +- **L-1 — Cross-clause matching.** URL and ownership hint currently search the whole body independently; a long mixed comment can still be ambiguous. +- **L-2 — No diagnostic for dropped non-owned PR references.** Low priority. +- **L-3 — Spawn-positive test could assert no guard more tightly.** Low priority. + +Strongest case for acceptance: + +- Direction is correct: moving from “any PR URL” to “PR URL plus ownership language” fixes the operator-hostile wedge. +- Canonical worker path `PR created: ` remains guarded. +- Tests reproduce the user-reported upstream/reference PR failure at both direct guard and dispatcher levels. + +Strongest case against acceptance: + +- Regex-on-prose is still a heuristic. Longer-term, task-owned PRs should be structured state or tagged events, not inferred from free-form comments. +- Bare `pr_url`/`pull_request_url` keys are ambiguous and should not be sufficient ownership evidence. + +OUTSIDE_FAMILY_SECONDARY_VERDICT: APPROVE + +Host resolution: + +- Accepted the `pr_url` critique. +- Added a negative regression test for bare upstream `pr_url` JSON. +- Removed ambiguous bare `pr_url` and `pull_request_url` from the ownership-hint regex; retained explicit `created_pr`, `opened_pr`, `submitted_pr`, and `task_pr` keys. + +CLAUDE_SUBAGENT_DONE diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 4711655249d2..f5f1450b400a 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -4658,6 +4658,36 @@ def schedule_task( re.IGNORECASE, ) +# A PR URL is only respawn-guard evidence when the surrounding comment says +# this task opened/created/submitted that PR. Worker handoffs often cite +# upstream/reference PRs as investigation evidence; those links must not wedge +# a ready task behind ``active_pr``. +_RESPAWN_GUARD_TASK_PR_HINT_RE = re.compile( + r""" + (?: + \b(?:pr|pull\s+request)\s*(?:created|opened|submitted|filed)\b + | \b(?:created|opened|submitted|filed)\s+(?:a\s+)?(?:pr|pull\s+request)\b + | \b(?:created|opened|submitted|filed)\s+https?://github\.com/ + | \b(?:created_pr|opened_pr|submitted_pr|task_pr)\b + ) + """, + re.IGNORECASE | re.VERBOSE, +) + + +def _comment_indicates_task_owned_pr(body: str) -> bool: + """Return True when ``body`` says this task opened/created a GitHub PR. + + The respawn guard prevents duplicate PR spam after a worker opens a PR. + It should not trigger on upstream, related, or evidence links that merely + mention someone else's PR. + """ + return bool( + body + and _RESPAWN_GUARD_PR_URL_RE.search(body) + and _RESPAWN_GUARD_TASK_PR_HINT_RE.search(body) + ) + @dataclass class DispatchResult: @@ -5641,13 +5671,13 @@ def check_respawn_guard(conn: sqlite3.Connection, task_id: str) -> Optional[str] ).fetchone(): return "recent_success" - # 3. GitHub PR URL in a recent comment — prior worker already opened a PR. + # 3. GitHub PR URL in a recent task-owned PR 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"]): + if _comment_indicates_task_owned_pr(c["body"] or ""): return "active_pr" return None diff --git a/tests/hermes_cli/test_kanban_db.py b/tests/hermes_cli/test_kanban_db.py index b2510855ea25..141aced5a0fa 100644 --- a/tests/hermes_cli/test_kanban_db.py +++ b/tests/hermes_cli/test_kanban_db.py @@ -1510,7 +1510,7 @@ def test_respawn_guard_stale_success_not_guarded(kanban_home): def test_respawn_guard_active_pr_in_comment(kanban_home): - """A GitHub PR URL in a recent comment triggers active_pr.""" + """A task-owned GitHub PR URL in a recent comment triggers active_pr.""" with kb.connect() as conn: t = kb.create_task(conn, title="has-pr", assignee="alice") kb.add_comment( @@ -1521,6 +1521,55 @@ def test_respawn_guard_active_pr_in_comment(kanban_home): assert reason == "active_pr" +def test_respawn_guard_upstream_pr_reference_not_guarded(kanban_home): + """A referenced upstream PR is evidence, not proof this task opened a PR.""" + with kb.connect() as conn: + t = kb.create_task(conn, title="references-upstream-pr", assignee="alice") + kb.add_comment( + conn, + t, + "worker", + "review-required handoff:\n" + "{\n" + ' "upstream_checked": [\n' + ' "https://github.com/NousResearch/hermes-agent/issues/35542 is OPEN",\n' + ' "https://github.com/NousResearch/hermes-agent/pull/35544 is OPEN and implements the same narrow fix"\n' + " ]\n" + "}", + ) + reason = kb.check_respawn_guard(conn, t) + assert reason is None + + +def test_respawn_guard_related_pr_reference_not_guarded(kanban_home): + """Related/reference PR links should not be mistaken for task-owned PRs.""" + with kb.connect() as conn: + t = kb.create_task(conn, title="related-pr-only", assignee="alice") + kb.add_comment( + conn, + t, + "reviewer", + "Related PR for context: https://github.com/acme/project/pull/123", + ) + reason = kb.check_respawn_guard(conn, t) + assert reason is None + + +def test_respawn_guard_upstream_pr_url_key_not_guarded(kanban_home): + """A bare pr_url key can describe upstream evidence, not task ownership.""" + with kb.connect() as conn: + t = kb.create_task(conn, title="upstream-pr-url-key", assignee="alice") + kb.add_comment( + conn, + t, + "worker", + 'review evidence: {"upstream": {"pr_url": ' + '"https://github.com/acme/project/pull/456"}}', + ) + reason = kb.check_respawn_guard(conn, t) + assert reason is None + + def test_respawn_guard_old_pr_comment_not_guarded(kanban_home): """A GitHub PR URL in a comment older than the PR window does not block.""" with kb.connect() as conn: @@ -1631,6 +1680,31 @@ def fake_spawn(task, workspace): assert kb.get_task(conn, t).status == "ready" +def test_dispatch_respawn_guard_spawns_with_upstream_pr_reference( + kanban_home, all_assignees_spawnable +): + """dispatch_once should not skip merely because a comment cites upstream PRs.""" + spawned_ids = [] + + def fake_spawn(task, workspace): + spawned_ids.append(task.id) + + with kb.connect() as conn: + t = kb.create_task(conn, title="has-reference-pr", assignee="alice") + kb.add_comment( + conn, + t, + "worker", + 'review-required handoff: {"upstream_checked": [' + '"https://github.com/NousResearch/hermes-agent/pull/35544 is OPEN"]}', + ) + res = kb.dispatch_once(conn, spawn_fn=fake_spawn) + + assert (t, "active_pr") not in res.respawn_guarded + assert t in spawned_ids + assert t not in res.auto_blocked + + def test_dispatch_respawn_guard_dry_run_no_auto_block( kanban_home, all_assignees_spawnable ):