Skip to content

fix(kanban): guard block_task against live-claim theft - #83728

Open
pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/kanban-block-task-live-claim-guard
Open

fix(kanban): guard block_task against live-claim theft#83728
pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/kanban-block-task-live-claim-guard

Conversation

@pierrenode

Copy link
Copy Markdown
Contributor

Summary

request_review() just gained an M1 guard (commit 1810cfc8dd, same day): a caller with no expected_run_id must pass force=True before it clears claim_lock/worker_pid on a running, live-claimed task — otherwise the transition is refused instead of silently stealing the live worker's claim.

block_task() has the identical shape (same optional expected_run_id, same unconditional claim_lock/worker_pid clear on the running/ready update) but never got the same guard. Concretely:

  • The dashboard's PATCH /tasks/{id} (single) and POST /tasks/bulk (bulk) "blocked" transitions call kanban_db.block_task() with no expected_run_id at all.
  • A plain hermes kanban block <id> from a shell resolves expected_run_id via _worker_run_id_for(tid), which only returns non-None inside a worker's own execution context (matching HERMES_KANBAN_TASK/HERMES_KANBAN_RUN_ID) — a human running the command from their own shell gets None.

So today, an operator (or a second/stale dashboard tab, or a scripted CLI caller) can mark a task blocked with zero ownership proof, silently clearing an actively-running worker's claim and ending its in-flight run out from under it — the exact class of bug M1 just closed for request_review(), left open here.

Fix

Mirrors the M1 pattern exactly:

  • block_task() gains a force: bool = False parameter. When the task is running under a live claim (claim_lock IS NOT NULL) and the caller supplies neither expected_run_id (worker ownership) nor force=True (explicit operator override), the call now returns False instead of clearing the claim.
  • Dashboard PATCH/bulk-update pass force=True — an explicit human action, matching how request_review's dashboard wiring was already updated.
  • hermes kanban block gains a --force flag, mirroring request-review's.

Existing worker call sites (cli.py's goal-loop _block(), tools/kanban_tools.py's kanban_block tool) already pass expected_run_id and are unaffected.

Test plan

  • New regression test test_block_task_refuses_to_clear_live_claim_without_ownership (mirrors request_review's own M1 regression test): asserts a run-id-less, force-less call is refused with the live claim untouched, the worker-ownership path (expected_run_id) still works, and force=True still works as an explicit override.
  • Mutation-verified: stashed the hermes_cli/kanban_db.py fix and confirmed the new test fails against pre-fix code (AssertionError: assert True is False).
  • Fixed three existing tests whose block_task() calls modeled a worker blocking its own live-claimed task without ever proving ownership (a shape that was never realistic before this guard existed, and is now correctly refused): tests/hermes_cli/test_kanban_block_kinds.py's two block-loop/dependency-routing tests now pass expected_run_id, and tests/tools/test_kanban_tools.py's worker_env fixture now also sets HERMES_KANBAN_RUN_ID alongside HERMES_KANBAN_TASK, matching how a real worker spawn's environment looks (kanban_db.py's dispatcher already sets both).
  • Ran the full neighboring kanban suite (test_kanban_core_functionality.py, test_kanban_blocked_sticky.py, test_kanban_review_lifecycle{,_complete}.py, test_kanban_block_kinds.py, test_kanban_review_surfaces.py, test_kanban_swarm.py, test_kanban_dashboard_plugin.py, test_kanban_tools.py): 136 passed, 1 skipped, 1 pre-existing failure unrelated to this change (test_review_tools_are_gated_and_visible_to_kanban_workers fails on main too — missing acp package in this environment, not caused by this diff).
  • Ran tests/stress/test_atypical_scenarios.py directly (its own scenario runner): 2 pre-existing failures (workspace_nonexistent_path, parent_in_different_status_states) reproduce identically on main without this change — confirmed via git stash, not a regression from this PR.
  • ruff check clean on all changed files.

Scope note

complete_task() has the same shape and arguably deserves the same guard, but two open PRs (#73188, #81170) are already circling that specific function with different, not-fully-overlapping approaches — this PR is scoped to block_task() only to avoid stepping on that in-flight work.

request_review() just gained an M1 guard (1810cfc): a caller with no
expected_run_id must pass force=True before it clears claim_lock/worker_pid
on a running, live-claimed task. block_task() has the identical shape
(same optional expected_run_id, same unconditional claim clear) but never
got the same guard — the dashboard's PATCH/bulk-update "blocked" transitions
and a plain `hermes kanban block <id>` from a shell can silently steal an
active worker's claim and end its in-flight run out from under it, with no
ownership proof and no way to make the override explicit.

Mirrors the M1 pattern: block_task() now refuses (returns False) when the
task is running under a live claim and the caller supplies neither
expected_run_id (worker ownership) nor force=True (explicit operator
override). Dashboard PATCH/bulk-update pass force=True (explicit human
action, matching request_review's dashboard wiring); `hermes kanban block`
gains a --force flag mirroring request-review's.

Also fixes three existing tests/fixtures whose block_task() calls modeled
a worker blocking its own live-claimed task without ever proving ownership
(expected_run_id was never realistic for that scenario before this guard
existed): tests/hermes_cli/test_kanban_block_kinds.py now passes the
current_run_id, and tests/tools/test_kanban_tools.py's worker_env fixture
now sets HERMES_KANBAN_RUN_ID alongside HERMES_KANBAN_TASK like a real
worker spawn does.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management comp/plugins Plugin system and bundled plugins labels Aug 11, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(kanban): guard block_task against live-claim theft — mirrors the request_review M1 guard correctly, and updating the worker-env test fixture with HERMES_KANBAN_RUN_ID is the right call. Observations:

  1. Stale-claim friction: the guard refuses when status == 'running' AND claim_lock IS NOT NULL regardless of whether the claim is still live. A task whose worker's claim has expired (claim_expires passed, not yet reclaimed) still has claim_lock set, so a human/CLI block without --force is refused even though the claim is dead. Consider also checking claim_expires — or running the _retry_status_for_run recomputation first — so dead claims don't require --force.

  2. Dashboard bulk_update now passes force=True for "blocked" on every task in the bulk payload — consistent with the explicit-human-action rationale for the single PATCH, but worth double-checking that bulk endpoints are auth-gated the same way, since force bypasses the ownership guard entirely.

  3. Worker-path dependency: the worker ownership path now relies on HERMES_KANBAN_RUN_ID being present in the spawn env (the dispatcher sets it, kanban_db.py env-build). Any other worker-spawn site that omits it will see its own block requests refused as if from an unrelated caller. Worth an audit of worker-spawn sites for that env var.

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 comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants