Skip to content

fix(kanban): reclaim_task must not launder a blocked card into ready - #71980

Closed
Kyzcreig wants to merge 2 commits into
NousResearch:mainfrom
ANG-Ventures:up/kanban-reclaim-blocked-guard
Closed

Kyzcreig wants to merge 2 commits into
NousResearch:mainfrom
ANG-Ventures:up/kanban-reclaim-blocked-guard

Conversation

@Kyzcreig

Copy link
Copy Markdown

Stacked on #71977 — this branch is based on
fix(kanban): make --initial-status blocked sticky against recompute_ready,
so the diff shown here includes that commit until #71977 merges. The two are
independent halves of the same hold-defeating bug (creation-time stickiness
vs. reclaim-time promotion); either can merge first, and this branch will
shrink to its own commit once #71977 lands. Review only
f511d3746 for this PR's change.

fix(kanban): reclaim must not launder a blocked card into ready

Stacked on the --initial-status blocked sticky fix. Review only the top
commit; the diff includes the base PR until it merges. The base PR is what
makes initial_status="blocked" sticky at all — without it, three of the
tests here can't distinguish this bug from that one. Merge order: base, then
this.

Symptom

An operator reclaim on a card parked in blocked promotes it to ready,
handing it straight to the dispatcher, which claims it and spawns a worker on
the next tick. This silently defeats every hold mechanism the board has:
--initial-status blocked, a worker's review-required handoff, and the
spawn-failure circuit breaker alike.

Root cause

hermes_cli/kanban_db.py, reclaim_task:

cur = conn.execute(
    "UPDATE tasks SET status = 'ready', claim_lock = NULL, "
    "claim_expires = NULL, worker_pid = NULL "
    "WHERE id = ? AND status IN ('running', 'ready', 'blocked') "
    "AND claim_lock IS ?",
    (task_id, prev_lock),
)

The WHERE clause accepts blocked while the SET writes an unconditional
literal 'ready'. A reclaim releases a claim; it is not a promotion.

The row it produces is worse than merely promoted: reclaim_task emits only a
reclaimed event, never unblocked, so _has_sticky_block still reports
True afterwards. The card ends up status='ready' and sticky-blocked
simultaneously — a state no other writer can produce, and one that
recompute_ready and the dispatcher disagree about.

Whole bug class

The sibling reclaim paths were already correct — release_stale_claims
(:4355), detect_crashed_workers (:7066, :7192), enforce_max_runtime (:7450)
and the failure-recording path (:7728) all scope their UPDATE to
status = 'running'. Only the operator-driven reclaim_task accepted a parking
status, so this is the whole class. A source-level invariant test pins that, so
a future writer widening a sibling's WHERE clause has to confront it.

triage and scheduled are the same category of pre-dispatch parking column
and get the same treatment. triage additionally gains the ability to be
reclaimed: it wasn't in the old WHERE clause at all, so a triage row carrying
stale claim residue simply returned False and stayed stuck.

Fix

  • Rows in blocked / triage / scheduled keep their status and shed only
    the stale claim residue (claim_lock / claim_expires / worker_pid).
    unblock_task and promote_task remain the only ways out of blocked.
  • The reclaimed event records status_preserved, so board history shows why
    a reclaimed blocked card did not go ready.
  • hermes kanban reclaim reports the held status and points at unblock,
    instead of printing a bare Reclaimed <id> that reads as "it's queued again"
    and leaves the operator waiting for a worker forever.

No new state, no signature changes. +38/−4 across two files.

Tests

New tests/hermes_cli/test_kanban_reclaim_blocked_status_guard.py, 9 tests:

test proves
..._initial_status_blocked_card claim residue cleared, status held at blocked
test_dispatcher_does_not_spawn_a_reclaimed_blocked_card E2E through a real dispatch_once: spawn_fn never called
..._coherent_with_sticky_block no status='ready' + sticky-blocked row
..._preserves_worker_review_required_block the review-required handoff survives
..._promote_a_triage_card triage is now reclaimable and not promoted
test_no_claim_release_path_promotes_a_parked_row source invariant over every claim-release UPDATE
..._records_the_preserved_status_in_board_history (control) status_preserved in the event payload
test_reclaim_still_returns_a_running_task_to_ready (control) primary contract unchanged: running → ready → dispatchable
test_cli_reclaim_tells_the_operator_the_card_is_still_held CLI no longer prints a misleading bare success line

The running → ready → actually spawns control is deliberate: it caught an
earlier version of this suite that was false-greening on
skipped_nonspawnable (the dispatcher refuses assignees that aren't real
profiles, so "did not spawn" can pass trivially). The fixture patches
profile_exists so the negative assertions prove the status guard, not the
profile guard.

RED proof (both production changes reverted, tests unchanged): 7 of 9
fail
. The two survivors are the controls. See TEST-EVIDENCE.md.

GREEN: 259 passed across test_kanban_db.py, this file,
test_kanban_decompose_db.py, test_kanban_worktree_isolation.py.

@Kyzcreig

Copy link
Copy Markdown
Author

Base PR: #71977. Sibling independent fixes from the same pass: #71979 (stall-detector false positive), #71978 (worktree reuse ignoring the requested branch).

@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 Jul 26, 2026
Kyzcreig added 2 commits July 26, 2026 22:04
…eady

create_task set status='blocked' but emitted only a `created` event, never a
`blocked` one, so `_has_sticky_block` returned False and `recompute_ready`
promoted the parentless card back to `ready`. Because `recompute_ready` runs
inside `dispatch_once`, the promotion and the claim+spawn land on the same
tick — a card explicitly parked for human ops gets a worker anyway.

Emit the same `blocked` event `block_task` emits at creation time.
`unblock_task` remains the legitimate exit.

4 new tests (3 RED-prove the fix; the 4th is the unblock-exit control).
`reclaim_task` accepted `blocked` in its WHERE clause while writing an
unconditional literal `status='ready'`, so an operator reclaim on a card parked
in `blocked` promoted it and the dispatcher claimed + spawned a worker on the
next tick. That defeats every hold the board has at once: `--initial-status
blocked`, a worker's review-required handoff, and the circuit breaker's
auto-block.

There is a second-order tell that confirms the shape: reclaim emits only a
`reclaimed` event, never `unblocked`, so `_has_sticky_block` still returns True
afterwards. The row lands `status='ready'` **and** sticky-blocked at the same
time — an incoherent state no other writer can produce, and one that
`recompute_ready` and the dispatcher then actively disagree about.

A reclaim releases a *claim*; it is not a promotion. Rows in `blocked` /
`triage` / `scheduled` now keep their status and shed only the stale claim
residue (`claim_lock` / `claim_expires` / `worker_pid`). `unblock_task` and
`promote_task` remain the only ways out of `blocked`. The preserved status is
recorded in the `reclaimed` event payload and surfaced by `hermes kanban
reclaim`, so an operator can see why the card did not go ready.

Whole bug class, not just the reported site: all five sibling claim-release
paths (`release_stale_claims`, `detect_crashed_workers` ×2,
`enforce_max_runtime`, `_record_task_failure`) already scope their UPDATE to
`status = 'running'` and are correct. A source-level invariant test pins that,
so a future sibling that widens its WHERE clause the same way fails here.

9 new tests; 7 RED-prove the fix, 2 are controls (running→ready still works,
no-claim-release-path-promotes invariant).
@Kyzcreig
Kyzcreig force-pushed the up/kanban-reclaim-blocked-guard branch from f511d37 to 044f3ff Compare July 27, 2026 05:05

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — the main reclaim bug is real: hermes_cli/kanban_db.py:4486-4490 writes literal ready while accepting a locked blocked task.

Problems

  • hermes_cli/kanban.py:1846 gives unblock guidance for triage, but unblock_task only accepts blocked and scheduled (hermes_cli/kanban_db.py:5808). A reclaimed triage card therefore receives a command that fails; its supported transition is the triage specify/decompose path (hermes_cli/kanban_db.py:5829).
  • tests/hermes_cli/test_kanban_reclaim_blocked_status_guard.py:216 uses inspect.getsource() and regexes SQL. AGENTS.md:1382 explicitly bans source-code-reading tests because they test implementation shape rather than behavior.

Suggested changes

  • Make the CLI remediation status-specific and add a triage-output regression test.
  • Replace the source-regex test with behavioral tests for the actual claim-release paths.

Automated hermes-sweeper review.

Comment thread hermes_cli/kanban.py
# operator assumes the card is queued and waits for a worker forever.
print(
f"Reclaimed {args.task_id} (status held at {status!r} — "
f"run 'hermes kanban unblock {args.task_id}' to re-queue it)"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

triage is included in this branch, but unblock_task only accepts blocked and scheduled (hermes_cli/kanban_db.py:5808). This prints a recovery command that fails for triage; direct triage to its specify/decompose transition instead.

import inspect
import re

src = inspect.getsource(kb)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace this source inspection with behavioral coverage. AGENTS.md bans tests that read production source because implementation-shape assertions break valid refactors without proving the runtime contract.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
@Kyzcreig

Copy link
Copy Markdown
Author

Closing — implemented on main.

Re-verified at 4209d371: reclaim_task no longer writes the unconditional literal this PR was filed against. hermes_cli/kanban_db.py:5157-5163 now computes retry_status = _retry_status_for_run(conn, task_id) and binds it as a parameter, with the guard WHERE ... status IN ('running','ready','blocked') retained. The SET status = 'ready' the root-cause section quotes is gone.

I also executed the laundering scenario end to end against a temp board on current main: a card blocked through the real block_task path lands status='blocked', claim_lock=NULL (kanban_db.py:6326), so reclaim_task returns False and the status holds at blocked. I could only reproduce a promotion by manufacturing claim_lock with a raw UPDATE, which no production writer does.

Branch preserved; reopenable if a genuine blocked-plus-claim path turns up.

@Kyzcreig Kyzcreig closed this Aug 29, 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 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants