From 5332df60d9637ac69a199ec1d4b47c3563c4e38a Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Tue, 23 Jun 2026 19:59:26 -0400 Subject: [PATCH 01/11] fix(agent-team): planner turn headroom + classified retry-once (Phase A) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The planner's single-shot Claude call intermittently failed with "Reached maximum number of turns (1)" — it needs slightly more headroom than the clarifier to finish emitting its JSON. Phase A of the planner-reliability plan: - plan_node now invokes with max_turns=4 (allowed_tools stays []; the extra turns buy completion, not exploration). - build_plan_prompt instructs the model to use no tools and return only JSON (a tool_use would consume the single turn before the plan is emitted). - plan_node auto-retries the model call exactly once on a TRANSIENT failure (turn-cap exhaustion or an empty reply), and fails fast on DETERMINISTIC ones (malformed JSON, missing/blank phases) — a retry would just reproduce those. review_loop_llm.py is intentionally GPT-4.1 cross-family (no claude_invoke), so it gets no turn-budget change. verifier_llm.py does use claude_invoke but is P3 build/verify scope — left for a follow-up. Tests: max_turns passthrough; retry on turn-cap and on empty; no retry on malformed JSON; the no-tools prompt line. 1387 passed. --- agent-team/agent_team/nodes/planner.py | 56 +++++++++++++++++++- agent-team/tests/test_planner.py | 72 ++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 2 deletions(-) diff --git a/agent-team/agent_team/nodes/planner.py b/agent-team/agent_team/nodes/planner.py index 6d55b65..8118590 100644 --- a/agent-team/agent_team/nodes/planner.py +++ b/agent-team/agent_team/nodes/planner.py @@ -33,6 +33,7 @@ state-transition function that is trivially unit-testable. from __future__ import annotations import json +import logging import re from collections.abc import Callable from typing import Any @@ -40,6 +41,8 @@ from typing import Any from agent_team.billing import ClaudeResult, claude_invoke from agent_team.task_model import Phase, PipelineState, TaskStatus +_log = logging.getLogger(__name__) + # Optional context-provider callable (WS5): () -> str. When injected, its # result is prepended to the planner prompt. Default None = unchanged behavior. ContextProvider = Callable[[], str] @@ -57,6 +60,14 @@ __all__ = [ # Adam (parked + ALARM) instead of looping forever. MAX_PLAN_REVISIONS = 3 +# Per-call turn headroom for the planner's single-shot Claude invoke. The +# subscription invoker defaults to 1 turn, which the planner intermittently +# exhausts before it can finish emitting its JSON ("Reached maximum number of +# turns (1)"). 4 gives enough room to FINISH the reply without inviting the +# model to wander off-task — tools stay disabled (allowed_tools default []), so +# the extra turns buy completion, not exploration. +_PLANNER_MAX_TURNS = 4 + # The reviewer verdict string that sends a plan back to the planner. Kept here # (rather than imported from a review node that does not exist yet) so this leaf # stays self-contained; the review leaf will emit this same token. @@ -206,6 +217,9 @@ def build_plan_prompt( '"summary" (string), "phases" (a non-empty list of objects each with ' '"name" and "steps", where "steps" is a non-empty list of strings). ' "Do not include prose outside the JSON.", + "Do NOT use any tools — respond directly with ONLY the JSON object. A " + "tool call would consume the single available turn before the plan is " + "emitted.", ] return "\n".join(sections) @@ -283,6 +297,22 @@ def _revision_count(state: PipelineState) -> int: return count +def _is_transient_failure(exc: Exception) -> bool: + """Decide whether a planner-call failure is worth retrying exactly once. + + TRANSIENT (retry): the single-shot Claude call hit the turn cap before it + could finish ("Reached maximum number of turns"), or the model returned an + EMPTY reply (``PlannerError`` whose message mentions "empty") — both can + succeed on a fresh attempt. Anything else (malformed JSON, missing/blank + phases) is DETERMINISTIC: a retry would just reproduce the same garbage, so + we let it propagate and fail fast. + """ + message = str(exc).lower() + if "maximum number of turns" in message: + return True + return isinstance(exc, PlannerError) and "empty" in message + + def plan_node( state: PipelineState, config: dict[str, Any] | None = None, @@ -317,8 +347,30 @@ def plan_node( ) prompt = build_plan_prompt(state, context_provider=context_provider) - result: ClaudeResult = claude_invoke(prompt, config=config) - plan = parse_plan(result.text) + + # Auto-retry-once on a TRANSIENT failure only (turn-cap exhaustion or an + # empty reply) — both can clear on a fresh attempt. A DETERMINISTIC failure + # (malformed JSON, missing/blank phases) propagates immediately: re-asking + # the same prompt would just reproduce it. + attempts = 0 + while True: + attempts += 1 + try: + result: ClaudeResult = claude_invoke( + prompt, config=config, max_turns=_PLANNER_MAX_TURNS + ) + plan = parse_plan(result.text) + break + except Exception as exc: # noqa: BLE001 - reclassified below + if attempts == 1 and _is_transient_failure(exc): + _log.warning( + "planner: transient failure on attempt %d (%s); retrying once", + attempts, + exc, + ) + continue + raise + # Record how many times we have planned so review/observability can see it. plan["revision"] = revisions diff --git a/agent-team/tests/test_planner.py b/agent-team/tests/test_planner.py index 988ddad..6e670c1 100644 --- a/agent-team/tests/test_planner.py +++ b/agent-team/tests/test_planner.py @@ -313,3 +313,75 @@ def test_plan_node_unconfigured_invoker_raises() -> None: billing._invoker = billing._unconfigured_invoker with pytest.raises(RuntimeError, match="no invoker bound"): plan_node(_state(plan={"task": "x"})) + + +# --------------------------------------------------------------------------- # +# plan_node — turn headroom + auto-retry-once (Phase A) +# --------------------------------------------------------------------------- # + + +def test_plan_node_passes_max_turns_to_invoke_seam() -> None: + # The single-shot Claude default (1 turn) is flaky; the planner asks for + # headroom so the model can FINISH its JSON. + calls = _bind_invoker(json.dumps(_VALID_PLAN)) + plan_node(_state(plan={"task": "x"})) + assert calls[0]["kw"].get("max_turns") == 4 + + +def _bind_sequenced_invoker( + replies: list[Any], +) -> list[dict[str, Any]]: + """Bind an invoker that, per call, raises if the next item is an Exception + or returns it as the reply text otherwise. Captures its calls.""" + calls: list[dict[str, Any]] = [] + queue = list(replies) + + def _fake(prompt: str, *, mode: BillingMode, **kw: Any) -> ClaudeResult: + calls.append({"prompt": prompt, "mode": mode, "kw": kw}) + item = queue.pop(0) + if isinstance(item, Exception): + raise item + return ClaudeResult(text=item, mode=mode, usage={"input_tokens": 1}) + + billing.set_invoker(_fake) + return calls + + +def test_plan_node_retries_once_on_turn_cap_then_succeeds() -> None: + # First call exhausts the single turn; the retry produces a valid plan. + calls = _bind_sequenced_invoker( + [ + Exception( + "Claude Code returned an error result: " + "Reached maximum number of turns (1)" + ), + json.dumps(_VALID_PLAN), + ] + ) + out = plan_node(_state(plan={"task": "x"})) + assert out["current_phase"] == Phase.REVIEW.value + assert out["plan"]["phases"][0]["name"] == "Phase 1 — bump" + assert len(calls) == 2 # exactly one retry + + +def test_plan_node_retries_once_on_empty_reply_then_succeeds() -> None: + # An EMPTY model reply is transient (PlannerError "empty"): retry once. + calls = _bind_sequenced_invoker([" ", json.dumps(_VALID_PLAN)]) + out = plan_node(_state(plan={"task": "x"})) + assert out["current_phase"] == Phase.REVIEW.value + assert len(calls) == 2 # one retry + + +def test_plan_node_does_not_retry_on_malformed_json() -> None: + # A non-empty but invalid reply is DETERMINISTIC: a retry would reproduce + # it, so the planner fails fast on the first attempt. + calls = _bind_sequenced_invoker(["not json {", json.dumps(_VALID_PLAN)]) + with pytest.raises(PlannerError): + plan_node(_state(plan={"task": "x"})) + assert len(calls) == 1 # no second call + + +def test_build_prompt_includes_no_tools_json_only_instruction() -> None: + prompt = build_plan_prompt(_state(plan={"task": "x"})) + assert "Do NOT use any tools" in prompt + assert "ONLY the JSON object" in prompt From 1b4d30e47f6f93d0ba4e384f91895a2648adf384 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Tue, 23 Jun 2026 20:05:14 -0400 Subject: [PATCH 02/11] feat(agent-team): add pending_questions.kind discriminator + migration (Phase B1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The plan-review gate (coming next) needs to tell its decision questions apart from clarifier questions in the durable ledger. Add a `kind` column to pending_questions (values 'clarify' | 'plan_decision'). - Fresh DBs: `kind TEXT NOT NULL DEFAULT 'clarify'` (+ CHECK) in the DDL. - Live ledger: idempotent additive migration (SCHEMA_VERSION 3→4) — a guarded ALTER (PRAGMA table_info) run from both migrate() and init_db; legacy rows take the 'clarify' default, never null. (SQLite can't add a CHECK via ALTER, so the migrated column is NOT NULL DEFAULT only; value constraint is enforced on fresh DBs by the CHECK and on all writes by the typed helper.) - ledger.post_question gains a keyword-only `kind="clarify"` (backward compatible — existing callers unchanged); PendingQuestion.from_row reads it. Tests: fresh-DB column+default, idempotent init_db, legacy-DB backfill to 'clarify', plan_decision round-trip. 1396 passed. --- agent-team/agent_team/db/schema.py | 51 ++++++++- agent-team/agent_team/db/schema.sql | 8 +- agent-team/agent_team/ledger.py | 17 ++- agent-team/tests/test_ledger.py | 23 +++++ agent-team/tests/test_schema.py | 154 ++++++++++++++++++++++++++++ 5 files changed, 247 insertions(+), 6 deletions(-) diff --git a/agent-team/agent_team/db/schema.py b/agent-team/agent_team/db/schema.py index 35f1dda..4fc3c73 100644 --- a/agent-team/agent_team/db/schema.py +++ b/agent-team/agent_team/db/schema.py @@ -52,7 +52,7 @@ __all__ = [ ] # Bump when the DDL below changes; migrate() steps a connection forward. -SCHEMA_VERSION: int = 3 +SCHEMA_VERSION: int = 4 # Default SQLite busy timeout (ms) so concurrent writers wait for the write # lock rather than failing immediately. @@ -76,7 +76,9 @@ CREATE TABLE IF NOT EXISTS pending_questions ( deadline_at TEXT, answer_json TEXT, answered_at TEXT, - answered_via TEXT + answered_via TEXT, + kind TEXT NOT NULL DEFAULT 'clarify' + CHECK (kind IN ('clarify', 'plan_decision')) ) """.strip() @@ -216,6 +218,35 @@ def connect(db_path: Path) -> sqlite3.Connection: return conn +def _pending_questions_has_kind(conn: sqlite3.Connection) -> bool: + """Return True if ``pending_questions`` already has the ``kind`` column. + + Inspects ``PRAGMA table_info(pending_questions)`` so the additive ``kind`` + migration can be applied only when absent — making it idempotent on an + already-migrated (or freshly created) DB. + """ + rows = conn.execute("PRAGMA table_info(pending_questions)").fetchall() + return any(row["name"] == "kind" for row in rows) + + +def _ensure_pending_questions_kind(conn: sqlite3.Connection) -> None: + """Idempotently add the ``kind`` discriminator column to ``pending_questions``. + + Fresh DBs get ``kind`` from :data:`PENDING_QUESTIONS_DDL`; an existing (live + R720) ledger whose table predates the column gets it via an in-place additive + ``ALTER TABLE``, guarded by :func:`_pending_questions_has_kind` so a second + run is a no-op. Existing rows take the ``'clarify'`` default. SQLite cannot + add a CHECK constraint via ALTER, so the added column carries only the + NOT NULL DEFAULT; the CHECK is enforced on fresh DBs via the CREATE DDL and + on writes via the typed insert helper. + """ + if not _pending_questions_has_kind(conn): + conn.execute( + "ALTER TABLE pending_questions " + "ADD COLUMN kind TEXT NOT NULL DEFAULT 'clarify'" + ) + + def init_db(db_path: Path) -> None: """Create the agent-team tables in ``db_path`` if absent. @@ -229,6 +260,10 @@ def init_db(db_path: Path) -> None: try: conn.execute(SCHEMA_META_DDL) conn.execute(PENDING_QUESTIONS_DDL) + # Additive in-place migration for an existing ledger whose + # pending_questions predates the ``kind`` column (CREATE IF NOT EXISTS + # above never alters an existing table). No-op on fresh/already-migrated. + _ensure_pending_questions_kind(conn) for stmt in _split_statements(PENDING_QUESTIONS_INDEXES_DDL): conn.execute(stmt) conn.execute(BUDGET_LEDGER_DDL) @@ -286,7 +321,17 @@ def migrate(conn: sqlite3.Connection) -> None: conn.execute(stmt) current = 3 - # Future steps go here: `if current < 4: ...; current = 4`. + if current < 4: + # v4: add the ``kind`` discriminator to pending_questions so the + # responder/resume layer can tell a clarifier question apart from a + # plan-review decision. Additive in-place ALTER (guarded), placed in its + # own version block ABOVE the unconditional tail per the ORDERING + # CONSTRAINT below — the tail's CREATE ... IF NOT EXISTS would NOT apply + # this alter. Existing rows take the 'clarify' default. + _ensure_pending_questions_kind(conn) + current = 4 + + # Future steps go here: `if current < 5: ...; current = 5`. # Applied UNCONDITIONALLY (idempotent IF NOT EXISTS) so an already-stamped DB # — which skips the version blocks above — still gains these tables without a diff --git a/agent-team/agent_team/db/schema.sql b/agent-team/agent_team/db/schema.sql index 6690104..b410d57 100644 --- a/agent-team/agent_team/db/schema.sql +++ b/agent-team/agent_team/db/schema.sql @@ -25,7 +25,13 @@ CREATE TABLE IF NOT EXISTS pending_questions ( deadline_at TEXT, answer_json TEXT, answered_at TEXT, - answered_via TEXT + answered_via TEXT, + -- kind: discriminates the human gate this question belongs to — + -- 'clarify' (the clarifier) or 'plan_decision' (the plan-review gate). + -- Defaults to 'clarify' so an in-place ALTER on a legacy ledger and any + -- existing rows take the clarifier value. + kind TEXT NOT NULL DEFAULT 'clarify' + CHECK (kind IN ('clarify', 'plan_decision')) ); CREATE INDEX IF NOT EXISTS idx_pending_questions_thread diff --git a/agent-team/agent_team/ledger.py b/agent-team/agent_team/ledger.py index cafb7c0..7b14c2d 100644 --- a/agent-team/agent_team/ledger.py +++ b/agent-team/agent_team/ledger.py @@ -90,10 +90,12 @@ class PendingQuestion: answer_json: str | None = None answered_at: str | None = None answered_via: str | None = None + kind: str = "clarify" @classmethod def from_row(cls, row: sqlite3.Row) -> PendingQuestion: """Build a :class:`PendingQuestion` from a ``sqlite3.Row``.""" + keys = row.keys() return cls( question_id=row["question_id"], thread_id=row["thread_id"], @@ -106,6 +108,9 @@ class PendingQuestion: answer_json=row["answer_json"], answered_at=row["answered_at"], answered_via=row["answered_via"], + # Tolerate a row read before the kind column exists (legacy/partial + # SELECT): fall back to the 'clarify' default rather than KeyError. + kind=row["kind"] if "kind" in keys else "clarify", ) @@ -123,6 +128,7 @@ def post_question( transport: str, deadline_at: str | None = None, posted_at: str | None = None, + kind: str = "clarify", ) -> None: """Insert a new ``open`` question row (delivery step 1 of §3.3.1). @@ -132,14 +138,20 @@ def post_question( retries delivery idempotently. The caller records the ref via :func:`set_channel_ref` once the post succeeds. + ``kind`` discriminates the human gate this question belongs to — + ``'clarify'`` (the clarifier, the default so existing callers are unchanged) + or ``'plan_decision'`` (the plan-review gate). The keyword-only default keeps + every existing call site writing clarifier rows with no signature change. + Raises :class:`sqlite3.IntegrityError` if ``question_id`` already exists (PK) — re-posting the same question is the caller's reconcile concern, not a silent overwrite. ``posted_at`` defaults to now (UTC ISO-8601). """ conn.execute( "INSERT INTO pending_questions " - "(question_id, thread_id, turn, status, transport, posted_at, deadline_at) " - "VALUES (?, ?, ?, 'open', ?, ?, ?)", + "(question_id, thread_id, turn, status, transport, posted_at, " + "deadline_at, kind) " + "VALUES (?, ?, ?, 'open', ?, ?, ?, ?)", ( question_id, thread_id, @@ -147,6 +159,7 @@ def post_question( transport, posted_at or _utc_now_iso(), deadline_at, + kind, ), ) diff --git a/agent-team/tests/test_ledger.py b/agent-team/tests/test_ledger.py index 144c36d..e2a2b17 100644 --- a/agent-team/tests/test_ledger.py +++ b/agent-team/tests/test_ledger.py @@ -91,6 +91,29 @@ def test_post_question_duplicate_id_raises(conn: sqlite3.Connection) -> None: post_question(conn, question_id="dup", thread_id="t", turn=1, transport="slack") +def test_post_question_defaults_kind_clarify(conn: sqlite3.Connection) -> None: + """Existing call sites (no kind arg) keep writing clarifier rows.""" + post_question(conn, question_id="q1", thread_id="t", turn=0, transport="slack") + q = get_question(conn, "q1") + assert q is not None + assert q.kind == "clarify" + + +def test_post_question_accepts_plan_decision_kind(conn: sqlite3.Connection) -> None: + """A plan-review row round-trips with kind='plan_decision' on the read path.""" + post_question( + conn, + question_id="q-pd", + thread_id="t", + turn=0, + transport="slack", + kind="plan_decision", + ) + q = get_question(conn, "q-pd") + assert q is not None + assert q.kind == "plan_decision" + + # -------------------------------------------------------------------------- # set_channel_ref — delivery step 2, guarded on status='open'. # -------------------------------------------------------------------------- diff --git a/agent-team/tests/test_schema.py b/agent-team/tests/test_schema.py index 1dca826..d7435a3 100644 --- a/agent-team/tests/test_schema.py +++ b/agent-team/tests/test_schema.py @@ -536,3 +536,157 @@ def test_migrate_adds_ingested_issues_to_a_legacy_v1_db(tmp_path: Path) -> None: assert ver == SCHEMA_VERSION finally: conn.close() + + +# --------------------------------------------------------------------------- # +# pending_questions.kind discriminator (schema v4) +# --------------------------------------------------------------------------- # + + +# Legacy (pre-kind) pending_questions DDL, used to construct a DB whose table +# predates the additive migration. +_LEGACY_PENDING_QUESTIONS_DDL = """ +CREATE TABLE IF NOT EXISTS pending_questions ( + question_id TEXT PRIMARY KEY, + thread_id TEXT NOT NULL, + turn INTEGER NOT NULL, + status TEXT NOT NULL + CHECK (status IN ('open', 'answered', 'expired', 'superseded')), + transport TEXT NOT NULL, + channel_ref TEXT, + posted_at TEXT, + deadline_at TEXT, + answer_json TEXT, + answered_at TEXT, + answered_via TEXT +) +""".strip() + + +def _pq_columns(conn: sqlite3.Connection) -> list[str]: + return [r["name"] for r in conn.execute("PRAGMA table_info(pending_questions)")] + + +def test_schema_version_is_at_least_4() -> None: + assert SCHEMA_VERSION >= 4 + + +def test_init_db_pending_questions_has_kind_defaulting_clarify( + tmp_path: Path, +) -> None: + """A fresh init_db gives pending_questions a kind column defaulting clarify.""" + db = tmp_path / "db.sqlite" + init_db(db) + conn = connect(db) + try: + assert "kind" in _pq_columns(conn) + _insert_open_question(conn, "q-default") + kind = conn.execute( + "SELECT kind FROM pending_questions WHERE question_id='q-default'" + ).fetchone()["kind"] + finally: + conn.close() + assert kind == "clarify" + + +def test_init_db_kind_is_idempotent(tmp_path: Path) -> None: + """Running init_db twice does not error and kind exists exactly once.""" + db = tmp_path / "db.sqlite" + init_db(db) + init_db(db) # must not raise (no duplicate-column error) + conn = connect(db) + try: + cols = _pq_columns(conn) + finally: + conn.close() + assert cols.count("kind") == 1 + + +def test_migrate_adds_kind_to_legacy_db_rows_read_clarify(tmp_path: Path) -> None: + """A legacy pending_questions (no kind) gains the column; old rows read clarify.""" + db = tmp_path / "legacy.sqlite" + conn = connect(db) + try: + # Build the OLD table by hand and seed a row, with NO kind column. + conn.execute(_LEGACY_PENDING_QUESTIONS_DDL) + conn.execute( + "INSERT INTO pending_questions " + "(question_id, thread_id, turn, status, transport) " + "VALUES ('legacy', 't', 0, 'open', 'slack')" + ) + assert "kind" not in _pq_columns(conn) + + init_db(db) + + assert "kind" in _pq_columns(conn) + # The pre-existing row reads back as 'clarify' (NOT null). + kind = conn.execute( + "SELECT kind FROM pending_questions WHERE question_id='legacy'" + ).fetchone()["kind"] + finally: + conn.close() + assert kind == "clarify" + + +def test_migrate_helper_adds_kind_to_legacy_db(tmp_path: Path) -> None: + """migrate() (not just init_db) installs the v4 kind column on a legacy DB.""" + db = tmp_path / "legacy2.sqlite" + conn = connect(db) + try: + conn.execute(_LEGACY_PENDING_QUESTIONS_DDL) + conn.execute( + "CREATE TABLE IF NOT EXISTS schema_meta " + "(id INTEGER PRIMARY KEY CHECK (id = 1), schema_version INTEGER NOT NULL)" + ) + conn.execute("INSERT INTO schema_meta (id, schema_version) VALUES (1, 3)") + assert "kind" not in _pq_columns(conn) + + migrate(conn) + + assert "kind" in _pq_columns(conn) + ver = conn.execute( + "SELECT schema_version FROM schema_meta WHERE id = 1" + ).fetchone()[0] + finally: + conn.close() + assert ver == SCHEMA_VERSION + + +def test_kind_plan_decision_round_trips(tmp_path: Path) -> None: + """A row written with kind='plan_decision' round-trips; default is 'clarify'.""" + db = tmp_path / "db.sqlite" + init_db(db) + conn = connect(db) + try: + conn.execute( + "INSERT INTO pending_questions " + "(question_id, thread_id, turn, status, transport, kind) " + "VALUES ('pd', 't', 0, 'open', 'slack', 'plan_decision')" + ) + _insert_open_question(conn, "cl") # no kind -> default + pd_kind = conn.execute( + "SELECT kind FROM pending_questions WHERE question_id='pd'" + ).fetchone()["kind"] + cl_kind = conn.execute( + "SELECT kind FROM pending_questions WHERE question_id='cl'" + ).fetchone()["kind"] + finally: + conn.close() + assert pd_kind == "plan_decision" + assert cl_kind == "clarify" + + +def test_kind_check_rejects_unknown_value(tmp_path: Path) -> None: + """The CHECK constraint on a fresh DB rejects an out-of-range kind.""" + db = tmp_path / "db.sqlite" + init_db(db) + conn = connect(db) + try: + with pytest.raises(sqlite3.IntegrityError): + conn.execute( + "INSERT INTO pending_questions " + "(question_id, thread_id, turn, status, transport, kind) " + "VALUES ('bad', 't', 0, 'open', 'slack', 'bogus')" + ) + finally: + conn.close() From 67b0f4c6ae841fe665a587f9221940fd6d412ea0 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Tue, 23 Jun 2026 20:11:32 -0400 Subject: [PATCH 03/11] feat(agent-team): resumable plan-review gate in the graph (Phase B2a) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the terminal review-cap PARK with a resumable human decision gate, opt-in via build_graph(plan_gate=True) (default False → all existing P1/P2/P3 wiring unchanged). - plan_gate_node interrupt()s mirroring the clarifier contract (same payload keys → existing pending_question() extractor + turn-guarded ResumeWorker drive it with zero special-casing) plus a kind="plan_decision" discriminator and the plan + latest review findings as context. - Decision contract {"decision": approve|request_changes|abandon, "notes": ...}: approve → the same terminal state an auto-approved plan reaches (ACTIVE/BUILD); request_changes → append a synthetic human verdict to review_verdicts (so the planner's _format_review_feedback surfaces the notes) and loop back to PLAN; abandon / unrecognized → terminal FAILED (safe default, never accidental approve). - Bounded termination: MAX_PLAN_GATE_VISITS=3 combined ceiling on plan_gate_visits (new channel on PipelineState + TaskRecord); on exhaustion the gate goes terminal PARKED ("revision ceiling reached") WITHOUT interrupting. Proven by a loop-past-ceiling test. Notes for the coordinator wiring (B2b): while suspended at the gate the status channel still reads 'parked' (carried over from review_node's escalate branch) — the load-bearing "awaiting decision, not terminal" signal is the live pending interrupt + kind="plan_decision", NOT the status channel. Tests: interrupt-at-cap, approve/request_changes(notes)/abandon routing, ceiling-terminates, auto-approve still bypasses the gate. 1404 passed. --- agent-team/agent_team/graph.py | 276 +++++++++++++++++++++++++++- agent-team/agent_team/task_model.py | 16 ++ agent-team/tests/test_graph.py | 207 +++++++++++++++++++++ 3 files changed, 497 insertions(+), 2 deletions(-) diff --git a/agent-team/agent_team/graph.py b/agent-team/agent_team/graph.py index efb8d19..d176f8e 100644 --- a/agent-team/agent_team/graph.py +++ b/agent-team/agent_team/graph.py @@ -89,10 +89,16 @@ __all__ = [ "CLARIFY", "DEFAULT_CLARIFY_DEADLINE", "DISPATCH_NODE", + "GATE_APPROVE_ROUTE", + "GATE_REVISE_ROUTE", + "GATE_TERMINAL_ROUTE", "INTAKE", + "MAX_PLAN_GATE_VISITS", "P1_PHASE_SEQUENCE", "PARKED_ROUTE", "PLAN", + "PLAN_DECISION_KIND", + "PLAN_GATE", "REVIEW", "VERIFY_NODE", "build_checkpoint_serde", @@ -102,9 +108,11 @@ __all__ = [ "get_pipeline_state", "intake_node", "pending_question", + "plan_gate_node", "plan_node", "plan_phase", "resume_task", + "route_after_plan_gate", "start_task", "thread_config", ] @@ -125,6 +133,44 @@ REVIEW = "review" BUILD_ROUTE = "build" PARKED_ROUTE = "parked" +# Plan-review human-decision gate (Phase B2a). PLAN_GATE is the graph vertex the +# review-cap dead-end (the route the review loop returns as PARKED_ROUTE/ESCALATE) +# is repointed at when the gate is wired: instead of terminally parking, the task +# suspends on a resumable ``interrupt()`` so the owner can decide. The gate +# mirrors the clarifier's interrupt/resume contract exactly (same payload shape + +# the same stable question_id/turn helpers), so the existing pending_question(...) +# extractor and the turn-guarded ResumeWorker drive it uniformly. The route ids +# the gate's conditional-edge function returns are kept distinct from the vertex +# id so a route key never collides with a node name. +PLAN_GATE = "plan_gate" +GATE_APPROVE_ROUTE = "gate_approve" +GATE_REVISE_ROUTE = "gate_revise" +GATE_TERMINAL_ROUTE = "gate_terminal" + +# The interrupt-payload discriminator that tells the responder/ledger this is a +# plan-decision gate (vs the clarifier's question-set). The clarifier payload has +# no ``kind`` (legacy = "clarify"); the gate stamps this so a mixed-state ledger +# can tell the two apart. +PLAN_DECISION_KIND = "plan_decision" + +# Combined ceiling on plan-gate visits (Phase B2a, bounded termination). A human +# ``request_changes`` re-enters plan<->review, which can hit the review cap and +# gate AGAIN. Each gate visit increments ``plan_gate_visits``; once it reaches +# this constant the gate stops offering request_changes and the task goes +# terminal PARKED with a "revision ceiling reached" marker. This is what proves +# the human-in-the-loop revision cycle terminates: the only non-terminal gate +# decision (request_changes) strictly consumes one of a finite number of visits. +MAX_PLAN_GATE_VISITS = 3 + +# Disjoint namespace base for the gate's stable ``turn`` derivation. The +# clarifier numbers its turns 0..N from ``qa_history`` length; the gate numbers +# its turns from a high base offset by ``plan_gate_visits`` so a gate turn can +# never collide with a clarifier turn for the same thread (the turn guard in +# ResumeWorker matches a resume to the open interrupt by ``turn``). Only one +# interrupt is ever open per thread, but keeping the spaces disjoint makes the +# stable-question_id derivation unambiguous across the task's whole life. +_PLAN_GATE_TURN_BASE = 1_000_000 + # P3 (build -> verify subgraph) vertex ids. These are the GRAPH VERTEX names the # opt-in P3 subgraph hangs off the review loop's "build" route; they are kept # distinct from the route-id constants above (BUILD_ROUTE / PARKED_ROUTE) and @@ -287,6 +333,201 @@ def plan_node(state: PipelineState) -> PipelineState: ) +def _plan_gate_turn(visits: int) -> int: + """Return the stable gate ``turn`` for the ``visits``-th gate visit (B2a). + + Offset into a high, disjoint namespace (:data:`_PLAN_GATE_TURN_BASE`) so a + gate turn can never collide with a clarifier turn (``qa_history`` length) for + the same thread. Monotonic in ``visits`` so each successive gate suspend has + its own stable ``(thread_id, turn)`` identity (and thus its own question_id). + """ + return _PLAN_GATE_TURN_BASE + visits + + +def plan_gate_node(state: PipelineState) -> PipelineState: + """PLAN-GATE stage: the resumable human decision at the review-cap dead-end. + + Replaces the terminal PARKED escalation. When the plan<->review loop cannot + converge (the review loop returned the cap/ESCALATE route) — or a salvaged + partial plan is presented — this node suspends with ``interrupt()`` exactly + like the clarifier so the owner can decide. The interrupt payload mirrors the + clarifier's §3.3.1 shape (``thread_id``, ``question_id``, ``turn``, + ``transport``, ``deadline``, ``slack_thread_ts``) so the existing + :func:`pending_question` extractor and the turn-guarded + :class:`~agent_team.resume_worker.ResumeWorker` drive it with no special + casing, PLUS: + + * ``kind`` = :data:`PLAN_DECISION_KIND` — the discriminator that tells the + responder/ledger this is a plan-decision (not a clarifier question-set); + * ``plan`` / ``findings`` — the review context the owner decides over (the + current plan and the latest review verdict's findings). + + **Bounded termination.** Before suspending, the node bumps + ``plan_gate_visits``. If the ceiling (:data:`MAX_PLAN_GATE_VISITS`) is already + exhausted it does **not** interrupt: it returns terminal PARKED with a + "revision ceiling reached" marker. So every gate suspend strictly consumes + one of a finite number of visits and the human loop always terminates. + + On resume, ``interrupt()`` returns whatever was passed to + ``Command(resume=...)`` — the decision contract + ``{"decision": "approve"|"request_changes"|"abandon", "notes": str}``. The + node consumes it and writes the routing state (see + :func:`route_after_plan_gate`): + + * ``approve`` -> the same terminal "approved plan" state an auto-approved + plan reaches (phase ``BUILD``, status ``ACTIVE``); + * ``request_changes`` -> phase ``PLAN``, status ``ACTIVE`` (loop back), with + the human ``notes`` folded into ``review_verdicts`` as a synthetic + REQUEST_CHANGES verdict so the planner's ``_format_review_feedback`` reads + it on the re-plan; + * ``abandon`` (or any unrecognized decision) -> terminal ``FAILED``. + """ + prior_visits = int(state.get("plan_gate_visits", 0) or 0) + + # Ceiling guard FIRST: an exhausted budget means the gate must not offer + # another request_changes loop. Park terminally rather than suspend. + if prior_visits >= MAX_PLAN_GATE_VISITS: + return PipelineState( + status=TaskStatus.PARKED.value, + current_phase=_phase_value(Phase.PARKED), + failure_reason=( + "plan-gate revision ceiling reached " + f"({prior_visits}/{MAX_PLAN_GATE_VISITS} gate visits)" + ), + updated_at=_utc_now_iso(), + ) + + visits = prior_visits + 1 + thread_id = state.get("thread_id", "") + transport = state.get("transport", "") + slack_thread_ts = state.get("slack_thread_ts", "") + turn = _plan_gate_turn(visits) + question_id = _question_id_for(thread_id, turn) + deadline = (datetime.now(timezone.utc) + DEFAULT_CLARIFY_DEADLINE).isoformat() + + decision = interrupt( + { + "thread_id": thread_id, + "question_id": question_id, + "turn": turn, + "kind": PLAN_DECISION_KIND, + "transport": transport, + "deadline": deadline, + "slack_thread_ts": slack_thread_ts, + "plan": state.get("plan"), + "findings": _latest_review_findings(state), + } + ) + + return _apply_plan_decision(state, decision, visits=visits) + + +def _latest_review_findings(state: PipelineState) -> str: + """Return the latest review verdict's findings text (gate presentation).""" + verdicts = state.get("review_verdicts") or [] + if not verdicts: + return "" + last = verdicts[-1] + if isinstance(last, dict): + return str(last.get("findings") or last.get("notes") or "") + return str(last) + + +def _apply_plan_decision( + state: PipelineState, decision: Any, *, visits: int +) -> PipelineState: + """Consume the owner's resume decision and return the routing state (B2a). + + ``decision`` is the value passed to ``Command(resume=...)`` — the contract + ``{"decision": ..., "notes": ...}``. A bare string is tolerated as the + decision verb. The default for an unrecognized/empty decision is the SAFE + direction: terminal ``FAILED`` (never an accidental approve). + """ + verb, notes = _parse_decision(decision) + now = _utc_now_iso() + + if verb == "approve": + # Settle exactly as an auto-APPROVED plan does today (review_node's + # APPROVE branch: phase BUILD, status ACTIVE) so the terminus matches. + return PipelineState( + status=TaskStatus.ACTIVE.value, + current_phase=_phase_value(Phase.BUILD), + plan_gate_visits=visits, + updated_at=now, + ) + + if verb == "request_changes": + # Loop back to the planner. Fold the human notes into review_verdicts as + # a synthetic REQUEST_CHANGES verdict whose ``findings`` carry the notes, + # so the planner's _format_review_feedback surfaces them on the re-plan. + verdicts = list(state.get("review_verdicts") or []) + verdicts.append( + { + "verdict": "request_changes", + "outcome": "loop_back", + "findings": notes, + "reviewer": "human_plan_gate", + "created_at": now, + } + ) + return PipelineState( + status=TaskStatus.ACTIVE.value, + current_phase=_phase_value(Phase.PLAN), + review_verdicts=verdicts, + plan_gate_visits=visits, + updated_at=now, + ) + + # abandon (or any unrecognized decision) -> terminal FAILED. + return PipelineState( + status=TaskStatus.FAILED.value, + current_phase=_phase_value(Phase.PARKED), + plan_gate_visits=visits, + failure_reason=f"plan abandoned at human gate: {notes}".rstrip(": "), + updated_at=now, + ) + + +def _parse_decision(decision: Any) -> tuple[str, str]: + """Normalize a resume decision into ``(verb, notes)`` (B2a). + + Accepts the ``{"decision": ..., "notes": ...}`` mapping or a bare string. The + verb is lowercased/stripped and mapped to one of ``approve`` / + ``request_changes`` / ``abandon``; anything unrecognized maps to ``abandon`` + (the SAFE terminal direction — never an accidental approve). + """ + raw_decision: Any = "" + notes = "" + if isinstance(decision, dict): + raw_decision = decision.get("decision", "") + notes = str(decision.get("notes", "") or "") + else: + raw_decision = decision + verb = str(raw_decision or "").strip().lower() + if verb in {"approve", "request_changes", "abandon"}: + return verb, notes + return "abandon", notes + + +def route_after_plan_gate(state: PipelineState) -> str: + """LangGraph conditional-edge after the plan gate (B2a). + + Reads the routing state :func:`plan_gate_node` wrote on resume (or on the + ceiling-reached terminal park) and maps it to a route id: + + * status ACTIVE + phase BUILD -> :data:`GATE_APPROVE_ROUTE` (END/approved); + * status ACTIVE + phase PLAN -> :data:`GATE_REVISE_ROUTE` (loop to planner); + * anything else (FAILED, or PARKED ceiling) -> :data:`GATE_TERMINAL_ROUTE`. + """ + status = state.get("status") + phase = state.get("current_phase") + if status == TaskStatus.ACTIVE.value and phase == _phase_value(Phase.PLAN): + return GATE_REVISE_ROUTE + if status == TaskStatus.ACTIVE.value and phase == _phase_value(Phase.BUILD): + return GATE_APPROVE_ROUTE + return GATE_TERMINAL_ROUTE + + def _author_questions(state: PipelineState) -> list[str]: """Deterministic stand-in for the Claude clarifier's question authoring. @@ -389,6 +630,7 @@ def build_graph( ] | None = None, dispatch_node: Callable[[PipelineState], Any] | None = None, + plan_gate: bool = False, ) -> CompiledStateGraph: """Assemble + compile the P1 pipeline ``StateGraph`` (§3.3, §7.1). @@ -489,6 +731,13 @@ def build_graph( "subgraph." ) + if plan_gate and review_node is None: + raise ValueError( + "build_graph: plan_gate requires review_node — the gate is the " + "resumable replacement for the review loop's terminal PARKED route, " + "so there is no review-cap dead-end to repoint without a review loop." + ) + builder: StateGraph = StateGraph(PipelineState) builder.add_node(INTAKE, _instrument(INTAKE, intake_node, transition_recorder)) builder.add_node(CLARIFY, _instrument(CLARIFY, clarify, transition_recorder)) @@ -506,12 +755,35 @@ def build_graph( builder.add_node(REVIEW, _instrument(REVIEW, review_node, transition_recorder)) builder.add_edge(PLAN, REVIEW) + # The review loop's PARKED/ESCALATE route normally terminates at END. When + # the plan gate is wired it is REPOINTED at the PLAN_GATE vertex instead: + # the cap dead-end suspends on a resumable human decision rather than + # terminally parking. The gate's own conditional edges then route + # approve -> END, request_changes -> PLAN (loop back), terminal -> END. + if plan_gate: + parked_target = PLAN_GATE + builder.add_node( + PLAN_GATE, + _instrument(PLAN_GATE, plan_gate_node, transition_recorder), + ) + builder.add_conditional_edges( + PLAN_GATE, + route_after_plan_gate, + { + GATE_APPROVE_ROUTE: END, + GATE_REVISE_ROUTE: PLAN, + GATE_TERMINAL_ROUTE: END, + }, + ) + else: + parked_target = END + if build_verify is None: # P2: the review's "build" route is the approved-plan terminus. builder.add_conditional_edges( REVIEW, route_review, - {BUILD_ROUTE: END, PLAN: PLAN, PARKED_ROUTE: END}, + {BUILD_ROUTE: END, PLAN: PLAN, PARKED_ROUTE: parked_target}, ) else: # P3 (opt-in): repoint the review's "build" route at the BUILD node, @@ -530,7 +802,7 @@ def build_graph( builder.add_conditional_edges( REVIEW, route_review, - {BUILD_ROUTE: BUILD_NODE, PLAN: PLAN, PARKED_ROUTE: END}, + {BUILD_ROUTE: BUILD_NODE, PLAN: PLAN, PARKED_ROUTE: parked_target}, ) # Linear order is BUILD -> DISPATCH -> VERIFY so DISPATCH triggers CI # and captures ``state["run_id"]`` BEFORE VERIFY reads it (design §4 diff --git a/agent-team/agent_team/task_model.py b/agent-team/agent_team/task_model.py index 5391f95..a40810f 100644 --- a/agent-team/agent_team/task_model.py +++ b/agent-team/agent_team/task_model.py @@ -114,6 +114,15 @@ class TaskRecord: # parks once it reaches VerifierConfig.max_build_loops so a perpetually- # failing task can never loop BUILD->DISPATCH->VERIFY forever (LOGIC-RACE-01). build_loops: int = 0 + # Count of plan-review human-decision gate visits already consumed for THIS + # task (Phase B2a). The graph routes a review-cap dead-end to the plan gate, + # which interrupts for an owner approve / request_changes / abandon decision. + # A human ``request_changes`` re-enters plan<->review, which can hit the cap + # and gate AGAIN; this counter is the combined ceiling on human-driven gate + # loops (mirrors PipelineState.plan_gate_visits) so the loop always + # terminates: once it reaches ``MAX_PLAN_GATE_VISITS`` the gate stops + # offering request_changes and the task goes terminal PARKED. + plan_gate_visits: int = 0 transport: str = "" created_at: str | None = None updated_at: str | None = None @@ -162,6 +171,12 @@ class PipelineState(TypedDict, total=False): # budget is real (LOGIC-RACE-01: it was previously read from the shared # wiring-time config and never advanced). build_loops: int + # Plan-review human-decision gate visits consumed for THIS task (mirrors + # TaskRecord.plan_gate_visits; Phase B2a). The combined ceiling on + # human-driven plan<->review loops: once it reaches MAX_PLAN_GATE_VISITS the + # gate stops offering request_changes and the task goes terminal PARKED, so + # the human-in-the-loop revision cycle can never spin forever. + plan_gate_visits: int transport: str created_at: str | None updated_at: str | None @@ -198,6 +213,7 @@ def task_from_dict(data: dict[str, Any]) -> TaskRecord: ci_correlation_tag=data.get("ci_correlation_tag"), dispatched_at=data.get("dispatched_at"), build_loops=data.get("build_loops", 0), + plan_gate_visits=data.get("plan_gate_visits", 0), transport=data.get("transport", ""), created_at=data.get("created_at"), updated_at=data.get("updated_at"), diff --git a/agent-team/tests/test_graph.py b/agent-team/tests/test_graph.py index afb8a15..52fa8ae 100644 --- a/agent-team/tests/test_graph.py +++ b/agent-team/tests/test_graph.py @@ -501,6 +501,213 @@ def test_p2_graph_loops_then_escalates_on_persistent_changes( assert len(final["review_verdicts"]) == 3 # looped to the cap, then escalated +# --- Plan-review human decision gate (Phase B2a). --------------------------- + + +def _plan_gate_graph(review_text: str): + """Compile a P2 graph with the plan gate wired (plan_gate=True). + + A reviewer that never approves drives the plan<->review loop to the review + cap, which — with the gate wired — suspends on the resumable PLAN_GATE + interrupt instead of terminally parking. + """ + from agent_team.nodes import review_loop + + review_loop.set_review_invoker(lambda prompt, **kw: review_text) + return build_graph( + checkpointer=_Saver(), + live_plan_node=_p2_plan_stub, + review_node=review_loop.bind_review_node(), + route_review=review_loop.route_after_review, + plan_gate=True, + ) + + +def _drive_to_plan_gate(graph): + """Start a task and resume the clarifier so it lands on the plan gate. + + Returns ``(thread_id, gate_payload)`` where ``gate_payload`` is the pending + plan-decision interrupt payload. + """ + from agent_team.graph import pending_question + + thread_id, _ = start_task(graph, transport="slack", slack_thread_ts="ROOT.1") + resume_task(graph, thread_id=thread_id, answer="scope is X") + payload = pending_question(graph, thread_id=thread_id) + return thread_id, payload + + +def test_plan_gate_requires_review_node() -> None: + with pytest.raises(ValueError, match="plan_gate requires review_node"): + build_graph(plan_gate=True) + + +def test_review_cap_interrupts_at_plan_gate(restore_review_invoker) -> None: + # Driving to the review-cap dead-end suspends on the plan gate (a pending + # plan_decision interrupt with the plan + findings) rather than parking. + from agent_team.graph import PLAN_DECISION_KIND + + graph = _plan_gate_graph("VERDICT: REQUEST CHANGES\nstill not ready") + thread_id, payload = _drive_to_plan_gate(graph) + + assert payload is not None + assert payload["kind"] == PLAN_DECISION_KIND + # Mirrors the clarifier contract: thread_id / question_id / turn / transport / + # deadline / slack_thread_ts all present (so pending_question + ResumeWorker + # drive it uniformly). + assert payload["thread_id"] == thread_id + assert payload["question_id"] + assert isinstance(payload["turn"], int) + assert payload["transport"] == "slack" + assert payload["deadline"] + assert payload["slack_thread_ts"] == "ROOT.1" + # Plus the review context the owner decides over. + assert payload["plan"] is not None + assert "still not ready" in payload["findings"] + + # It did NOT terminally park: the task is suspended on the gate interrupt + # (resumable), not finished. (The status channel still reads the review + # node's carried-over "parked" until the gate's resume overwrites it; the + # load-bearing signal is the live pending interrupt.) + from agent_team.graph import pending_question + + assert pending_question(graph, thread_id=thread_id) is not None + snapshot = graph.get_state(thread_config(thread_id)) + assert snapshot.next # graph is suspended, not at a terminal END + + +def test_plan_gate_approve_settles_as_approved_plan(restore_review_invoker) -> None: + # Command(resume={"decision":"approve"}) -> the same terminal "approved plan" + # state an auto-approved plan reaches today (phase BUILD, status ACTIVE). + graph = _plan_gate_graph("VERDICT: REQUEST CHANGES\nnope") + thread_id, _ = _drive_to_plan_gate(graph) + final = resume_task(graph, thread_id=thread_id, answer={"decision": "approve"}) + + assert final["current_phase"] == Phase.BUILD.value + assert final["status"] == TaskStatus.ACTIVE.value + + +def test_plan_gate_request_changes_loops_back_with_notes( + restore_review_invoker, +) -> None: + # Command(resume={"decision":"request_changes","notes":"do X"}) loops back to + # the planner; the notes must reach _format_review_feedback / the planner. + from agent_team.nodes.planner import _format_review_feedback + + captured: dict[str, object] = {} + + def capturing_plan(state): + captured["feedback"] = _format_review_feedback( + list(state.get("review_verdicts") or []) + ) + # After observing the folded-in notes, APPROVE on the re-plan so the + # graph settles (the review invoker is bound per-test below). + return _p2_plan_stub(state) + + from agent_team.nodes import review_loop + + # First review round REQUEST CHANGES (to reach the gate); after the human + # request_changes loops back, the next review APPROVES so the task settles. + texts = iter( + [ + "VERDICT: REQUEST CHANGES\nnot ready", + "VERDICT: APPROVE\nnow good", + ] + ) + last = "VERDICT: APPROVE\nnow good" + + def invoker(prompt, **kw): + nonlocal last + try: + last = next(texts) + except StopIteration: + pass + return last + + review_loop.set_review_invoker(invoker) + graph = build_graph( + checkpointer=_Saver(), + live_plan_node=capturing_plan, + review_node=review_loop.bind_review_node({"max_review_rounds": 1}), + route_review=review_loop.route_after_review, + plan_gate=True, + ) + + thread_id, _ = start_task(graph, transport="slack") + resume_task(graph, thread_id=thread_id, answer="scope is X") + final = resume_task( + graph, + thread_id=thread_id, + answer={"decision": "request_changes", "notes": "do X"}, + ) + + # The human notes reached the planner's review-feedback formatter on re-plan. + assert "do X" in str(captured.get("feedback", "")) + # And the loop re-entered plan -> review and settled (not stuck at the gate). + assert final["current_phase"] == Phase.BUILD.value + + +def test_plan_gate_abandon_fails(restore_review_invoker) -> None: + # Command(resume={"decision":"abandon"}) -> terminal FAILED. + graph = _plan_gate_graph("VERDICT: REQUEST CHANGES\nnope") + thread_id, _ = _drive_to_plan_gate(graph) + final = resume_task(graph, thread_id=thread_id, answer={"decision": "abandon"}) + + assert final["status"] == TaskStatus.FAILED.value + + +def test_plan_gate_unrecognized_decision_fails_safe(restore_review_invoker) -> None: + # An unrecognized decision must NOT accidentally approve — it fails terminal. + graph = _plan_gate_graph("VERDICT: REQUEST CHANGES\nnope") + thread_id, _ = _drive_to_plan_gate(graph) + final = resume_task(graph, thread_id=thread_id, answer={"decision": "huh?"}) + + assert final["status"] == TaskStatus.FAILED.value + + +def test_plan_gate_request_changes_terminates_at_ceiling( + restore_review_invoker, +) -> None: + # Repeated request_changes resumes must eventually hit the gate ceiling and + # go terminal PARKED ("ceiling reached") — NOT loop unbounded. The reviewer + # NEVER approves, so every gate visit is request_changes until the ceiling. + from agent_team.graph import MAX_PLAN_GATE_VISITS, pending_question + + graph = _plan_gate_graph("VERDICT: REQUEST CHANGES\nstill not ready") + thread_id, _ = _drive_to_plan_gate(graph) + + # Each request_changes consumes exactly one gate visit; bound the loop well + # above the ceiling to prove it terminates on its own, not by our cap. + for _ in range(MAX_PLAN_GATE_VISITS + 5): + if pending_question(graph, thread_id=thread_id) is None: + break + resume_task( + graph, + thread_id=thread_id, + answer={"decision": "request_changes", "notes": "again"}, + ) + + state = get_pipeline_state(graph, thread_id=thread_id) + assert pending_question(graph, thread_id=thread_id) is None + assert state["status"] == TaskStatus.PARKED.value + assert "ceiling" in (state.get("failure_reason") or "") + assert state.get("plan_gate_visits") == MAX_PLAN_GATE_VISITS + + +def test_auto_approved_plan_does_not_hit_gate(restore_review_invoker) -> None: + # No regression: an auto-APPROVED plan settles WITHOUT visiting the gate even + # when the gate is wired (gate only catches the review-cap dead-end). + from agent_team.graph import pending_question + + graph = _plan_gate_graph("VERDICT: APPROVE\nlooks solid") + thread_id, _ = start_task(graph, transport="slack") + final = resume_task(graph, thread_id=thread_id, answer="scope is X") + + assert final["current_phase"] == Phase.BUILD.value + assert pending_question(graph, thread_id=thread_id) is None + assert final.get("plan_gate_visits", 0) == 0 + + # --- P3 build -> verify subgraph wiring (opt-in). --------------------------- From ba4fe68fdb10778c992d61ac88df79c92a5be896 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Tue, 23 Jun 2026 20:19:51 -0400 Subject: [PATCH 04/11] feat(agent-team): wire the plan-review gate into the coordinator (Phase B2b) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Connect the graph plan-gate (B2a) to the durable ledger + Slack presentation. - setup() passes build_graph(plan_gate=True) only on the wired review path (plan_gate flag ANDed with review_node present); P1/stub paths force it off. - _post_resume_followups detects a settled interrupt by payload kind == PLAN_DECISION_KIND (NOT status, which still reads 'parked' at the gate per B2a) and posts the decision gate: opens a pending_questions row with kind='plan_decision' (24h deadline, threaded, channel_ref = root ts) and presents _summarize_plan + findings + reply instructions, truncated to a ~2700-char Slack budget. A clarify/legacy interrupt keeps the existing path. - Single-open-gate invariant: the opener skips if any open row already exists for the thread (one row, one presentation). - Expiry: _park posts a plan-decision-specific recovery notice (re-assign / force-resume) for an expired gate row. - Resume path unchanged: the decision answer flows through submit_answer → ResumeWorker → plan_gate_node with no resume-worker special-casing. notify_question has no kind param in this tree, so the opener calls ledger.post_question(kind=...) + ledger.set_channel_ref directly; the clarifier path still uses notify_question unchanged. Tests: gate row+presentation+threading, approve/request_changes/abandon via submit_answer, single-open-gate skip, expiry notice. 1412 passed. --- agent-team/agent_team/coordinator.py | 354 ++++++++++++++++++++++++++- agent-team/tests/test_coordinator.py | 295 ++++++++++++++++++++++ 2 files changed, 645 insertions(+), 4 deletions(-) diff --git a/agent-team/agent_team/coordinator.py b/agent-team/agent_team/coordinator.py index 9bea15b..c9424a7 100644 --- a/agent-team/agent_team/coordinator.py +++ b/agent-team/agent_team/coordinator.py @@ -67,6 +67,7 @@ from agent_team.transport.slack_adapter import SlackTransport if TYPE_CHECKING: # pragma: no cover - typing only from agent_team.task_model import PipelineState + from agent_team.transport.base import QuestionSet __all__ = [ "Coordinator", @@ -149,6 +150,26 @@ AlarmHook = Callable[[str], None] ListenerFactory = Callable[[], Any] +def _open_question_id_for_thread(conn: Any, thread_id: str) -> str | None: + """Return the ``question_id`` of an open ledger row for ``thread_id``, or None. + + The single-open-gate invariant (B-2) holds that a thread has at most one open + ``pending_questions`` row at a time. This is the read side of that guard: a + non-None result means an open gate already exists for the thread, so a second + gate (clarifier follow-up OR plan-decision) must NOT be opened. Returns the + first open row's id (there should never be more than one) or ``None``. + """ + if not thread_id: + return None + row = conn.execute( + "SELECT question_id FROM pending_questions " + "WHERE thread_id=? AND status='open' " + "ORDER BY posted_at ASC, question_id ASC LIMIT 1", + (thread_id,), + ).fetchone() + return None if row is None else str(row["question_id"]) + + def default_slack_listener_factory( *, transport: SlackTransport, @@ -582,6 +603,7 @@ class Coordinator: ci_poller: "Callable[[Any], Any] | None" = None, ci_timeout: timedelta | None = None, draft_pr_provider: "Callable[[], list[Any]] | None" = None, + plan_gate: bool = True, ) -> None: self._db_path = Path(db_path) self._transport = transport @@ -639,6 +661,16 @@ class Coordinator: # re-ALARMed / re-reminded every tick. self._draft_pr_provider = draft_pr_provider self._draft_pr_memory: Any = None + # Plan-review human decision gate (Phase B2b). When True AND a review loop + # is wired (``review_wiring`` is not None), ``setup`` builds the graph with + # ``plan_gate=True`` so the review-cap dead-end suspends on a resumable + # human decision instead of terminally parking. When the review loop is + # NOT wired (P1-only / stub paths, e.g. the unit suite's default + # coordinator) the gate cannot exist — the graph would raise — so setup + # forces it off regardless of this flag. Defaulting to True makes the live + # serve/P2 path gate-enabled out of the box; tests build both shapes by + # toggling this with/without ``review_wiring``. + self._plan_gate = plan_gate # Built by setup(). self._graph: Any = None @@ -744,6 +776,14 @@ class Coordinator: # transition_recorder=None (no instrumentation). transition_recorder = TransitionRecorder(self._db_path) + # Plan-review human decision gate (B2b): only enable it on the wired + # review path. ``build_graph`` raises if ``plan_gate`` is set without a + # ``review_node`` (the gate IS the review-cap dead-end's replacement), so + # gate off whenever the review loop is absent (P1-only / stub paths) even + # if the ctor flag asked for it. On the live serve/P2 path the review + # wiring is injected, so the gate turns on. + plan_gate = self._plan_gate and review_node is not None + self._graph = graph_mod.build_graph( checkpointer, transition_recorder=transition_recorder, @@ -753,6 +793,7 @@ class Coordinator: route_review=route_review, build_verify=build_verify, dispatch_node=dispatch_node_callable, + plan_gate=plan_gate, ) # The ResumeWorker is satisfied directly by the compiled LangGraph app @@ -1067,10 +1108,25 @@ class Coordinator: question = None if question is not None: - # Multi-turn: a new clarifier question is waiting. Post it to the - # transport (the drain path otherwise leaves it unposted) and tell - # the human more input is needed. Thread it (and its channel_ref) - # under the task's root message so the next reply maps back. + # A human gate is open. Two kinds (B2b): the plan-review DECISION + # gate (kind == 'plan_decision') and the clarifier QUESTION gate + # (kind == 'clarify' / legacy no-kind). The load-bearing signal is + # the pending-interrupt kind — NOT the task status, which still + # reads the review node's carried-over 'parked' while suspended at + # the gate (B2a). Branch on the kind so the plan gate posts its + # plan + findings presentation while the clarifier keeps its + # existing follow-up path. + if question.get("kind") == graph_mod.PLAN_DECISION_KIND: + self._post_plan_decision_gate( + question, label=label, root_ts=root_ts, short=short + ) + continue + + # Multi-turn clarify: a new clarifier question is waiting. Post it + # to the transport (the drain path otherwise leaves it unposted) + # and tell the human more input is needed. Thread it (and its + # channel_ref) under the task's root message so the next reply maps + # back. try: conn = connect(self._db_path) try: @@ -1160,6 +1216,234 @@ class Coordinator: thread_ts=root_ts, ) + # ------------------------------------------------------------------ # + # Plan-review decision gate (B2b). + # ------------------------------------------------------------------ # + + # Slack section-block text caps at ~3000 chars; the gate presentation budgets + # a bit under that for safety once the decision instructions are appended. + _GATE_PRESENTATION_BUDGET = 2700 + _GATE_TRUNCATION_NOTE = "\n…(truncated — full plan on the dashboard)" + + def _post_plan_decision_gate( + self, + question: "dict[str, Any]", + *, + label: str, + root_ts: str | None, + short: str, + ) -> None: + """Open + present the plan-review decision gate for a suspended task (B2b). + + The graph has suspended on the resumable PLAN_GATE interrupt (the + review-cap dead-end, B2a) whose payload mirrors the clarifier's PLUS + ``kind == 'plan_decision'``, ``plan``, and ``findings``. This: + + 1. Writes the durable ``pending_questions`` row with ``kind='plan_decision'`` + (24h deadline like the clarifier), threaded under the task root — via + :meth:`_open_plan_decision_row_if_absent` so the single-open-gate + invariant (B-2) holds (the clarifier row is already answered before the + plan stage runs, so a second open row would be a bug; we log + skip it). + 2. POSTs a presentation message — the plan (:meth:`_summarize_plan`) + the + review findings (:meth:`_summarize_blocker`) + the decision + instructions — threaded under the task root, truncated to Slack's block + limit. + + The decision answer then flows back through the UNCHANGED + ``submit_answer`` → resume-queue → ResumeWorker path (the graph's + ``plan_gate_node`` consumes the resume value); nothing here special-cases + the resume worker. Best-effort + fully guarded: a gate-post failure leaves + the durable interrupt in place (recovery re-derives it) and never breaks + the tick loop. + """ + deadline = question.get("deadline") or self._default_deadline() + thread_id = str(question.get("thread_id") or "") + question_id = str(question.get("question_id") or "") + turn = int(question.get("turn") or 0) + transport_name = str(question.get("transport") or "") + + # 1. Durable ledger row first (open, kind='plan_decision'), guarded by the + # single-open-gate invariant. + try: + conn = connect(self._db_path) + try: + opened = self._open_plan_decision_row_if_absent( + conn, + thread_id=thread_id, + question_id=question_id, + turn=turn, + transport_name=transport_name, + deadline=deadline, + root_ts=root_ts, + ) + finally: + conn.close() + except Exception: # noqa: BLE001 - a ledger error must not break the tick + _LOG.warning( + "failed to open plan-decision gate row for %s", short, exc_info=True + ) + opened = False + + if not opened: + # An open row already exists for this thread (single-open-gate + # invariant): do not present a second gate. Already logged in the + # opener; just stop here. + return + + # 2. Present the plan + findings + decision instructions, threaded under + # the task root. + body = self._plan_decision_presentation(question, label=label) + self._emit(body, thread_ts=root_ts) + + def _plan_decision_presentation( + self, question: "dict[str, Any]", *, label: str + ) -> str: + """Render the plan-gate presentation (plan + findings + instructions, B2b). + + Reuses :meth:`_summarize_plan` (plan shape) and :meth:`_summarize_blocker` + (review findings) so the gate view matches the rest of the lifecycle + threading, then appends the explicit decision instructions. The combined + body is truncated to Slack's section-block limit + (:data:`_GATE_PRESENTATION_BUDGET`) with a "(truncated — full plan on the + dashboard)" note so a multi-KB plan never overruns the block. + """ + plan_view = self._summarize_plan({"plan": question.get("plan")}) + findings = " ".join(str(question.get("findings") or "").split()) + if not findings: + findings = "(no review findings recorded)" + elif len(findings) > 600: + findings = findings[:600] + "…" + + instructions = ( + "• Decide: reply *approve* / *request changes * / *abandon* " + "in this thread." + ) + header = f"🧭 {label} — plan needs your decision (review could not approve it)." + + body = "\n".join( + [ + header, + plan_view, + f"• Review findings: {findings}", + instructions, + ] + ) + return self._truncate_for_slack(body) + + @classmethod + def _truncate_for_slack(cls, body: str) -> str: + """Truncate ``body`` to the gate presentation budget with a marker (B2b).""" + if len(body) <= cls._GATE_PRESENTATION_BUDGET: + return body + keep = cls._GATE_PRESENTATION_BUDGET - len(cls._GATE_TRUNCATION_NOTE) + return body[: max(keep, 0)].rstrip() + cls._GATE_TRUNCATION_NOTE + + def _open_plan_decision_row_if_absent( + self, + conn: Any, + *, + thread_id: str, + question_id: str, + turn: int, + transport_name: str, + deadline: str, + root_ts: str | None, + ) -> bool: + """Open a ``kind='plan_decision'`` ledger row, enforcing one-open-gate (B-2). + + The single-open-gate invariant: a thread has AT MOST ONE open + ``pending_questions`` row at a time. The clarifier row is already answered + before the plan stage runs, so under normal operation no open row exists + here. If one somehow does (a bug, or a redelivered drain re-presenting the + same gate), we do NOT open a second — we log + skip and return ``False`` + so the caller does not re-present. Returns ``True`` iff a fresh row was + opened + posted. + + The row is written ``open`` first (durable before the post), then the + transport posts the gate presentation threaded under ``root_ts`` and the + row's ``channel_ref`` is set to the root ts (so an inbound reply's + ``thread_ts`` maps back), mirroring :func:`responder.notify_question`. + """ + existing = _open_question_id_for_thread(conn, thread_id) + if existing is not None: + _LOG.warning( + "single-open-gate invariant: thread %s already has open question " + "%s; not opening a second (plan_decision) gate row", + thread_id[:8], + existing, + ) + return False + + # Durable row first (open, no ref), kind='plan_decision'. + from agent_team import ledger as ledger_mod # noqa: PLC0415 + + ledger_mod.post_question( + conn, + question_id=question_id, + thread_id=thread_id, + turn=turn, + transport=transport_name or type(self._transport).__name__, + deadline_at=deadline, + kind=graph_mod.PLAN_DECISION_KIND, + ) + + # Side-effecting post: thread the gate presentation under the root and + # record the channel_ref. A post failure is recoverable (row stays open, + # no ref) — swallow it exactly like notify_question. + channel_ref: str | None = None + try: + post_kwargs: dict[str, Any] = {} + if root_ts: + post_kwargs["thread_ts"] = root_ts + posted_ref = self._transport.post_question( + thread_id=thread_id, + question_id=question_id, + turn=turn, + question_set=self._plan_decision_question_set( + thread_id=thread_id, question_id=question_id, turn=turn + ), + deadline=deadline, + **post_kwargs, + ) + channel_ref = root_ts if root_ts else posted_ref + except Exception: # noqa: BLE001 - lost post is recoverable; keep the row + _LOG.warning( + "plan-decision gate post failed for thread %s; row stays open " + "for redelivery", + thread_id[:8], + exc_info=True, + ) + return True + + if channel_ref: + ledger_mod.set_channel_ref( + conn, question_id=question_id, channel_ref=channel_ref + ) + return True + + @staticmethod + def _plan_decision_question_set( + *, thread_id: str, question_id: str, turn: int + ) -> "QuestionSet": + """Build a minimal QuestionSet for the gate post (answer-mapping only). + + The transport's ``post_question`` requires a ``question_set`` so the + inbound answer can map back to ``question_id``; the gate's human-readable + presentation is posted via the lifecycle ``_emit`` sink, so this set only + needs to carry the identity (the decision-instruction wording lives in the + presentation message). The single question text is a terse decision + prompt for transports that render the set directly. + """ + from agent_team.transport.base import QuestionSet # noqa: PLC0415 + + return QuestionSet( + thread_id=thread_id, + question_id=question_id, + turn=turn, + questions=["Approve, request changes, or abandon this plan?"], + context={"kind": graph_mod.PLAN_DECISION_KIND}, + ) + @staticmethod def _summarize_plan(values: "dict[str, Any]") -> str: """Condensed, Slack-friendly view of the approved plan (summary + phases). @@ -1603,8 +1887,70 @@ class Coordinator: parked state for P1); this raises the injected ALARM hook so the stall is surfaced rather than silently spun on. Kept separate so the park policy is one obvious, testable place. + + **Plan-decision expiry (B2b).** A ``kind='plan_decision'`` gate row is an + ordinary ``pending_questions`` row, so the same deadline sweep expires it. + When the expired row is a plan-decision gate we ALSO emit a lifecycle + notice naming the task + that the plan DECISION expired unanswered + the + operator recovery path (re-assign / force-resume), threaded under the + task root, so an unanswered gate reads sensibly rather than as a generic + "clarifier question expired". Best-effort + guarded — never breaks tick. """ self._alarm_hook(question_id) + self._maybe_notify_plan_decision_expiry(question_id) + + def _maybe_notify_plan_decision_expiry(self, question_id: str) -> None: + """Emit a Slack lifecycle notice when a plan-decision gate row expires (B2b). + + Looks up the just-expired row; if it is a ``plan_decision`` gate it posts + a notice naming the task (read from the live graph state) + the recovery + path, threaded under the task root. A clarifier expiry is left to the + existing ALARM path (no extra notice). Fully guarded: any read/post error + is logged and swallowed so a notice failure never breaks the deadline + sweep. + """ + try: + conn = connect(self._db_path) + try: + from agent_team import ledger as ledger_mod # noqa: PLC0415 + + row = ledger_mod.get_question(conn, question_id) + finally: + conn.close() + except Exception: # noqa: BLE001 - a read error must not break the sweep + _LOG.warning( + "could not read expired question %s for plan-decision notice", + question_id, + exc_info=True, + ) + return + + if row is None or row.kind != graph_mod.PLAN_DECISION_KIND: + return + + # Name the task + thread the notice under its root, reading the live state. + label = f"(`{row.thread_id[:8]}`)" + root_ts: str | None = None + try: + snap = self._graph.get_state(graph_mod.thread_config(row.thread_id)) + values = getattr(snap, "values", {}) or {} + desc = str(values.get("task") or "").strip() + if desc: + if len(desc) > 90: + desc = desc[:90] + "…" + label = f'"{desc}" ({label})' + root_ts = str(values.get("slack_thread_ts") or "") or None + except Exception: # noqa: BLE001 - fall back to the bare id label + pass + + self._emit( + f"⌛ PLAN DECISION EXPIRED — {label}\n" + "• The plan-review decision was not answered within 24h, so the task " + "is parked.\n" + "• Recovery: re-assign the task, or force-resume it with a decision " + "(approve / request changes / abandon).", + thread_ts=root_ts, + ) @staticmethod def _default_alarm_hook(question_id: str) -> None: diff --git a/agent-team/tests/test_coordinator.py b/agent-team/tests/test_coordinator.py index 375036e..07b7d2a 100644 --- a/agent-team/tests/test_coordinator.py +++ b/agent-team/tests/test_coordinator.py @@ -1659,3 +1659,298 @@ def test_tick_ci_watch_parks_fail_closed_on_error(db_path: Path) -> None: assert report is not None assert report.parked_error == 1 assert alarms == ["t-err"] + + +# --------------------------------------------------------------------------- # +# Plan-review decision gate (Phase B2b) — coordinator wiring +# --------------------------------------------------------------------------- # + + +def _p2_plan_stub(state: Any) -> Any: + """Plan node that emits a plan and advances to REVIEW (no model call).""" + from agent_team.task_model import Phase, PipelineState, TaskStatus + + revisions = len(state.get("review_verdicts") or []) + return PipelineState( + plan={ + "summary": "ship the thing", + "phases": [{"name": "do it"}], + "revision": revisions, + }, + current_phase=Phase.REVIEW.value, + status=TaskStatus.ACTIVE.value, + ) + + +def _make_gate_coordinator( + db_path: Path, + *, + review_text: str = "VERDICT: REQUEST CHANGES\nstill not ready", + review_invoker: Any = None, + review_rounds: int | None = None, + transport: Transport | None = None, + notify: Any = None, + plan_node: Any = None, +) -> Coordinator: + """Build a gate-enabled Coordinator: stub clarify + real review loop + plan_gate. + + The review invoker NEVER approves (by default), so the plan<->review loop hits + the review cap and — with ``plan_gate=True`` — suspends on the resumable + PLAN_GATE interrupt instead of terminally parking. The clarify node stays the + deterministic stub (no Claude). + """ + from agent_team.nodes import review_loop + + saver = _Saver() + invoker = review_invoker or (lambda prompt, **kw: review_text) + review_loop.set_review_invoker(invoker) + + def _review_wiring() -> Any: + kw = {"max_review_rounds": review_rounds} if review_rounds is not None else {} + return review_loop.bind_review_node(kw), review_loop.route_after_review + + return Coordinator( + db_path=db_path, + transport=transport or FakeTransport(), + build_clarify_node=lambda: graph_mod.clarify_node, + build_plan_node=lambda: plan_node or _p2_plan_stub, + review_wiring=_review_wiring, + build_checkpointer=lambda _path: saver, + notify=notify, + plan_gate=True, + ) + + +@pytest.fixture() +def _restore_review_invoker(): + from agent_team.nodes import review_loop + + saved = review_loop._review_invoker + yield + review_loop._review_invoker = saved + + +def _drive_to_gate(coord: Coordinator, *, root_ts: str = "ROOT.TS") -> str: + """Start a task, answer the clarifier, and drain so it lands at the plan gate.""" + thread_id = coord.start_task( + task_text="add a thing", transport_name="slack", slack_thread_ts=root_ts + ) + qid = _only_open_row(db_path_of(coord))["question_id"] + coord.submit_answer({"question_id": qid, "answer": "scope is X", "via": "v"}) + results = coord.drain_resumes() + coord._post_resume_followups(results) + return thread_id + + +def db_path_of(coord: Coordinator) -> Path: + return coord._db_path + + +def test_setup_with_plan_gate_off_when_no_review_wiring(db_path: Path) -> None: + # A P1-only coordinator (no review wiring) must build cleanly even though the + # ctor flag defaults plan_gate True: setup forces it off (no review_node). + coord = _make_coordinator(db_path) # plan_gate defaults True, no review_wiring + coord.setup() + assert coord.graph is not None + + +def test_gate_writes_plan_decision_row_and_presents_plan( + db_path: Path, _restore_review_invoker: Any +) -> None: + posted: list[tuple[str, str | None]] = [] + coord = _make_gate_coordinator( + db_path, + notify=lambda message, thread_ts=None: posted.append((message, thread_ts)), + ) + coord.setup() + _drive_to_gate(coord, root_ts="ROOT.TS") + + # A durable open row with kind='plan_decision' exists. + row = _only_open_row(db_path) + assert row["kind"] == "plan_decision" + + # A presentation was posted, threaded under the task root, containing the + # plan summary + the review findings + the decision instructions. + gate = [p for p in posted if "plan needs your decision" in p[0]] + assert len(gate) == 1 + message, thread_ts = gate[0] + assert thread_ts == "ROOT.TS" + assert "ship the thing" in message # the plan summary is presented + assert "still not ready" in message # the review findings are presented + assert "approve" in message and "abandon" in message + + +def test_gate_approve_settles_task_not_interrupted( + db_path: Path, _restore_review_invoker: Any +) -> None: + coord = _make_gate_coordinator(db_path) + coord.setup() + thread_id = _drive_to_gate(coord) + + # Approve the gate via the SAME submit_answer -> resume path (unchanged). + qid = _only_open_row(db_path)["question_id"] + coord.submit_answer( + {"question_id": qid, "answer": {"decision": "approve"}, "via": "v"} + ) + coord.drain_resumes() + + # The task settled approved (phase BUILD, status ACTIVE) and is no longer + # interrupted. + assert graph_mod.pending_question(coord.graph, thread_id=thread_id) is None + state = graph_mod.get_pipeline_state(coord.graph, thread_id=thread_id) + assert state["current_phase"] == "build" + assert state["status"] == "active" + + +def test_gate_request_changes_reenters_pipeline( + db_path: Path, _restore_review_invoker: Any +) -> None: + # First review round REQUEST CHANGES (reach gate); after request_changes the + # next review APPROVES so the loop re-enters plan->review and settles. + texts = iter(["VERDICT: REQUEST CHANGES\nnope", "VERDICT: APPROVE\nnow good"]) + last = {"v": "VERDICT: APPROVE\nnow good"} + + def invoker(prompt: Any, **kw: Any) -> str: + try: + last["v"] = next(texts) + except StopIteration: + pass + return last["v"] + + coord = _make_gate_coordinator(db_path, review_invoker=invoker, review_rounds=1) + coord.setup() + thread_id = _drive_to_gate(coord) + + qid = _only_open_row(db_path)["question_id"] + coord.submit_answer( + { + "question_id": qid, + "answer": {"decision": "request_changes", "notes": "do X"}, + "via": "v", + } + ) + coord.drain_resumes() + + # The task re-entered the pipeline and settled (approved on the re-plan). + state = graph_mod.get_pipeline_state(coord.graph, thread_id=thread_id) + assert state["current_phase"] == "build" + # The human notes were folded in as a synthetic verdict. + assert any(v.get("reviewer") == "human_plan_gate" for v in state["review_verdicts"]) + + +def test_gate_abandon_fails_task(db_path: Path, _restore_review_invoker: Any) -> None: + coord = _make_gate_coordinator(db_path) + coord.setup() + thread_id = _drive_to_gate(coord) + + qid = _only_open_row(db_path)["question_id"] + coord.submit_answer( + {"question_id": qid, "answer": {"decision": "abandon"}, "via": "v"} + ) + coord.drain_resumes() + + state = graph_mod.get_pipeline_state(coord.graph, thread_id=thread_id) + assert state["status"] == "failed" + + +def test_single_open_gate_invariant_skips_second_open( + db_path: Path, _restore_review_invoker: Any +) -> None: + # Re-presenting the gate (a redelivered drain) must NOT open a second row. + posted: list[tuple[str, str | None]] = [] + coord = _make_gate_coordinator( + db_path, + notify=lambda message, thread_ts=None: posted.append((message, thread_ts)), + ) + coord.setup() + thread_id = _drive_to_gate(coord) + + open_rows_before = _all_rows(db_path, status="open") + assert len(open_rows_before) == 1 + + # Force a second presentation of the same gate (simulating a redelivered + # resume settling on the same interrupt). The single-open-gate guard must + # reject the second open. + question = graph_mod.pending_question(coord.graph, thread_id=thread_id) + coord._post_plan_decision_gate( + question, label='"x" (`short`)', root_ts="ROOT.TS", short="short123" + ) + + open_rows_after = _all_rows(db_path, status="open") + assert len(open_rows_after) == 1 # still exactly one open gate row + # No second presentation was posted. + gate = [p for p in posted if "plan needs your decision" in p[0]] + assert len(gate) == 1 + + +def test_clarifier_followup_still_posts_no_regression(db_path: Path) -> None: + # A clarifier-kind interrupt keeps the existing follow-up behavior (the gate + # branch must not steal it). Reuses the legacy multi-turn follow-up shape. + from agent_team import coordinator as coord_mod + + posted: list[Any] = [] + msgs: list[str] = [] + coord = _make_coordinator(db_path, notify=lambda m, **k: msgs.append(m)) + coord.setup() + coord.start_task(task_text="x", transport_name="slack") + + # A pending CLARIFY interrupt (no 'kind' => clarify); a stub notify_question. + coord._notify = lambda m, **k: msgs.append(m) + + class _QS: + thread_id = "abc12345deadbeef" + + import unittest.mock as _mock + + with ( + _mock.patch.object( + coord_mod.graph_mod, + "pending_question", + lambda _g, *, thread_id: {"question_set": _QS(), "deadline": "2099-01-01"}, + ), + _mock.patch.object( + coord_mod.responder_mod, + "notify_question", + lambda conn, transport, qset, *, deadline, thread_ts=None: posted.append( + qset + ), + ), + ): + coord._post_resume_followups([_resume_result("abc12345deadbeef")]) + + assert len(posted) == 1 # the clarifier follow-up was delivered + assert any("needs more input" in m for m in msgs) + + +def test_plan_decision_expiry_posts_recovery_notice( + db_path: Path, _restore_review_invoker: Any +) -> None: + # When a plan-decision gate row expires, tick() posts a recovery notice + # naming the task + the recovery path (re-assign / force-resume). + posted: list[tuple[str, str | None]] = [] + coord = _make_gate_coordinator( + db_path, + notify=lambda message, thread_ts=None: posted.append((message, thread_ts)), + ) + coord.setup() + _drive_to_gate(coord, root_ts="ROOT.TS") + posted.clear() + + # Force the open gate row's deadline into the past, then sweep. + conn = connect(db_path) + try: + conn.execute( + "UPDATE pending_questions SET deadline_at=? WHERE status='open'", + ("2000-01-01T00:00:00+00:00",), + ) + conn.commit() + finally: + conn.close() + + coord.tick() + + notice = [p for p in posted if "PLAN DECISION EXPIRED" in p[0]] + assert len(notice) == 1 + message, thread_ts = notice[0] + assert thread_ts == "ROOT.TS" + assert "re-assign" in message.lower() or "force-resume" in message.lower() From f4de957915fbc6ba650663087d50e68eb6b11160 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Tue, 23 Jun 2026 20:34:58 -0400 Subject: [PATCH 05/11] feat(agent-team): Slack decision surface for the plan-review gate (Phase B3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turn a human's Slack interaction at the plan gate into a structured decision the graph can route, with the kind-aware mapping that closes a silent-FAIL hazard. - KIND-AWARE NORMALIZATION (load-bearing): map_plan_decision() in slack_adapter maps a reply to {"decision","notes"} — approve ∈ {approve,approved,yes,ok,lgtm, ship}; abandon ∈ {abandon,reject,cancel,stop,kill}; EVERYTHING ELSE → request_changes with the full reply as notes (never accidental abandon). Wired in the listener's _resolve_payload for plan_decision rows ONLY (clarify passes through). Without this, arbitrary change-notes hit the graph's unrecognized-verb→FAILED path and silently fail the task. Anti-FAIL tests assert prose → request_changes (!= abandon) at both the mapper and the end-to-end listener seam; a regression test guards clarify pass-through. New find_open_question_kind_by_channel_ref (anti-replay, status='open') powers the thread-reply fallback's kind lookup. - BUTTONS + MODAL: build_plan_decision_blocks() renders Approve (primary) / Request changes / Abandon (danger+confirm); question_id double-anchored in message metadata AND each button value (":"). Approve/abandon submit via the existing @app.action(.*); request_changes has a dedicated handler that AUTHORIZES before views_open (proven by test) and opens a notes modal (private_metadata carries the id) → view_submission → request_changes + notes. Free-text reply stays the always-available equal path. AUTHZ-01 ordering preserved. - No manifest change (views.open needs no extra scope). 1454 passed (1412 + 42). --- agent-team/agent_team/coordinator.py | 44 ++- agent-team/agent_team/db/schema.py | 27 ++ .../agent_team/transport/slack_adapter.py | 325 +++++++++++++++++- .../agent_team/transport/slack_listener.py | 161 ++++++++- agent-team/tests/test_schema.py | 44 +++ agent-team/tests/test_slack_adapter.py | 274 +++++++++++++++ agent-team/tests/test_slack_listener.py | 221 ++++++++++++ 7 files changed, 1061 insertions(+), 35 deletions(-) diff --git a/agent-team/agent_team/coordinator.py b/agent-team/agent_team/coordinator.py index c9424a7..48abc23 100644 --- a/agent-team/agent_team/coordinator.py +++ b/agent-team/agent_team/coordinator.py @@ -1263,7 +1263,11 @@ class Coordinator: transport_name = str(question.get("transport") or "") # 1. Durable ledger row first (open, kind='plan_decision'), guarded by the - # single-open-gate invariant. + # single-open-gate invariant. The row's gate post (via the transport's + # post_question) carries the three decision BUTTONS over the same + # presentation body the lifecycle milestone shows (B3), so the buttons + # land on the decision message itself. + body = self._plan_decision_presentation(question, label=label) try: conn = connect(self._db_path) try: @@ -1275,6 +1279,7 @@ class Coordinator: transport_name=transport_name, deadline=deadline, root_ts=root_ts, + presentation=body, ) finally: conn.close() @@ -1290,9 +1295,11 @@ class Coordinator: # opener; just stop here. return - # 2. Present the plan + findings + decision instructions, threaded under - # the task root. - body = self._plan_decision_presentation(question, label=label) + # 2. Emit the human-readable presentation as a lifecycle milestone too + # (B2b), threaded under the task root, so the decision is visible to + # sinks that don't render Block Kit (and to the dashboard/notify + # trail). The buttons live on the transport gate post above; this is + # the always-available text path. self._emit(body, thread_ts=root_ts) def _plan_decision_presentation( @@ -1315,8 +1322,8 @@ class Coordinator: findings = findings[:600] + "…" instructions = ( - "• Decide: reply *approve* / *request changes * / *abandon* " - "in this thread." + "• Decide: use the buttons below, OR reply *approve* / " + "*request changes * / *abandon* in this thread." ) header = f"🧭 {label} — plan needs your decision (review could not approve it)." @@ -1348,6 +1355,7 @@ class Coordinator: transport_name: str, deadline: str, root_ts: str | None, + presentation: str = "", ) -> bool: """Open a ``kind='plan_decision'`` ledger row, enforcing one-open-gate (B-2). @@ -1400,7 +1408,10 @@ class Coordinator: question_id=question_id, turn=turn, question_set=self._plan_decision_question_set( - thread_id=thread_id, question_id=question_id, turn=turn + thread_id=thread_id, + question_id=question_id, + turn=turn, + presentation=presentation, ), deadline=deadline, **post_kwargs, @@ -1423,15 +1434,17 @@ class Coordinator: @staticmethod def _plan_decision_question_set( - *, thread_id: str, question_id: str, turn: int + *, thread_id: str, question_id: str, turn: int, presentation: str = "" ) -> "QuestionSet": - """Build a minimal QuestionSet for the gate post (answer-mapping only). + """Build the QuestionSet for the gate post (decision surface, B3). The transport's ``post_question`` requires a ``question_set`` so the - inbound answer can map back to ``question_id``; the gate's human-readable - presentation is posted via the lifecycle ``_emit`` sink, so this set only - needs to carry the identity (the decision-instruction wording lives in the - presentation message). The single question text is a terse decision + inbound answer can map back to ``question_id``. The ``context`` carries + ``kind == 'plan_decision'`` (so the Slack transport renders the three + decision buttons instead of generic question blocks) and the human- + readable ``presentation`` body (so the buttons render over the plan + + findings + instructions, and the message text fallback shows the same to + non-interactive clients). The single question text is a terse decision prompt for transports that render the set directly. """ from agent_team.transport.base import QuestionSet # noqa: PLC0415 @@ -1441,7 +1454,10 @@ class Coordinator: question_id=question_id, turn=turn, questions=["Approve, request changes, or abandon this plan?"], - context={"kind": graph_mod.PLAN_DECISION_KIND}, + context={ + "kind": graph_mod.PLAN_DECISION_KIND, + "presentation": presentation, + }, ) @staticmethod diff --git a/agent-team/agent_team/db/schema.py b/agent-team/agent_team/db/schema.py index 4fc3c73..d354753 100644 --- a/agent-team/agent_team/db/schema.py +++ b/agent-team/agent_team/db/schema.py @@ -43,6 +43,7 @@ __all__ = [ "delete_issue_ingested", "expire_question", "find_open_question_by_channel_ref", + "find_open_question_kind_by_channel_ref", "init_db", "issue_already_ingested", "migrate", @@ -579,6 +580,32 @@ def find_open_question_by_channel_ref( return None if row is None else str(row["question_id"]) +def find_open_question_kind_by_channel_ref( + conn: sqlite3.Connection, + channel_ref: str, +) -> tuple[str, str] | None: + """Map a transport ``channel_ref`` to its open ``(question_id, kind)``. + + Like :func:`find_open_question_by_channel_ref` but also returns the ``kind`` + column so the caller can route ``plan_decision`` rows through the + kind-aware decision normalizer without a second query. + + Returns ``(question_id, kind)`` for the matching open row, or ``None`` if + ``channel_ref`` is empty or matches no open row. The lookup is constrained + to ``status='open'`` (anti-replay) — same semantics as the parent function. + """ + if not channel_ref: + return None + row = conn.execute( + "SELECT question_id, kind FROM pending_questions " + "WHERE channel_ref=? AND status='open'", + (channel_ref,), + ).fetchone() + if row is None: + return None + return str(row["question_id"]), str(row["kind"]) + + def supersede_question( conn: sqlite3.Connection, *, diff --git a/agent-team/agent_team/transport/slack_adapter.py b/agent-team/agent_team/transport/slack_adapter.py index 417c33b..e458625 100644 --- a/agent-team/agent_team/transport/slack_adapter.py +++ b/agent-team/agent_team/transport/slack_adapter.py @@ -35,12 +35,21 @@ from agent_team.transport.base import ( __all__ = [ "CALLBACK_ID_PREFIX", + "PLAN_DECISION_ABANDON_ACTION", + "PLAN_DECISION_ABANDON_VERBS", + "PLAN_DECISION_APPROVE_ACTION", + "PLAN_DECISION_APPROVE_VERBS", + "PLAN_DECISION_KIND", + "PLAN_DECISION_REQUEST_CHANGES_ACTION", "VIA_SLACK", "SlackPostError", "SlackPoster", "SlackTransport", "build_callback_id", + "build_plan_decision_blocks", "build_question_blocks", + "build_request_changes_modal", + "map_plan_decision", "parse_callback_id", ] @@ -54,6 +63,67 @@ VIA_SLACK = "slack" # question) from the base module. CALLBACK_ID_PREFIX = "shq" +# The ``pending_questions.kind`` discriminator for the plan-review decision gate. +# Mirrors ``agent_team.graph.PLAN_DECISION_KIND`` and the DB CHECK constraint; +# duplicated here (as a plain string constant) so the transport/listener layer +# can route kind-aware decisions WITHOUT importing the graph module. +PLAN_DECISION_KIND = "plan_decision" + +# Plan-decision verb allowlists (B3). A reply / button-value to a +# ``kind == 'plan_decision'`` question is normalized (lowercase + strip) and +# matched against these allowlists. Anything ELSE — arbitrary change-request +# prose — maps to ``request_changes`` carrying the FULL original reply as +# ``notes`` (the safe default; NEVER an accidental approve or abandon). See +# :func:`map_plan_decision`. +PLAN_DECISION_APPROVE_VERBS: frozenset[str] = frozenset( + {"approve", "approved", "yes", "ok", "lgtm", "ship"} +) +PLAN_DECISION_ABANDON_VERBS: frozenset[str] = frozenset( + {"abandon", "reject", "cancel", "stop", "kill"} +) + +# Block Kit action_id namespace for the three plan-gate buttons. Each carries +# the decision verb; ``request_changes`` opens a modal for free-form notes. +PLAN_DECISION_ACTION_PREFIX = "plan_decision" +PLAN_DECISION_APPROVE_ACTION = f"{PLAN_DECISION_ACTION_PREFIX}:approve" +PLAN_DECISION_REQUEST_CHANGES_ACTION = f"{PLAN_DECISION_ACTION_PREFIX}:request_changes" +PLAN_DECISION_ABANDON_ACTION = f"{PLAN_DECISION_ACTION_PREFIX}:abandon" + + +def map_plan_decision(raw_answer: Any) -> dict[str, str]: + """Map a raw plan-gate reply to a structured ``{"decision", "notes"}`` dict. + + THE LOAD-BEARING B3 SAFETY MAPPING. The graph's ``_parse_decision`` maps any + unrecognized verb to ``abandon`` → terminal FAILED, so free-text change + notes (e.g. "use pytest fixtures instead") would silently FAIL the task if + they reached the graph unmapped. This normalizes BEFORE the graph sees it: + + * normalize the text (``str`` → lowercase → strip); + * **approve** iff in :data:`PLAN_DECISION_APPROVE_VERBS` + (``approve/approved/yes/ok/lgtm/ship``), with ``notes=""``; + * **abandon** iff in :data:`PLAN_DECISION_ABANDON_VERBS` + (``abandon/reject/cancel/stop/kill``), with ``notes=""``; + * **everything else → ``request_changes`` with the FULL original reply as + ``notes``** — the safe default. Arbitrary prose is a change request, never + an accidental approve or abandon. Empty / whitespace-only input also maps + to ``request_changes`` (with empty notes): a blank reply is treated as a + benign no-op change request, never a destructive abandon. + + The answer is opaque DATA throughout — never executed or interpreted beyond + this verb match. Returns the dict the graph's ``_parse_decision`` consumes. + """ + original = "" if raw_answer is None else str(raw_answer) + verb = original.strip().lower() + if verb in PLAN_DECISION_APPROVE_VERBS: + return {"decision": "approve", "notes": ""} + if verb in PLAN_DECISION_ABANDON_VERBS: + return {"decision": "abandon", "notes": ""} + # Everything else (including empty/whitespace) → request_changes, carrying + # the full ORIGINAL reply (not the lowercased form) so the notes preserve + # the human's exact wording. Never an accidental abandon. + return {"decision": "request_changes", "notes": original.strip()} + + # Type of the network seam: given the rendered Slack message kwargs, perform the # ``chat.postMessage`` and return the response payload. The only field this # adapter requires from the response is the message ``ts`` (the ``channel_ref``). @@ -163,6 +233,121 @@ def build_question_blocks( return blocks +def build_plan_decision_blocks( + question_id: str, body_text: str +) -> list[dict[str, Any]]: + """Render the plan-gate decision surface as Block Kit blocks (B3). + + A section carrying the human-readable ``body_text`` (the plan summary + + review findings + decision instructions assembled by the coordinator) plus + an ``actions`` block with the three decision buttons: + + * **Approve** (``action_id="plan_decision:approve"``, ``style="primary"``); + * **Request changes** (``action_id="plan_decision:request_changes"``) — its + dedicated handler opens a modal for free-form notes; + * **Abandon** (``action_id="plan_decision:abandon"``, ``style="danger"``, + guarded by a ``confirm`` dialog so it is never a single-click mistake). + + Each button's ``value`` encodes ``":"`` so an inbound + ``block_actions`` payload can recover BOTH the verb and the ledger + ``question_id`` even if the message metadata is absent. The ``question_id`` + is ALSO carried in the message ``metadata.event_payload`` by the caller + (mirroring the clarifier), so recovery is double-anchored. The section text + is the always-visible fallback for non-interactive clients (a free-text + thread reply remains an equal path to all three decisions). + + Returns a plain JSON-serializable list (no Slack SDK types). + """ + section_text = body_text if body_text else "Plan needs your decision." + # Slack section text caps at ~3000 chars; keep a margin. + if len(section_text) > 2900: + section_text = section_text[:2900].rstrip() + "…" + return [ + {"type": "section", "text": {"type": "mrkdwn", "text": section_text}}, + { + "type": "actions", + "elements": [ + { + "type": "button", + "action_id": PLAN_DECISION_APPROVE_ACTION, + "text": {"type": "plain_text", "text": "Approve"}, + "style": "primary", + "value": f"approve:{question_id}", + }, + { + "type": "button", + "action_id": PLAN_DECISION_REQUEST_CHANGES_ACTION, + "text": {"type": "plain_text", "text": "Request changes"}, + "value": f"request_changes:{question_id}", + }, + { + "type": "button", + "action_id": PLAN_DECISION_ABANDON_ACTION, + "text": {"type": "plain_text", "text": "Abandon"}, + "style": "danger", + "value": f"abandon:{question_id}", + "confirm": { + "title": {"type": "plain_text", "text": "Abandon this task?"}, + "text": { + "type": "mrkdwn", + "text": ( + "This terminally FAILS the task. The plan is " + "discarded and the pipeline stops." + ), + }, + "confirm": {"type": "plain_text", "text": "Abandon"}, + "deny": {"type": "plain_text", "text": "Keep"}, + "style": "danger", + }, + }, + ], + }, + ] + + +# Block / action ids for the request-changes modal input, so the inbound +# ``view_submission`` extraction can find the notes value deterministically. +REQUEST_CHANGES_MODAL_CALLBACK_ID = ( + f"{PLAN_DECISION_ACTION_PREFIX}:request_changes_modal" +) +REQUEST_CHANGES_NOTES_BLOCK_ID = "plan_decision_notes_block" +REQUEST_CHANGES_NOTES_ACTION_ID = "plan_decision_notes_input" + + +def build_request_changes_modal(question_id: str) -> dict[str, Any]: + """Build the "Request changes" notes modal (views.open view, B3). + + One required multiline ``plain_text_input`` ("What should change?"). The + ledger ``question_id`` round-trips through ``private_metadata`` as + ``"request_changes:"`` so the eventual ``view_submission`` + recovers it (mirroring the button-value encoding). On submit, the input text + becomes the ``request_changes`` notes via the kind-aware normalizer. + + Returns a plain JSON-serializable view dict (no Slack SDK types) so the + caller passes it straight to ``client.views_open(trigger_id=..., view=...)``. + """ + return { + "type": "modal", + "callback_id": REQUEST_CHANGES_MODAL_CALLBACK_ID, + "private_metadata": f"request_changes:{question_id}", + "title": {"type": "plain_text", "text": "Request changes"}, + "submit": {"type": "plain_text", "text": "Send"}, + "close": {"type": "plain_text", "text": "Cancel"}, + "blocks": [ + { + "type": "input", + "block_id": REQUEST_CHANGES_NOTES_BLOCK_ID, + "label": {"type": "plain_text", "text": "What should change?"}, + "element": { + "type": "plain_text_input", + "action_id": REQUEST_CHANGES_NOTES_ACTION_ID, + "multiline": True, + }, + } + ], + } + + def _join_answer_actions(actions: Sequence[Mapping[str, Any]]) -> Any: """Reduce one-or-more interactive actions to a single answer value. @@ -256,14 +441,29 @@ class SlackTransport(Transport): durable ``channel_ref`` (it uses the root ``thread_ts`` when threading so an inbound reply's ``thread_ts`` maps back to this question). """ - blocks = build_question_blocks(question_set, deadline) + # Plan-review gate (B3): when the question_set is a plan decision, render + # the three decision buttons (Approve / Request changes / Abandon) over + # the presentation text instead of the generic question blocks. The + # presentation body is passed through the question_set ``context`` (under + # ``presentation``) by the coordinator; the buttons carry the recoverable + # question_id (and the message metadata double-anchors it). A free-text + # thread reply remains an equal path to all three decisions. + if question_set.context.get("kind") == PLAN_DECISION_KIND: + body_text = str(question_set.context.get("presentation") or "") + blocks = build_plan_decision_blocks(question_id, body_text) + fallback_text = body_text or ( + f"Plan decision needed on task {thread_id} (turn {turn})." + ) + else: + blocks = build_question_blocks(question_set, deadline) + fallback_text = ( + f"Agent-team needs input on task {thread_id} " + f"(turn {turn}); reply by {deadline}." + ) message: dict[str, Any] = { "channel": self.channel, "callback_id": build_callback_id(question_id), - "text": ( - f"Agent-team needs input on task {thread_id} " - f"(turn {turn}); reply by {deadline}." - ), + "text": fallback_text, "blocks": blocks, "metadata": { "event_type": "agent_team_question", @@ -343,8 +543,23 @@ def _extract_question_id(raw: Mapping[str, Any]) -> str: # Interactive payloads nest the callback metadata under ``view`` / ``message``. view = raw.get("view") - if isinstance(view, Mapping) and view.get("callback_id"): - return parse_callback_id(str(view["callback_id"])) + if isinstance(view, Mapping): + # Modal (view_submission) round-trips the question_id through + # ``private_metadata`` (B3 request-changes modal). It carries + # ``":"`` (or a bare id); recover the id suffix. This + # is checked BEFORE the view ``callback_id`` because the B3 modal's + # callback_id is a modal identifier (``plan_decision:...``), NOT a + # ``shq:`` carrier. + private_metadata = view.get("private_metadata") + if private_metadata: + return _question_id_from_value(str(private_metadata)) + # A view whose callback_id IS an ``shq:`` carrier (a non-B3 modal that + # embedded the question id there directly) still resolves. + view_callback_id = view.get("callback_id") + if view_callback_id and str(view_callback_id).startswith( + f"{CALLBACK_ID_PREFIX}:" + ): + return parse_callback_id(str(view_callback_id)) message = raw.get("message") if isinstance(message, Mapping): metadata = message.get("metadata") @@ -353,6 +568,25 @@ def _extract_question_id(raw: Mapping[str, Any]) -> str: if isinstance(payload, Mapping) and payload.get("question_id"): return str(payload["question_id"]) + # Block-action button value: the B3 plan-gate buttons encode + # ``":"`` so the id is recoverable even with no + # callback_id / message metadata on the payload. + actions = raw.get("actions") + if ( + isinstance(actions, Sequence) + and not isinstance(actions, (str, bytes)) + and actions + ): + for action in actions: + if not isinstance(action, Mapping): + continue + value = action.get("value") + if not value: + continue + qid = _question_id_from_plan_decision_value(str(value)) + if qid: + return qid + question_id = raw.get("question_id") if question_id: return str(question_id) @@ -360,17 +594,88 @@ def _extract_question_id(raw: Mapping[str, Any]) -> str: raise ValueError("Slack payload carries no recoverable question_id") +_PLAN_DECISION_VALUE_VERBS = frozenset({"approve", "request_changes", "abandon"}) + + +def _question_id_from_value(value: str) -> str: + """Recover the question_id from a ``private_metadata`` string. + + The B3 request-changes modal stores ``":"`` (or a bare + ``question_id``) in ``private_metadata``. A leading known decision verb is a + prefix to strip; otherwise the whole value IS the id. Raises + :class:`ValueError` on an empty value. + """ + qid = _question_id_from_plan_decision_value(value) + if qid: + return qid + if not value: + raise ValueError("empty private_metadata; no recoverable question_id") + return value + + +def _question_id_from_plan_decision_value(value: str) -> str | None: + """Return the ``question_id`` suffix of a ``":"`` value. + + Only matches when the prefix is a known plan-decision verb so an ordinary + button value (e.g. a clarifier's free-text answer) is never mis-parsed. + Returns ``None`` if the value is not a ``":"`` encoding. + """ + verb, sep, rest = value.partition(":") + if sep and verb in _PLAN_DECISION_VALUE_VERBS and rest: + return rest + return None + + +def _modal_input_text(view: Mapping[str, Any]) -> Any: + """Recover the submitted text from a ``view_submission`` view (B3 modal). + + Walks ``view.state.values`` (``{block_id: {action_id: {value: ...}}}``) and + returns the first non-empty ``plain_text_input`` value. The B3 request- + changes modal has a single input, so the first value is the notes text. + Returns ``None`` if no input value is present. + """ + state = view.get("state") + if not isinstance(state, Mapping): + return None + values = state.get("values") + if not isinstance(values, Mapping): + return None + for block in values.values(): + if not isinstance(block, Mapping): + continue + for action in block.values(): + if isinstance(action, Mapping) and action.get("value") is not None: + return action["value"] + return None + + def _extract_answer(raw: Mapping[str, Any]) -> Any: """Recover the answer value from any supported inbound payload.""" + # view_submission (modal): the answer is the submitted input text (B3). + view = raw.get("view") + if isinstance(view, Mapping): + text = _modal_input_text(view) + if text is not None: + return text + actions = raw.get("actions") if ( isinstance(actions, Sequence) and not isinstance(actions, (str, bytes)) and actions ): - return _join_answer_actions( - [action for action in actions if isinstance(action, Mapping)] - ) + mappings = [action for action in actions if isinstance(action, Mapping)] + # A single plan-decision button encodes ``":"``; the + # question_id is recovered separately (callback_id / metadata / value), + # so the ANSWER is the bare verb. Stripping the suffix here lets the + # kind-aware normalizer match it against the approve/abandon allowlists. + if len(mappings) == 1: + value = mappings[0].get("value") + if value is not None: + verb, sep, rest = str(value).partition(":") + if sep and verb in _PLAN_DECISION_VALUE_VERBS and rest: + return verb + return _join_answer_actions(mappings) if "answer" in raw: return raw["answer"] diff --git a/agent-team/agent_team/transport/slack_listener.py b/agent-team/agent_team/transport/slack_listener.py index 685b86d..cd900b4 100644 --- a/agent-team/agent_team/transport/slack_listener.py +++ b/agent-team/agent_team/transport/slack_listener.py @@ -74,15 +74,26 @@ from __future__ import annotations import logging import re -from collections.abc import Mapping +from collections.abc import Mapping, Sequence from pathlib import Path from typing import Any from collections.abc import Callable -from agent_team.db.schema import connect, find_open_question_by_channel_ref +from agent_team.db.schema import ( + connect, + find_open_question_kind_by_channel_ref, +) from agent_team.responder import AnswerOutcome, EnqueueResume, submit_answer -from agent_team.transport.slack_adapter import SlackPoster, SlackTransport +from agent_team.transport.slack_adapter import ( + PLAN_DECISION_KIND, + PLAN_DECISION_REQUEST_CHANGES_ACTION, + SlackPoster, + SlackTransport, + build_request_changes_modal, + map_plan_decision, +) +from agent_team.ledger import get_question __all__ = [ "NewTaskCallback", @@ -521,9 +532,11 @@ class SlackListener: 1. **Explicit id** — if :meth:`SlackTransport.parse_answer` can already recover a ``question_id`` (callback_id / nested view-or-message - metadata / bare ``question_id``), pass the original payload straight - through unchanged. This covers block_actions, view_submission, slash - commands, and any synthetic-but-explicit reply. + metadata / button value / modal ``private_metadata`` / bare + ``question_id``), pass the original payload through unchanged UNLESS + the row's ``kind == 'plan_decision'`` (see KIND-AWARE below). This + covers block_actions, view_submission, slash commands, and any + synthetic-but-explicit reply. 2. **Thread-reply fallback** — only when (1) raises ``ValueError`` (no recoverable id): a real free-text Events API reply. Resolve the question by the inner event's ``thread_ts`` against the OPEN ledger @@ -533,17 +546,40 @@ class SlackListener: ``status='open'``-constrained lookup is anti-replay: a thread_ts for a closed / answered / expired row resolves to ``None`` → ignored. + KIND-AWARE DECISION NORMALIZATION (B3, the load-bearing safety map). For + a row whose ``kind == 'plan_decision'`` — in BOTH the explicit-id and + thread-reply paths — the raw answer is mapped to a structured decision + ``{"decision","notes"}`` via :func:`map_plan_decision` BEFORE it reaches + the graph. Without this, the graph's decision parser maps any + unrecognized verb to ``abandon`` → terminal FAILED, so arbitrary + change-request prose ("use pytest fixtures instead") would silently FAIL + the task instead of requesting changes. ``approve``/``abandon`` map to + the named verb; EVERYTHING else maps to ``request_changes`` carrying the + full original reply as notes (never an accidental abandon). A + ``clarify`` row passes through UNCHANGED (its free text IS the answer). + Returns the payload to submit, or ``None`` when no question can be resolved (the caller ignores the event without crashing). Re-raises nothing: a genuinely unrecoverable explicit payload surfaces as the ``ValueError`` from the final ``submit_answer`` call in ``handle_event``. """ try: - self._transport.parse_answer(raw_payload) + question_id, answer, _via = self._transport.parse_answer(raw_payload) except ValueError: pass else: - # Explicit id recovered; submit the original payload unchanged. + # Explicit id recovered (button / view_submission / explicit reply). + # Kind-aware normalization (B3): a ``plan_decision`` row's raw answer + # MUST be mapped to a structured decision BEFORE it reaches the graph + # (whose parser FAILs any unrecognized verb). Look up the row's kind + # and, for a plan decision, synthesize an explicit payload carrying + # the {"decision","notes"} dict; otherwise pass through unchanged. + kind = self._question_kind(conn, question_id) + if kind == PLAN_DECISION_KIND: + return { + "question_id": question_id, + "answer": map_plan_decision(answer), + } return raw_payload # No explicit id: try the events-API thread-reply mapping. @@ -556,8 +592,8 @@ class SlackListener: # and ignores. return raw_payload - question_id = find_open_question_by_channel_ref(conn, str(thread_ts)) - if question_id is None: + resolved = find_open_question_kind_by_channel_ref(conn, str(thread_ts)) + if resolved is None: # thread_ts matched no OPEN row (stale / replayed / answered): ignore. _LOG.debug( "ignoring Slack thread reply: thread_ts %r matched no open " @@ -565,14 +601,84 @@ class SlackListener: thread_ts, ) return None + question_id, kind = resolved text = event.get("text") answer = text.strip() if isinstance(text, str) else text + # Kind-aware normalization (B3): a free-text reply to a plan-decision row + # — arbitrary change-request prose — is mapped to a structured decision + # (request_changes carrying the full reply as notes; NEVER an accidental + # abandon). A clarify row's free text passes through unchanged: the text + # IS the answer. + if kind == PLAN_DECISION_KIND: + answer = map_plan_decision(answer) # Synthesize an explicit payload the transport already understands # (bare question_id + answer). The answer is opaque DATA — stored # verbatim as JSON by the responder, never interpreted. return {"question_id": question_id, "answer": answer} + def _question_kind(self, conn: Any, question_id: str) -> str | None: + """Return the ``kind`` of an open ledger row, or ``None`` if absent. + + Used by :meth:`_resolve_payload` to decide whether an explicit-id answer + (button / modal / explicit reply) needs kind-aware decision + normalization. A missing row (forged / stale id) returns ``None`` so the + answer passes through unchanged and the responder's compare-and-set + rejects it (anti-replay). Tolerant: any read error degrades to ``None`` + (pass-through), never crashing the listen loop. + """ + try: + row = get_question(conn, question_id) + except Exception: # noqa: BLE001 - a ledger read error must not crash handling + _LOG.debug( + "kind lookup failed for question_id=%s; treating as non-plan", + question_id, + exc_info=True, + ) + return None + return row.kind if row is not None else None + + def _open_request_changes_modal(self, body: Mapping[str, Any], client: Any) -> None: + """Open the "Request changes" notes modal for an authorized owner (B3). + + Wired to the dedicated ``@app.action(plan_decision:request_changes)`` + listener. Preserves AUTHZ-01 ordering: authorize the sender BEFORE any + side effect (the ``views.open`` is the side effect here). On rejection, + does nothing (the catch-all already skips this action, so no answer is + submitted). The modal's eventual ``view_submission`` carries the notes + and flows through :meth:`handle_event` like any other answer. + + Fully guarded: recovering the trigger_id / question_id or the + ``views_open`` call failing is swallowed (best-effort) — the owner can + always fall back to a free-text thread reply. Never raises into Bolt. + """ + if not self._is_authorized(body): + return + trigger_id = body.get("trigger_id") + if not trigger_id: + _LOG.debug("request-changes action carried no trigger_id; ignoring") + return + try: + question_id = self._transport.parse_answer(body)[0] + except Exception: # noqa: BLE001 - no recoverable id: cannot open a modal + _LOG.debug( + "request-changes action carried no recoverable question_id; " + "ignoring (owner can still reply free-text)", + ) + return + try: + client.views_open( + trigger_id=str(trigger_id), + view=build_request_changes_modal(question_id), + ) + except Exception: # noqa: BLE001 - a modal-open failure must not crash the loop + _LOG.warning( + "failed to open request-changes modal for question_id=%s; " + "owner can still reply free-text in-thread", + question_id, + exc_info=True, + ) + def serve(self) -> None: # pragma: no cover - live socket, not unit-tested """Open the Socket Mode connection and forward events to ``handle_event``. @@ -626,14 +732,30 @@ class SlackListener: def _forward(body: Mapping[str, Any]) -> None: self.handle_event(body) + # DEDICATED handler for the plan-gate "Request changes" button — MUST be + # registered BEFORE the generic catch-all so it wins. Unlike Approve / + # Abandon (which submit immediately), this button has no free-form notes, + # so instead of forwarding to handle_event it AUTHORIZES the sender + # (AUTHZ-01, before any side effect) and opens a notes modal. The modal's + # eventual ``view_submission`` carries the notes and flows through + # handle_event like any other answer. + @app.action(PLAN_DECISION_REQUEST_CHANGES_ACTION) + def _on_request_changes(ack: Any, body: Mapping[str, Any], client: Any) -> None: + ack() + self._open_request_changes_modal(body, client) + # Match ANY block_actions interaction. slack_bolt rejects an empty-dict # constraint (``BoltError: action ({}) must be any of str, Pattern, and # dict``); a catch-all ``action_id`` regex is the supported way to # register a single handler for every block action. ``handle_event`` does - # the real filtering + auth, so over-matching here is safe. + # the real filtering + auth, so over-matching here is safe. The + # request-changes button is handled by its dedicated listener above; skip + # it here so it does not ALSO submit an empty (notes-less) decision. @app.action(re.compile(r".*")) # any block_actions interaction def _on_action(ack: Any, body: Mapping[str, Any]) -> None: ack() + if _is_request_changes_action(body): + return _forward(body) @app.event("message") @@ -690,6 +812,23 @@ class SlackListener: _LOG.debug("SlackListener.close: handler teardown raised; ignoring") +def _is_request_changes_action(body: Mapping[str, Any]) -> bool: + """Return ``True`` iff ``body`` is a plan-gate "Request changes" button click. + + The generic catch-all action listener uses this to SKIP the request-changes + button (handled by its dedicated modal-opening listener), so it does not + ALSO submit an empty (notes-less) decision through ``handle_event``. + """ + actions = body.get("actions") + if not isinstance(actions, Sequence) or isinstance(actions, (str, bytes)): + return False + return any( + isinstance(action, Mapping) + and action.get("action_id") == PLAN_DECISION_REQUEST_CHANGES_ACTION + for action in actions + ) + + def _inner_event(raw_payload: Mapping[str, Any]) -> Mapping[str, Any]: """Return the Events API inner event, or an empty mapping if there is none. diff --git a/agent-team/tests/test_schema.py b/agent-team/tests/test_schema.py index d7435a3..3904b68 100644 --- a/agent-team/tests/test_schema.py +++ b/agent-team/tests/test_schema.py @@ -16,6 +16,7 @@ from agent_team.db.schema import ( answer_question, connect, expire_question, + find_open_question_kind_by_channel_ref, init_db, issue_already_ingested, migrate, @@ -58,6 +59,49 @@ def test_question_states_match_ddl_check() -> None: assert f"'{state}'" in PENDING_QUESTIONS_DDL +def test_find_open_question_kind_by_channel_ref_returns_qid_and_kind( + tmp_path: Path, +) -> None: + db = tmp_path / "db.sqlite" + init_db(db) + conn = connect(db) + try: + conn.execute( + "INSERT INTO pending_questions " + "(question_id, thread_id, turn, status, transport, channel_ref, kind) " + "VALUES ('q1', 't1', 0, 'open', 'slack', 'TS.1', 'plan_decision')" + ) + conn.commit() + assert find_open_question_kind_by_channel_ref(conn, "TS.1") == ( + "q1", + "plan_decision", + ) + # Empty ref / no match -> None. + assert find_open_question_kind_by_channel_ref(conn, "") is None + assert find_open_question_kind_by_channel_ref(conn, "NOPE") is None + finally: + conn.close() + + +def test_find_open_question_kind_by_channel_ref_constrained_to_open( + tmp_path: Path, +) -> None: + """Anti-replay: a non-open row's channel_ref resolves to None.""" + db = tmp_path / "db.sqlite" + init_db(db) + conn = connect(db) + try: + conn.execute( + "INSERT INTO pending_questions " + "(question_id, thread_id, turn, status, transport, channel_ref, kind) " + "VALUES ('q1', 't1', 0, 'answered', 'slack', 'TS.1', 'plan_decision')" + ) + conn.commit() + assert find_open_question_kind_by_channel_ref(conn, "TS.1") is None + finally: + conn.close() + + def test_connect_sets_pragmas(tmp_path: Path) -> None: conn = connect(tmp_path / "db.sqlite") try: diff --git a/agent-team/tests/test_slack_adapter.py b/agent-team/tests/test_slack_adapter.py index 25376e7..4d10e2e 100644 --- a/agent-team/tests/test_slack_adapter.py +++ b/agent-team/tests/test_slack_adapter.py @@ -16,11 +16,18 @@ import pytest from agent_team.transport.base import QuestionSet, Transport from agent_team.transport.slack_adapter import ( CALLBACK_ID_PREFIX, + PLAN_DECISION_ABANDON_ACTION, + PLAN_DECISION_APPROVE_ACTION, + PLAN_DECISION_KIND, + PLAN_DECISION_REQUEST_CHANGES_ACTION, VIA_SLACK, SlackPostError, SlackTransport, build_callback_id, + build_plan_decision_blocks, build_question_blocks, + build_request_changes_modal, + map_plan_decision, parse_callback_id, ) @@ -396,3 +403,270 @@ def test_post_then_parse_round_trips_question_id() -> None: assert qid == "q-round" assert answer == "approved" assert via == VIA_SLACK + + +# --------------------------------------------------------------------------- # +# B3 — kind-aware plan-decision normalization (map_plan_decision) # +# --------------------------------------------------------------------------- # + + +@pytest.mark.parametrize( + "raw", + ["approve", "approved", "yes", "ok", "lgtm", "ship", "APPROVE", " Yes "], +) +def test_map_plan_decision_approve_verbs(raw: str) -> None: + """All approve-allowlist verbs map to approve (case + whitespace insensitive).""" + result = map_plan_decision(raw) + assert result == {"decision": "approve", "notes": ""} + + +@pytest.mark.parametrize( + "raw", + ["abandon", "reject", "cancel", "stop", "kill", "ABANDON", " Cancel "], +) +def test_map_plan_decision_abandon_verbs(raw: str) -> None: + """All abandon-allowlist verbs map to abandon (case + whitespace insensitive).""" + result = map_plan_decision(raw) + assert result == {"decision": "abandon", "notes": ""} + + +def test_map_plan_decision_arbitrary_prose_is_request_changes_not_abandon() -> None: + """THE ANTI-FAIL TEST: arbitrary change prose -> request_changes, NOT abandon. + + The graph maps any unrecognized verb to abandon -> terminal FAILED. This is + the load-bearing guard that real change-request notes never silently FAIL a + task: free prose becomes request_changes carrying the FULL original reply as + notes, and is explicitly asserted to NOT be abandon (or approve). + """ + raw = "use pytest fixtures instead of setUp methods" + result = map_plan_decision(raw) + assert result["decision"] == "request_changes" + assert result["decision"] != "abandon" + assert result["decision"] != "approve" + # The full original reply is preserved as the notes. + assert result["notes"] == raw + + +def test_map_plan_decision_preserves_original_casing_in_notes() -> None: + """request_changes notes keep the human's exact wording (not lowercased).""" + raw = "Please Add Type Hints To The New Helper" + result = map_plan_decision(raw) + assert result == {"decision": "request_changes", "notes": raw} + + +@pytest.mark.parametrize("raw", ["", " ", "\n\t "]) +def test_map_plan_decision_empty_is_request_changes_safe_default(raw: str) -> None: + """Empty / whitespace-only input maps to request_changes (never abandon).""" + result = map_plan_decision(raw) + assert result["decision"] == "request_changes" + assert result["decision"] != "abandon" + assert result["notes"] == "" + + +def test_map_plan_decision_none_is_request_changes() -> None: + """A None answer degrades to request_changes with empty notes (never abandon).""" + result = map_plan_decision(None) + assert result == {"decision": "request_changes", "notes": ""} + + +def test_map_plan_decision_verb_with_trailing_text_is_request_changes() -> None: + """A verb embedded in a sentence is NOT a bare verb -> request_changes. + + "approve but tweak X" is a change request, not an approval — only an exact + bare verb match approves. + """ + raw = "approve but please tweak the error handling first" + result = map_plan_decision(raw) + assert result["decision"] == "request_changes" + assert result["notes"] == raw + + +# --------------------------------------------------------------------------- # +# B3 — gate decision blocks (the three buttons) # +# --------------------------------------------------------------------------- # + + +def test_build_plan_decision_blocks_has_three_buttons_with_action_ids() -> None: + blocks = build_plan_decision_blocks("q-xyz", "Plan summary + findings") + # A section (the body) + an actions block with three buttons. + assert blocks[0]["type"] == "section" + assert "Plan summary" in blocks[0]["text"]["text"] + actions = blocks[1] + assert actions["type"] == "actions" + action_ids = [e["action_id"] for e in actions["elements"]] + assert action_ids == [ + PLAN_DECISION_APPROVE_ACTION, + PLAN_DECISION_REQUEST_CHANGES_ACTION, + PLAN_DECISION_ABANDON_ACTION, + ] + + +def test_build_plan_decision_blocks_buttons_embed_recoverable_question_id() -> None: + """Each button value encodes ``":"`` for recovery.""" + blocks = build_plan_decision_blocks("q-recover", "body") + values = [e["value"] for e in blocks[1]["elements"]] + assert values == [ + "approve:q-recover", + "request_changes:q-recover", + "abandon:q-recover", + ] + # The question_id is recoverable from every button value. + for v in values: + assert v.endswith(":q-recover") + + +def test_build_plan_decision_blocks_abandon_has_danger_confirm() -> None: + blocks = build_plan_decision_blocks("q1", "body") + abandon = blocks[1]["elements"][2] + assert abandon["style"] == "danger" + assert "confirm" in abandon + approve = blocks[1]["elements"][0] + assert approve["style"] == "primary" + + +# --------------------------------------------------------------------------- # +# B3 — block_actions button parse_answer recovery # +# --------------------------------------------------------------------------- # + + +def test_parse_answer_recovers_qid_and_verb_from_button_value() -> None: + """An approve button with no callback_id recovers qid + bare verb from value.""" + t = SlackTransport(channel="C1", poster=_RecordingPoster()) + payload = { + "type": "block_actions", + "actions": [ + {"action_id": PLAN_DECISION_APPROVE_ACTION, "value": "approve:q-btn"} + ], + } + qid, answer, via = t.parse_answer(payload) + assert qid == "q-btn" + # The answer is the bare verb (suffix stripped) so map_plan_decision matches. + assert answer == "approve" + assert via == VIA_SLACK + + +def test_parse_answer_recovers_abandon_button_value() -> None: + t = SlackTransport(channel="C1", poster=_RecordingPoster()) + payload = { + "type": "block_actions", + "actions": [{"action_id": PLAN_DECISION_ABANDON_ACTION, "value": "abandon:q9"}], + } + qid, answer, _via = t.parse_answer(payload) + assert qid == "q9" + assert answer == "abandon" + + +def test_parse_answer_button_metadata_takes_precedence_for_qid() -> None: + """When message metadata carries the qid, it is used (callback_id dropped live).""" + t = SlackTransport(channel="C1", poster=_RecordingPoster()) + payload = { + "type": "block_actions", + "message": { + "metadata": {"event_payload": {"question_id": "q-meta"}}, + }, + "actions": [ + {"action_id": PLAN_DECISION_APPROVE_ACTION, "value": "approve:q-meta"} + ], + } + qid, answer, _via = t.parse_answer(payload) + assert qid == "q-meta" + assert answer == "approve" + + +# --------------------------------------------------------------------------- # +# B3 — request-changes modal + view_submission # +# --------------------------------------------------------------------------- # + + +def test_build_request_changes_modal_round_trips_question_id() -> None: + modal = build_request_changes_modal("q-modal") + assert modal["type"] == "modal" + assert modal["private_metadata"] == "request_changes:q-modal" + # One required multiline input. + block = modal["blocks"][0] + assert block["type"] == "input" + assert block["element"]["type"] == "plain_text_input" + assert block["element"]["multiline"] is True + + +def _view_submission(question_id: str, notes: str) -> dict[str, Any]: + """A realistic ``view_submission`` payload for the request-changes modal.""" + return { + "type": "view_submission", + "user": {"id": "U_OWNER"}, + "view": { + "callback_id": "plan_decision:request_changes_modal", + "private_metadata": f"request_changes:{question_id}", + "state": { + "values": { + "plan_decision_notes_block": { + "plan_decision_notes_input": { + "type": "plain_text_input", + "value": notes, + } + } + } + }, + }, + } + + +def test_parse_answer_recovers_qid_and_notes_from_view_submission() -> None: + """The modal submit recovers qid from private_metadata + notes from input.""" + t = SlackTransport(channel="C1", poster=_RecordingPoster()) + payload = _view_submission("q-sub", "switch to dependency injection") + qid, answer, via = t.parse_answer(payload) + assert qid == "q-sub" + assert answer == "switch to dependency injection" + assert via == VIA_SLACK + + +def test_view_submission_notes_map_to_request_changes() -> None: + """The modal's free-text notes normalize to request_changes (via the mapper).""" + t = SlackTransport(channel="C1", poster=_RecordingPoster()) + _qid, answer, _via = t.parse_answer( + _view_submission("q1", "use a factory function") + ) + decision = map_plan_decision(answer) + assert decision == { + "decision": "request_changes", + "notes": "use a factory function", + } + + +# --------------------------------------------------------------------------- # +# B3 — post_question renders decision buttons for a plan_decision question # +# --------------------------------------------------------------------------- # + + +def test_post_question_renders_plan_decision_buttons() -> None: + poster = _RecordingPoster() + t = SlackTransport(channel="C1", poster=poster) + qs = QuestionSet( + thread_id="t1", + question_id="q-gate", + turn=2, + questions=["Approve, request changes, or abandon this plan?"], + context={"kind": PLAN_DECISION_KIND, "presentation": "Plan body here"}, + ) + t.post_question( + thread_id="t1", + question_id="q-gate", + turn=2, + question_set=qs, + deadline="2026-06-18T00:00:00Z", + ) + blocks = poster.calls[0]["blocks"] + action_ids = [ + e["action_id"] + for b in blocks + if b.get("type") == "actions" + for e in b["elements"] + ] + assert PLAN_DECISION_APPROVE_ACTION in action_ids + assert PLAN_DECISION_REQUEST_CHANGES_ACTION in action_ids + assert PLAN_DECISION_ABANDON_ACTION in action_ids + # The presentation body is the message text fallback. + assert poster.calls[0]["text"] == "Plan body here" + # The question_id is double-anchored in message metadata. + assert poster.calls[0]["metadata"]["event_payload"]["question_id"] == "q-gate" diff --git a/agent-team/tests/test_slack_listener.py b/agent-team/tests/test_slack_listener.py index f135c4a..ed1e7a1 100644 --- a/agent-team/tests/test_slack_listener.py +++ b/agent-team/tests/test_slack_listener.py @@ -878,3 +878,224 @@ def test_new_task_root_post_failure_degrades_to_empty_root_ts(db_path: Path) -> assert listener.handle_event(payload) is None # Task still started, with an empty root_ts (top-level questions). assert seen == [("do the thing", "slack", "")] + + +# --------------------------------------------------------------------------- +# B3 — kind-aware plan-decision normalization in _resolve_payload. +# --------------------------------------------------------------------------- + + +def _seed_plan_decision_question( + db_path: Path, + *, + thread_id: str = "t1", + question_id: str = "q1", + turn: int = 0, + channel_ref: str = SEED_CHANNEL_REF, +) -> None: + """Insert a real ``open`` ``kind='plan_decision'`` ledger row + channel_ref. + + Mirrors a B2b plan-gate row: open, plan_decision kind, with the gate + message ts recorded as channel_ref so an inbound thread reply maps back. + """ + from agent_team import ledger as ledger_mod + + conn = connect(db_path) + try: + ledger_mod.post_question( + conn, + question_id=question_id, + thread_id=thread_id, + turn=turn, + transport="SlackTransport", + deadline_at="2026-06-18T00:00:00+00:00", + kind="plan_decision", + ) + ledger_mod.set_channel_ref( + conn, question_id=question_id, channel_ref=channel_ref + ) + finally: + conn.close() + + +def test_plan_decision_free_text_prose_maps_to_request_changes_not_fail( + db_path: Path, +) -> None: + """THE ANTI-FAIL TEST at the listener seam (end-to-end-ish). + + A free-text reply of arbitrary change-request prose to a plan_decision row + must reach submit_answer as a structured {"decision":"request_changes",...} + carrying the full reply as notes — NEVER an abandon (which the graph would + map to terminal FAILED). This is the whole point of B3. + """ + _seed_plan_decision_question( + db_path, question_id="q1", channel_ref=SEED_CHANNEL_REF + ) + queue = RecordingQueue() + listener = _listener(db_path, queue, owner_ids={OWNER_ID}) + + outcome = listener.handle_event( + _events_api_reply(text="please add type hints", thread_ts=SEED_CHANNEL_REF) + ) + + assert outcome is not None and outcome.accepted is True + assert len(queue.jobs) == 1 + answer = queue.jobs[0].answer + assert isinstance(answer, dict) + assert answer["decision"] == "request_changes" + assert answer["decision"] != "abandon" + assert answer["notes"] == "please add type hints" + + +def test_plan_decision_free_text_approve_maps_to_approve(db_path: Path) -> None: + _seed_plan_decision_question( + db_path, question_id="q1", channel_ref=SEED_CHANNEL_REF + ) + queue = RecordingQueue() + listener = _listener(db_path, queue, owner_ids={OWNER_ID}) + + listener.handle_event(_events_api_reply(text="approve", thread_ts=SEED_CHANNEL_REF)) + + answer = queue.jobs[0].answer + assert answer == {"decision": "approve", "notes": ""} + + +def test_plan_decision_free_text_abandon_maps_to_abandon(db_path: Path) -> None: + _seed_plan_decision_question( + db_path, question_id="q1", channel_ref=SEED_CHANNEL_REF + ) + queue = RecordingQueue() + listener = _listener(db_path, queue, owner_ids={OWNER_ID}) + + listener.handle_event(_events_api_reply(text="cancel", thread_ts=SEED_CHANNEL_REF)) + + answer = queue.jobs[0].answer + assert answer == {"decision": "abandon", "notes": ""} + + +def test_clarify_free_text_passes_through_unchanged_regression(db_path: Path) -> None: + """A clarify row's free text is the answer verbatim (NOT decision-mapped).""" + # _seed_open_question seeds a default kind='clarify' row. + _seed_open_question(db_path, question_id="q1", channel_ref=SEED_CHANNEL_REF) + queue = RecordingQueue() + listener = _listener(db_path, queue, owner_ids={OWNER_ID}) + + listener.handle_event( + _events_api_reply(text="use the release branch", thread_ts=SEED_CHANNEL_REF) + ) + + # The answer is the raw string, NOT a {"decision",...} dict. + assert queue.jobs[0].answer == "use the release branch" + + +def test_plan_decision_approve_button_resolves_decision(db_path: Path) -> None: + """An approve BUTTON (block_actions) yields {"decision":"approve"}.""" + _seed_plan_decision_question( + db_path, question_id="q1", channel_ref=SEED_CHANNEL_REF + ) + queue = RecordingQueue() + listener = _listener(db_path, queue, owner_ids={OWNER_ID}) + + payload = { + "type": "block_actions", + "user": {"id": OWNER_ID}, + "message": {"metadata": {"event_payload": {"question_id": "q1"}}}, + "actions": [{"action_id": "plan_decision:approve", "value": "approve:q1"}], + } + outcome = listener.handle_event(payload) + + assert outcome is not None and outcome.accepted is True + assert queue.jobs[0].answer == {"decision": "approve", "notes": ""} + + +def test_plan_decision_abandon_button_resolves_decision(db_path: Path) -> None: + _seed_plan_decision_question( + db_path, question_id="q1", channel_ref=SEED_CHANNEL_REF + ) + queue = RecordingQueue() + listener = _listener(db_path, queue, owner_ids={OWNER_ID}) + + payload = { + "type": "block_actions", + "user": {"id": OWNER_ID}, + "message": {"metadata": {"event_payload": {"question_id": "q1"}}}, + "actions": [{"action_id": "plan_decision:abandon", "value": "abandon:q1"}], + } + listener.handle_event(payload) + assert queue.jobs[0].answer == {"decision": "abandon", "notes": ""} + + +def test_view_submission_modal_yields_request_changes_with_notes(db_path: Path) -> None: + """A request-changes modal submit yields {"decision":"request_changes",notes}.""" + _seed_plan_decision_question( + db_path, question_id="q1", channel_ref=SEED_CHANNEL_REF + ) + queue = RecordingQueue() + listener = _listener(db_path, queue, owner_ids={OWNER_ID}) + + payload = { + "type": "view_submission", + "user": {"id": OWNER_ID}, + "view": { + "callback_id": "plan_decision:request_changes_modal", + "private_metadata": "request_changes:q1", + "state": { + "values": { + "plan_decision_notes_block": { + "plan_decision_notes_input": { + "type": "plain_text_input", + "value": "tighten the error handling", + } + } + } + }, + }, + } + outcome = listener.handle_event(payload) + + assert outcome is not None and outcome.accepted is True + assert queue.jobs[0].answer == { + "decision": "request_changes", + "notes": "tighten the error handling", + } + + +def test_open_request_changes_modal_authorizes_before_opening(db_path: Path) -> None: + """AUTHZ-01: a non-owner request-changes click never opens a modal.""" + + class _Client: + def __init__(self) -> None: + self.opened: list[dict[str, Any]] = [] + + def views_open(self, *, trigger_id: str, view: dict[str, Any]) -> None: + self.opened.append({"trigger_id": trigger_id, "view": view}) + + _seed_plan_decision_question( + db_path, question_id="q1", channel_ref=SEED_CHANNEL_REF + ) + listener = _listener(db_path, RecordingQueue(), owner_ids={OWNER_ID}) + client = _Client() + + # Non-owner: must NOT open a modal. + intruder = { + "type": "block_actions", + "user": {"id": "U_INTRUDER"}, + "trigger_id": "TRIG.1", + "message": {"metadata": {"event_payload": {"question_id": "q1"}}}, + "actions": [ + { + "action_id": "plan_decision:request_changes", + "value": "request_changes:q1", + } + ], + } + listener._open_request_changes_modal(intruder, client) + assert client.opened == [] + + # Owner: opens a modal whose private_metadata round-trips the question_id. + owner = dict(intruder) + owner["user"] = {"id": OWNER_ID} + listener._open_request_changes_modal(owner, client) + assert len(client.opened) == 1 + assert client.opened[0]["trigger_id"] == "TRIG.1" + assert client.opened[0]["view"]["private_metadata"] == "request_changes:q1" From 082e45bf881a83330de769c2a93beddca768d798 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Tue, 23 Jun 2026 20:40:51 -0400 Subject: [PATCH 06/11] test(agent-team): end-to-end plan-review gate composition (Phase C) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hermetic e2e tests driving the WHOLE stack composed together — real build_graph(plan_gate=True) + real Coordinator + real SQLite ledger + real review_loop router, with only the LLM nodes stubbed — through the daemon API (start_task/submit_answer/tick), never nodes directly. Flows: approve settles at BUILD; request-changes via RAW PROSE through the real SlackListener -> _resolve_payload -> map_plan_decision proves the prose maps to request_changes (NOT FAILED) and the notes reach the planner; abandon -> FAILED; repeated request_changes terminates at MAX_PLAN_GATE_VISITS -> PARKED; and a legacy (no-kind) ledger migrates in place then routes clarify vs plan_decision correctly. 1459 passed. --- agent-team/tests/test_plan_gate_e2e.py | 544 +++++++++++++++++++++++++ 1 file changed, 544 insertions(+) create mode 100644 agent-team/tests/test_plan_gate_e2e.py diff --git a/agent-team/tests/test_plan_gate_e2e.py b/agent-team/tests/test_plan_gate_e2e.py new file mode 100644 index 0000000..28a526e --- /dev/null +++ b/agent-team/tests/test_plan_gate_e2e.py @@ -0,0 +1,544 @@ +"""Phase C end-to-end integration tests for the plan-review decision gate. + +These drive a task through the WHOLE Plane-2 stack composed together — the real +:func:`agent_team.graph.build_graph` (with ``plan_gate=True``), the real +:class:`agent_team.coordinator.Coordinator`, a real SQLite ledger (tmp), an +in-memory LangGraph checkpointer, and (for the kind-aware path) the real +:class:`agent_team.transport.slack_listener.SlackListener` + +:func:`agent_team.transport.slack_adapter.map_plan_decision`. ONLY the three LLM +nodes are stubbed so the run is deterministic and hermetic (no live model, no +Slack network, no GitHub): + +* the **clarifier** is the deterministic single-turn ``graph.clarify_node`` stub + (it really suspends on ``interrupt()`` — the human gate is real); +* the **planner** is a stub that emits a plan and advances to REVIEW (mirroring + ``planner.plan_node``'s contract), reused from the committed graph tests; +* the **reviewer** is the REAL ``review_loop`` node + router bound to a stubbed + review invoker (a callable returning canned ``VERDICT:`` text), so the + plan<->review loop, the round cap, and the cap→plan-gate dead-end are all real. + +Everything else — INTAKE, the human-gate suspend/resume mechanic, the +``plan_gate_node`` interrupt + decision parsing + visit ceiling, the +coordinator's ledger row opening / single-open-gate invariant / presentation +threading / kind routing, the listener's Slack→decision composition, and the +schema migration — is the real production code path. + +The drive surface is the daemon's public API (``start_task`` / ``submit_answer`` +/ ``tick`` which drains + posts follow-ups), NEVER the nodes directly. +""" + +from __future__ import annotations + +import queue +from pathlib import Path +from typing import Any + +import pytest + +try: # InMemorySaver is the modern name; fall back on older langgraph. + from langgraph.checkpoint.memory import InMemorySaver as _Saver +except ImportError: # pragma: no cover - environment-dependent + from langgraph.checkpoint.memory import MemorySaver as _Saver + +from agent_team import graph as graph_mod +from agent_team.coordinator import Coordinator +from agent_team.db.schema import connect, init_db +from agent_team.nodes import review_loop +from agent_team.task_model import Phase, PipelineState, TaskStatus +from agent_team.transport.base import QuestionSet, Transport +from agent_team.transport.slack_adapter import SlackTransport +from agent_team.transport.slack_listener import SlackListener + +# --------------------------------------------------------------------------- # +# Test doubles + harness (composed from the committed coordinator/graph tests). +# --------------------------------------------------------------------------- # + +# The single authorized owner id (AUTHZ-01) used on the listener path. +OWNER_ID = "U_OWNER" + + +class FakeTransport(Transport): + """Record-only transport: no Slack, no network (the §3.3.1 injection seam). + + Mirrors the FakeTransport in ``tests/test_coordinator.py``: ``post_question`` + records the posted question-set + the thread_ts it was threaded under and + returns a deterministic ``channel_ref`` embedding the ``question_id``; + ``parse_answer`` reads a plain ``{"question_id","answer","via"}`` dict so a + decision can be submitted without a Slack payload. + """ + + def __init__(self) -> None: + self.posted: list[QuestionSet] = [] + self.thread_tss: list[str | None] = [] + + def post_question( + self, + *, + thread_id: str, + question_id: str, + turn: int, + question_set: QuestionSet, + deadline: str, + thread_ts: str | None = None, + ) -> str: + self.posted.append(question_set) + self.thread_tss.append(thread_ts) + return f"fake:{question_id}" + + def parse_answer(self, raw: Any) -> tuple[str, Any, str]: + return raw["question_id"], raw["answer"], raw.get("via", "fake") + + +def _plan_stub(state: PipelineState) -> PipelineState: + """Planner stub: emit a plan and advance to REVIEW (no model call). + + Mirrors ``planner.plan_node``'s contract (sets ``plan`` + phase REVIEW), + reused verbatim from the committed P2 graph tests. The revision index tracks + prior review rounds so a re-plan after request_changes is observable, and the + folded-in human notes (review feedback) can be asserted by a wrapping stub. + """ + revisions = len(state.get("review_verdicts") or []) + return PipelineState( + plan={ + "summary": "Add a hermetic plan-gate smoke test.", + "phases": [{"name": "Audit infra"}, {"name": "Write test"}], + "revision": revisions, + }, + current_phase=Phase.REVIEW.value, + status=TaskStatus.ACTIVE.value, + ) + + +def _e2e_coordinator( + db_path: Path, + *, + review_text: "str | Any", + transport: Transport | None = None, + resume_queue: "queue.Queue[Any] | None" = None, + plan_node: Any = None, + max_review_rounds: int = 1, + notify: Any = None, +) -> Coordinator: + """Compose the REAL graph + coordinator with only the LLM nodes stubbed. + + * clarifier = the real ``graph.clarify_node`` suspend stub; + * planner = ``_plan_stub`` (or an injected wrapping stub); + * reviewer = the REAL ``review_loop`` node + router, bound to a stubbed + review invoker (``review_text`` may be a string or a callable that + LangGraph-style ``invoker(prompt, **kw)`` consumes); + * ``plan_gate=True`` so the review-cap dead-end suspends on the resumable + ``plan_decision`` interrupt instead of terminally parking. + + A fresh in-memory saver + the tmp SQLite ledger make the durable composition + real. ``max_review_rounds=1`` reaches the gate after a single REQUEST CHANGES + so the loop is short and deterministic. + """ + if callable(review_text): + review_loop.set_review_invoker(review_text) + else: + review_loop.set_review_invoker(lambda prompt, **kw: review_text) + saver = _Saver() + return Coordinator( + db_path=db_path, + transport=transport or FakeTransport(), + build_clarify_node=lambda: graph_mod.clarify_node, + build_plan_node=lambda: plan_node or _plan_stub, + review_wiring=lambda: ( + review_loop.bind_review_node({"max_review_rounds": max_review_rounds}), + review_loop.route_after_review, + ), + build_checkpointer=lambda _path: saver, + resume_queue=resume_queue, + notify=notify, + plan_gate=True, + ) + + +@pytest.fixture() +def restore_review_invoker(): + """Save/restore the review-loop module-global invoker around each test.""" + saved = review_loop._review_invoker + yield + review_loop._review_invoker = saved + + +@pytest.fixture() +def db_path(tmp_path: Path) -> Path: + path = tmp_path / "state" / "agent_team.sqlite" + init_db(path) + return path + + +# --------------------------------------------------------------------------- # +# Ledger helpers. +# --------------------------------------------------------------------------- # + + +def _open_rows(db_path: Path) -> list[dict[str, Any]]: + conn = connect(db_path) + try: + rows = conn.execute( + "SELECT * FROM pending_questions WHERE status='open'" + ).fetchall() + finally: + conn.close() + return [dict(r) for r in rows] + + +def _rows_by_kind(db_path: Path, kind: str) -> list[dict[str, Any]]: + conn = connect(db_path) + try: + rows = conn.execute( + "SELECT * FROM pending_questions WHERE kind=?", (kind,) + ).fetchall() + finally: + conn.close() + return [dict(r) for r in rows] + + +def _open_clarifier_qid(db_path: Path) -> str: + rows = _open_rows(db_path) + assert len(rows) == 1 + return str(rows[0]["question_id"]) + + +def _drive_to_plan_gate( + coord: Coordinator, *, root_ts: str = "ROOT.1" +) -> tuple[str, dict[str, Any]]: + """Start a task, answer the clarifier, and tick to the plan gate. + + Returns ``(thread_id, gate_row)`` where ``gate_row`` is the opened + ``kind='plan_decision'`` ledger row. Exercises the real surface end to end: + ``start_task`` -> clarifier suspend -> ``submit_answer`` -> ``tick`` (drain + runs intake->clarify->plan->review loop to the cap, suspends on the gate, and + ``_post_resume_followups`` opens + presents the decision row). + """ + thread_id = coord.start_task( + task_text="add a smoke test", + transport_name="slack", + slack_thread_ts=root_ts, + ) + clar_qid = _open_clarifier_qid(db_path_of(coord)) + coord.submit_answer({"question_id": clar_qid, "answer": "scope is X", "via": "v"}) + coord.tick() + + gate_rows = _rows_by_kind(db_path_of(coord), graph_mod.PLAN_DECISION_KIND) + assert len(gate_rows) == 1, "exactly one plan_decision gate row must be opened" + return thread_id, gate_rows[0] + + +def db_path_of(coord: Coordinator) -> Path: + return coord._db_path + + +# --------------------------------------------------------------------------- # +# 1. Approve path. +# --------------------------------------------------------------------------- # + + +def test_e2e_approve_path_settles_approved( + db_path: Path, restore_review_invoker +) -> None: + """start -> clarify -> plan -> review(ESCALATE) -> gate opened+presented -> + APPROVE decision -> task settles in the approved terminal state, no open row. + + The reviewer never approves, so the plan<->review loop hits the cap and + suspends on the real ``plan_decision`` gate. The coordinator opens the durable + row and posts the presentation THREADED under the task root. An ``approve`` + decision (through ``submit_answer``) drives the real ``plan_gate_node`` to the + same approved terminus an auto-approved plan reaches (phase BUILD, ACTIVE). + """ + transport = FakeTransport() + coord = _e2e_coordinator( + db_path, review_text="VERDICT: REQUEST CHANGES\nnot ready", transport=transport + ) + coord.setup() + + thread_id, gate_row = _drive_to_plan_gate(coord) + + # The opened gate row is durable, open, kind='plan_decision', threaded under + # the task root (its channel_ref is the root ts so a reply maps back). + assert gate_row["kind"] == graph_mod.PLAN_DECISION_KIND + assert gate_row["status"] == "open" + assert gate_row["channel_ref"] == "ROOT.1" + # A presentation was posted threaded under the root, carrying the plan. + assert transport.thread_tss[-1] == "ROOT.1" + gate_qset = transport.posted[-1] + assert gate_qset.context.get("kind") == graph_mod.PLAN_DECISION_KIND + assert "Summary:" in gate_qset.context.get("presentation", "") + + # Approve the plan through the real submit_answer -> resume -> graph path. + coord.submit_answer( + { + "question_id": gate_row["question_id"], + "answer": {"decision": "approve", "notes": ""}, + "via": "v", + } + ) + coord.tick() + + state = graph_mod.get_pipeline_state(coord.graph, thread_id=thread_id) + assert state["current_phase"] == Phase.BUILD.value + assert state["status"] == TaskStatus.ACTIVE.value + # No gate stays open and no pending interrupt remains. + assert _open_rows(db_path) == [] + assert graph_mod.pending_question(coord.graph, thread_id=thread_id) is None + + +# --------------------------------------------------------------------------- # +# 2. Request-changes path — RAW PROSE through the kind-aware listener. +# --------------------------------------------------------------------------- # + + +def test_e2e_request_changes_prose_via_listener_loops_to_planner( + db_path: Path, restore_review_invoker +) -> None: + """A FREE-TEXT prose reply, routed through the REAL SlackListener + + map_plan_decision, maps to request_changes (NOT abandon/FAILED) and loops the + task back to the planner with the human notes folded into review feedback. + + This is the load-bearing composition assertion: the prose + ("please use pytest fixtures") goes through ``handle_event`` -> + ``_resolve_payload`` -> ``map_plan_decision`` (the kind-aware Slack→decision + map), is enqueued onto the coordinator's resume queue, and the next ``tick`` + re-plans. Without the kind-aware map, the graph's decision parser would map + the prose verb to ``abandon`` -> terminal FAILED. + """ + # First review round REQUEST CHANGES (to reach the gate); after the human's + # prose request_changes loops back, the next review APPROVES so the task + # settles rather than re-gating (keeps the assertion crisp). + texts = iter(["VERDICT: REQUEST CHANGES\nnot ready", "VERDICT: APPROVE\nnow good"]) + last = {"v": "VERDICT: APPROVE\nnow good"} + + def review_invoker(prompt, **kw): + try: + last["v"] = next(texts) + except StopIteration: + pass + return last["v"] + + # A wrapping planner stub that captures the review feedback the planner sees + # on re-plan, so we can prove the human notes reached it. + from agent_team.nodes.planner import _format_review_feedback + + captured: dict[str, Any] = {} + + def capturing_plan(state: PipelineState) -> PipelineState: + captured["feedback"] = _format_review_feedback( + list(state.get("review_verdicts") or []) + ) + return _plan_stub(state) + + transport = FakeTransport() + # The coordinator and the listener SHARE one resume queue: the listener + # enqueues onto it, the coordinator drains it (the production handoff seam). + shared_q: "queue.Queue[Any]" = queue.Queue() + coord = _e2e_coordinator( + db_path, + review_text=review_invoker, + transport=transport, + resume_queue=shared_q, + plan_node=capturing_plan, + ) + coord.setup() + + thread_id, gate_row = _drive_to_plan_gate(coord) + # The gate post threaded under the root; its channel_ref is the root ts, so a + # thread reply with thread_ts == root ts maps back to THIS gate row. + root_ts = gate_row["channel_ref"] + assert root_ts == "ROOT.1" + + # Build the REAL listener over the SAME ledger DB, enqueueing onto the SAME + # queue the coordinator drains. The listener uses a real SlackTransport + # (parse_answer + channel only; no network). + listener = SlackListener( + SlackTransport(channel="C123"), + db_path, + shared_q.put, + owner_ids={OWNER_ID}, + ) + + # A REAL slack_bolt Events API thread-reply envelope carrying arbitrary + # change-request PROSE (no callback_id / question_id / metadata): the question + # is resolved by thread_ts == the gate row's channel_ref, and because that row + # is kind='plan_decision' the prose is mapped via map_plan_decision. + prose_reply = { + "type": "event_callback", + "event": { + "type": "message", + "text": "please use pytest fixtures", + "thread_ts": root_ts, + "ts": "1700000001.000200", + "channel": "C123", + "user": OWNER_ID, + }, + } + outcome = listener.handle_event(prose_reply) + assert outcome is not None and outcome.accepted is True + + # Drain the listener-enqueued resume through the coordinator: the graph loops + # plan -> review (now APPROVE) and settles. It must NOT have FAILED/abandoned. + coord.tick() + + state = graph_mod.get_pipeline_state(coord.graph, thread_id=thread_id) + assert state["status"] != TaskStatus.FAILED.value, ( + "raw prose must map to request_changes, never an accidental abandon/FAILED" + ) + # The human's exact prose reached the planner's review-feedback formatter on + # the re-plan (proving request_changes folded the notes in). + assert "please use pytest fixtures" in str(captured.get("feedback", "")) + # And the loop re-entered plan -> review and settled past the gate. + assert state["current_phase"] == Phase.BUILD.value + assert _open_rows(db_path) == [] + + +# --------------------------------------------------------------------------- # +# 3. Abandon path. +# --------------------------------------------------------------------------- # + + +def test_e2e_abandon_path_fails(db_path: Path, restore_review_invoker) -> None: + """An ``abandon`` decision at the gate drives the task to terminal FAILED.""" + coord = _e2e_coordinator(db_path, review_text="VERDICT: REQUEST CHANGES\nnope") + coord.setup() + + thread_id, gate_row = _drive_to_plan_gate(coord) + coord.submit_answer( + { + "question_id": gate_row["question_id"], + "answer": {"decision": "abandon", "notes": ""}, + "via": "v", + } + ) + coord.tick() + + state = graph_mod.get_pipeline_state(coord.graph, thread_id=thread_id) + assert state["status"] == TaskStatus.FAILED.value + assert _open_rows(db_path) == [] + + +# --------------------------------------------------------------------------- # +# 4. Ceiling termination — repeated request_changes terminates PARKED. +# --------------------------------------------------------------------------- # + + +def test_e2e_repeated_request_changes_hits_ceiling_parked( + db_path: Path, restore_review_invoker +) -> None: + """Repeated request_changes eventually hits ``MAX_PLAN_GATE_VISITS`` and goes + terminal PARKED — the gate loop does NOT spin forever. + + The reviewer NEVER approves, so every re-plan re-hits the review cap and + re-suspends on the gate; each request_changes consumes one gate visit. The + test loops well above the ceiling and asserts it terminates on its own (no + open gate, status PARKED, ceiling reason) at exactly the visit cap. + """ + coord = _e2e_coordinator( + db_path, review_text="VERDICT: REQUEST CHANGES\nstill not ready" + ) + coord.setup() + + thread_id, gate_row = _drive_to_plan_gate(coord) + + # Each iteration: answer the open gate request_changes, then tick (re-plan -> + # review cap -> re-suspend OR terminate). Bound the loop well above the + # ceiling to prove it self-terminates, not via our cap. + for _ in range(graph_mod.MAX_PLAN_GATE_VISITS + 5): + open_rows = _rows_by_kind(db_path, graph_mod.PLAN_DECISION_KIND) + open_now = [r for r in open_rows if r["status"] == "open"] + if not open_now: + break + coord.submit_answer( + { + "question_id": open_now[0]["question_id"], + "answer": {"decision": "request_changes", "notes": "again"}, + "via": "v", + } + ) + coord.tick() + + state = graph_mod.get_pipeline_state(coord.graph, thread_id=thread_id) + assert graph_mod.pending_question(coord.graph, thread_id=thread_id) is None + assert state["status"] == TaskStatus.PARKED.value + assert "ceiling" in (state.get("failure_reason") or "") + assert state.get("plan_gate_visits") == graph_mod.MAX_PLAN_GATE_VISITS + assert _open_rows(db_path) == [] + + +# --------------------------------------------------------------------------- # +# 5. Migration mixed-state composition — legacy clarifier row + new gate row. +# --------------------------------------------------------------------------- # + + +# Legacy (pre-kind) pending_questions DDL, mirrored from tests/test_schema.py, to +# construct a DB whose table predates the additive ``kind`` migration. +_LEGACY_PENDING_QUESTIONS_DDL = """ +CREATE TABLE IF NOT EXISTS pending_questions ( + question_id TEXT PRIMARY KEY, + thread_id TEXT NOT NULL, + turn INTEGER NOT NULL, + status TEXT NOT NULL + CHECK (status IN ('open', 'answered', 'expired', 'superseded')), + transport TEXT NOT NULL, + channel_ref TEXT, + posted_at TEXT, + deadline_at TEXT, + answer_json TEXT, + answered_at TEXT, + answered_via TEXT +) +""".strip() + + +def test_e2e_migration_mixed_state_legacy_clarify_plus_new_gate( + tmp_path: Path, restore_review_invoker +) -> None: + """A coordinator pointed at an OLD-schema ledger (no ``kind`` column) carrying + a legacy clarifier row migrates it, then drives a NEW task to the plan gate. + + Proves the migration + kind routing COMPOSE on a real upgraded ledger: + + * the pre-existing legacy row reads back ``kind='clarify'`` (the migration + default), and + * the new gate row the coordinator opens is ``kind='plan_decision'`` (the + kind discriminator routes correctly on the upgraded DB). + """ + db_path = tmp_path / "legacy" / "agent_team.sqlite" + db_path.parent.mkdir(parents=True, exist_ok=True) + + # 1. Build the OLD table by hand (no kind column) and seed a legacy clarifier + # row, then close — simulating a pre-upgrade ledger on disk. + conn = connect(db_path) + try: + conn.execute(_LEGACY_PENDING_QUESTIONS_DDL) + conn.execute( + "INSERT INTO pending_questions " + "(question_id, thread_id, turn, status, transport) " + "VALUES ('legacy-clarify', 'legacy-thread', 0, 'answered', 'slack')" + ) + assert "kind" not in { + r["name"] for r in conn.execute("PRAGMA table_info(pending_questions)") + } + finally: + conn.close() + + # 2. Coordinator.setup() runs init_db (the migration) on the existing DB, then + # we drive a NEW task to the plan gate over the upgraded ledger. + coord = _e2e_coordinator(db_path, review_text="VERDICT: REQUEST CHANGES\nnope") + coord.setup() # migrates the legacy table in place (adds kind column) + + _thread_id, gate_row = _drive_to_plan_gate(coord) + + # The legacy row survived the migration and reads the 'clarify' default. + conn = connect(db_path) + try: + legacy_kind = conn.execute( + "SELECT kind FROM pending_questions WHERE question_id='legacy-clarify'" + ).fetchone()["kind"] + finally: + conn.close() + assert legacy_kind == "clarify" + + # And the new gate row the coordinator opened is kind='plan_decision' — the + # migration + kind routing compose on a real upgraded ledger. + assert gate_row["kind"] == graph_mod.PLAN_DECISION_KIND From f46d691e360a8ae1cbd38119ca0810447c1825a2 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 24 Jun 2026 11:30:54 -0400 Subject: [PATCH 07/11] docs(agent-team): document the plan-review decision gate (Phase C) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit README: two human gates (clarifier + plan-decision), the approve/request-changes/ abandon verbs, free-text-defaults-to-request-changes, MAX_PLAN_GATE_VISITS, and the planner max_turns reliability fix. OPERATOR-RUNBOOK: how the gate appears in Slack, the three decision paths (buttons/modal/free-text), the single-open-gate invariant, ceiling→PARKED, and the 24h expiry→PARKED→recovery (re-assign / force-resume). DEPLOY-R720: the pending_questions.kind ledger migration (SCHEMA_VERSION→4, idempotent additive ALTER on startup) + rollback (restore the ledger backup before restart if the migration fails). --- agent-team/DEPLOY-R720.md | 35 +++++++++++++++ agent-team/README.md | 56 ++++++++++++++++++----- docs/provisioning/OPERATOR-RUNBOOK.md | 64 +++++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 10 deletions(-) diff --git a/agent-team/DEPLOY-R720.md b/agent-team/DEPLOY-R720.md index 76a6e09..016d4e5 100644 --- a/agent-team/DEPLOY-R720.md +++ b/agent-team/DEPLOY-R720.md @@ -118,6 +118,23 @@ The unit runs `python3 run-team.py serve` from secrets from `EnvironmentFile=/home/adam/secrev.env`. `Restart=on-failure` keeps it up across transient faults; `journalctl -u` is the live log. +> **⚠️ This deploy carries a ledger migration (SCHEMA_VERSION → 4).** It adds a +> `kind` column to `pending_questions` (values `clarify` | `plan_decision`, +> existing rows default to `clarify`) for the plan-review decision gate. The +> migration is an **additive, idempotent in-place `ALTER TABLE`** run on startup +> (`init_db` / `migrate`, guarded so a second run is a no-op — it never recreates +> the table), so it applies in place against the live box ledger. **Take the +> ledger backup (§1 / the deploy script step) BEFORE restart** — it is the +> migration's safety net (see Rollback, §6). After restart, confirm the column +> landed and the daemon came up clean: +> ```bash +> sqlite3 ~/orchestrator/agent-team/state/agent_team.sqlite \ +> "PRAGMA table_info(pending_questions);" | grep kind # expect a 'kind' row +> sqlite3 ~/orchestrator/agent-team/state/agent_team.sqlite \ +> "SELECT schema_version FROM schema_meta WHERE id=1;" # expect 4 +> journalctl -u agent-team-coordinator.service -e | tail # no migration/import errors +> ``` + ## 4b. WS0–WS5 rollout — UPDATE an already-deployed box The steps above (§1–4) are the **first-time** P1 provision. To bring an @@ -286,6 +303,24 @@ it without a full snapshot restore: back it up first, then wipe. cp ~/orchestrator/agent-team/state/agent_team.sqlite{,.bak} # back up rm ~/orchestrator/agent-team/state/agent_team.sqlite* # wipe (then re-run init-db) ``` + +**Ledger-migration rollback (the schema-v4 `kind` migration).** The deploy takes +a dated ledger backup (`~/agent_team.sqlite.bak-`, written by the +`/sh-deploy-r720` flow / `scripts/deploy-r720.sh`) **before** restart — that +backup is the migration's safety net. The `kind` migration is additive and +idempotent, but **if the `init_db`/`migrate` step fails, or the deploy is rolled +back to pre-v4 code after the migration ran, restore the ledger from that backup +BEFORE restarting the coordinator** (old code does not expect the new column to +matter, but restoring guarantees a clean, pre-migration ledger): +``` +sudo systemctl stop agent-team-coordinator.service +cp ~/agent_team.sqlite.bak- ~/orchestrator/agent-team/state/agent_team.sqlite +rm -f ~/orchestrator/agent-team/state/agent_team.sqlite-wal \ + ~/orchestrator/agent-team/state/agent_team.sqlite-shm # drop stale WAL/SHM +sudo systemctl start agent-team-coordinator.service +``` +Restore the ledger backup BEFORE the coordinator restarts — never start the +daemon against a half-migrated or suspect ledger. Note the secrets in `~/secrev.env` are NOT removed by rollback - leave them, or strip the four agent-team keys if you are decommissioning entirely. diff --git a/agent-team/README.md b/agent-team/README.md index 95729af..d7b126e 100644 --- a/agent-team/README.md +++ b/agent-team/README.md @@ -2,15 +2,28 @@ The durable, human-gated agentic SDLC pipeline for the R720 (`sh-secrev` VM), design: `../docs/r720-agent-team-design.md`. A task flows INTAKE → CLARIFY (the -human gate) → PLAN → REVIEW, and (P3) → BUILD → DISPATCH → VERIFY → draft +first human gate) → PLAN → REVIEW, and (P3) → BUILD → DISPATCH → VERIFY → draft PR. Every stage is durable and resumable (LangGraph + a SQLite checkpointer); -the human gate suspends on `interrupt()` and resumes on a real answer. +both human gates suspend on `interrupt()` and resume on a real answer. + +There are now **two human gates**: the **clarifier** (CLARIFY asks question-sets +until confident) and the **plan-decision gate** (the dead-end when PLAN ⇄ REVIEW +cannot auto-converge). When the review loop hits its revision cap (or the planner +salvages only a partial plan), the pipeline no longer terminally PARKs — it +suspends on a resumable `interrupt()` and the coordinator posts the plan + +reviewer findings to Slack `#agent-team`, threaded under the task root, for the +owner to decide. ``` INTAKE → CLARIFY (Claude, human gate) → PLAN (Claude) → REVIEW (GPT-4.1) ▲ │ └── loop-back ───┤ - approve/escalate → END + approve → BUILD/END + review-cap / partial plan + → PLAN-DECISION GATE (human) + approve → BUILD/END + request changes → PLAN + abandon → FAILED (P3 box path — gated behind the C1 re-review): approve → BUILD (DeepSeek) → DISPATCH (push branch, trigger CI, capture run_id, suspend) → [CI-watcher resumes on terminal @@ -64,7 +77,8 @@ agent-team/ operator_cli.py nodes/ # pipeline stages + their model bindings clarifier.py + clarifier_llm.py # human gate (Claude) - planner.py # plan (Claude) + planner.py # plan (Claude); per-call max_turns=4 + + # classified retry-once (reliability fix) review_loop.py + review_loop_llm.py # adversarial review (GPT-4.1 via orchestrator) builders.py + builders_llm.py # candidate diff (DeepSeek) — INERT, proposes only verifier.py + verifier_llm.py # ci_gate sole PASS authority; LLM = fix-proposer; @@ -102,12 +116,34 @@ snake_case (`agent_team/`), per the engineering handbook. ## Key design points -- **Durable human gate (§3.3.1).** The `pending_questions` ledger is the single - source of truth for the question lifecycle. Every race (duplicate answers, - transport redelivery, answer-vs-timeout) resolves via one atomic - compare-and-set against `status`, inside a `BEGIN IMMEDIATE` transaction — - first-answer-wins (`rowcount == 1`), late/duplicate ignored. The LangGraph - `SqliteSaver` checkpointer shares the same DB file. +- **Durable human gates (§3.3.1).** The `pending_questions` ledger is the single + source of truth for the question lifecycle, with a `kind` discriminator + (`clarify` | `plan_decision`) marking which gate a row belongs to (schema v4, + idempotent additive migration). Every race (duplicate answers, transport + redelivery, answer-vs-timeout) resolves via one atomic compare-and-set against + `status`, inside a `BEGIN IMMEDIATE` transaction — first-answer-wins + (`rowcount == 1`), late/duplicate ignored. **Single-open-gate invariant:** a + thread holds at most one open question at a time (the clarifier row is answered + before the plan stage runs), so clarifier and plan-decision gates can never be + open simultaneously for one thread. The LangGraph `SqliteSaver` checkpointer + shares the same DB file. +- **Plan-review decision gate.** When PLAN ⇄ REVIEW cannot auto-converge + (review-revision cap) or only a partial plan is salvaged, the coordinator posts + the plan (`_summarize_plan`) + reviewer findings (`_summarize_blocker`) to Slack + `#agent-team` and the task owner decides via three verbs — **Approve** (settle + the plan → BUILD), **Request changes** (loop back to the planner with the notes + folded into review feedback), **Abandon** (FAILED). The decision arrives via + Block Kit buttons, a notes modal, or a free-text thread reply; **free-text + prose that isn't a recognized approve/abandon verb defaults to request-changes** + (carrying the full reply as the notes) so a change request can never be + misread as an accidental approve or abandon. Bounded by `MAX_PLAN_GATE_VISITS` + (= 3) so the human loop always terminates. +- **Planner reliability.** The planner's single-shot Claude call runs with + `max_turns=4` (tools stay disabled) so it has room to finish emitting its JSON + rather than exhausting the default 1-turn budget mid-reply, plus a classified + retry-once: a *transient* failure (turn-cap exhaustion or an empty reply) is + retried exactly once; a *deterministic* failure (malformed JSON, missing + phases) fails fast. - **Fail-safe model seams.** Every node treats model output as untrusted and fails SAFE: garbage never clears the 98% clarifier gate, never auto-approves a plan, never fabricates a build success, and the verifier's `ci_gate` is the diff --git a/docs/provisioning/OPERATOR-RUNBOOK.md b/docs/provisioning/OPERATOR-RUNBOOK.md index cec3cbf..21c1999 100644 --- a/docs/provisioning/OPERATOR-RUNBOOK.md +++ b/docs/provisioning/OPERATOR-RUNBOOK.md @@ -124,6 +124,70 @@ Jira ticket per the ladder) rather than starving silently. --- +## Incident 2b — Plan-review decision gate (PLAN ⇄ REVIEW dead-end) + +A second human gate opens when the plan↔review loop **cannot auto-converge** (the +review-revision cap is hit) or the planner produced only a partial plan. Instead +of terminally parking, the coordinator suspends on a resumable `interrupt()` and +posts the **plan + reviewer findings** to Slack `#agent-team`, **threaded under +the task root**, with three decision verbs. The ledger row carries +`kind = 'plan_decision'` (a clarifier row is `kind = 'clarify'`); both are +ordinary `pending_questions` rows, so the same `list` / `show` / `force-resume` +verbs apply. + +**The three decision paths (all equivalent — pick whichever is handy):** + +| Path | How | Effect | +|---|---|---| +| **Buttons** | Block Kit *Approve* / *Request changes* / *Abandon* on the gate message | Approve & Abandon submit immediately; *Request changes* opens a notes modal | +| **Modal** | the *Request changes* button → a one-field "What should change?" modal | submits `request_changes` with your notes | +| **Free-text reply** | reply in the gate thread | parsed kind-aware (below) | + +**What each decision does:** +- **Approve** — settles the plan (advances to BUILD / continues the pipeline). +- **Request changes (+ notes)** — loops back to the **planner**, folding the notes + into the review feedback so the re-plan addresses them. +- **Abandon** — fails the task (terminal `FAILED`). + +**Free-text mapping (the safe-default rule).** A thread reply is normalized +(lowercase/strip) and matched against small allowlists: +`approve ∈ {approve, approved, yes, ok, lgtm, ship}`; +`abandon ∈ {abandon, reject, cancel, stop, kill}`. **Anything else — any other +prose, including empty/whitespace — maps to *request changes*, carrying the full +reply as the notes.** So typing change notes in the thread (e.g. "use pytest +fixtures instead") requests changes; it can never be misread as an accidental +approve or abandon. + +**Single-open-gate invariant.** A thread holds **at most one open question at a +time** — the clarifier row is already answered before the plan stage runs, so a +clarifier-answer and a plan-decision can never be open simultaneously for one +thread. If a second open row for a thread ever appears, treat it as a bug +(the coordinator logs + skips opening it) and inspect with `list --all`. + +**Ceiling behavior.** The human request-changes loop is bounded by +`MAX_PLAN_GATE_VISITS` (graph constant, currently **3**, distinct from the +planner's `MAX_PLAN_REVISIONS`). When the gate-visit ceiling is exhausted the +node does **not** re-open the gate — it returns terminal **PARKED** with a +"revision ceiling reached" note, so the loop always terminates. + +**Expiry / recovery (gate goes unanswered).** A `plan_decision` row uses the +**same 24h deadline window as the clarifier**. If unanswered, the deadline sweep +**expires** the row → the task **PARKS**, and the coordinator posts a +`⌛ PLAN DECISION EXPIRED` lifecycle notice naming the task + the recovery path, +threaded under the task root. Recover the same way as a parked clarifier: + +```bash +python3 run-team.py list --parked +python3 run-team.py show # kind == "plan_decision", status == expired +python3 run-team.py force-resume --confirm # reopens the expired gate for re-delivery +``` + +Then answer it (Slack buttons / modal / free-text reply, or CLI `answer`). If the +task is better restarted from scratch, **re-assign** it instead. There is no +auto-retry — a PARKED plan gate stays PARKED until an operator acts. + +--- + ## Incident 3 — Failed human-in-the-loop resume **Symptoms:** an answer was submitted (Slack or CLI) but the graph did not From 48a81c0802c227d583cae6f2a3ad86058fd6d614 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 24 Jun 2026 11:48:04 -0400 Subject: [PATCH 08/11] fix(agent-team): centralize safe decision mapping + remove free-text abandon hair-trigger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security-review follow-up (LOGIC-01/02/05, all confirmed correctness). - New transport-neutral `decisions.normalize_decision(raw, *, allow_abandon)` is the single source of truth: approve-allowlist→approve; abandon-allowlist→abandon ONLY when allow_abandon; everything else (prose, empty, abandon-verbs when disallowed) → request_changes with the full reply as notes; idempotent on an already-formed decision dict. slack_adapter.map_plan_decision is now a thin wrapper (default allow_abandon=True, no caller churn). - LOGIC-01/02: graph._parse_decision now delegates to normalize_decision (was: any unrecognized verb → abandon → FAILED). The graph is now the universal safe backstop, so EVERY writer that bypassed the listener mapping — operator CLI answer_on_behalf (raw), Coordinator.submit_answer (raw), the recovery sweep — loops back on prose instead of silently FAILing the task. Explicit abandon still abandons (preserves the confirmed-button path). - LOGIC-05: the Slack FREE-TEXT reply path maps with allow_abandon=False, so a bare "cancel"/"stop"/"abandon" typed in-thread → request_changes (never terminal abandon); abandon stays reachable only via the confirm-guarded button. Tests: graph unrecognized→loops-back (not FAILED), operator raw-prose→request_ changes, free-text destructive verbs→request_changes vs button→abandon, normalizer idempotency. 1484 passed. --- agent-team/agent_team/decisions.py | 107 ++++++++++++++++++ agent-team/agent_team/graph.py | 49 ++++---- .../agent_team/transport/slack_adapter.py | 76 ++++++------- .../agent_team/transport/slack_listener.py | 45 ++++++-- agent-team/tests/test_graph.py | 60 +++++++++- agent-team/tests/test_slack_adapter.py | 74 ++++++++++++ agent-team/tests/test_slack_listener.py | 18 ++- 7 files changed, 356 insertions(+), 73 deletions(-) create mode 100644 agent-team/agent_team/decisions.py diff --git a/agent-team/agent_team/decisions.py b/agent-team/agent_team/decisions.py new file mode 100644 index 0000000..7a8a205 --- /dev/null +++ b/agent-team/agent_team/decisions.py @@ -0,0 +1,107 @@ +"""Transport-neutral plan-decision normalizer (the ONE safe mapping). + +THE LOAD-BEARING B3 SAFETY MAPPING, in a single transport-neutral home so BOTH +the graph (:func:`agent_team.graph._parse_decision`) and the transports (the +Slack listener / adapter) call ONE function rather than each re-deriving the +allowlist. Keeping it here avoids a graph→transport import: the graph is the +universal chokepoint and depends only on this leaf module, and the Slack adapter +keeps a thin :func:`~agent_team.transport.slack_adapter.map_plan_decision` +wrapper that delegates here. + +A plan-gate decision can be WRITTEN by several paths — the Slack listener (which +pre-maps), but ALSO the operator CLI (``answer_on_behalf`` → ``answer_question``, +RAW) and ``Coordinator.submit_answer`` (RAW). On resume, whatever was stored +reaches the graph's decision parser. Previously the graph mapped any UNRECOGNIZED +verb to ``abandon`` → terminal FAILED, so an operator (or any non-listener +writer) answering change-request prose silently FAILED the task. Centralizing the +safe mapping here and calling it AT THE GRAPH makes every writer safe. + +``allow_abandon`` is the LOGIC-05 distinction. A destructive verb +(``cancel``/``stop``/``kill``/``reject``/``abandon``) is honoured as an abandon +ONLY when the caller vouches that the input is a *confirmed* destructive intent +— i.e. the Slack Block Kit "Abandon" button, which is guarded by a confirm +dialog. A bare destructive verb typed as FREE TEXT in a thread (``allow_abandon +=False``) must NOT terminally fail the task with no confirmation; it normalizes +to ``request_changes`` carrying the full reply as notes. +""" + +from __future__ import annotations + +from typing import Any + +__all__ = [ + "ABANDON_VERBS", + "APPROVE_VERBS", + "VALID_DECISIONS", + "normalize_decision", +] + +# Approve-allowlist: a reply that is exactly one of these (case/whitespace +# insensitive) is an approval; anything else is never an accidental approve. +APPROVE_VERBS: frozenset[str] = frozenset( + {"approve", "approved", "yes", "ok", "lgtm", "ship"} +) + +# Abandon-allowlist (DESTRUCTIVE). A bare verb here abandons the task → +# terminal FAILED, BUT ONLY when ``allow_abandon=True`` — i.e. the input is a +# CONFIRMED destructive intent (the confirm-dialog-guarded Block Kit Abandon +# button, or the graph chokepoint honouring an explicit prior decision). The +# SAME verbs typed as FREE TEXT (``allow_abandon=False``) are NOT honoured as a +# destructive abandon (LOGIC-05): they fall through to ``request_changes`` so a +# casually-typed "cancel" never terminally fails a task with no confirmation. +ABANDON_VERBS: frozenset[str] = frozenset( + {"abandon", "reject", "cancel", "stop", "kill"} +) + +# The three valid structured decisions a normalized result can carry. +VALID_DECISIONS: frozenset[str] = frozenset({"approve", "request_changes", "abandon"}) + + +def normalize_decision(raw: Any, *, allow_abandon: bool) -> dict[str, str]: + """Map a raw plan-gate reply to a structured ``{"decision","notes"}`` dict. + + The single transport-neutral normalizer. Behavior: + + * **Idempotent** — if ``raw`` is already a dict carrying a valid + ``"decision"`` in :data:`VALID_DECISIONS`, return it normalized (decision + lowercased/stripped, notes coerced to ``str``) WITHOUT re-mapping. This + lets the graph chokepoint accept an already-decided value (e.g. the + listener's pre-mapped ``request_changes`` with notes) unchanged, while a + RAW string from another writer is mapped below. An ``abandon`` dict is + honoured here regardless of ``allow_abandon`` because it is an explicit, + already-formed decision, not raw free text. + * **String** — normalize (``str`` → strip → lowercase): + * in :data:`APPROVE_VERBS` → ``approve`` (notes ``""``); + * if ``allow_abandon`` AND in :data:`ABANDON_VERBS` → ``abandon`` + (notes ``""``); + * **everything else** — prose, empty/whitespace, AND abandon-verbs when + ``allow_abandon=False`` — → ``request_changes`` carrying the FULL + ORIGINAL reply (original casing preserved) as ``notes``. This is the + safe default: arbitrary input is a change request, never an accidental + approve or (unconfirmed) abandon. + + The reply is opaque DATA throughout — never executed or interpreted beyond + this verb match. + """ + # Idempotency: an already-formed decision dict passes through normalized. + if isinstance(raw, dict): + decision = str(raw.get("decision", "") or "").strip().lower() + if decision in VALID_DECISIONS: + return { + "decision": decision, + "notes": str(raw.get("notes", "") or ""), + } + # A dict WITHOUT a recognized decision falls through to the string path + # using its stringified form (defensive; should not occur in practice). + + original = "" if raw is None else str(raw) + verb = original.strip().lower() + if verb in APPROVE_VERBS: + return {"decision": "approve", "notes": ""} + if allow_abandon and verb in ABANDON_VERBS: + return {"decision": "abandon", "notes": ""} + # Everything else → request_changes, carrying the full ORIGINAL reply (not + # the lowercased form) so the notes preserve the human's exact wording. A + # bare destructive verb typed as free text lands here when allow_abandon is + # False (LOGIC-05): it requests changes, never a silent terminal abandon. + return {"decision": "request_changes", "notes": original.strip()} diff --git a/agent-team/agent_team/graph.py b/agent-team/agent_team/graph.py index d176f8e..3615030 100644 --- a/agent-team/agent_team/graph.py +++ b/agent-team/agent_team/graph.py @@ -380,7 +380,9 @@ def plan_gate_node(state: PipelineState) -> PipelineState: the human ``notes`` folded into ``review_verdicts`` as a synthetic REQUEST_CHANGES verdict so the planner's ``_format_review_feedback`` reads it on the re-plan; - * ``abandon`` (or any unrecognized decision) -> terminal ``FAILED``. + * ``abandon`` (an explicit abandon only) -> terminal ``FAILED``. Any + UNRECOGNIZED input is mapped to ``request_changes`` by ``_parse_decision`` + (LOGIC-01/02), so it loops back rather than silently failing the task. """ prior_visits = int(state.get("plan_gate_visits", 0) or 0) @@ -441,7 +443,9 @@ def _apply_plan_decision( ``decision`` is the value passed to ``Command(resume=...)`` — the contract ``{"decision": ..., "notes": ...}``. A bare string is tolerated as the decision verb. The default for an unrecognized/empty decision is the SAFE - direction: terminal ``FAILED`` (never an accidental approve). + direction: ``request_changes`` carrying the reply as notes (never an + accidental approve, and — per LOGIC-01/02 — never a silent terminal abandon). + Only an EXPLICIT ``abandon`` reaches the terminal FAILED branch. """ verb, notes = _parse_decision(decision) now = _utc_now_iso() @@ -478,7 +482,8 @@ def _apply_plan_decision( updated_at=now, ) - # abandon (or any unrecognized decision) -> terminal FAILED. + # abandon (an EXPLICIT abandon verb only; unrecognized input was already + # mapped to request_changes by _parse_decision) -> terminal FAILED. return PipelineState( status=TaskStatus.FAILED.value, current_phase=_phase_value(Phase.PARKED), @@ -489,24 +494,30 @@ def _apply_plan_decision( def _parse_decision(decision: Any) -> tuple[str, str]: - """Normalize a resume decision into ``(verb, notes)`` (B2a). + """Normalize a resume decision into ``(verb, notes)`` (B2a, LOGIC-01/02). - Accepts the ``{"decision": ..., "notes": ...}`` mapping or a bare string. The - verb is lowercased/stripped and mapped to one of ``approve`` / - ``request_changes`` / ``abandon``; anything unrecognized maps to ``abandon`` - (the SAFE terminal direction — never an accidental approve). + The UNIVERSAL SAFE BACKSTOP. A plan-gate decision row can be written by + several paths — the Slack listener (which pre-maps), the operator CLI + (``answer_on_behalf`` → ``answer_question``, RAW), ``Coordinator.submit_answer`` + (RAW), and the recovery sweep — and whatever was stored reaches this parser + on resume. Delegating to the transport-neutral + :func:`agent_team.decisions.normalize_decision` (with ``allow_abandon=True``) + makes EVERY writer fail safe at this single chokepoint: + + * an explicit ``approve`` / ``request_changes`` / ``abandon`` (string or the + ``{"decision","notes"}`` dict) is honoured as-is (idempotent on a dict); + * an explicit ``abandon`` still abandons → terminal FAILED (preserves the + confirm-guarded Abandon BUTTON path); + * anything UNRECOGNIZED — change-request prose, an empty/blank reply, a verb + embedded in a sentence — maps to ``request_changes`` carrying the full + reply as ``notes``, NOT to ``abandon``. Previously the unrecognized case + fell to ``abandon`` → terminal FAILED, silently failing an operator (or any + non-listener writer) who answered with prose; this is the LOGIC-01/02 fix. """ - raw_decision: Any = "" - notes = "" - if isinstance(decision, dict): - raw_decision = decision.get("decision", "") - notes = str(decision.get("notes", "") or "") - else: - raw_decision = decision - verb = str(raw_decision or "").strip().lower() - if verb in {"approve", "request_changes", "abandon"}: - return verb, notes - return "abandon", notes + from agent_team.decisions import normalize_decision + + result = normalize_decision(decision, allow_abandon=True) + return result["decision"], result["notes"] def route_after_plan_gate(state: PipelineState) -> str: diff --git a/agent-team/agent_team/transport/slack_adapter.py b/agent-team/agent_team/transport/slack_adapter.py index e458625..f654da0 100644 --- a/agent-team/agent_team/transport/slack_adapter.py +++ b/agent-team/agent_team/transport/slack_adapter.py @@ -27,6 +27,15 @@ from __future__ import annotations from collections.abc import Callable, Mapping, Sequence from typing import Any +from agent_team.decisions import ( + ABANDON_VERBS as _ABANDON_VERBS, +) +from agent_team.decisions import ( + APPROVE_VERBS as _APPROVE_VERBS, +) +from agent_team.decisions import ( + normalize_decision, +) from agent_team.transport.base import ( NormalizedAnswer, QuestionSet, @@ -69,18 +78,16 @@ CALLBACK_ID_PREFIX = "shq" # can route kind-aware decisions WITHOUT importing the graph module. PLAN_DECISION_KIND = "plan_decision" -# Plan-decision verb allowlists (B3). A reply / button-value to a -# ``kind == 'plan_decision'`` question is normalized (lowercase + strip) and -# matched against these allowlists. Anything ELSE — arbitrary change-request -# prose — maps to ``request_changes`` carrying the FULL original reply as -# ``notes`` (the safe default; NEVER an accidental approve or abandon). See -# :func:`map_plan_decision`. -PLAN_DECISION_APPROVE_VERBS: frozenset[str] = frozenset( - {"approve", "approved", "yes", "ok", "lgtm", "ship"} -) -PLAN_DECISION_ABANDON_VERBS: frozenset[str] = frozenset( - {"abandon", "reject", "cancel", "stop", "kill"} -) +# Plan-decision verb allowlists (B3). Re-exported from the transport-neutral +# :mod:`agent_team.decisions` home (the single source of truth) under their +# historical names so existing importers/tests are unchanged. A reply / +# button-value to a ``kind == 'plan_decision'`` question is normalized (lowercase +# + strip) and matched against these allowlists. Anything ELSE — arbitrary +# change-request prose — maps to ``request_changes`` carrying the FULL original +# reply as ``notes`` (the safe default; NEVER an accidental approve or abandon). +# See :func:`map_plan_decision` / :func:`agent_team.decisions.normalize_decision`. +PLAN_DECISION_APPROVE_VERBS: frozenset[str] = _APPROVE_VERBS +PLAN_DECISION_ABANDON_VERBS: frozenset[str] = _ABANDON_VERBS # Block Kit action_id namespace for the three plan-gate buttons. Each carries # the decision verb; ``request_changes`` opens a modal for free-form notes. @@ -90,38 +97,31 @@ PLAN_DECISION_REQUEST_CHANGES_ACTION = f"{PLAN_DECISION_ACTION_PREFIX}:request_c PLAN_DECISION_ABANDON_ACTION = f"{PLAN_DECISION_ACTION_PREFIX}:abandon" -def map_plan_decision(raw_answer: Any) -> dict[str, str]: +def map_plan_decision(raw_answer: Any, *, allow_abandon: bool = True) -> dict[str, str]: """Map a raw plan-gate reply to a structured ``{"decision", "notes"}`` dict. - THE LOAD-BEARING B3 SAFETY MAPPING. The graph's ``_parse_decision`` maps any - unrecognized verb to ``abandon`` → terminal FAILED, so free-text change - notes (e.g. "use pytest fixtures instead") would silently FAIL the task if - they reached the graph unmapped. This normalizes BEFORE the graph sees it: + Thin wrapper over the transport-neutral + :func:`agent_team.decisions.normalize_decision` (the single source of truth), + kept so existing importers/tests are unchanged. THE LOAD-BEARING B3 SAFETY + MAPPING normalizes a reply BEFORE it reaches the graph so arbitrary change + prose becomes ``request_changes`` (carrying the full original reply as + ``notes``) rather than an accidental approve/abandon. - * normalize the text (``str`` → lowercase → strip); - * **approve** iff in :data:`PLAN_DECISION_APPROVE_VERBS` - (``approve/approved/yes/ok/lgtm/ship``), with ``notes=""``; - * **abandon** iff in :data:`PLAN_DECISION_ABANDON_VERBS` - (``abandon/reject/cancel/stop/kill``), with ``notes=""``; - * **everything else → ``request_changes`` with the FULL original reply as - ``notes``** — the safe default. Arbitrary prose is a change request, never - an accidental approve or abandon. Empty / whitespace-only input also maps - to ``request_changes`` (with empty notes): a blank reply is treated as a - benign no-op change request, never a destructive abandon. + ``allow_abandon`` (LOGIC-05) distinguishes a CONFIRMED destructive intent + from free text: + + * ``allow_abandon=True`` (the default, used by the confirm-dialog-guarded + Block Kit Abandon BUTTON and explicit-id paths) — a bare abandon-verb in + :data:`PLAN_DECISION_ABANDON_VERBS` maps to ``abandon``. + * ``allow_abandon=False`` (FREE-TEXT thread replies) — those SAME verbs are + NOT honoured as a destructive abandon; they fall through to + ``request_changes`` so a casually-typed "cancel" never terminally fails a + task with no confirmation. Only the confirmed button can abandon. The answer is opaque DATA throughout — never executed or interpreted beyond - this verb match. Returns the dict the graph's ``_parse_decision`` consumes. + the verb match. Returns the dict the graph's decision parser consumes. """ - original = "" if raw_answer is None else str(raw_answer) - verb = original.strip().lower() - if verb in PLAN_DECISION_APPROVE_VERBS: - return {"decision": "approve", "notes": ""} - if verb in PLAN_DECISION_ABANDON_VERBS: - return {"decision": "abandon", "notes": ""} - # Everything else (including empty/whitespace) → request_changes, carrying - # the full ORIGINAL reply (not the lowercased form) so the notes preserve - # the human's exact wording. Never an accidental abandon. - return {"decision": "request_changes", "notes": original.strip()} + return normalize_decision(raw_answer, allow_abandon=allow_abandon) # Type of the network seam: given the rendered Slack message kwargs, perform the diff --git a/agent-team/agent_team/transport/slack_listener.py b/agent-team/agent_team/transport/slack_listener.py index cd900b4..c37fdc1 100644 --- a/agent-team/agent_team/transport/slack_listener.py +++ b/agent-team/agent_team/transport/slack_listener.py @@ -550,13 +550,27 @@ class SlackListener: a row whose ``kind == 'plan_decision'`` — in BOTH the explicit-id and thread-reply paths — the raw answer is mapped to a structured decision ``{"decision","notes"}`` via :func:`map_plan_decision` BEFORE it reaches - the graph. Without this, the graph's decision parser maps any - unrecognized verb to ``abandon`` → terminal FAILED, so arbitrary - change-request prose ("use pytest fixtures instead") would silently FAIL - the task instead of requesting changes. ``approve``/``abandon`` map to - the named verb; EVERYTHING else maps to ``request_changes`` carrying the - full original reply as notes (never an accidental abandon). A - ``clarify`` row passes through UNCHANGED (its free text IS the answer). + the graph, with ``approve`` mapped to the named verb and EVERYTHING + non-verb mapped to ``request_changes`` carrying the full original reply + as notes. A ``clarify`` row passes through UNCHANGED (its free text IS + the answer). + + FREE-TEXT vs BUTTON ABANDON (LOGIC-05). The two paths differ ONLY in + whether a destructive verb may abandon: + + * **explicit-id path** (the confirm-dialog-guarded Block Kit BUTTONS and + the request-changes modal) calls the mapper with ``allow_abandon=True`` + — a confirmed Abandon button honours the ``abandon`` verb; + * **thread-reply path** (raw free text) calls it with + ``allow_abandon=False`` — a bare destructive verb typed in-thread + ("cancel"/"abandon"/...) maps to ``request_changes``, NOT a terminal + abandon, so a casually-typed word never silently FAILS a task. Only the + confirmed button can abandon. + + Without this normalization the graph's parser would still fail safe + (it maps unrecognized → request_changes), but pre-mapping at the + transport keeps the listener's free-text guard authoritative for the + confirm distinction. Returns the payload to submit, or ``None`` when no question can be resolved (the caller ignores the event without crashing). Re-raises @@ -576,9 +590,13 @@ class SlackListener: # the {"decision","notes"} dict; otherwise pass through unchanged. kind = self._question_kind(conn, question_id) if kind == PLAN_DECISION_KIND: + # Explicit-id path = the confirm-guarded Block Kit BUTTONS + # (Approve / Abandon — Abandon behind a confirm dialog) and the + # request-changes modal submit. These are CONFIRMED destructive + # intents, so an abandon verb is honoured: allow_abandon=True. return { "question_id": question_id, - "answer": map_plan_decision(answer), + "answer": map_plan_decision(answer, allow_abandon=True), } return raw_payload @@ -605,13 +623,20 @@ class SlackListener: text = event.get("text") answer = text.strip() if isinstance(text, str) else text - # Kind-aware normalization (B3): a free-text reply to a plan-decision row + # Kind-aware normalization (B3): a FREE-TEXT reply to a plan-decision row # — arbitrary change-request prose — is mapped to a structured decision # (request_changes carrying the full reply as notes; NEVER an accidental # abandon). A clarify row's free text passes through unchanged: the text # IS the answer. + # + # LOGIC-05: free text NEVER abandons. ``allow_abandon=False`` so a bare + # destructive verb typed in-thread ("cancel"/"stop"/"abandon"/...) maps + # to request_changes (carrying that text as notes), NOT a terminal + # abandon. Only the confirm-dialog-guarded Block Kit Abandon BUTTON + # (the explicit-id path above, allow_abandon=True) can terminally + # abandon the task. if kind == PLAN_DECISION_KIND: - answer = map_plan_decision(answer) + answer = map_plan_decision(answer, allow_abandon=False) # Synthesize an explicit payload the transport already understands # (bare question_id + answer). The answer is opaque DATA — stored # verbatim as JSON by the responder, never interpreted. diff --git a/agent-team/tests/test_graph.py b/agent-team/tests/test_graph.py index 52fa8ae..d7a13ae 100644 --- a/agent-team/tests/test_graph.py +++ b/agent-team/tests/test_graph.py @@ -656,13 +656,67 @@ def test_plan_gate_abandon_fails(restore_review_invoker) -> None: assert final["status"] == TaskStatus.FAILED.value -def test_plan_gate_unrecognized_decision_fails_safe(restore_review_invoker) -> None: - # An unrecognized decision must NOT accidentally approve — it fails terminal. +def test_plan_gate_unrecognized_decision_loops_back_not_failed( + restore_review_invoker, +) -> None: + # LOGIC-01/02: an UNRECOGNIZED decision must NOT silently FAIL the task (the + # old behavior). The graph backstop maps it to request_changes — it loops + # back to the planner (and, with a reviewer that never approves, re-suspends + # on the gate), NEVER terminal FAILED, and NEVER an accidental approve. + from agent_team.graph import pending_question + graph = _plan_gate_graph("VERDICT: REQUEST CHANGES\nnope") thread_id, _ = _drive_to_plan_gate(graph) final = resume_task(graph, thread_id=thread_id, answer={"decision": "huh?"}) - assert final["status"] == TaskStatus.FAILED.value + assert final["status"] != TaskStatus.FAILED.value + # request_changes loops plan->review and re-suspends on the gate (reviewer + # never approves): the task is still pending a human decision, not terminal. + assert pending_question(graph, thread_id=thread_id) is not None + + +def test_plan_gate_operator_raw_prose_loops_back_not_failed( + restore_review_invoker, +) -> None: + # LOGIC-01/02 (operator-CLI-style RAW write): a non-listener writer (operator + # CLI / Coordinator.submit_answer) stores RAW change-request prose on a + # plan_decision row — it never passes through the Slack listener's pre-map. + # The graph backstop (_parse_decision via normalize_decision) maps it to + # request_changes carrying the full prose as notes, so it loops back to the + # planner rather than the old silent abandon -> terminal FAILED. + from agent_team.graph import pending_question + from agent_team.nodes.planner import _format_review_feedback + + captured: dict[str, object] = {} + + def capturing_plan(state): + captured["feedback"] = _format_review_feedback( + list(state.get("review_verdicts") or []) + ) + return _p2_plan_stub(state) + + from agent_team.nodes import review_loop + + review_loop.set_review_invoker(lambda prompt, **kw: "VERDICT: REQUEST CHANGES\nno") + graph = build_graph( + checkpointer=_Saver(), + live_plan_node=capturing_plan, + review_node=review_loop.bind_review_node(), + route_review=review_loop.route_after_review, + plan_gate=True, + ) + thread_id, _ = _drive_to_plan_gate(graph) + + # A bare RAW string (exactly what answer_question stores for an operator who + # typed prose) resumes the gate. It must NOT fail the task. + final = resume_task( + graph, thread_id=thread_id, answer="please use pytest fixtures instead" + ) + + assert final["status"] != TaskStatus.FAILED.value + assert pending_question(graph, thread_id=thread_id) is not None + # The raw prose was folded into the re-plan as request_changes notes. + assert "pytest fixtures" in str(captured.get("feedback", "")) def test_plan_gate_request_changes_terminates_at_ceiling( diff --git a/agent-team/tests/test_slack_adapter.py b/agent-team/tests/test_slack_adapter.py index 4d10e2e..449b790 100644 --- a/agent-team/tests/test_slack_adapter.py +++ b/agent-team/tests/test_slack_adapter.py @@ -481,6 +481,80 @@ def test_map_plan_decision_verb_with_trailing_text_is_request_changes() -> None: assert result["notes"] == raw +# --------------------------------------------------------------------------- # +# normalize_decision — shared transport-neutral normalizer (decisions.py) # +# --------------------------------------------------------------------------- # + + +@pytest.mark.parametrize("verb", ["abandon", "reject", "cancel", "stop", "kill"]) +def test_normalize_decision_free_text_destructive_verb_not_abandon(verb: str) -> None: + """LOGIC-05: with allow_abandon=False, destructive verbs -> request_changes. + + A bare destructive verb (the free-text path) is NOT a confirmed abandon; it + carries the verb as notes and requests changes instead of terminally + failing. + """ + from agent_team.decisions import normalize_decision + + result = normalize_decision(verb, allow_abandon=False) + assert result["decision"] == "request_changes" + assert result["decision"] != "abandon" + assert result["notes"] == verb + + +@pytest.mark.parametrize("verb", ["abandon", "reject", "cancel", "stop", "kill"]) +def test_normalize_decision_button_destructive_verb_abandons(verb: str) -> None: + """With allow_abandon=True (confirmed button), destructive verbs abandon.""" + from agent_team.decisions import normalize_decision + + result = normalize_decision(verb, allow_abandon=True) + assert result == {"decision": "abandon", "notes": ""} + + +@pytest.mark.parametrize( + "decision", + [ + {"decision": "approve", "notes": ""}, + {"decision": "request_changes", "notes": "do X"}, + {"decision": "abandon", "notes": ""}, + ], +) +@pytest.mark.parametrize("allow_abandon", [True, False]) +def test_normalize_decision_idempotent_on_decision_dict( + decision: dict, allow_abandon: bool +) -> None: + """An already-formed valid decision dict passes through normalized, un-remapped. + + Idempotency: re-normalizing a dict that already carries a valid decision + returns it unchanged (notes preserved). An explicit abandon dict is honoured + even with allow_abandon=False — it is an explicit decision, not free text. + """ + from agent_team.decisions import normalize_decision + + result = normalize_decision(decision, allow_abandon=allow_abandon) + assert result == decision + # Double-application is stable. + assert normalize_decision(result, allow_abandon=allow_abandon) == decision + + +def test_normalize_decision_idempotent_normalizes_casing() -> None: + """A decision dict with odd casing/whitespace is normalized (not re-mapped).""" + from agent_team.decisions import normalize_decision + + result = normalize_decision( + {"decision": " Request_Changes ", "notes": "Keep Casing"}, + allow_abandon=True, + ) + assert result == {"decision": "request_changes", "notes": "Keep Casing"} + + +def test_map_plan_decision_free_text_abandon_verb_requests_changes() -> None: + """The wrapper threads allow_abandon: free-text 'cancel' -> request_changes.""" + result = map_plan_decision("cancel", allow_abandon=False) + assert result["decision"] == "request_changes" + assert result["notes"] == "cancel" + + # --------------------------------------------------------------------------- # # B3 — gate decision blocks (the three buttons) # # --------------------------------------------------------------------------- # diff --git a/agent-team/tests/test_slack_listener.py b/agent-team/tests/test_slack_listener.py index ed1e7a1..9550703 100644 --- a/agent-team/tests/test_slack_listener.py +++ b/agent-team/tests/test_slack_listener.py @@ -960,17 +960,29 @@ def test_plan_decision_free_text_approve_maps_to_approve(db_path: Path) -> None: assert answer == {"decision": "approve", "notes": ""} -def test_plan_decision_free_text_abandon_maps_to_abandon(db_path: Path) -> None: +@pytest.mark.parametrize("verb", ["cancel", "stop", "abandon", "kill", "reject"]) +def test_plan_decision_free_text_destructive_verb_maps_to_request_changes( + db_path: Path, verb: str +) -> None: + """LOGIC-05: a bare destructive verb typed as FREE TEXT must NOT abandon. + + The Block Kit Abandon button is guarded by a confirm dialog; a casually-typed + "cancel"/"stop"/"abandon" in a thread is NOT a confirmed destructive intent. + It must map to request_changes (carrying the verb as notes), never a terminal + abandon. Only the confirmed button path (allow_abandon=True) abandons. + """ _seed_plan_decision_question( db_path, question_id="q1", channel_ref=SEED_CHANNEL_REF ) queue = RecordingQueue() listener = _listener(db_path, queue, owner_ids={OWNER_ID}) - listener.handle_event(_events_api_reply(text="cancel", thread_ts=SEED_CHANNEL_REF)) + listener.handle_event(_events_api_reply(text=verb, thread_ts=SEED_CHANNEL_REF)) answer = queue.jobs[0].answer - assert answer == {"decision": "abandon", "notes": ""} + assert answer["decision"] == "request_changes" + assert answer["decision"] != "abandon" + assert answer["notes"] == verb def test_clarify_free_text_passes_through_unchanged_regression(db_path: Path) -> None: From 42f2438d0c25c1c2e47bd37814c4d8694daa106f Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 24 Jun 2026 11:53:12 -0400 Subject: [PATCH 09/11] fix(agent-team): make planner convergence cap count correctly (LOGIC-03) _revision_count read verdict.get("decision"), but verdicts are keyed "verdict" (both reviewer and synthetic human-gate), so the count was always 0 and the plan_node MAX_PLAN_REVISIONS self-park was dead code. Now reads "verdict" first (fallback "decision"), matching _format_review_feedback's precedence; the existing .strip().upper()==_REQUEST_CHANGES compare covers both request_changes and REQUEST_CHANGES. Counts reviewer + human request_changes. Cap composition: the review-loop round cap and MAX_PLAN_GATE_VISITS govern the live loops; the planner MAX_PLAN_REVISIONS is now a correct backstop (was inert), not a behavior change to the gate. Tests drive the real "verdict" key and prove the previously-dead park fires. 1487 passed. --- agent-team/agent_team/nodes/planner.py | 13 +++++-- agent-team/tests/test_planner.py | 48 +++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/agent-team/agent_team/nodes/planner.py b/agent-team/agent_team/nodes/planner.py index 8118590..ae7d9a3 100644 --- a/agent-team/agent_team/nodes/planner.py +++ b/agent-team/agent_team/nodes/planner.py @@ -289,9 +289,16 @@ def _revision_count(state: PipelineState) -> int: """ count = 0 for verdict in state.get("review_verdicts", []): - decision = ( - verdict.get("decision") if isinstance(verdict, dict) else str(verdict) - ) + if isinstance(verdict, dict): + # Mirror _format_review_feedback's precedence: real verdicts (both + # review_loop.ReviewResult.to_dict and graph._apply_plan_decision's + # synthetic human-gate verdict) write the decision under "verdict"; + # "decision" never existed on a real verdict and is kept only as a + # defensive fallback. Reading "decision" alone is why this counter + # was always 0 (LOGIC-03). + decision = verdict.get("verdict") or verdict.get("decision") + else: + decision = str(verdict) if isinstance(decision, str) and decision.strip().upper() == _REQUEST_CHANGES: count += 1 return count diff --git a/agent-team/tests/test_planner.py b/agent-team/tests/test_planner.py index 6e670c1..bf208cc 100644 --- a/agent-team/tests/test_planner.py +++ b/agent-team/tests/test_planner.py @@ -12,6 +12,7 @@ from agent_team.billing import BillingMode, ClaudeResult from agent_team.nodes.planner import ( MAX_PLAN_REVISIONS, PlannerError, + _revision_count, build_plan_prompt, parse_plan, plan_node, @@ -256,11 +257,43 @@ def test_plan_node_garbled_reply_raises() -> None: # --------------------------------------------------------------------------- # +def test_revision_count_reads_verdict_key_real_producer_shape() -> None: + # LOGIC-03: real verdicts (review_loop.ReviewResult.to_dict and the synthetic + # human-gate verdict) write the decision under "verdict" with the lowercase + # value "request_changes". The counter must read THAT key, not "decision" + # (which never exists on a real verdict and made this counter always 0). + verdicts: list[Any] = [ + {"verdict": "request_changes", "findings": "fix it"}, + {"verdict": "request_changes", "reviewer": "human_plan_gate"}, + ] + assert _revision_count(_state(review_verdicts=verdicts)) == 2 + + +def test_revision_count_matches_request_changes_case_insensitively() -> None: + # Both the canonical "request_changes" (reviewer/human producers) and the + # legacy "REQUEST_CHANGES" token are counted, regardless of case; approve and + # other verdicts are ignored. + verdicts: list[Any] = [ + {"verdict": "request_changes"}, # canonical lowercase + {"verdict": "REQUEST_CHANGES"}, # legacy uppercase token + {"verdict": "Request_Changes"}, # mixed case + {"verdict": "approve"}, # must NOT count + {"verdict": "something_else"}, # must NOT count + {"decision": "request_changes"}, # defensive fallback key still counts + ] + assert _revision_count(_state(review_verdicts=verdicts)) == 4 + + +def test_revision_count_ignores_non_request_changes() -> None: + verdicts: list[Any] = [{"verdict": "approve"}] * 5 + assert _revision_count(_state(review_verdicts=verdicts)) == 0 + + def test_plan_node_replans_on_loopback_and_counts_revision() -> None: _bind_invoker(json.dumps(_VALID_PLAN)) state = _state( plan={"task": "x"}, - review_verdicts=[{"decision": "REQUEST_CHANGES", "notes": "fix it"}], + review_verdicts=[{"verdict": "request_changes", "findings": "fix it"}], ) out = plan_node(state) assert out["current_phase"] == Phase.REVIEW.value @@ -268,10 +301,15 @@ def test_plan_node_replans_on_loopback_and_counts_revision() -> None: def test_plan_node_parks_after_max_revisions() -> None: - # Invoker bound but must NOT be called once we are over the bound. + # Convergence park (§3.3): once the plan has been sent back + # MAX_PLAN_REVISIONS times the planner parks instead of burning more budget. + # This was DEAD CODE before the LOGIC-03 fix: _revision_count read "decision" + # while real verdicts carry "request_changes" under "verdict", so the count + # was always 0 and this branch never fired in production. Drive it with the + # REAL verdict shape to prove the park is now live. calls = _bind_invoker(json.dumps(_VALID_PLAN)) verdicts = [ - {"decision": "REQUEST_CHANGES", "notes": f"round {i}"} + {"verdict": "request_changes", "findings": f"round {i}"} for i in range(MAX_PLAN_REVISIONS) ] out = plan_node(_state(plan={"task": "x"}, review_verdicts=verdicts)) @@ -284,7 +322,7 @@ def test_plan_node_parks_after_max_revisions() -> None: def test_plan_node_does_not_park_just_below_bound() -> None: _bind_invoker(json.dumps(_VALID_PLAN)) verdicts = [ - {"decision": "REQUEST_CHANGES", "notes": f"round {i}"} + {"verdict": "request_changes", "findings": f"round {i}"} for i in range(MAX_PLAN_REVISIONS - 1) ] out = plan_node(_state(plan={"task": "x"}, review_verdicts=verdicts)) @@ -295,7 +333,7 @@ def test_plan_node_does_not_park_just_below_bound() -> None: def test_plan_node_ignores_non_request_changes_verdicts_for_bound() -> None: # APPROVE/other verdicts must not count toward the park bound. _bind_invoker(json.dumps(_VALID_PLAN)) - verdicts = [{"decision": "APPROVE"}] * (MAX_PLAN_REVISIONS + 2) + verdicts = [{"verdict": "approve"}] * (MAX_PLAN_REVISIONS + 2) out = plan_node(_state(plan={"task": "x"}, review_verdicts=verdicts)) assert out["current_phase"] == Phase.REVIEW.value assert out["plan"]["revision"] == 0 From 71edeb3f3f0969de9ae2c2bd7401785d8ca6704b Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 24 Jun 2026 11:57:03 -0400 Subject: [PATCH 10/11] fix(agent-team): review-round cap counts only reviewer verdicts (LOGIC-04) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _review_round_index counted EVERY review_verdicts entry, including the synthetic human-gate verdict graph._apply_plan_decision folds in on a "request changes" (reviewer == "human_plan_gate"). That inflated the count so a revised plan could escalate prematurely without a fresh adversarial review. Now counts only reviewer-authored verdicts: a new _is_reviewer_verdict excludes entries tagged reviewer=="human_plan_gate" (read from the verdict dict's own field — no graph.py import). A human request_changes now grants the revised plan a fresh reviewer-round budget. Termination still bounded by MAX_PLAN_GATE_VISITS (each request_changes consumes one gate visit). 1490 passed. --- agent-team/agent_team/nodes/review_loop.py | 37 ++++++++++++-- agent-team/tests/test_review_loop.py | 58 ++++++++++++++++++++++ 2 files changed, 92 insertions(+), 3 deletions(-) diff --git a/agent-team/agent_team/nodes/review_loop.py b/agent-team/agent_team/nodes/review_loop.py index d61e959..aa1061d 100644 --- a/agent-team/agent_team/nodes/review_loop.py +++ b/agent-team/agent_team/nodes/review_loop.py @@ -75,6 +75,17 @@ PARKED_NODE = "parked" # per task via config["max_review_rounds"]. DEFAULT_MAX_REVIEW_ROUNDS = 3 +# ``reviewer`` tag on the SYNTHETIC verdict that graph._apply_plan_decision folds +# into ``review_verdicts`` when the owner picks "request changes" at the plan +# gate. Those are HUMAN-gate verdicts, not adversarial reviewer rounds, so they +# must NOT count toward the review-round cap (LOGIC-04): a human request_changes +# should grant the revised plan a fresh review-round budget. We match on the +# verdict dict's ``reviewer`` field (data already present in ``review_verdicts``) +# rather than importing graph.py, to avoid a layering cycle. The plan-review GATE +# is independently bounded by graph.MAX_PLAN_GATE_VISITS, so excluding these from +# the REVIEW cap cannot create an unbounded loop. +HUMAN_PLAN_GATE_REVIEWER = "human_plan_gate" + # Config / env key naming the per-task review-round cap. _MAX_ROUNDS_CONFIG_KEY = "max_review_rounds" _MAX_ROUNDS_ENV = "AGENT_TEAM_MAX_REVIEW_ROUNDS" @@ -371,14 +382,34 @@ def build_review_prompt(state: PipelineState) -> str: ) +def _is_reviewer_verdict(entry: Any) -> bool: + """True if ``entry`` is an adversarial-REVIEWER verdict (not a human-gate one). + + The plan gate folds SYNTHETIC verdicts into ``review_verdicts`` tagged + ``reviewer == "human_plan_gate"`` (graph._apply_plan_decision) when the owner + requests changes. Those are human decisions, not reviewer rounds, so they are + excluded from the round count (LOGIC-04). Any entry without that tag — every + real reviewer verdict this node appends — counts as a reviewer round. + """ + if isinstance(entry, Mapping): + return entry.get("reviewer") != HUMAN_PLAN_GATE_REVIEWER + return True + + def _review_round_index(state: PipelineState) -> int: """Return the 1-based index of the review round about to run. - Counts only prior *review* verdict entries already in ``review_verdicts`` - (entries this node appended), so a loop-back/re-entry increments correctly. + Counts only prior *reviewer* verdict entries already in ``review_verdicts`` + (entries this node appended), EXCLUDING the synthetic ``human_plan_gate`` + verdicts the plan gate folds in on a human request_changes (LOGIC-04). So a + reviewer loop-back/re-entry increments correctly, while a human request_changes + grants the revised plan a fresh review-round budget. Termination is still + guaranteed: each human request_changes consumes one of the finite + graph.MAX_PLAN_GATE_VISITS gate visits. """ prior = state.get("review_verdicts") or [] - return len(prior) + 1 + reviewer_rounds = sum(1 for entry in prior if _is_reviewer_verdict(entry)) + return reviewer_rounds + 1 def review_node( diff --git a/agent-team/tests/test_review_loop.py b/agent-team/tests/test_review_loop.py index 5870d54..e060d42 100644 --- a/agent-team/tests/test_review_loop.py +++ b/agent-team/tests/test_review_loop.py @@ -223,6 +223,64 @@ def test_review_node_appends_to_prior_verdicts() -> None: assert update["review_verdicts"][-1]["round_index"] == 2 +# --------------------------------------------------------------------------- # +# review_node — human-gate verdicts excluded from the round cap (LOGIC-04) +# --------------------------------------------------------------------------- # + + +def test_review_round_index_excludes_human_plan_gate_verdicts() -> None: + # A human request_changes folds a synthetic verdict tagged + # reviewer == "human_plan_gate" into review_verdicts. It must NOT count as a + # reviewer round: with one reviewer round + two human-gate verdicts present, + # the next reviewer round is still round 2 (not round 4). + state = _state( + review_verdicts=[ + {"verdict": "request_changes", "outcome": "loop_back"}, + {"verdict": "request_changes", "reviewer": "human_plan_gate"}, + {"verdict": "request_changes", "reviewer": "human_plan_gate"}, + ] + ) + assert review_loop._review_round_index(state) == 2 + + +def test_review_node_human_gate_verdict_grants_fresh_round() -> None: + # After a human request_changes (synthetic human_plan_gate verdict), the + # revised plan's review gets a fresh round rather than immediate escalation, + # even though len(review_verdicts) would otherwise be at the cap. + set_review_invoker(_invoker_returning("VERDICT: REQUEST CHANGES\nstill rough")) + state = _state( + review_verdicts=[ + {"verdict": "request_changes", "outcome": "loop_back"}, # reviewer r1 + {"verdict": "request_changes", "outcome": "loop_back"}, # reviewer r2 + {"verdict": "request_changes", "reviewer": "human_plan_gate"}, # human + ] + ) + update = review_node(state, config={"max_review_rounds": 3}) + + # Only 2 reviewer rounds counted, so this is reviewer round 3 == cap: it + # escalates here, NOT one round early because of the human verdict. + last = update["review_verdicts"][-1] + assert last["round_index"] == 3 + assert last["outcome"] == ReviewOutcome.ESCALATE.value + + +def test_review_node_only_human_verdicts_is_fresh_first_round() -> None: + # With NO prior reviewer verdicts but a human-gate verdict present, the very + # first reviewer pass is round 1 and loops back under the cap (the bug made + # this escalate immediately at cap 1). + set_review_invoker(_invoker_returning("VERDICT: REQUEST CHANGES\nredo")) + state = _state( + review_verdicts=[ + {"verdict": "request_changes", "reviewer": "human_plan_gate"}, + ] + ) + update = review_node(state, config={"max_review_rounds": 2}) + + assert update["current_phase"] == Phase.PLAN.value + assert update["status"] == TaskStatus.ACTIVE.value + assert update["review_verdicts"][-1]["round_index"] == 1 + + # --------------------------------------------------------------------------- # # review_node — invoker wiring & errors # --------------------------------------------------------------------------- # From 3ebeacdf3168f25572b3ea3c9f44ce18219a8db3 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 24 Jun 2026 12:18:22 -0400 Subject: [PATCH 11/11] fix(agent-team): init_db drives migrate() so the version stamp actually advances MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit init_db's own schema_meta write was ON CONFLICT DO NOTHING, and the daemon (Coordinator.setup) calls init_db, never migrate() — so on an existing ledger the column was ensured but schema_version was never advanced (observed live: kind column present, schema_meta stuck at 3). migrate() already upserts the version correctly but was effectively dead code (no production caller). init_db now ends by calling migrate(conn), which steps the version and runs any version-gated steps. Idempotent — re-running the create/ensure statements is harmless. Regression test: an existing v3-stamped DB run through init_db now reports schema_version == SCHEMA_VERSION (4) and has the kind column. 1491 passed. --- agent-team/agent_team/db/schema.py | 17 ++++++++------- agent-team/tests/test_schema.py | 33 ++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/agent-team/agent_team/db/schema.py b/agent-team/agent_team/db/schema.py index d354753..a569a55 100644 --- a/agent-team/agent_team/db/schema.py +++ b/agent-team/agent_team/db/schema.py @@ -274,15 +274,14 @@ def init_db(db_path: Path) -> None: conn.execute(TASK_TRANSITIONS_DDL) for stmt in _split_statements(TASK_TRANSITIONS_INDEXES_DDL): conn.execute(stmt) - # Record the schema version (single-row table). DO NOTHING leaves an - # existing row's version untouched (an already-stamped DB just gains any - # IF-NOT-EXISTS tables above); migrate() is what steps the version stamp - # forward on an existing DB. - conn.execute( - "INSERT INTO schema_meta (id, schema_version) VALUES (1, ?) " - "ON CONFLICT(id) DO NOTHING", - (SCHEMA_VERSION,), - ) + # Step the version stamp forward AND apply any version-gated migrations. + # init_db is the only schema entry point the daemon calls (Coordinator. + # setup -> init_db), so it MUST drive migrate() — otherwise an existing + # DB's schema_version is never advanced (migrate() upserts it; init_db's + # own writes do not) and version-gated steps in migrate() never run in + # production. migrate() is idempotent, so re-running the create/ensure + # statements above is harmless. + migrate(conn) finally: conn.close() diff --git a/agent-team/tests/test_schema.py b/agent-team/tests/test_schema.py index 3904b68..2bcdb2a 100644 --- a/agent-team/tests/test_schema.py +++ b/agent-team/tests/test_schema.py @@ -696,6 +696,39 @@ def test_migrate_helper_adds_kind_to_legacy_db(tmp_path: Path) -> None: assert ver == SCHEMA_VERSION +def test_init_db_advances_existing_version_stamp(tmp_path: Path) -> None: + """init_db (the daemon's only schema entry point) bumps a stale version stamp. + + Regression: init_db's own schema_meta write was ON CONFLICT DO NOTHING, so an + already-stamped DB (e.g. an old v3 ledger) kept its stale version forever — + the daemon calls init_db, never migrate(), so the stamp never advanced even + though the column was ensured. init_db now drives migrate(), which upserts. + """ + db = tmp_path / "stale.sqlite" + conn = connect(db) + try: + conn.execute(_LEGACY_PENDING_QUESTIONS_DDL) + conn.execute( + "CREATE TABLE IF NOT EXISTS schema_meta " + "(id INTEGER PRIMARY KEY CHECK (id = 1), schema_version INTEGER NOT NULL)" + ) + conn.execute("INSERT INTO schema_meta (id, schema_version) VALUES (1, 3)") + finally: + conn.close() + + init_db(db) + + conn = connect(db) + try: + ver = conn.execute( + "SELECT schema_version FROM schema_meta WHERE id = 1" + ).fetchone()[0] + assert "kind" in _pq_columns(conn) + finally: + conn.close() + assert ver == SCHEMA_VERSION + + def test_kind_plan_decision_round_trips(tmp_path: Path) -> None: """A row written with kind='plan_decision' round-trips; default is 'clarify'.""" db = tmp_path / "db.sqlite"