diff --git a/contributors/emails/choimoonyoung3631@gmail.com b/contributors/emails/choimoonyoung3631@gmail.com new file mode 100644 index 000000000000..eb3af9d26cef --- /dev/null +++ b/contributors/emails/choimoonyoung3631@gmail.com @@ -0,0 +1 @@ +moonweave diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 25e94bda87a5..1bb81af65514 100644 --- a/hermes_cli/kanban_db.py +++ b/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_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 diff --git a/tests/hermes_cli/test_kanban_core_functionality.py b/tests/hermes_cli/test_kanban_core_functionality.py index affb3ce7091a..e3e7b9c26a74 100644 --- a/tests/hermes_cli/test_kanban_core_functionality.py +++ b/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", )