Skip to content

fix(kanban): enforce review change loop before completion - #58695

Open
herbalizer404 wants to merge 1 commit into
NousResearch:mainfrom
herbalizer404:fix/kanban-review-change-loop
Open

herbalizer404 wants to merge 1 commit into
NousResearch:mainfrom
herbalizer404:fix/kanban-review-change-loop

Conversation

@herbalizer404

Copy link
Copy Markdown
Contributor

Problem

Kanban reviewer handoffs can be accidentally finalized or re-triaged after a reviewer returns CHANGES REQUESTED.

Two bad outcomes follow:

  • a review-required: re-block can hit the generic unblock-loop breaker and route to triage instead of preserving the normal reviewer → coder change loop;
  • a scoped kanban_complete can complete a task even though the latest reviewer verdict requested changes and no later approval exists.

Root cause

The Kanban lifecycle already treats review blocks specially in several places, but the generic blocked-loop guard and completion path did not enforce the review verdict invariant.

Fix

  • Keep repeated review-required: blocks out of the generic unblock-loop breaker so the normal change loop can continue.
  • Reject scoped kanban_complete when the latest reviewer verdict is CHANGES REQUESTED and no later APPROVE comment exists.
  • Add focused regression coverage for both paths.

Tests

python -m pytest \
  tests/hermes_cli/test_kanban_db.py \
  tests/hermes_cli/test_kanban_block_kinds.py \
  tests/hermes_cli/test_kanban_blocked_sticky.py \
  tests/tools/test_kanban_tools.py \
  -q

Result: 340 passed in 7.68s.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have labels Jul 5, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the focused regression coverage. The repeated-review-block premise is present on current main: hermes_cli/kanban_db.py:4876 sends every same-kind second block to triage, while tools/kanban_tools.py:600-632 has no review-verdict completion gate.

Problems

  • The proposed completion gate does not establish that a verdict came from a reviewer. It scans every comment body, but kanban_comment allows cross-task comments and derives authorship from the worker profile (tools/kanban_tools.py:816-830). A completing worker can therefore post APPROVE and bypass the intended invariant.
  • “Latest” is not reliable here: add_comment stores only second-resolution timestamps (hermes_cli/kanban_db.py:2930), and list_comments orders only by created_at (hermes_cli/kanban_db.py:2945-2948). The proposed index comparison is ambiguous for same-second comments.

Suggested changes

  • Gate on an explicit, ordered review verdict written by an authorized reviewer identity; otherwise retain the reblock fix as a focused change.
  • If comments remain the source of truth, order by created_at, id and test conflicting same-second verdicts.

Automated hermes-sweeper review.

Comment thread tools/kanban_tools.py
# _goal_judge_available for why an unavailable judge fails open.
task = kb.get_task(conn, tid)
if task and os.environ.get("HERMES_KANBAN_TASK") == tid:
comments = kb.list_comments(conn, tid)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

list_comments() returns arbitrary worker comments, not authenticated reviewer verdicts: kanban_comment permits cross-task comments and records the caller's profile as author. The completing worker can post APPROVE before completion, so this does not enforce the stated reviewer invariant. Also, its current ORDER BY created_at uses second-resolution timestamps and cannot determine a latest comment on ties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants