🐛 fix(kanban): reconcile a crashed provably-done card to review, not gave_up - #80
Conversation
…gave_up detect_crashed_workers consulted _lane_work_provably_done only on the clean-exit / uncaptured-exit reap path. A worker that died a REAL captured death (signaled: SIGKILL / OOM killer; or nonzero_exit) after its lane work already landed fell into the generic crashed branch, which counted a failure and, on retry-budget exhaustion, emitted gave_up and stranded the card in blocked — even though the deliverable was on disk and re-verifiable. Crash-vs-clean-exit is orthogonal to whether the work landed: the same proof the clean-exit path already trusts now gates the crash path too. When a would-be crash (signaled / nonzero_exit / unknown) has landed-work proof, it is treated as a benign no-op and the card reconciles FORWARD to its owner-map review owner (deliverable real but UNREVIEWED — never done). With no proof, signaled / nonzero_exit fall through to the strict crashed branch unchanged, so a genuine crash with no deliverable still gives up. A no-PR edit-in-place card advances to review only on the crash path; the clean-exit path keeps releasing it to ready unchanged. The shape predicate keeps a PR-requiring card held to a real PR / completed-run artifact, so the no-PR handoff-comment carve-out never leaks into the PR-backed crash path. Behavior-contract tests against real kanban_db + temp HERMES_HOME cover the reconcile-to-review case and both proof-absent regressions (no-PR and PR-requiring still gave_up).
cwest
left a comment
There was a problem hiding this comment.
No changes needed.
The fix targets the real asymmetry: detect_crashed_workers only consulted _lane_work_provably_done on the clean_exit/unknown reap path, so a captured worker death (signaled/nonzero_exit) after the lane work landed fell into the generic crashed branch and gave_up on retry-budget exhaustion. Widening the carve-out to the crash kinds and reusing the same proof helper — unchanged — is the right shape.
The gating holds where it matters. Proof 3 (the no-PR handoff-comment) stays scoped by _card_requires_pr, so a PR-requiring worktree card with a handoff comment but no PR never reads as provably-done and still gave_ups. The clean-exit no-PR path still releases to ready (no review lane), and only the crash path advances a proven no-PR card to review — matching #75/#76.
Verified by ground truth in a throwaway clone at the head SHA: full test_kanban_db.py is 283 passed / 0 failed; ruff clean on both files. Reverting the source fix reproduces the incident — test_crash_no_pr_edit_in_place_with_handoff_advances_to_review fails on the exact assertion (crashed card treated as a crash), while the two proof-absent regressions stay green. Dropping the _card_requires_pr gate makes test_crash_pr_requiring_card_with_handoff_no_pr_still_gives_up fail, so the leak guard has teeth. Commit is signed, Conventional, no attribution.
Worth restating what the PR body already flags: this runs inside the long-lived dispatcher, so it is inert until the gateway restarts.
…gave_up (#80) detect_crashed_workers consulted _lane_work_provably_done only on the clean-exit / uncaptured-exit reap path. A worker that died a REAL captured death (signaled: SIGKILL / OOM killer; or nonzero_exit) after its lane work already landed fell into the generic crashed branch, which counted a failure and, on retry-budget exhaustion, emitted gave_up and stranded the card in blocked — even though the deliverable was on disk and re-verifiable. Crash-vs-clean-exit is orthogonal to whether the work landed: the same proof the clean-exit path already trusts now gates the crash path too. When a would-be crash (signaled / nonzero_exit / unknown) has landed-work proof, it is treated as a benign no-op and the card reconciles FORWARD to its owner-map review owner (deliverable real but UNREVIEWED — never done). With no proof, signaled / nonzero_exit fall through to the strict crashed branch unchanged, so a genuine crash with no deliverable still gives up. A no-PR edit-in-place card advances to review only on the crash path; the clean-exit path keeps releasing it to ready unchanged. The shape predicate keeps a PR-requiring card held to a real PR / completed-run artifact, so the no-PR handoff-comment carve-out never leaks into the PR-backed crash path. Behavior-contract tests against real kanban_db + temp HERMES_HOME cover the reconcile-to-review case and both proof-absent regressions (no-PR and PR-requiring still gave_up).
Why
A no-PR edit-in-place card whose worker crashed (
pid not alive) afterposting a landed-work handoff was false-
gave_upand stranded inblockedeventhough its deliverable was complete, on disk, and independently re-verified. A
human had to hand-reconcile it.
The prior landed-work proof (
_lane_work_provably_done) was wired into theclean-exit reap path only. It was never consulted on the crash path: a
worker that died a real captured death (
signaled— SIGKILL / OOM killer — ornonzero_exit) fell into the genericcrashedbranch, which counts a failureand, on retry-budget exhaustion, emits
gave_up. The same card, samedeliverable, same proof, was correctly reconciled on a clean exit but wrongly
stranded on a crash. Crash-vs-clean-exit is orthogonal to whether the work
landed.
What
Wire the existing
_lane_work_provably_doneproof into the crash/gave_uppathin
hermes_cli/kanban_db.py:detect_crashed_workersnow consults the same helper (unchanged — noforked predicate) for a would-be crash reap (
signaled/nonzero_exit/unknown), not justclean_exit.the owner map's
reviewowner (neverdone— the deliverable is real butUNREVIEWED), rather than
gave_up+blocked.with no deliverable still
gave_ups._card_requires_prstill gates the no-PRhandoff-comment carve-out, so PR-backed crash behavior is byte-for-byte
identical. The no-PR edit-in-place card advances to
reviewonly on thecrash path; the clean-exit path keeps releasing it to
readyunchanged.No new config, env var, or schema — reuses the existing window constant and shape
predicate.
Done when
handoff reconciles to
review+ the owner-map reviewer, notgave_up. ✅gave_ups —regression-tested explicitly. ✅
gave_ups — the no-PR carve-out does not leak into the PR-backed path. ✅kanban_db+ tempHERMES_HOME, nomocks. ✅
test_kanban_db.pygreen (283 passed), ruff clean, 0 regressions. ✅Scope / Restart-gating
Single-file change in
hermes_cli/kanban_db.pyplus tests. This fix is inertuntil the gateway is restarted —
detect_crashed_workersruns inside thelong-lived dispatcher, so a merged change does not take effect on the running
process until it reboots.
Out of scope: why the worker pid dies silently. This stops the crash path from
discarding proven work; it does not prevent the crash.