fix(kanban): first-class review lifecycle — salvage of #75451 with review fixes - #83412
Merged
Merged
Conversation
Add a non-terminal "review" status so a worker that finished implementation can hand off for human review without abusing kanban_block. The old kanban_block(reason="review-required: ...") convention routed the handoff through the unblock-loop breaker, so a normal review -> changes -> review cycle was falsely escalated to triage. - kanban_db: request_review (running/ready -> review, non-block, emits review_requested), reopen_review_task (review -> ready/todo, review_reopened), complete_task accepts review -> done, and a review_dispatch gate (default off, shared by the dispatcher loop and the gateway health probe). - kanban_request_review worker tool + `request-review` / `reopen-review` CLI verbs; tool wired through toolsets, EXPOSED_TOOLS, _POLISHED_TOOLS. - Gateway notifier wakes the origin subscriber on review_requested and block_loop_detected; the subscription survives until done/archived, so every review cycle re-notifies. - Dashboard PATCH + bulk route the review transitions (request_review / reopen_review_task) and render the review column. - goals.py goal-loop and KANBAN_GUIDANCE recognize review as a terminator. - Docs (reference tables, user guide, AGENTS.md, zh-Hans mirrors) + tests. needs_input / failed are unchanged: they still route through kanban_block, still count toward block_recurrences, and still escalate to triage.
Close the autonomous implement-review-rework loop, preserve parent gating and implementer provenance, distinguish downstream review cards, and surface legacy review dependency deadlocks immediately. Co-authored-by: kaishi00 <6590895+kaishi00@users.noreply.github.com>
Plain write_txn raises loudly on nesting again (the historical main invariant); composition primitives (create_task, add_comment) opt in with allow_nested=True for savepoint semantics. create_swarm activates the swarm root with an inline blocked->done CAS flip + synthesized run + event instead of nesting complete_task, so complete_task's post-commit side effects (workspace cleanup, failure-counter clear, recompute_ready) can no longer fire under an open outer transaction; recompute_ready now runs after the outer commit. recompute_ready docstring corrected. Regression: plain nesting raises; allow_nested composes and an outer rollback discards inner work with no side effects fired.
Thread lane= into check_respawn_guard. For review-lane dispatch the active_pr and recent_success rules are skipped: a fresh PR URL comment (and often a recent completed run) is the precondition of the canonical review handoff, not a duplicate-work signal. Rate-limit cooldown and the auth-blocker check still apply in every lane. Regression: a review task with a <24h PR comment is spawned by dispatch while a ready-lane task with the same comment stays deferred; a rate_limited latest run still defers the review lane.
request_review on a running task under a live claim now requires the caller to prove ownership (expected_run_id, the unchanged worker path) or pass an explicit force=True override (CLI --force; dashboard human actions pass force=True) instead of silently clearing claim_lock / worker_pid of a live run. Failures now carry distinct diagnostic reasons via with_reason=True (mirroring request_changes' tuple pattern): live-claim refusal, malformed re-review provenance, unsatisfied parents, unknown task, and CAS miss. Tool/CLI handlers surface the specific reason instead of the generic 'unknown id or not in running/ready'. Regression tests: live-claim refusal + force/worker paths; malformed provenance gets a distinct reason and explicit reviewer= recovers.
request_changes and reopen_review_task no longer reset consecutive_failures (and last_failure_error) to 0 — review transitions are neither success nor failure signals, so the circuit-breaker counter is preserved (not incremented either), mirroring unblock_task (#35072). Only complete_task's success path clears the counter. Regression: counter=1 survives a full request_review -> request_changes -> re-request cycle; a crash after request_changes accumulates to 2 and trips a failure_limit=2 breaker; complete_task still resets to 0.
…n-silent
Ancestor-reopen descendant invalidation previously lived only in the
dashboard plugin (_set_status_direct), so board semantics diverged by
surface and the retraction was silent: completed work snapped back to
todo and live workers were killed with no operator-visible signal.
Move it into kanban_db.invalidate_descendants_for_parent_reopen as THE
single domain implementation (recursive-CTE discovery and per-run
_retry_status_for_run handling preserved). It composes under a caller's
open transaction via write_txn(allow_nested=True) — the ancestor flip
and the descendant retractions must commit atomically — and opens its
own transaction standalone. The dashboard shim now delegates; the CLI
deliberately has no done-reopen verb (reopen-review is review-phase
only), so the DB-layer function being the single implementation is the
fix, documented in its docstring.
Non-silent: every invalidated descendant gets a descendant_invalidated
event ({ancestor, prior_status, new_status, resume_status}), the legacy
status event for existing live-feed consumers, and a task comment
naming the reopened ancestor. Running descendants keep the termination
behavior (a child building on a retracted premise is wasted spend), but
the events/comment are committed BEFORE the kill, which routes through
_terminate_reclaimed_worker — the same helper the reclaim paths use.
consecutive_failures resets to 0 on invalidated descendants: operator-
initiated invalidation is a deliberate fresh start, deliberately the
opposite of the review-loop rule (reopen_review_task preserves the
counter, #35072) so the autonomous review loop can't launder its own
failure streak.
Regression: DB-function reopen demotes done descendants with events +
comments; running descendant's audit trail is durable before its worker
dies; counter resets; dashboard and DB paths produce identical task
states, event kinds, and comment counts.
Contributor
૮ >ﻌ< ა ci reviewran on 647f2e9 — fix(kanban): move descendant invalidation to domain layer, m
|
This was referenced Aug 10, 2026
Contributor
|
@teknium1 I'd built a local version of this just before it landed. Yours is better — checking source_status + run_id is a real guarantee; mine was just reading a verdict out of a comment. |
19 tasks
19 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Salvages PR #75451's first-class kanban review lifecycle onto current main with five review-identified fixes on top: same-card coder⇄reviewer loops now work end-to-end (
request_review→ reviewer profile →kanban_complete/kanban_request_changes→ same card back to the implementer, provenance preserved), closing the deterministicreview-required:dependency deadlock.Consolidates the community review-loop cluster: base work by @frizikk (#75451), building on @capybara-x / Nikita Barkov (#72554) and @kaishi00 (#49368) — all 14 contributor commits preserved via cherry-pick.
Changes
sdlc-reviewbundled skill (environments-gated to kanban), ~1,700 lines of lifecycle tests, en+zh-Hans docswrite_txnnesting is explicit opt-in again (allow_nested=True); plain nesting raises like main always did; swarm root activation inlined so complete_task side effects can't fire under a rollback-able outer txnactive_pr/recent_successrespawn-guard rules (an existing PR is the precondition for review, not a dupe signal); rate-limit cooldown still appliesrequest_reviewrefuses to yank a live worker's claim without run-id proof or--force; malformed re-review provenance gets a distinct failure reason surfaced through tool/CLIconsecutive_failurespreserved (not reset) across review round-trips — crash-looping implementers still trip the breakerkanban_db(single implementation, all surfaces), non-silent (descendant_invalidatedevents + comments), kills routed through the reclaim helper post-commitValidation
Supersedes (to close with credit after merge): #49368, #43060, #54849, #81217, #79061, #76892, #71926, #67826; rejected direction: #68210. Fixes #67963.
Infographic