feat(kanban): separate builder corrections from owner asks

contributors/emails/choimoonyoung3631@gmail.com

@@ -0,0 +1 @@
+moonweave

hermes_cli/kanban_db.py

@@ -88,7 +88,7 @@
 from contextvars import ContextVar, Token
 from dataclasses import dataclass, field
 from pathlib import Path
-from typing import Any, Iterable, Mapping, Optional
+from typing import Any, Callable, Iterable, Mapping, Optional
 
 from hermes_cli.sqlite_util import add_column_if_missing as _add_column_if_missing
 from toolsets import get_toolset_names
@@ -10939,31 +10939,21 @@ def _unresolved_run_stats(conn: sqlite3.Connection) -> dict:
 #: diagnostic matches on (kanban_diagnostics.py), deliberately: two definitions
 #: of "waiting for review" that could drift apart would be worse than one.
 _REVIEW_REQUIRED_REASON_PREFIX = "review-required:"
+_NEEDS_WORK_REASON_PREFIX = "needs_work:"
+_REVIEW_HOLD_REASON_RE = re.compile(r"^review_r\d+_hold(?:\b|:)", re.IGNORECASE)
 
-def _awaiting_review_stats(
-    conn: sqlite3.Connection, human_stopped_statuses: list[str]
-) -> int:
-    """How many needs-input rows are waiting on a review, not on the operator.
 
-    ``request-review`` moves a finished implementation to ``review``, and its
-    own CLI help ends with "NOT a block". A worker that instead calls
-    ``kanban_block(kind="needs_input", reason="review-required: ...")`` leaves
-    the row in ``blocked``, which the review dispatcher never claims -- it
-    claims ``status = 'review'`` only. The row is then unreachable by any
-    autonomous step and indistinguishable, in the queue count, from a genuine
-    question for the operator.
-
-    The ``review_dependency_deadlock`` diagnostic already recognises this exact
-    reason prefix, but only fires when the stalled parent is starving a ``todo``
-    child, so it stays silent on a row whose subtree happens to be empty.
-    Measured across all four live boards: 28 rows carry the prefix, one has a
-    waiting child. This counts the population; the diagnostic keeps reporting
-    the starvation case.
+def _needs_input_reason_subset_stats(
+    conn: sqlite3.Connection,
+    human_stopped_statuses: list[str],
+    matches: Callable[[str], bool],
+) -> int:
+    """Count stopped needs-input rows whose current reason matches ``matches``.
 
-    Deliberately a strict subset of the ``needs_input`` count above -- same
-    ``block_kind`` and same ``blocked``/``triage`` scope -- so the two numbers
-    reconcile and the remainder is a real "waiting on a person" figure. Counts
-    only: no id, title, or reason text leaves this function.
+    The task stores only the block kind. The reason that is currently in force
+    lives on the latest ``blocked`` or ``block_loop_detected`` event, so every
+    reason-derived aggregate must use the same event selection and must never
+    expose the reason itself.
     """
     placeholders = ", ".join("?" for _ in human_stopped_statuses)
     rows = conn.execute(
@@ -10972,16 +10962,8 @@ def _awaiting_review_stats(
         human_stopped_statuses,
     ).fetchall()
 
-    awaiting = 0
+    matched = 0
     for row in rows:
-        # Both kinds carry the reason that set the block currently in force.
-        # `blocked` alone is not enough: BLOCK_RECURRENCE_LIMIT re-routes a
-        # repeatedly-blocked row to `triage` and records the new reason under
-        # `block_loop_detected`, so reading only `blocked` would answer with a
-        # superseded reason -- and `triage` is half of this function's own
-        # population. Ordered by `id`, not `created_at`: block and re-block
-        # land in the same second routinely, which is exactly when the
-        # distinction matters.
         event = conn.execute(
             "SELECT payload FROM task_events "
             "WHERE task_id = ? AND kind IN ('blocked', 'block_loop_detected') "
@@ -10996,10 +10978,60 @@ def _awaiting_review_stats(
             continue
         if not isinstance(payload, dict):
             continue
-        reason = str(payload.get("reason") or "").strip().lower()
-        if reason.startswith(_REVIEW_REQUIRED_REASON_PREFIX):
-            awaiting += 1
-    return awaiting
+        if matches(str(payload.get("reason") or "").strip()):
+            matched += 1
+    return matched
+
+def _awaiting_review_stats(
+    conn: sqlite3.Connection, human_stopped_statuses: list[str]
+) -> int:
+    """How many needs-input rows are waiting on a review, not on the operator.
+
+    ``request-review`` moves a finished implementation to ``review``, and its
+    own CLI help ends with "NOT a block". A worker that instead calls
+    ``kanban_block(kind="needs_input", reason="review-required: ...")`` leaves
+    the row in ``blocked``, which the review dispatcher never claims -- it
+    claims ``status = 'review'`` only. The row is then unreachable by any
+    autonomous step and indistinguishable, in the queue count, from a genuine
+    question for the operator.
+
+    The ``review_dependency_deadlock`` diagnostic already recognises this exact
+    reason prefix, but only fires when the stalled parent is starving a ``todo``
+    child, so it stays silent on a row whose subtree happens to be empty.
+    Measured across all four live boards: 28 rows carry the prefix, one has a
+    waiting child. This counts the population; the diagnostic keeps reporting
+    the starvation case.
+
+    Deliberately a strict subset of the ``needs_input`` count above -- same
+    ``block_kind`` and same ``blocked``/``triage`` scope. The owner-facing
+    remainder must also subtract the distinct builder-correction subset below.
+    Counts only: no id, title, or reason text leaves this function.
+    """
+    return _needs_input_reason_subset_stats(
+        conn,
+        human_stopped_statuses,
+        lambda reason: reason.lower().startswith(_REVIEW_REQUIRED_REASON_PREFIX),
+    )
+
+
+def _needs_work_stats(
+    conn: sqlite3.Connection, human_stopped_statuses: list[str]
+) -> int:
+    """How many needs-input rows are a builder correction, not an owner ask.
+
+    ``needs_work:`` and ``REVIEW_R<n>_HOLD`` are explicit reviewer-to-builder
+    routing markers. They can be emitted under ``needs_input`` even though the
+    next action is code or evidence repair. Keep the aggregate disjoint from
+    review-required handoffs and expose only its count.
+    """
+    return _needs_input_reason_subset_stats(
+        conn,
+        human_stopped_statuses,
+        lambda reason: (
+            reason.lower().startswith(_NEEDS_WORK_REASON_PREFIX)
+            or _REVIEW_HOLD_REASON_RE.match(reason) is not None
+        ),
+    )
 
 
 def _needs_input_stats(conn: sqlite3.Connection) -> dict:
@@ -11156,6 +11188,9 @@ def _for_kind(kind: Optional[str]) -> tuple[int, Optional[int], int]:
     capability_rows, oldest_capability, capability_children = _for_kind("capability")
     untyped_rows, oldest_untyped, untyped_children = _for_kind(None)
     awaiting_review_rows = _awaiting_review_stats(conn, human_stopped_statuses)
+    needs_work_rows = _needs_work_stats(conn, human_stopped_statuses)
+    if awaiting_review_rows + needs_work_rows > needs_input_rows:
+        raise ValueError("needs-input reason subsets exceed needs-input rows")
 
     return {
         "needs_input_rows": needs_input_rows,
@@ -11171,6 +11206,11 @@ def _for_kind(kind: Optional[str]) -> tuple[int, Optional[int], int]:
         # whole queue as one number tells an operator that 53 things need them
         # when most need a handoff that silently never happens.
         "needs_input_awaiting_review_rows": awaiting_review_rows,
+        # Strict subset of `needs_input_rows`: reviewer-to-builder correction
+        # markers, not a question or action for the owner. The CLI projection
+        # deliberately returns the count only; task IDs and review reasons stay
+        # inside the board database.
+        "needs_input_needs_work_rows": needs_work_rows,
         # A hard wall the agent cannot pass: no access, missing credentials, an
         # action no AI agent can perform. The schema calls it "genuinely
         # human-only", so it belongs in the same queue as needs_input but is a
@@ -11284,6 +11324,13 @@ def board_stats(conn: sqlite3.Connection) -> dict:
         "needs_input_awaiting_review_rows": needs_input[
             "needs_input_awaiting_review_rows"
         ],
+        # Strict subset of `needs_input_rows`: review findings whose explicit
+        # next action is code or evidence correction by the builder/CTO, not a
+        # decision from the owner. Count-only for the same privacy boundary as
+        # the adjacent review-handoff aggregate.
+        "needs_input_needs_work_rows": needs_input[
+            "needs_input_needs_work_rows"
+        ],
         # The other two kinds the schema comment (kanban_db.py:110-125) routes
         # to a human. `capability` is a hard wall -- no access, missing creds,
         # an action no AI agent can perform, "genuinely human-only". An

tests/hermes_cli/test_kanban_core_functionality.py

@@ -1885,6 +1885,37 @@ def _blocked(title, *, reason, kind="needs_input"):
     # `capability` keeps its own bucket and contributes nothing here.
     assert stats["capability_rows"] == 1
 
+
+def test_board_stats_separates_builder_corrections_from_owner_questions(kanban_home):
+    """Review HOLDs must not inflate the queue sent to the owner."""
+    with kb.connect() as conn:
+
+        def _blocked(title, reason):
+            task_id = kb.create_task(conn, title=title, assignee="builder")
+            kb.claim_task(conn, task_id)
+            assert kb.block_task(conn, task_id, reason=reason, kind="needs_input")
+            return task_id
+
+        _blocked("builder correction", "needs_work: fix reviewer finding")
+        _blocked("review hold", "REVIEW_R12_HOLD: refresh evidence")
+        _blocked("an actual operator ask", "operator: select the release window")
+
+        stats = kb.board_stats(conn)
+
+    assert stats["needs_input_rows"] == 3
+    assert stats["needs_input_awaiting_review_rows"] == 0
+    assert stats["needs_input_needs_work_rows"] == 2
+    assert (
+        stats["needs_input_rows"]
+        - stats["needs_input_awaiting_review_rows"]
+        - stats["needs_input_needs_work_rows"]
+        == 1
+    )
+    serialized = json.dumps(stats)
+    assert "fix reviewer finding" not in serialized
+    assert "refresh evidence" not in serialized
+
+
 def test_a_later_block_reason_replaces_an_earlier_one_in_the_review_split(kanban_home):
     """Only the reason currently in force decides, whichever event carries it.
 
@@ -1910,7 +1941,7 @@ def test_a_later_block_reason_replaces_an_earlier_one_in_the_review_split(kanban
         assert kb.block_task(
             conn,
             task_id,
-            reason="needs_work: which origin should this use?",
+            reason="operator: which origin should this use?",
             kind="needs_input",
         )