fix(kanban): fence task completion to claim ownership and make retries idempotent

[ryangu00]

What does this PR do?

complete_task() accepts a completion from any caller, without checking that the caller still owns the claim. Two consequences on a board where a task can be reclaimed:

1. A stale worker can overwrite the current owner. Worker A claims a task and stalls; the task is reclaimed and handed to worker B; A wakes up and calls complete_task(). The legacy path (no run id) writes A's result over B's in-flight run.
2. Completion is not idempotent. The first successful completion clears claim_lock and current_run_id, so a network retry of a call that actually succeeded returns False — the caller can't tell "someone else finished this" from "your own retry".

This threads the dispatcher's existing claim_lock through as expected_claim_lock and fences the write on it, inside the same BEGIN IMMEDIATE transaction that already guards the update. A completion whose lock doesn't match is rejected; a retry carrying the same lock succeeds idempotently without re-running the completion side effects (a one-way digest of the lock is recorded on the completed event so the retry can be recognised after the lock itself is cleared).

Callers that don't have claim data — the existing legacy path — behave exactly as before, so nothing that works today breaks.

Related Issue

No existing issue. Searched open and closed PRs for kanban complete_task idempotent / claim_lock — nothing prior.

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

• hermes_cli/kanban_db.py: complete_task() gains expected_claim_lock; ownership proof + idempotent-retry handling inside the existing write transaction. All current behaviour (completion artifacts, attachments, hallucinated-card handling, block-state clearing, lifecycle) preserved.
• hermes_cli/kanban.py, tools/kanban_tools.py: pass the worker's claim lock through at the two call sites that have it.
• tests/hermes_cli/test_kanban_claim_lock.py (new): 4 regressions.

How to Test

1. pytest tests/hermes_cli/test_kanban_claim_lock.py tests/hermes_cli/test_kanban_db.py -q — 234 passed.
2. Regression proof: git checkout origin/main -- hermes_cli/kanban_db.py and re-run the new file — 3 of 4 fail (matching-lock completion, stale-lock rejection, idempotent retry). The 4th is the legacy no-lock path, which passes either way by design. Restore and all 4 pass.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(kanban):)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix
• I've run the relevant tests and they pass
• I've added tests for my changes
• I've tested on my platform: macOS 15 (Darwin 25.5), Python 3.12

Documentation & Housekeeping

• Documentation — N/A (internal API; the new parameter is optional and documented in the docstring)
• cli-config.yaml.example — N/A (no config keys)
• CONTRIBUTING.md / AGENTS.md — N/A
• Cross-platform impact — N/A (SQLite + pure Python)
• Tool descriptions/schemas — N/A (the kanban tool's public schema is unchanged; the lock is passed internally)

[ryangu00]

Rebased onto a4973c3f1 (current main) and re-verified there — it had drifted ~920 commits behind since opening.

• pytest tests/hermes_cli/test_kanban_claim_lock.py tests/hermes_cli/test_kanban_db.py -q → 234 passed
• Regression proof re-run on top of current main: git checkout main -- hermes_cli/kanban_db.py → 3 of the 4 new tests fail; restore → all pass. The 4th covers the legacy no-lock path, which passes either way by design — that's the back-compat guarantee: callers without claim data behave exactly as they do today.

The ownership check runs inside the BEGIN IMMEDIATE transaction that already guards the update, so it adds no new locking surface.

[teknium1]

Thanks for adding focused claim-lock and retry coverage.

Problems

• The new expected_claim_lock update at hermes_cli/kanban_db.py:4820 drops the existing status IN ('running', 'ready', 'blocked') guard. The added test deliberately changes a claimed task to triage and accepts completion (tests/hermes_cli/test_kanban_claim_lock.py:27-42), but current main uses triage as a human-routing state (hermes_cli/kanban_db.py:5581-5618). Preserve the status predicate in the new branch.
• The stale-worker premise is already covered for dispatcher workers by the current run-id fence: the dispatcher exports HERMES_KANBAN_RUN_ID (hermes_cli/kanban_db.py:8864-8867), both worker completion paths pass it (tools/kanban_tools.py:670-675, hermes_cli/kanban.py:2216-2222), and complete_task requires it to match (hermes_cli/kanban_db.py:4777-4794). The claim lock is therefore defense-in-depth plus the new retry behavior, rather than the first ownership fence.

Suggested changes

• Restore the accepted-status predicate and add a matching-lock/non-completable-state regression.

Automated hermes-sweeper review.

[teknium1]

Please retain AND status IN ('running', 'ready', 'blocked') here (and the run-id condition when supplied). This branch otherwise completes any row that still has this lock; the new test currently demonstrates an unintended triage -> done transition, while the legacy branches correctly reject triage.

[ryangu00]

Thanks — and the correction on framing is taken. With the dispatcher's HERMES_KANBAN_RUN_ID fence already in place and both worker completion paths passing it, the claim lock is defense-in-depth plus the retry behavior, not the first ownership gate. I've stopped describing it as the primary fence.

Problem 1 — status predicate. Restored AND status IN ('running', 'ready', 'blocked') in the fenced branch. The test that pushed a claimed task to triage and accepted completion was wrong for exactly the reason you gave — triage is a human-routing state — so it is now a rejection case, alongside a second regression for a matching lock against a non-completable state (todo).

One more I added. Converting that test left the expected_claim_lock branch with only negative coverage (the other complete_task tests all exercise the unfenced branch), so there is a positive case back — parametrized over all three completable statuses, because a single case on running would stay green if the predicate silently dropped ready or blocked from the IN list. Mutation-tested: degrading the predicate to IN ('running') fails exactly the ready/blocked cases; removing it entirely fails both rejection tests.

Rebased onto current main (8defb9fd60).

pytest tests/hermes_cli/test_kanban_claim_lock.py tests/hermes_cli/test_kanban_db.py -q   # 38 passed

# both rejection tests fail without the predicate:
git checkout HEAD~3 -- hermes_cli/kanban_db.py
pytest tests/hermes_cli/test_kanban_claim_lock.py -q   # 2 failed, 6 passed
git checkout HEAD -- hermes_cli/kanban_db.py

[ryangu00]

Rebased onto current main (0904f50e3e) — the branch had gone conflicting against the kanban work that landed over the past week — and extended the fence in response to the production report on #71175.

Rebase. Two conflicts, both in complete_task. Upstream's parent-completion invariant and prior_status read now run first, and the claim-lock branch chains onto them; the expected_run_id is None path stays reachable for legacy, manual and orchestrator completion. Suite parity against origin/main: the same 227 pre-existing environment failures on both, 5375 passing here against 5374 there — the difference is the one test added below.

The fence needed more than the claim lock. @SharadKumar's reproduction on #71175 shows why: a nested Hermes CLI inherits HERMES_KANBAN_TASK and HERMES_KANBAN_CLAIM_LOCK from its parent, presents a claim lock that matches, and completes the parent's card from a different pid. Every component this branch was fencing on travels down to a child that way — inherited state can say where a caller came from, not who it is. That applies to the expected_run_id guard on main as well, which is the same shape.

worker_pid is the one component a child cannot inherit as its own, but only if it is read from the running process. Taking it from an environment variable would rebuild the hole one layer down, so both call sites pass os.getpid() and the comparison happens in the precheck and inside the statement that flips the row.

Two things worth flagging in how this landed:

• The first attempt wired the pid through the CLI only. The bypass travels the tool handoff, so the fix sat beside the door it was meant to close — kanban_tools still sent the claim lock alone. Both call sites carry it now.
• A row with no recorded worker_pid was initially allowed through, which turned the fence back into lock-only exactly when identity could not be checked. A caller that supplies its own pid is asserting an identity, so an unverifiable row is refused. Callers that pass no expected_worker_pid are unaffected — the predicate short-circuits — which is what keeps manual and orchestrator completion working.

Tests. The new one is at the tool layer on purpose: the database-level tests prove a mismatched pid is rejected, but say nothing about whether the tool sends one, which was the actual gap. Both new cases were confirmed to discriminate by removing the predicate they cover and watching only that case turn red.

pytest tests/hermes_cli/test_kanban_claim_lock.py -q   # 11 passed

Still happy to fold this into a different shape if you'd rather the identity check live in one place for complete, request_review and the reaper together — #81508 suggests the same question is open for the process-identity path.

[Enough1122]

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

fix(kanban): fence task completion to claim ownership and make retries idempotent

1. Concurrent-completion race on the fenced branch — hermes_cli/kanban_db.py complete_task: two callers holding the same claim lock can both pass the SELECT checks before either UPDATE commits. The sequential-retry test covers the "already done" branch, but a concurrent loser still proceeds past the UPDATE unless its rowcount is verified before the completed event is recorded. Please confirm the fenced branch checks cur.rowcount (like the legacy branch) before inserting the event — otherwise a lost race yields duplicate completion events.
2. Idempotent done-branch is looser when run_id is omitted — when expected_run_id is None (claim lock only), the retry check accepts the latest completed event whose completion_claim_lock_sha256 matches, regardless of run. The CLI always passes expected_run_id, so this is mostly theoretical, but requiring the run_id for the digest comparison would make the check exact.
3. worker_pid lifecycle — the fence's strength depends on the dispatcher having stamped worker_pid at spawn (claim_task itself does not, as the tests note), and the completing process's os.getpid() matching it. Confirm every reclaim/re-dispatch cycle refreshes worker_pid, and that the pid actually stamped is the pid of the process that will call complete_task (e.g. not a wrapping shell that forks the CLI) — otherwise legitimate retries get rejected by the new fence.

[ryangu00]

Thanks — point 3 found a regression I had just introduced. Taking all three in order.

1. Concurrent-completion race — already covered, but by placement rather than by design. The fenced branch does reach a rowcount check: if cur.rowcount != 1: return False sits after the if / elif / else converge (kanban_db.py:5564), so all three branches pass through it before any completed event is written. A concurrent loser's UPDATE matches zero rows once the winner has cleared claim_lock, so it returns there and records nothing. Worth saying plainly though: that is a property of where the check happens, not something the fenced branch states for itself, so a future branch added below the converge point would miss it silently.

2. Digest comparison without a run id. Agreed, and it is reachable rather than purely theoretical — tools/kanban_tools.py reaches the same helper. The looser form only matters when a task has more than one completed event carrying the same claim lock digest, which needs a lock reused across runs. I would rather fix that by requiring the run id for the digest comparison than by tightening the lock's lifetime; happy to fold it into this branch if you want it here rather than as a follow-up.

3. worker_pid lifecycle — you caught a real regression. Checking your two conditions:

• The stamped pid is the completing process. _default_spawn runs subprocess.Popen(argv_list) with a fixed argv and returns proc.pid — no shell wrapper — so the pid recorded is the hermes process that later calls complete_task. The concern does not apply to the default path.
• Every reclaim refreshes it. reclaim_task clears claim_expires and worker_pid to NULL, so a stale pid does not survive into the next claim.

But following that thread found the case you were pointing at. _set_worker_pid only runs if pid: (kanban_db.py:9989), and a pid from spawn_fn is a crash-detection nicety rather than a contract — a deployment whose spawn returns none leaves the column NULL forever. An earlier round of this branch had made an unrecorded pid a refusal, on the reasoning that an identity we cannot verify should fail closed. Reproduced against that build:

worker_pid on row: None
legitimate worker completing its own task → REFUSED

That is worse than the hole it was closing, so it is reverted: a NULL row is left alone, with a test pinning it so the next tightening instinct has to argue with a red suite. Where no pid was ever recorded the fence is no weaker than before this branch; where one was, it is strictly stronger, which is all it should claim.

pytest tests/hermes_cli/test_kanban_claim_lock.py -q → 12 passed. Suite parity against origin/main: same 227 pre-existing environment failures on both, 5376 passing here against 5374 there.