Skip to content

🐛 fix(kanban): route an author completion to the review lane, not done - #59

Merged
cwest merged 1 commit into
cwest/integrationfrom
topic/complete-author-to-review
Jul 10, 2026
Merged

🐛 fix(kanban): route an author completion to the review lane, not done#59
cwest merged 1 commit into
cwest/integrationfrom
topic/complete-author-to-review

Conversation

@cwest

@cwest cwest commented Jul 9, 2026

Copy link
Copy Markdown
Owner

Why

done on this board means exactly one thing — the work was merged/accepted.
A recurring defect (hit twice live on one card) let an author finish their lane
and the card jump straight to done, skipping the review lane and the
acceptance gate
. Each occurrence had to be reconciled by hand.

Root cause: complete_task in hermes_cli/kanban_db.py unconditionally set
status='done' on any completion from the author lane (running/ready). It
had an acceptance-lane guard (won't flip an awaiting-casey-signoff card to
done) but nothing routed an author's completion into the review lane.

Code cards were only accidentally rescued: opening their PR fires the
github-prs webhook, which moves the card to review. A board-driven review card
(no PR-review webhook on its drafting step) had no such rescue, so its author's
completion landed it in done past the reviewer and past acceptance. This was
the last seam where the board-native review loop diverged from the code loop.

What

Make the board-native path correct for every kind with a review lane. Before
the -> done write, when

  • the merge override is not set (allow_acceptance_complete=False),
  • the card is in the author lane (running/ready), and
  • the card's own stamped owner map declares a review owner,

complete_task moves the card to status='review' + that review owner and
emits a status_changed event, instead of completing it to done.

  • No kind-default fallback. A card with no stamped review lane (legacy /
    un-stamped / a plain non-pipeline task, e.g. a swarm root) completes to done
    exactly as before — the redirect never shunts an undeclared card into review.
  • Idempotent with the webhook path. Once the card is in review it is no
    longer running/ready, so a second completion is a clean no-op and the
    -> done UPDATE (status IN ('running','ready','blocked')) cannot match it.
    A rework re-completion from ready after a review bounce correctly moves back
    to review for re-review.
  • Every existing guard is preserved: the acceptance-lane refusal, the merge
    override (allow_acceptance_complete=True still lands done), the
    hallucinated-cards gate, and expected_run_id atomicity.

New helper _review_owner_from_owner_map resolves state_owners["review"] from
the card's submit-stage audit comment (the owner map lives in the audit trail,
not a column — the same signal the lane transitions read), returning None when
unstamped so the completion path is unchanged for such cards.

Testing

scripts/run_tests.sh tests/hermes_cli/test_kanban_complete_author_to_review.py
— 9 new tests, all green:

  • a writing author's completion → review + the card's review owner, not done
    (and emits status_changed, no completed event)
  • a code author's completion → review + the code review owner
  • the redirect fires from a ready card too (CLI complete of a never-claimed card)
  • idempotency: a card already in review is a clean no-op (no re-move, no done)
  • a card with no stamped review owner (and one with an owner map lacking a
    review lane) still completes to done
  • the merge override still reaches done regardless of the owner map
  • a generic blocked card (needs_input) stays completable to done

Regression:
scripts/run_tests.sh tests/hermes_cli/test_kanban_complete_acceptance_guard.py tests/hermes_cli/test_kanban_db.py tests/hermes_cli/test_kanban_reconcile_pass_acceptance.py tests/hermes_cli/test_kanban_move_blocked_sticky.py tests/hermes_cli/test_kanban_auto_route_review_bounce.py — 292 green; plus the
worker-tool / CLI / swarm / goal-mode / lifecycle caller suites — 363 green.

Full suite: 37,514 passed, 39 failed. Every one of the 39 is a pre-existing,
host-environment-sensitive failure (systemd/service-manager/WSL, anthropic/qwen
OAuth with no creds, signal/timeout races, LSP e2e, TUI browser launch,
approval-cluster timing, modal/docker sandbox) that fails identically on the
base branch with this change absent
— this change introduces zero regressions.

`complete_task` unconditionally set `status='done'` on any worker
completion from the author lane (running/ready). `done` on this board
means exactly one thing — the work was merged/accepted — so an author's
end-of-lane completion landing there skips the review lane and the
acceptance gate entirely.

Code cards were only accidentally rescued: opening their PR fires the
`github-prs` webhook, which moves the card to review. A board-driven
review card (no PR-review webhook on its drafting step) had no such
rescue, so its author's completion flipped it straight to `done` past the
reviewer and past acceptance — a false-`done` that had to be reconciled
by hand every cycle.

Make the board-native path correct for every kind with a review lane.
Before the `-> done` write, when the merge override is NOT set, the card
is in the author lane (running/ready), and the card's own stamped owner
map declares a `review` owner, MOVE the card to `status='review'` +
that owner and emit a `status_changed` event instead of completing it.

- No kind-default fallback: a card with no stamped review lane (legacy /
  un-stamped / plain task, research swarm root) completes to `done`
  exactly as before — the redirect never shunts an undeclared card.
- Idempotent with the webhook path: once in `review` the card is no
  longer running/ready, so a second completion is a clean no-op and the
  `-> done` UPDATE cannot match it.
- The acceptance-lane refusal, the merge override
  (`allow_acceptance_complete=True`), the hallucinated-cards gate, and
  `expected_run_id` atomicity are all preserved.

Add `_review_owner_from_owner_map`, a reader that resolves
`state_owners["review"]` from the card's submit-stage audit comment
(the owner map lives in the audit trail, not a column), returning None
when unstamped so the completion path is unchanged for such cards.

<!-- card:t_a5a4fbf6 -->
@cwest
cwest marked this pull request as ready for review July 10, 2026 14:40

@cwest cwest left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

No changes needed.

The redirect sits in the right place — after the hallucinated-cards gate and the acceptance-lane refusal, before the -> done write — and fires only when the merge override is off, the card is in the author lane (running/ready), and the card's own stamped owner map names a review owner. Closing the run with _end_run(status='review', outcome='completed') nulls current_run_id, so idempotency is structural rather than a check that can drift: a second completion on a review card finds no matching status and the -> done UPDATE can't fire. Threading expected_run_id into the move UPDATE keeps it scoped to the worker's own run.

Every pre-existing guard survives: the acceptance refusal, the merge override, the phantom-card gate, and run-id atomicity. Ran the new file (9/9) plus the complete_task callers and lifecycle suites — acceptance guard, kanban_db, core, CLI, swarm, goal-mode, lifecycle hooks, reconcile-acceptance, auto-route-bounce, move-blocked-sticky, promote, reset-block-loop — 564 green, no regressions. The state_owners["review"] reader is new here rather than reused; the helper the spec referenced isn't on the integration branch, so writing one was the correct call.

One thing to keep in mind for later, not a change to make now: the author-lane test is status IN ('running','ready'), which a review-in-progress card also matches (claim_review_task takes review to running). Nothing exercises that path today because the review lane exits via block, not complete, and if it ever did the card would just return to review — it can never reach done — so the invariant this fix exists to protect holds unconditionally. Worth a comment or a lane check if the completion surface ever widens.

@cwest
cwest merged commit e20f1ed into cwest/integration Jul 10, 2026
31 checks passed
@cwest
cwest deleted the topic/complete-author-to-review branch July 10, 2026 14:46
cwest added a commit that referenced this pull request Jul 26, 2026
#59)

`complete_task` unconditionally set `status='done'` on any worker
completion from the author lane (running/ready). `done` on this board
means exactly one thing — the work was merged/accepted — so an author's
end-of-lane completion landing there skips the review lane and the
acceptance gate entirely.

Code cards were only accidentally rescued: opening their PR fires the
`github-prs` webhook, which moves the card to review. A board-driven
review card (no PR-review webhook on its drafting step) had no such
rescue, so its author's completion flipped it straight to `done` past the
reviewer and past acceptance — a false-`done` that had to be reconciled
by hand every cycle.

Make the board-native path correct for every kind with a review lane.
Before the `-> done` write, when the merge override is NOT set, the card
is in the author lane (running/ready), and the card's own stamped owner
map declares a `review` owner, MOVE the card to `status='review'` +
that owner and emit a `status_changed` event instead of completing it.

- No kind-default fallback: a card with no stamped review lane (legacy /
  un-stamped / plain task, research swarm root) completes to `done`
  exactly as before — the redirect never shunts an undeclared card.
- Idempotent with the webhook path: once in `review` the card is no
  longer running/ready, so a second completion is a clean no-op and the
  `-> done` UPDATE cannot match it.
- The acceptance-lane refusal, the merge override
  (`allow_acceptance_complete=True`), the hallucinated-cards gate, and
  `expected_run_id` atomicity are all preserved.

Add `_review_owner_from_owner_map`, a reader that resolves
`state_owners["review"]` from the card's submit-stage audit comment
(the owner map lives in the audit trail, not a column), returning None
when unstamped so the completion path is unchanged for such cards.

<!-- card:t_a5a4fbf6 -->

(cherry picked from commit e20f1ed)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant