-
Notifications
You must be signed in to change notification settings - Fork 46.7k
fix(kanban): narrow active PR respawn guard #35593
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.
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
Open
vmlinuzx
wants to merge
1
commit into
NousResearch:main
Choose a base branch
from
vmlinuzx:fix/kanban-pr-respawn-guard
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 <github PR URL>`, 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 |
11 changes: 11 additions & 0 deletions
11
DOCS/planning/autocode/DEEPSEEK_SECONDARY_DIALECTIC_REVIEW.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
30 changes: 30 additions & 0 deletions
30
DOCS/planning/autocode/OUTSIDE_FAMILY_SECONDARY_DIALECTIC_REVIEW.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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: <url>` 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: <url>` 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a comment-global conjunction, not a relationship between the ownership claim and this URL. A handoff can say
Opened a PR for investigationand separately cite an upstream PR URL, which still returnsTrue. Please bind ownership evidence to the matched URL/field and add that mixed-comment regression.