Skip to content

🐛 fix(kanban): route a review bounce via the card's owner map, not just event history - #69

Merged
cwest merged 1 commit into
cwest/integrationfrom
topic/auto-route-review-bounce-by-owner-map
Jul 19, 2026
Merged

🐛 fix(kanban): route a review bounce via the card's owner map, not just event history#69
cwest merged 1 commit into
cwest/integrationfrom
topic/auto-route-review-bounce-by-owner-map

Conversation

@cwest

@cwest cwest commented Jul 19, 2026

Copy link
Copy Markdown
Owner

Why

A reviewer bounce logged as kanban_block(kind="needs_input", reason="review-changes-requested: …") could land the card blocked and stay there — routing back to the author never fired, and a human had to hand-unblock. Live symptom 2026-07-19: two cards sat blocked ~1h.

The root cause is not the block kindauto_route_review_bounce already matches purely on the reason prefix (any kind lands in blocked with a review-changes-requested: reason). The defect is author resolution. The router resolved the author only via _resolve_review_author, which keys on the assigned {from, to} event shape emitted by the one-card move helper. When the review move instead recorded the assigned {assignee: X} shape (a non-move_card reassignment), author resolution returned None, and the card was left parked — so a bounce whose author only lived in the card's stamped owner map never routed.

What

auto_route_review_bounce now prefers the card's stamped state_owners[ready] owner (the implementing author) when resolving the routing target, falling back to event-history resolution for legacy / un-stamped cards. The owner map is authoritative and independent of the assigned-event shape, so the whole review-bounce routing class is fixed: the router matches on the reason prefix (regardless of block kind) and now always finds the author from the map. A block whose reason is not the review-bounce prefix still parks for the human.

Refactor: extract _owner_from_owner_map(conn, task_id, lane); _review_owner_from_owner_map delegates to it (still defined exactly once) and a new _ready_owner_from_owner_map reads the ready lane.

Tests

Behavior-contract tests added to test_kanban_auto_route_review_bounce.py:

  • a needs_input review-changes-requested bounce whose author is unresolvable from event history routes blocked → ready + the author from state_owners[ready], with an audit comment naming the PR;
  • a needs_input block with a non-bounce reason stays parked, even with an owner map stamped.

Full kanban suite green (866 tests). The one unrelated pre-existing timing flake in test_signal_handler_kanban_worker.py fails identically on the untouched base.

…st event history

A review-changes-requested bounce could sit blocked forever when the card's
author was unresolvable from event history. auto_route_review_bounce resolved
the author only via _resolve_review_author, which matches the assigned
{from, to} event shape emitted by the one-card move helper. When the review
move instead recorded the assigned {assignee: X} shape (a non-move_card
reassignment), author resolution returned None and the card was left parked
for a human — regardless of block kind, so a needs_input-kinded bounce with a
review-changes-requested reason never routed to the author.

Prefer the card's stamped state_owners[ready] owner (the implementing author)
when resolving the routing target, falling back to event-history resolution
for legacy / un-stamped cards. The owner map is authoritative and independent
of the assigned-event shape, so the whole review-bounce routing class is fixed:
the router matches on the reason prefix (any block kind) and now always finds
the author from the map. A block whose reason is not the review-bounce prefix
still parks for the human.

Extract _owner_from_owner_map(conn, task_id, lane); _review_owner_from_owner_map
delegates to it (still defined once) and a new _ready_owner_from_owner_map reads
the ready lane. Behavior-contract tests: a needs_input bounce with an
unresolvable event-history author routes via the owner map; a needs_input block
with a non-bounce reason stays parked.

@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 root-cause read holds up against the code. auto_route_review_bounce already scans every blocked card and matches purely on the review-changes-requested reason prefix, so the block kind was never the problem — a needs_input bounce lands in blocked with that reason and is matched like any other. The actual gap was author resolution: _resolve_review_author only understands the assigned {from, to} event shape, and returns None when the review move recorded the {assignee: X} shape instead, which is what left those two cards parked.

Reading the author from the card's stamped state_owners[ready] first, with the event-history resolver as fallback, is the right fix. The owner map is authoritative and doesn't depend on the assigned-event shape, so the whole routing class is covered regardless of how the reassignment was recorded. The extraction into _owner_from_owner_map keeps the review-lane reader defined exactly once, and the lane comparison stays exact string equality, so blocked-acceptance can't shadow a ready or review lookup.

The two new tests earn their place. The routing test fails on the base with routed 0 vs 1 and asserts up front that event-history resolution genuinely returns None, so it pins the failure to the resolution gap rather than the routing path. The park test confirms a non-bounce needs_input block stays put even with an owner map stamped. Ran the kanban surface at the head commit: the target file is 10/10, and the wider kanban suite is green. The one failing signal-handler timing test reproduces identically on the untouched base and sits in a file this change never touches.

@cwest
cwest marked this pull request as ready for review July 19, 2026 19:02
@cwest
cwest merged commit 956b04c into cwest/integration Jul 19, 2026
31 checks passed
@cwest
cwest deleted the topic/auto-route-review-bounce-by-owner-map branch July 19, 2026 19:04
cwest added a commit that referenced this pull request Jul 26, 2026
…st event history (#69)

A review-changes-requested bounce could sit blocked forever when the card's
author was unresolvable from event history. auto_route_review_bounce resolved
the author only via _resolve_review_author, which matches the assigned
{from, to} event shape emitted by the one-card move helper. When the review
move instead recorded the assigned {assignee: X} shape (a non-move_card
reassignment), author resolution returned None and the card was left parked
for a human — regardless of block kind, so a needs_input-kinded bounce with a
review-changes-requested reason never routed to the author.

Prefer the card's stamped state_owners[ready] owner (the implementing author)
when resolving the routing target, falling back to event-history resolution
for legacy / un-stamped cards. The owner map is authoritative and independent
of the assigned-event shape, so the whole review-bounce routing class is fixed:
the router matches on the reason prefix (any block kind) and now always finds
the author from the map. A block whose reason is not the review-bounce prefix
still parks for the human.

Extract _owner_from_owner_map(conn, task_id, lane); _review_owner_from_owner_map
delegates to it (still defined once) and a new _ready_owner_from_owner_map reads
the ready lane. Behavior-contract tests: a needs_input bounce with an
unresolvable event-history author routes via the owner map; a needs_input block
with a non-bounce reason stays parked.

(cherry picked from commit 956b04c)
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