Skip to content

fix(kanban): let PR feedback unblock respawn guard - #46204

Open
dannyfranca wants to merge 1 commit into
NousResearch:mainfrom
dannyfranca:fix/kanban-pr-feedback-respawn-guard
Open

fix(kanban): let PR feedback unblock respawn guard#46204
dannyfranca wants to merge 1 commit into
NousResearch:mainfrom
dannyfranca:fix/kanban-pr-feedback-respawn-guard

Conversation

@dannyfranca

Copy link
Copy Markdown

What does this PR do?

Fixes a Kanban dispatcher edge case where a task can remain stuck in ready after being explicitly unblocked for GitHub PR feedback.

Today, the active_pr respawn guard scans recent Kanban comments for raw GitHub PR URLs. That correctly prevents duplicate PR-producing workers, but it also means an older worker handoff comment like https://github.com/.../pull/123 can keep blocking a later bridge/operator unblock whose purpose is to wake the same task to address fresh PR review feedback.

This change makes the active_pr guard use the latest unblocked task event as an additional cutoff:

  • PR URLs from older handoff comments no longer veto an explicit unblock.
  • PR URLs added at or after the unblock still trigger active_pr, preserving the duplicate-PR protection.
  • Same-second comments remain guarded conservatively because Kanban timestamps are second-granular.

This keeps the existing safety behavior without adding new config, environment variables, tools, or manual cleanup commands.

Related Issue

Related to #29458.

This does not close #29458 entirely because that issue also discusses broader operator clear paths and closed-PR handling. This PR fixes the narrower PR-feedback wake-up case where an explicit unblock should supersede older PR handoff URLs.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • hermes_cli/kanban_db.py

    • Updates check_respawn_guard() so active_pr only considers raw GitHub PR URLs at or after the latest unblocked event.
    • Preserves the existing priority/order of the other guard reasons: rate_limit_cooldown, blocker_auth, and recent_success.
    • Keeps same-second PR comments guarded by using created_at >= cutoff, because task events/comments are stored with second-level timestamps.
  • tests/hermes_cli/test_kanban_db.py

    • Adds regression coverage for an old raw PR URL followed by a later explicit unblock.
    • Adds coverage showing PR URLs after the latest unblock still return active_pr.
    • Adds coverage for the same-second unblock/comment case so the guard remains conservative.
    • Adds a dispatch_once() integration test proving a PR-feedback unblock can spawn once the only raw PR URL is older than the unblock.
  • website/docs/user-guide/features/kanban.md

    • Documents the updated active_pr respawn guard semantics.
    • Updates the event reference to explain that older PR handoff URLs are ignored after a deliberate unblock for PR feedback.

How to Test

  1. Run the targeted Kanban DB regression suite:

    python -m pytest tests/hermes_cli/test_kanban_db.py -q

    Observed result:

    218 passed, 1 warning
    
  2. Run the Windows compatibility footgun checker on the changed Python files:

    python scripts/check-windows-footguns.py hermes_cli/kanban_db.py tests/hermes_cli/test_kanban_db.py

    Observed result:

    ✓ No Windows footguns found (2 file(s) scanned).
    
  3. Check whitespace/diff cleanliness:

    git diff --check origin/main..HEAD

    Observed result: no output / clean.

  4. Optional live smoke check on a board with the affected state:

    import sqlite3
    from hermes_cli import kanban_db as kb
    
    conn = sqlite3.connect("~/.hermes/kanban.db")
    conn.row_factory = sqlite3.Row
    print(kb.check_respawn_guard(conn, "<task-id>"))

    A task with only pre-unblock raw PR URLs should now return None; a PR URL at or after the latest unblock should still return active_pr.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
    • Targeted suite run: python -m pytest tests/hermes_cli/test_kanban_db.py -q218 passed, 1 warning
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Ubuntu Linux

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
    • N/A: no config keys added or changed.
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
    • N/A: no architecture or workflow contract changes.
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
    • Ran scripts/check-windows-footguns.py on the changed Python files.
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A
    • N/A: no model-facing tool schema changes.

Screenshots / Logs

Targeted regression suite:

$ python -m pytest tests/hermes_cli/test_kanban_db.py -q
........................................................................ [ 33%]
........................................................................ [ 66%]
........................................................................ [ 99%]
..                                                                       [100%]
218 passed, 1 warning

Windows footgun check:

$ python scripts/check-windows-footguns.py hermes_cli/kanban_db.py tests/hermes_cli/test_kanban_db.py
✓ No Windows footguns found (2 file(s) scanned).

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have labels Jun 14, 2026
@pfrenssen

Copy link
Copy Markdown

This is a great solution for this annoying problem. The approach makes sense. When the user manually unblocks a task, then the mentions of a PR in older messages should be disregarded, only the messages after the manual unblock matter for the next iteration.

I reviewed the PR and tried it out on my local install, and it works perfectly. Thanks very much, this makes reviewing PRs much smoother!

@dannyfranca

Copy link
Copy Markdown
Author

Addresses #29458 by making stale/closed active_pr respawn guards recoverable and adding an explicit operator-clear path for blocked Kanban dispatch.

@dannyfranca

Copy link
Copy Markdown
Author

This is a great solution for this annoying problem. The approach makes sense. When the user manually unblocks a task, then the mentions of a PR in older messages should be disregarded, only the messages after the manual unblock matter for the next iteration.

I reviewed the PR and tried it out on my local install, and it works perfectly. Thanks very much, this makes reviewing PRs much smoother!

Thanks, man. If you work with PRs on Kanban, then this should also be a good combo for you: https://github.com/dannyfranca/hermes-github-pr-kanban-bridge, I use it to automatically react and move the Kanban back to ready when a comment is left.

I initially thoght about doing it webhook-based, but polling is totally fine if you can afford up to a minute delay to get a ticket to ready.

It was initially vibe coded, but I have hardened it over time. Has been pretty stable for me.

cwest added a commit to cwest/hermes-agent that referenced this pull request Jun 28, 2026
The active_pr respawn guard in check_respawn_guard refuses to spawn a
ready task that has a GitHub PR URL in a recent comment (within the 24h
_RESPAWN_GUARD_PR_WINDOW), to prevent a builder re-opening a duplicate
PR. But in the one-card review lifecycle, when a reviewer bounces a card
back to the author to rework the SAME PR, the card lands in
status='ready' still carrying the build-handoff PR-URL comment — so the
guard wedges the author out of spawning for up to 24h and the rework
loop cannot complete.

Use the latest 'unblocked' task event as an additional lower bound on
the PR-comment scan window: PR URLs posted before a deliberate unblock
no longer veto the respawn (the unblock is the operator/orchestrator's
green light to resume work on that PR), while URLs at or after the
unblock still guard against a genuine duplicate-PR. Same-second comments
stay guarded conservatively (timestamps are second-granular).

This is additive over the carried NousResearch#46549 (which bypasses the guard only
for status='review'); the author-rework card is in 'ready', which NousResearch#46549
does not cover. Applied inside the existing `if not is_review:` block so
the two compose.

Carries the mechanism from upstream PR NousResearch#46204 by dannyfranca, adapted to
this fork's line context (our base already wraps the active_pr block in
the NousResearch#46549 is_review guard, so a raw cherry-pick would conflict). Adds 3
invariant tests and the matching docstring + kanban.md docs. Tracked in
PATCHES.md and docs/patches/2026-06-28-pr46204-active-pr-unblock-cutoff.md;
auto-retires when NousResearch#46204 merges upstream.

Upstream-PR: NousResearch#46204
Upstream-Issue: NousResearch#29458

Co-authored-by: dannyfranca <dannyfranca@users.noreply.github.com>
cwest added a commit to cwest/hermes-agent that referenced this pull request Jul 1, 2026
Make the active_pr respawn guard honor an explicit unblock so the review->author
rework loop can spawn without waiting out the 24h PR window. Uses the latest
unblocked task event as an additional lower bound on the PR-comment scan window
(pr_cutoff = max(window, latest_unblock_ts)): PR URLs posted before a deliberate
unblock no longer veto respawn, while URLs at/after still guard. Additive over
NousResearch#46549 (which covers status='review' only; the author-rework card is in 'ready').

upstream-pending: PR NousResearch#46204
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused respawn-guard fix. The premise still reproduces on current main: check_respawn_guard() scans all PR URL comments in the 24-hour window at hermes_cli/kanban_db.py:7105-7112, without considering the unblocked event emitted by unblock_task() at hermes_cli/kanban_db.py:5110-5113.

The proposed latest-unblock cutoff is a narrow fit for that defect and the added before/after/same-second cases cover the intended boundary. Current main has since added the adjacent recent_success explicit-requeue exception in 77db9d6bf (hermes_cli/kanban_db.py:7080-7103), so salvage should retain that behavior and apply this change only to the following active-PR cutoff block.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 14, 2026
@agent007x1

Copy link
Copy Markdown

Nice fix — the unblock cutoff is the right shape for the rework case, and I'd like to see it land.

One gap worth naming, since this PR and #29458 are becoming the focal point for the guard: both the cutoff here and the PR-state validation discussed there assume the matched URL belongs to the task's own PR. There's a third case where neither helps — a PR URL that arrives inside content the task fetched (release notes, a quoted changelog, an ingested web document). The task never opened a PR and was never blocked, so there is no unblocked event for the cutoff to anchor on; the card simply parks for 24 h on its first dispatch. We hit this in the wild with a fetched GitHub release body citing an unrelated PR.

I filed that, plus an operator-facing window override, as #67249. I see the deliberate "no new config" stance here and I'm not suggesting this PR change — the knob would be a separate change; patch and tests are in that issue if it's wanted.

cwest added a commit to cwest/hermes-agent that referenced this pull request Jul 26, 2026
Make the active_pr respawn guard honor an explicit unblock so the review->author
rework loop can spawn without waiting out the 24h PR window. Uses the latest
unblocked task event as an additional lower bound on the PR-comment scan window
(pr_cutoff = max(window, latest_unblock_ts)): PR URLs posted before a deliberate
unblock no longer veto respawn, while URLs at/after still guard. Additive over
NousResearch#46549 (which covers status='review' only; the author-rework card is in 'ready').

upstream-pending: PR NousResearch#46204
(cherry picked from commit e741b78)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Kanban active_pr respawn guard has no operator clear path and ignores closed PRs

5 participants