From 0b1227d8b9181b8129b49ba87750ec06cea568dc Mon Sep 17 00:00:00 2001 From: Jarvis Date: Thu, 11 Jun 2026 08:43:25 -0700 Subject: [PATCH 1/2] =?UTF-8?q?feat(kanban):=20v6.8=20#5=20=E2=80=94=20int?= =?UTF-8?q?egrative-review=20gate=20walks=20transitive=20descendants?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The v6.7 validation chain (2026-06-10 on hermes-dashboard) exposed that ``_v6_7_should_spawn_integrative_review`` checked ``task_links WHERE parent_id = umbrella_id`` — direct children only. The validation chain shape was: umbrella → pepper → friday → tony → friday-rem → tony-rev → tchalla …where each task's parent was the PREVIOUS task, not the umbrella. The gate saw exactly 1 direct child (Pepper, non-review) → no spawn. In production today, integrative review at archive does NOT fire for chains that fan out via task chaining — the common shape. Closes hermes-jarvis#73. ## Fix Replace direct-child SQL with a recursive CTE in a new helper ``_v6_7_walk_descendants``. Walks ``task_links`` transitively from the umbrella down. Cycle-safe (sqlite's recursive CTE with UNION deduplicates so cyclic graphs don't infinite-loop). Both ``has_review_child`` and ``has_non_review_child`` now consider any descendant. The previous fan-out behavior is preserved because direct children are also transitive descendants. ## Tests 7 new tests in ``TestTransitiveDescendantWalk``: - chained shape (2026-06-10 validation case) now triggers spawn - fan-out shape (original v6.7 design) still triggers - mixed shape (some direct, some chained) triggers - in-flight chained descendant blocks spawn (existing semantics) - cyclic links don't infinite-loop - _v6_7_walk_descendants returns all levels - _v6_7_walk_descendants empty on orphan umbrella 37/37 in test_v6_7_integrative_review.py pass. 183/183 across v6.7 + adjacent regression set — zero failures. Co-Authored-By: Claude Opus 4.7 (1M context) --- hermes_cli/kanban_db.py | 59 +++++++++--- tests/cli/test_v6_7_integrative_review.py | 111 ++++++++++++++++++++++ 2 files changed, 157 insertions(+), 13 deletions(-) diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 2478f76ed3ff..71b938a0e602 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -4922,35 +4922,68 @@ def _v6_7_count_integrative_reviews( return int(row["c"]) if row else 0 +def _v6_7_walk_descendants( + conn: sqlite3.Connection, umbrella_id: str, +) -> list: + """Return all transitive descendants of ``umbrella_id`` via + ``task_links``. Cycle-safe (visited set prevents infinite loop on + malformed graphs). Self-excluded. + + Returns rows with id/status/assignee/title columns. Order is + breadth-first from umbrella; callers shouldn't rely on a specific + order beyond that. + + Closes hermes-jarvis#73. Replaces the previous direct-children-only + walk which failed to fire on the common chained shape: + ``umbrella → A → B-review`` (per-block reviews chained off the + build task rather than fanned out from umbrella). + """ + rows = conn.execute( + """ + WITH RECURSIVE descendants(id) AS ( + SELECT child_id FROM task_links WHERE parent_id = ? + UNION + SELECT l.child_id + FROM task_links l + JOIN descendants d ON l.parent_id = d.id + ) + SELECT t.id, t.status, t.assignee, t.title + FROM tasks t + JOIN descendants d ON d.id = t.id + """, + (umbrella_id,), + ).fetchall() + return list(rows) + + def _v6_7_should_spawn_integrative_review( conn: sqlite3.Connection, umbrella_id: str, ) -> bool: """True if the umbrella looks like a JARVIS goal-mode chain whose - per-block children are all terminal and either no integrative - review exists yet, OR the latest one was rejected (re-spawn). + per-block work is all terminal and either no integrative review + exists yet, OR the latest one was rejected (re-spawn). Conservative — fires only when ALL of: 1. Umbrella is ``goal_mode=True`` (JARVIS keep_running chain) - 2. Has ≥1 child via task_links - 3. Every non-review child is in {done, archived} (NOT blocked) - 4. ≥1 review-role child exists (no point integrating over a chain - with no per-block reviews) - 5. ≥1 non-review child exists (don't fire on review-only chains) + 2. Has ≥1 transitive descendant via ``task_links`` + 3. Every non-review descendant is in {done, archived} (NOT blocked) + 4. ≥1 review-role descendant exists (no point integrating over a + chain with no per-block reviews) + 5. ≥1 non-review descendant exists (don't fire on review-only chains) 6. Either no integrative review exists yet, OR the latest one is done with a rejecting verdict (so a re-spawn after remediation is the right move) + + Descendant walk uses a recursive CTE so chained shapes + ``umbrella → build → review`` count the review as a "child" for + the purpose of this gate. Closes hermes-jarvis#73. """ row = conn.execute( "SELECT goal_mode FROM tasks WHERE id = ?", (umbrella_id,), ).fetchone() if row is None or not row["goal_mode"]: return False - children = conn.execute( - "SELECT t.id, t.status, t.assignee, t.title " - " FROM tasks t JOIN task_links l ON l.child_id = t.id " - " WHERE l.parent_id = ?", - (umbrella_id,), - ).fetchall() + children = _v6_7_walk_descendants(conn, umbrella_id) if not children: return False terminal_statuses = {"done", "archived"} # blocked is NOT terminal diff --git a/tests/cli/test_v6_7_integrative_review.py b/tests/cli/test_v6_7_integrative_review.py index 14d39bcd9fea..9d9f51d5c9ad 100644 --- a/tests/cli/test_v6_7_integrative_review.py +++ b/tests/cli/test_v6_7_integrative_review.py @@ -523,3 +523,114 @@ def test_lookup_requires_created_by_dispatcher(self, board_conn) -> None: ).fetchone() assert real_rev is not None assert real_rev["id"] != "t_fake_rev" + + +# ===================================================================== +# hermes-jarvis#73 — transitive descendant walk +# ===================================================================== + + +class TestTransitiveDescendantWalk: + """The v6.7 validation chain (2026-06-10) revealed that the gate + only walked direct children of the umbrella, missing chained + shapes like ``umbrella → build → review`` where the review's + parent is the build task (not the umbrella). + + These tests pin the transitive walk behavior across common shapes. + """ + + def test_chained_shape_triggers_spawn(self, board_conn) -> None: + """The v6.7 validation case: ``umbrella → pepper → friday → + tony → tchalla`` — each task's parent is the previous task, + not the umbrella. Should still trigger #30 because tony and + tchalla ARE descendants.""" + _mk_task(board_conn, "t_chain_umb", goal_mode=True, status="running") + _mk_task(board_conn, "t_chain_pepper", assignee="pepper", status="done") + _mk_task(board_conn, "t_chain_friday", assignee="friday", status="done") + _mk_task(board_conn, "t_chain_tony", assignee="tony", status="done") + _mk_task(board_conn, "t_chain_tchalla", assignee="tchalla", status="done") + # Chained edges — NOT fanned out from umbrella + _link(board_conn, "t_chain_umb", "t_chain_pepper") + _link(board_conn, "t_chain_pepper", "t_chain_friday") + _link(board_conn, "t_chain_friday", "t_chain_tony") + _link(board_conn, "t_chain_tony", "t_chain_tchalla") + board_conn.commit() + ok = archive_task(board_conn, "t_chain_umb") + # Was returning True (no spawn, immediate archive) before #73 fix + assert ok is False + rev = _latest_integrative_review(board_conn, "t_chain_umb") + assert rev is not None + assert rev["status"] == "ready" + + def test_fanout_shape_still_triggers_spawn(self, board_conn) -> None: + """The fan-out shape (direct children, original v6.7 design) + must continue to work — the fix should be additive.""" + _seed_canonical_umbrella(board_conn) + ok = archive_task(board_conn, "t_umb") + assert ok is False + rev = _latest_integrative_review(board_conn, "t_umb") + assert rev is not None + + def test_mixed_shape_triggers_spawn(self, board_conn) -> None: + """Mixed shape — some direct, some chained — also works.""" + _mk_task(board_conn, "t_mix_umb", goal_mode=True, status="running") + _mk_task(board_conn, "t_mix_fr", assignee="friday", status="done") + _link(board_conn, "t_mix_umb", "t_mix_fr") + _mk_task(board_conn, "t_mix_tn", assignee="tony", status="done") + _link(board_conn, "t_mix_fr", "t_mix_tn") + board_conn.commit() + ok = archive_task(board_conn, "t_mix_umb") + assert ok is False + + def test_inflight_chained_descendant_blocks_spawn(self, board_conn) -> None: + """A non-terminal non-review descendant 3 levels deep should + prevent the gate from firing.""" + _mk_task(board_conn, "t_deep_umb", goal_mode=True, status="running") + _mk_task(board_conn, "t_deep_pepper", assignee="pepper", status="done") + _mk_task(board_conn, "t_deep_friday", assignee="friday", + status="running") # IN FLIGHT + _mk_task(board_conn, "t_deep_tony", assignee="tony", status="done") + _link(board_conn, "t_deep_umb", "t_deep_pepper") + _link(board_conn, "t_deep_pepper", "t_deep_friday") + _link(board_conn, "t_deep_friday", "t_deep_tony") + board_conn.commit() + # Friday is still running — archive should pass through (no + # integrative review spawned because the build chain isn't + # done; matches the existing in-flight semantics). + ok = archive_task(board_conn, "t_deep_umb") + assert ok is True + + def test_cyclic_links_dont_infinite_loop(self, board_conn) -> None: + """Defensive: a cyclic task_links graph must not infinite-loop + the gate. sqlite's recursive CTE handles UNION correctly so + cycles deduplicate automatically.""" + _mk_task(board_conn, "t_cyc_umb", goal_mode=True, status="running") + _mk_task(board_conn, "t_cyc_a", assignee="friday", status="done") + _mk_task(board_conn, "t_cyc_b", assignee="tony", status="done") + _link(board_conn, "t_cyc_umb", "t_cyc_a") + _link(board_conn, "t_cyc_a", "t_cyc_b") + _link(board_conn, "t_cyc_b", "t_cyc_a") # cycle: a → b → a + board_conn.commit() + ok = archive_task(board_conn, "t_cyc_umb") + assert ok is False # review spawned, no hang + + def test_walk_descendants_returns_all_levels(self, board_conn) -> None: + """Direct unit test for _v6_7_walk_descendants helper.""" + from hermes_cli.kanban_db import _v6_7_walk_descendants + _mk_task(board_conn, "t_w_root", goal_mode=True, status="running") + _mk_task(board_conn, "t_w_l1", assignee="pepper", status="done") + _mk_task(board_conn, "t_w_l2", assignee="friday", status="done") + _mk_task(board_conn, "t_w_l3", assignee="tony", status="done") + _link(board_conn, "t_w_root", "t_w_l1") + _link(board_conn, "t_w_l1", "t_w_l2") + _link(board_conn, "t_w_l2", "t_w_l3") + board_conn.commit() + rows = _v6_7_walk_descendants(board_conn, "t_w_root") + ids = {r["id"] for r in rows} + assert ids == {"t_w_l1", "t_w_l2", "t_w_l3"} + + def test_walk_descendants_empty_on_orphan(self, board_conn) -> None: + from hermes_cli.kanban_db import _v6_7_walk_descendants + _mk_task(board_conn, "t_orphan_w", goal_mode=True, status="done") + board_conn.commit() + assert _v6_7_walk_descendants(board_conn, "t_orphan_w") == [] From 77c9cf08f646faa66fcd1079fb23a090600f7724 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Thu, 11 Jun 2026 08:49:13 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fix(kanban):=20Part=201=20self-review=20?= =?UTF-8?q?=E2=80=94=20clean=20up=20comment,=20type=20annotation,=20edge-c?= =?UTF-8?q?ase=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review of the transitive-walk fix flagged three polish items. All addressed: - Docstring claimed "visited set prevents infinite loop" — false (no Python set exists). Cycle safety actually comes from sqlite's UNION dedup in the recursive CTE's working table. Replaced the lying comment and added a note that link_tasks rejects cycles at insert time so this defense is defensive-only. - Return type was bare `list`. Now `list[sqlite3.Row]`. - Added multi-parent caveat to the docstring — schema allows it and the walk is unaware of distinct-umbrella context. 3 new tests for edge cases the reviewer flagged: - test_diamond_shape_dedups_descendant — UNION correctly returns shared diamond-target exactly once. - test_deep_integrative_review_descendant_is_skipped — the title- prefix skip in the gate's iteration works at any depth, not just direct children. The test stubs a deep integrative review with a reject verdict and confirms archive evaluates cleanly without treating the deep IR as a regular review-role descendant. - test_shared_descendant_across_umbrellas_documented — pins the current (accepted) behavior that multi-parent descendants leak across umbrellas, so future readers aren't surprised. Real production chains don't share live descendants across distinct umbrellas. 40/40 in test_v6_7_integrative_review.py pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- hermes_cli/kanban_db.py | 31 ++++++--- tests/cli/test_v6_7_integrative_review.py | 82 +++++++++++++++++++++++ 2 files changed, 103 insertions(+), 10 deletions(-) diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 71b938a0e602..721e4895c1b6 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -4924,17 +4924,28 @@ def _v6_7_count_integrative_reviews( def _v6_7_walk_descendants( conn: sqlite3.Connection, umbrella_id: str, -) -> list: +) -> list[sqlite3.Row]: """Return all transitive descendants of ``umbrella_id`` via - ``task_links``. Cycle-safe (visited set prevents infinite loop on - malformed graphs). Self-excluded. - - Returns rows with id/status/assignee/title columns. Order is - breadth-first from umbrella; callers shouldn't rely on a specific - order beyond that. - - Closes hermes-jarvis#73. Replaces the previous direct-children-only - walk which failed to fire on the common chained shape: + ``task_links``. Self-excluded. + + Cycle-safe via SQL ``UNION`` dedup in the recursive CTE's working + table (sqlite stops adding rows already present). Defensive only — + ``link_tasks`` rejects cycles at insert time so production data + shouldn't have any. + + Returns rows with id/status/assignee/title columns. Callers + shouldn't rely on a specific order. + + Multi-parent caveat: the schema allows a task to have multiple + parents (``task_links`` PK is ``(parent_id, child_id)``, not + ``child_id``-unique). If a descendant of this umbrella is ALSO a + descendant of another umbrella, walking from either id returns + the shared descendant. Gate decisions about non-terminal status + therefore intersect across umbrellas — acceptable since real + chains don't share live descendants across distinct umbrellas. + + Closes hermes-jarvis#73. Replaces the previous direct-children- + only walk which failed to fire on the common chained shape: ``umbrella → A → B-review`` (per-block reviews chained off the build task rather than fanned out from umbrella). """ diff --git a/tests/cli/test_v6_7_integrative_review.py b/tests/cli/test_v6_7_integrative_review.py index 9d9f51d5c9ad..74064f13b5cc 100644 --- a/tests/cli/test_v6_7_integrative_review.py +++ b/tests/cli/test_v6_7_integrative_review.py @@ -634,3 +634,85 @@ def test_walk_descendants_empty_on_orphan(self, board_conn) -> None: _mk_task(board_conn, "t_orphan_w", goal_mode=True, status="done") board_conn.commit() assert _v6_7_walk_descendants(board_conn, "t_orphan_w") == [] + + def test_diamond_shape_dedups_descendant(self, board_conn) -> None: + """Reviewer-flagged gap: diamond shape (umb → A → C, + umb → B → C) must return C exactly once. sqlite's UNION dedups + in the recursive CTE.""" + from hermes_cli.kanban_db import _v6_7_walk_descendants + _mk_task(board_conn, "t_di_umb", goal_mode=True, status="running") + _mk_task(board_conn, "t_di_a", assignee="friday", status="done") + _mk_task(board_conn, "t_di_b", assignee="shuri", status="done") + _mk_task(board_conn, "t_di_c", assignee="tony", status="done") + _link(board_conn, "t_di_umb", "t_di_a") + _link(board_conn, "t_di_umb", "t_di_b") + _link(board_conn, "t_di_a", "t_di_c") + _link(board_conn, "t_di_b", "t_di_c") + board_conn.commit() + rows = _v6_7_walk_descendants(board_conn, "t_di_umb") + ids = [r["id"] for r in rows] + # C appears exactly once even though two paths reach it + assert ids.count("t_di_c") == 1 + assert set(ids) == {"t_di_a", "t_di_b", "t_di_c"} + + def test_deep_integrative_review_descendant_is_skipped( + self, board_conn, + ) -> None: + """Reviewer-flagged gap: with the transitive walk, an + integrative-review task that's deep in the descendant tree + (not a direct child) must still be skipped by the title- + prefix check in the gate's children iteration.""" + _mk_task(board_conn, "t_dpir_umb", goal_mode=True, status="running") + _mk_task(board_conn, "t_dpir_fr", assignee="friday", status="done") + _mk_task(board_conn, "t_dpir_tn", assignee="tony", status="done") + _link(board_conn, "t_dpir_umb", "t_dpir_fr") + _link(board_conn, "t_dpir_fr", "t_dpir_tn") + # An old integrative review hanging off Tony (deep, not direct) + _mk_task( + board_conn, "t_dpir_deep_ir", + assignee="tchalla", status="done", + title=_v6_7_integrative_title_for("t_dpir_umb"), + ) + board_conn.execute( + "UPDATE tasks SET created_by = 'dispatcher' WHERE id = 't_dpir_deep_ir'", + ) + _link(board_conn, "t_dpir_tn", "t_dpir_deep_ir") + board_conn.commit() + # The deep integ-review should be skipped in the gate's + # has_review_child / has_non_review_child counting (it's an + # artifact of #30, not a per-block review). + # Since the integrative-review lookup also finds it (created_by + # dispatcher, matching title), archive should evaluate the + # existing review's verdict path. Stub a reject so we get a + # spawn attempt rather than a noop. + board_conn.execute( + "UPDATE tasks SET result = 'verdict: reject' WHERE id = 't_dpir_deep_ir'", + ) + board_conn.commit() + # No infinite loop, gate evaluates cleanly. + result = archive_task(board_conn, "t_dpir_umb") + # Either spawns a new (round 2) review or blocks — both False + assert result is False + + def test_shared_descendant_across_umbrellas_documented( + self, board_conn, + ) -> None: + """Reviewer-flagged gap: the schema allows a task to have + multiple parents. If umbrella A and umbrella B share a + descendant, walking from either id returns the shared + descendant. This test pins the (accepted) behavior so + future readers don't get surprised.""" + from hermes_cli.kanban_db import _v6_7_walk_descendants + _mk_task(board_conn, "t_sh_a", goal_mode=True, status="running") + _mk_task(board_conn, "t_sh_b", goal_mode=True, status="running") + _mk_task(board_conn, "t_sh_shared", assignee="friday", status="done") + _link(board_conn, "t_sh_a", "t_sh_shared") + _link(board_conn, "t_sh_b", "t_sh_shared") + board_conn.commit() + rows_a = _v6_7_walk_descendants(board_conn, "t_sh_a") + rows_b = _v6_7_walk_descendants(board_conn, "t_sh_b") + # Both umbrellas see the shared descendant — multi-parent + # leakage is documented behavior. Real chains don't share + # live descendants across umbrellas, so this is acceptable. + assert "t_sh_shared" in {r["id"] for r in rows_a} + assert "t_sh_shared" in {r["id"] for r in rows_b}