🐛 fix(kanban): refuse author-lane complete on an unresolved review-eligible card - #67
Conversation
…igible card The author-lane redirect in complete_task moves a running|ready author completion to the review lane instead of done — but only when the card's stamped owner map yields a review owner. A review-eligible card filed without a stage=submit owner map resolved None, the redirect precondition failed, and completion fell through to the -> done UPDATE with no signal: a code card with an open PR reaching done past the reviewer and past acceptance. done must mean "merged/accepted". Close the hole for the population identifiable without a card kind column (there is none — the owner map lives in the audit trail by design): when an author-lane completion cannot resolve a review owner AND the card is PR-requiring (an isolated git worktree owing a PR, the same _card_requires_pr signal the missing-PR guard uses), refuse with a clean no-op (return False, no state mutation) and an auditable completion_redirect_unresolved event — the same shape as the acceptance and missing-PR guards. Non-pipeline cards (scratch / dir / edit-in-place) with no review owner still complete to done unchanged. Also collapse the duplicate _review_owner_from_owner_map definition (and its duplicate owner-map regex) to one authoritative reader, removing a latent divergence bug. Adds behavior-contract regression tests (real imports, temp HERMES_HOME): an unstamped PR-requiring card refuses (not done) and emits the audit event; a stamped one still redirects to review; the merge override still reaches done; scratch/dir/edit-in-place cards still complete; the reader is defined once.
cwest
left a comment
There was a problem hiding this comment.
No changes needed.
The fix closes the false-done hole at the right seam. When an author-lane completion can't resolve a review owner, the code now branches three ways: a resolvable owner still redirects to review unchanged; a PR-requiring card (worktree owing a PR) refuses with a completion_redirect_unresolved event and no state mutation; and a non-pipeline card falls through to done as before. That refusal shape mirrors the existing acceptance and missing-PR guards exactly, and the summary_preview construction is copied verbatim from the missing-PR guard rather than reinvented.
Scoping the guard to _card_requires_pr instead of deriving a review owner from card kind is the correct call. I checked the schema: the tasks table has no kind column (the only kind column is on task_events), so kind-derived routing would mean new schema and team routing baked into core. Refusing on the population core can already identify keeps the invariant without either.
The dedup is real: _review_owner_from_owner_map and _OWNER_MAP_RE each now exist once, both callers (the redirect and review_skills_for_card) bind to the surviving definition, and a test pins the single-definition contract.
Verified the suite on the head SHA in a throwaway clone: 295 passed, 0 failed across the four kanban test files, including the 8 new redirect tests. The updated assertion in the missing-PR guard test is a correct contract change — the old done it asserted was the exact false-done this PR eliminates, and the unstamped-refusal path is separately covered by a new test.
The filing-layer follow-up (stamping the owner map at creation for board-driven writing cards) is correctly deferred to a separate hermes-config change, since it lives outside this repo.
…igible card (#67) The author-lane redirect in complete_task moves a running|ready author completion to the review lane instead of done — but only when the card's stamped owner map yields a review owner. A review-eligible card filed without a stage=submit owner map resolved None, the redirect precondition failed, and completion fell through to the -> done UPDATE with no signal: a code card with an open PR reaching done past the reviewer and past acceptance. done must mean "merged/accepted". Close the hole for the population identifiable without a card kind column (there is none — the owner map lives in the audit trail by design): when an author-lane completion cannot resolve a review owner AND the card is PR-requiring (an isolated git worktree owing a PR, the same _card_requires_pr signal the missing-PR guard uses), refuse with a clean no-op (return False, no state mutation) and an auditable completion_redirect_unresolved event — the same shape as the acceptance and missing-PR guards. Non-pipeline cards (scratch / dir / edit-in-place) with no review owner still complete to done unchanged. Also collapse the duplicate _review_owner_from_owner_map definition (and its duplicate owner-map regex) to one authoritative reader, removing a latent divergence bug. Adds behavior-contract regression tests (real imports, temp HERMES_HOME): an unstamped PR-requiring card refuses (not done) and emits the audit event; a stamped one still redirects to review; the merge override still reaches done; scratch/dir/edit-in-place cards still complete; the reader is defined once. (cherry picked from commit a7ed14f)
Why
doneon the board means exactly one thing: Casey merged/accepted the work. Theauthor-lane redirect in
complete_taskmoves arunning|readyauthor completionto the review lane instead of
done— but only when the card's stamped ownermap yields a
reviewowner. When a review-eligible card is filed without astage=submitowner map, the reader returnsNone, the redirect's preconditionfails, and completion fell through to the
-> doneUPDATE with no signal: acode card with an open PR reaching
donepast the reviewer and past acceptance —the exact false-
donethe redirect exists to prevent. This false-doneclasshas recurred and been hand-reconciled repeatedly.
What
Make the author-lane redirect robust to a missing/unstamped owner map for a
card that is unambiguously a review-eligible pipeline card, so an author
completion can never silently reach
done.Core has no card
kindcolumn — the owner map lives in the audit trail bydesign (
stage-pr-review/bounce-review-to-authorread the same signal), socore cannot derive team/kind review owners without fighting that design. The fix
is therefore scoped to the population core can identify with zero new schema:
cannot resolve a review owner and the card is PR-requiring (an isolated git
worktree owing a PR — the same
_card_requires_prsignal the missing-PR guardalready uses),
complete_taskrefuses: a clean no-op (returnsFalse, no statemutation) with an auditable
completion_redirect_unresolvedevent — the sameshape as the existing acceptance and missing-PR guards. A non-pipeline card
(
scratch/dir/~/.hermesedit-in-place) with no review owner completesto
doneexactly as before; the redirect never shunted those and neither doesthis refusal.
_review_owner_from_owner_map. It was defined twice (with aduplicate owner-map regex); the second copy shadowed the first at import. Both
callers now bind to one authoritative reader, removing a latent divergence bug.
Why refuse rather than kind-derive a fallback owner
A kind-derived review owner (writing→…, code→…) would require core to know a
card's kind, which has no column — the owner map in the audit trail is the single
source of truth by design. Inferring kind in core would bake team routing into
the narrow waist and fight that design. Refusing on the unresolvable case keeps
the invariant (
done⇒ reviewed/accepted) without adding schema or team literalsto core. Board-driven review cards whose only pipeline marker is the owner map
(e.g. writing) are addressed at the filing layer — stamping the map at
creation — which is a separate change in the filing tooling, not core.
Done when
complete_taskon a review-eligible card with nostage=submitmaprefuses with an auditable event — it never silently reaches
done. ✅HERMES_HOME) prove it,plus the stamped redirect, merge-override, and non-pipeline paths are unchanged. ✅
_review_owner_from_owner_mapexists exactly once. ✅Testing
New file
tests/hermes_cli/test_kanban_complete_unresolved_redirect.pycovers therefusal, the audit event, the stamped-redirect regression, the merge-override
bypass, the scratch/dir/edit-in-place pass-throughs, and the single-definition
contract. One assertion in the missing-PR guard test was updated: an unstamped
worktree card with a PR now correctly moves to
review(stamped) rather thanlanding
done— the pre-fixdonethere was the false-donebeing eliminated.