Skip to content

fix(kanban): let card lifecycle govern PR follow-ups - #70240

Draft
rmacbot wants to merge 1 commit into
NousResearch:mainfrom
rmacbot:fix/kanban-pr-lifecycle-admission
Draft

fix(kanban): let card lifecycle govern PR follow-ups#70240
rmacbot wants to merge 1 commit into
NousResearch:mainfrom
rmacbot:fix/kanban-pr-lifecycle-admission

Conversation

@rmacbot

@rmacbot rmacbot commented Jul 23, 2026

Copy link
Copy Markdown

Summary

  • remove GitHub pull-request URLs from Kanban respawn eligibility decisions
  • keep card lifecycle, dependency gates, atomic claims, worker PIDs, and heartbeats authoritative
  • document pull-request comments as delivery/audit metadata and cover blocked-to-unblocked continuation on the same card and workspace

Why

Creating a pull request is a delivery milestone, not a terminal Kanban lifecycle transition. A non-terminal card may need to continue after review feedback, and the existing active_pr guard could leave an otherwise ready card permanently unspawnable based only on comment text. Review waits should be represented by the card's lifecycle state; an explicit unblock then re-enters the existing dependency and claim checks without adding GitHub network access to the dispatch path.

This is an architectural alternative to open PRs #32063, #44899, #65581, #65996, #67826, and #68198. Those candidates tune, expose, or bypass the PR-comment hard guard; this change removes that guard because PR presence alone is not sufficient lifecycle authority. It does not alter first-class review handling or structured owner actions.

Verification

  • scripts/run_tests.sh tests/hermes_cli/test_kanban_db.py -k 'windows or respawn_guard or pr_delivery_metadata or stale_claim_with_live_pid_extends' -v --tb=short — 21 passed
  • relevant Kanban DB, dispatch-lock, diagnostics, reclaim-lock, and gateway watcher run — 291 passed; one unrelated order-sensitive SQLite warning assertion failed in the full DB file, while that test passes in isolation
  • Ruff — passed
  • Python compilation — passed
  • Windows footgun scan — passed
  • git diff --check — passed

Residual risk

This intentionally allows a ready card with PR metadata to dispatch. Operators and workers must use the existing blocked/review lifecycle state while human review is pending; live-worker duplication remains prevented by existing atomic claim and liveness protections.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have needs-decision Awaiting maintainer decision before any implementation sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 23, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #32063, #44899, #65581, #65996, #67826, and #68198. This takes a distinct policy path: it removes the PR-comment guard entirely and relies on card lifecycle plus existing claim/PID/heartbeat checks. Maintainer decision needed on whether PR metadata should ever block dispatch.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for tracing the follow-up problem. The current-head premise is real: check_respawn_guard() returns active_pr for recent PR URLs at hermes_cli/kanban_db.py:7988-7995, and dispatch_once() defers the card at hermes_cli/kanban_db.py:8360-8372.

Problems

  • Removing that guard leaves an existing duplicate-work path open. A worker that posts a PR URL but exits without kanban_complete or kanban_block is returned to ready by detect_crashed_workers() (hermes_cli/kanban_db.py:7434-7501); below the protocol-violation retry limit it remains eligible for retry (hermes_cli/kanban_db.py:7586-7593). The proposed tests only inject a ready PR-comment card, so they do not cover this path.

Suggested changes

  • Preserve duplicate-work protection until the PR handoff creates a durable lifecycle transition or an atomic revision authorization.
  • Add a reclaim-path regression: PR comment, worker exit without terminal lifecycle call, reclaim, then verify an explicit continuation can run without an unintended duplicate spawn.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
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 needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants