agent-team/DEPLOY-R720.md

@@ -0,0 +1,202 @@
+# P1 - agent-team Plane-2 coordinator deployment (R720 VM)
+
+Status: **P1 DEPLOY ARTIFACTS** - the systemd unit + this runbook. The coordinator
+daemon (`run-team.py serve`) is built by a separate agent; this file is the
+operator runbook for standing it up on the always-on R720 VM.
+
+The agent-team coordinator is the long-running Plane-2 brain: it drives the
+LangGraph pipeline, owns the durable `pending_questions` ledger, and runs the
+Slack Socket Mode inbound listener that receives clarifier answers. It shares the
+`sh-secrev` VM and the `~/secrev.env` secrets file with the Path B security sweep
+(see `../security-review/DEPLOY-R720.md`), but it is a **service** (always-on),
+not a timer-driven oneshot.
+
+## Host
+
+- **Hypervisor:** R720 at `10.10.60.40` (Windows Server 2022, Hyper-V role).
+- **VM:** `sh-secrev`, always-on Ubuntu 24.04, Gen2, **4GB / 2 vCPU / 40GB**
+  dynamic vhdx at `10.10.60.120`.
+- **Reach it:** `ssh -i ~/.ssh/r720_seahaven adam@10.10.60.120` (key-only,
+  NOPASSWD sudo).
+
+Operate on the VM, not from the Mac against the host by hand.
+
+## 1. SNAPSHOT FIRST
+
+**Standing rule: snapshot the VM before any provisioning change.** This box is a
+4GB / 2 vCPU / 40GB VM at `10.10.60.120`. Take a Hyper-V checkpoint on the R720
+host **before** you install pip deps, the unit, or touch `~/secrev.env`, so the
+whole change is one-command reversible (see ROLLBACK). Do not skip this because
+"it is only a pip install" - a bad dep set or a wedged service is exactly what
+the snapshot exists to undo.
+
+## 2. Prereqs on the VM
+
+Already present from the secrev deploy:
+
+- **Python 3** (3.12) and the `claude` CLI (Node) - the subscription-auth path.
+- **Repo:** `~/orchestrator/` (rsync from the Mac, NOT a git clone). The
+  agent-team package lives at `~/orchestrator/agent-team/`.
+
+New for the coordinator - a dedicated venv under `agent-team/.venv` (excluded
+from rsync) with the coordinator/transport deps:
+
+| pip dep | Why |
+|---|---|
+| `langgraph` | the coordinator pipeline graph |
+| `langgraph-checkpoint-sqlite` | `SqliteSaver` checkpointer against the ledger DB |
+| `claude-agent-sdk` | subscription-auth Claude invocation seam |
+| `slack_sdk` | Slack Web API (post questions, `chat:write`) |
+| `slack_bolt` | Socket Mode inbound listener (receive answers) |
+
+The ledger DB defaults to `agent-team/state/agent_team.sqlite`; the audit log to
+`agent-team/state/audit.log.jsonl`. Both live under `state/` (gitignored,
+never committed).
+
+## 3. Secrets - append to `~/secrev.env` (mode 600, never committed)
+
+The coordinator reads its secrets from the same `~/secrev.env` the secrev sweep
+uses. Append these (do not echo them into shell history files; lock the file
+down after):
+
+```
+echo 'CLAUDE_CODE_OAUTH_TOKEN=...'   >> ~/secrev.env   # from `claude setup-token`
+echo 'SLACK_BOT_TOKEN=xoxb-...'      >> ~/secrev.env   # bot token, chat:write
+echo 'SLACK_APP_TOKEN=xapp-...'      >> ~/secrev.env   # app-level, connections:write (Socket Mode)
+echo 'SLACK_CHANNEL_ID=C0XXXXXXX'    >> ~/secrev.env   # target clarifier channel
+echo 'AGENT_TEAM_SLACK_OWNER_IDS=U0XXXXXXX'  >> ~/secrev.env   # authorized answerer(s), comma-separated
+chmod 600 ~/secrev.env
+```
+
+- `CLAUDE_CODE_OAUTH_TOKEN` - subscription OAuth from `claude setup-token`. The
+  same token type the secrev sweep uses.
+- `SLACK_BOT_TOKEN` (`xoxb-`) - bot token with `chat:write`; posts questions.
+- `SLACK_APP_TOKEN` (`xapp-`) - app-level token with `connections:write`;
+  **required for Socket Mode** (opens the inbound WebSocket that hears answers).
+- `SLACK_CHANNEL_ID` - the channel id the coordinator posts clarifiers to.
+- `AGENT_TEAM_SLACK_OWNER_IDS` - comma-separated Slack **user ids** of the
+  authorized answerers (e.g. Adam's `U…` id). The inbound listener enforces this
+  as an owner allowlist (AUTHZ-01): only a sender in this set may answer/steer
+  the pipeline. **The listener fails closed** - if this is unset/empty it rejects
+  **every** answer (logs a warning naming `AGENT_TEAM_SLACK_OWNER_IDS`), so it
+  must be set for the human gate to function. Look up your user id via Slack
+  profile → "Copy member ID", or the `users.identity` / `auth.test` API.
+
+**CRITICAL:** `ANTHROPIC_API_KEY` must **NOT** be set on this host. A raw API key
+would silently win over the subscription OAuth and meter to API rates. The box
+runs on subscription OAuth only.
+
+## 4. Deploy steps
+
+```
+# 4a. From the Mac - rsync the repo (same pattern/excludes as secrev):
+rsync -av --exclude .env --exclude .venv \
+  ~/Documents/repositories/orchestrator/ adam@10.10.60.120:orchestrator/
+
+# 4b. On the VM - create + activate the agent-team venv and install deps:
+ssh -i ~/.ssh/r720_seahaven adam@10.10.60.120
+cd ~/orchestrator/agent-team
+python3 -m venv .venv
+. .venv/bin/activate
+pip install langgraph langgraph-checkpoint-sqlite claude-agent-sdk slack_sdk slack_bolt
+
+# 4c. Initialize the durable ledger DB (idempotent; creates state/agent_team.sqlite):
+python3 run-team.py init-db
+
+# 4d. Install + start the service:
+sudo cp systemd/agent-team-coordinator.service /etc/systemd/system/
+sudo systemctl daemon-reload
+sudo systemctl enable --now agent-team-coordinator.service
+
+# 4e. Verify it is up:
+systemctl status agent-team-coordinator.service
+journalctl -u agent-team-coordinator.service -e -f
+```
+
+The unit runs `python3 run-team.py serve` from
+`WorkingDirectory=/home/adam/orchestrator/agent-team` as `User=adam`, loading
+secrets from `EnvironmentFile=/home/adam/secrev.env`. `Restart=on-failure` keeps
+it up across transient faults; `journalctl -u` is the live log.
+
+## 5. P1 live exit-criteria demo (§3.3.1)
+
+Demonstrate all four once the service is live. Map each to the operator commands
+(`run-team.py list / show / force-resume`, `systemctl`). Run the CLI from the
+working dir so it hits the default ledger: `cd ~/orchestrator/agent-team`.
+
+**(a) Crash-safe resume - kill mid-wait, restart, task resumes.**
+Start a task, get it to a clarifier wait (`run-team.py list` shows an `open`
+question), then:
+```
+sudo systemctl stop agent-team-coordinator.service
+sudo systemctl start agent-team-coordinator.service
+journalctl -u agent-team-coordinator.service -e   # confirm the task resumes from the ledger/checkpoint
+run-team.py show <question_id>                     # the question is still open, not lost
+```
+Pass: the task picks up the same waiting question after restart (the LangGraph
+`SqliteSaver` checkpoint + the durable ledger survive the kill).
+
+**(b) Duplicate Slack answer is a no-op.**
+Answer a question in Slack, then answer the **same** question again.
+```
+run-team.py show <question_id>   # status flipped to answered exactly once; answered_via is the first answer
+```
+Pass: the first answer wins (`rowcount == 1`); the duplicate hits the
+`BEGIN IMMEDIATE` compare-and-set and is ignored (`rowcount == 0`) - no second
+resume, no error.
+
+**(c) Past-deadline answer is rejected + the task parks.**
+Let a question's `deadline_at` pass with no answer, then answer late.
+```
+run-team.py show <question_id>           # status == expired (auto-expired at deadline)
+run-team.py list --parked                # the now-parked task surfaces here
+```
+Pass: the expired question rejects the late answer and the task parks rather than
+spins. To un-park it deliberately:
+```
+run-team.py force-resume <question_id> --confirm   # reopens the expired question for re-delivery
+```
+
+**(d) Two concurrent tasks resume independently to the correct thread.**
+Start two tasks concurrently, each reaching its own clarifier wait.
+```
+run-team.py list   # two distinct open questions, distinct thread_id values
+```
+Restart the service (as in (a)); answer each in Slack.
+Pass: each task resumes to its own `thread_id` / channel - no cross-talk, no
+answer routed to the wrong task.
+
+## 6. Rollback
+
+```
+# Stop + disable the service and remove the unit:
+sudo systemctl disable --now agent-team-coordinator.service
+sudo rm /etc/systemd/system/agent-team-coordinator.service
+sudo systemctl daemon-reload
+
+# Restore the VM from the pre-provision Hyper-V checkpoint (§1) to undo
+# pip deps + any host changes in one step.
+```
+
+The ledger is **local state** under `agent-team/state/` (not in git). To reset
+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)
+```
+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.
+
+## 7. Security
+
+- **Slack inbound listener (Socket Mode) is the auth + untrusted-input surface.**
+  It accepts inbound messages over a WebSocket and turns them into ledger
+  mutations (answering live clarifier questions). It **must pass
+  `/sh-security-review`** before this is enabled in production - that review is
+  mandatory for authentication/authorization and untrusted-input handling
+  changes, and this is both.
+- **No IAM / OIDC is involved in P1.** The box runs on subscription OAuth
+  (`CLAUDE_CODE_OAUTH_TOKEN`) and Slack tokens only; there is no AWS role, no
+  OIDC trust relationship, no cloud permission surface in this deploy.
+- `~/secrev.env` stays mode 600 and out of git; `state/` (ledger + audit log) is
+  gitignored and written 0600.

agent-team/agent_team/coordinator.py

@@ -0,0 +1,710 @@
+"""COORDINATOR daemon — the live Plane-2 runtime keystone (design §3.3, §3.3.1, §7.1 P1).
+
+This module ties the committed Plane-2 leaves into a single running process. It
+owns no new durable contract of its own: it *composes* the foundation pieces
+into the live loop the design's §3.3.1 lifecycle requires:
+
+    live clarifier (Claude)  ─►  graph (durable, interrupt/resume)  ─►  ResumeWorker
+
+The :class:`Coordinator` is the object ``run-team.py serve`` runs and
+``run-team.py start`` drives one task through. Every network/SDK touch and the
+transport are **injected**, so the coordinator is unit-testable with no Claude
+SDK and no Slack:
+
+* ``transport`` is a :class:`~agent_team.transport.base.Transport` — a live
+  ``SlackTransport`` in production, a fake in tests.
+* ``build_clarify_node`` defaults to the real Claude clarifier factory
+  (:func:`agent_team.nodes.clarifier_llm.build_claude_clarifier_callables` →
+  :func:`agent_team.nodes.clarifier.make_clarifier_node`); tests inject a stub
+  node so no model is ever called.
+* the resume queue is a plain :class:`queue.Queue` of
+  :class:`~agent_team.responder.ResumeJob`, shared with the slack_listener so an
+  inbound answer and the drainer meet on one durable seam.
+
+Composition (the keystone wiring, §3.3):
+
+1. :meth:`setup` builds the SQLite checkpointer + the graph
+   (:func:`agent_team.graph.build_graph` with the injected ``live_clarify_node``)
+   and constructs a :class:`agent_team.resume_worker.ResumeWorker` over the
+   compiled graph (the worker is satisfied directly by the LangGraph app via
+   ``get_state`` / ``invoke``). ``serve`` additionally calls
+   :func:`agent_team.invoker.bind_subscription_invoker` BEFORE the clarifier node
+   is built so the Claude path is live; tests skip ``serve`` and call ``setup``
+   directly, so no invoker is bound.
+2. :meth:`start_task` runs the graph to the first human-gate ``interrupt()``,
+   reads the pending question payload, and notifies it (ledger row OPEN, then
+   transport post) via :func:`agent_team.responder.notify_question`.
+3. :meth:`submit_answer` runs the responder's first-answer-wins compare-and-set
+   and enqueues a resume job onto the shared queue.
+4. :meth:`drain_resumes` pulls jobs off the queue and runs them through the
+   single-flight, turn-guarded :class:`ResumeWorker`.
+5. :meth:`tick` is one maintenance pass: the deadline sweep parks each expired
+   task (ALARM hook) and then drains resumes.
+6. :meth:`recover` is the §3.3.1 startup sweep: re-drive ``answered`` rows and
+   clear ``open`` rows lacking a ``channel_ref`` so a reboot converges.
+
+No SQL and no transport rendering live here; everything goes through the
+committed leaves (responder, resume_worker, db.schema, graph, transport).
+"""
+
+from __future__ import annotations
+
+import logging
+import queue
+from datetime import timedelta
+from pathlib import Path
+from typing import TYPE_CHECKING, Any, Callable
+
+from agent_team import graph as graph_mod
+from agent_team import responder as responder_mod
+from agent_team.db.schema import connect, init_db
+from agent_team.resume_worker import ResumeResult, ResumeWorker
+from agent_team.transport.base import Transport
+
+if TYPE_CHECKING:  # pragma: no cover - typing only
+    from agent_team.task_model import PipelineState
+
+__all__ = [
+    "Coordinator",
+    "default_clarify_node_factory",
+]
+
+_LOG = logging.getLogger("agent_team.coordinator")
+
+# Default poll cadence for the serve() maintenance loop (the deadline-sweep
+# tick rate, §3.3.1). The transport inbound feed is the slack_listener's job;
+# this cadence governs only the deadline/recovery maintenance pass.
+DEFAULT_POLL_INTERVAL = timedelta(seconds=30)
+
+# How long a parked-task ALARM message is; kept here so the ALARM hook default
+# is one obvious place (§6.6 "ALARM rather than spin").
+_PARK_ALARM_TEMPLATE = (
+    "task parked: clarifier question %s expired with no answer in window "
+    "(§3.3.1 deadline policy); ALARM raised, not spinning"
+)
+
+
+# A clarify-node factory: build the live LangGraph clarifier node. Injected so
+# tests substitute a stub (no Claude). Returns a node callable suitable for
+# build_graph(..., live_clarify_node=...).
+ClarifyNodeFactory = Callable[[], "Callable[[PipelineState], PipelineState]"]
+# Builds the live planner node (P2). None -> the graph's P1 plan stub (terminus).
+PlanNodeFactory = Callable[[], "Callable[[PipelineState], PipelineState]"]
+# Binds the review invoker and yields (review_node, route_review) for P2. None ->
+# no review loop (P1: plan is the terminus).
+ReviewWiring = Callable[
+    [],
+    "tuple[Callable[[PipelineState], PipelineState], Callable[[PipelineState], str]]",
+]
+
+# A checkpointer factory over the db path: returns the BaseCheckpointSaver the
+# graph is compiled with. Defaults to the production SQLite checkpointer
+# (:func:`agent_team.graph.build_sqlite_checkpointer`); tests inject a factory
+# returning an in-memory saver so setup() needs no SQLite checkpointer package
+# and no contextmanager unwrap.
+CheckpointerFactory = Callable[[Path], Any]
+
+# A park-ALARM hook: called once per task whose clarifier question expired. The
+# default logs; production can inject a CloudWatch/Slack ALARM emitter. Injected
+# so the deadline policy stays I/O-free in tests (§6.6).
+AlarmHook = Callable[[str], None]
+
+
+def default_clarify_node_factory() -> Callable[[PipelineState], PipelineState]:
+    """Build the live Claude-backed clarifier node (§3.3, §7.1 P1).
+
+    Composes the two committed leaves: the Claude clarifier callables
+    (:func:`agent_team.nodes.clarifier_llm.build_claude_clarifier_callables`,
+    which route every reasoning step through the ``billing.claude_invoke`` seam)
+    and the clarifier loop node
+    (:func:`agent_team.nodes.clarifier.make_clarifier_node`). The result is the
+    real multi-turn ``interrupt()`` node handed to
+    :func:`agent_team.graph.build_graph` as ``live_clarify_node``.
+
+    Imported here (not at module load) so the coordinator module imports cleanly
+    and tests that inject a stub node never pull the Claude wiring. The billing
+    seam must already be bound (``bind_subscription_invoker``) for the node to
+    actually call Claude at run time; ``serve`` binds it before this runs.
+    """
+    from agent_team.nodes.clarifier import make_clarifier_node
+    from agent_team.nodes.clarifier_llm import build_claude_clarifier_callables
+
+    assess_confidence, generate_questions = build_claude_clarifier_callables()
+    return make_clarifier_node(
+        assess_confidence=assess_confidence,
+        generate_questions=generate_questions,
+    )
+
+
+def default_plan_node_factory() -> Callable[[PipelineState], PipelineState]:
+    """Build the live planner node wrapped fail-safe (§3.3 P2).
+
+    The canonical planner is :func:`agent_team.nodes.planner.plan_node` (it owns
+    the prompt, the loop-back review-feedback folding, and the revision-cap
+    escalation). It calls Claude through the billing seam and *raises*
+    :class:`~agent_team.nodes.planner.PlannerError` on an unparseable reply. A
+    raise inside a LangGraph node would crash the run, so this wrapper catches it
+    and instead returns a **park** delta (status/phase PARKED) so a bad planner
+    reply escalates to Adam rather than taking down the pipeline — the same
+    fail-SAFE discipline the clarifier and review stages use.
+
+    Imported lazily so the coordinator module stays import-clean and the SDK is
+    only pulled when the live node is actually built.
+    """
+    from agent_team.nodes import planner
+    from agent_team.task_model import Phase, PipelineState, TaskStatus
+
+    def plan_node(state: PipelineState) -> PipelineState:
+        try:
+            return planner.plan_node(state)
+        except planner.PlannerError:
+            # Unparseable plan -> park + ALARM rather than crash the graph.
+            return PipelineState(
+                status=TaskStatus.PARKED.value,
+                current_phase=Phase.PARKED.value,
+            )
+
+    return plan_node
+
+
+def default_review_wiring() -> tuple[
+    Callable[[PipelineState], PipelineState], Callable[[PipelineState], str]
+]:
+    """Bind the GPT-4.1 review invoker and return the P2 review node + router.
+
+    Binds :func:`agent_team.nodes.review_loop_llm.default_plan_reviewer` (the
+    cross-family reviewer that shells the local orchestrator ``run.py`` ->
+    ``cross_reviewer`` GPT-4.1) onto the review-loop seam via
+    :func:`~agent_team.nodes.review_loop.set_review_invoker`, then returns
+    ``(review_node, route_after_review)`` for
+    :func:`agent_team.graph.build_graph`. Lazy-imported for the same
+    import-hygiene reason as the clarifier/planner factories.
+    """
+    from agent_team.nodes import review_loop
+    from agent_team.nodes.review_loop_llm import default_plan_reviewer
+
+    review_loop.set_review_invoker(default_plan_reviewer)
+    # bind_review_node yields a single-arg node so LangGraph does not inject its
+    # RunnableConfig over the review config (no warning, config threads through).
+    return review_loop.bind_review_node(), review_loop.route_after_review
+
+
+class Coordinator:
+    """Owns the live Plane-2 runtime: graph + resume worker + transport (§3.3).
+
+    Construct with the durable DB path, an injected ``transport``, and optional
+    injected seams (clarify-node factory, resume queue, deadline window, ALARM
+    hook). Call :meth:`setup` once to build the graph + resume worker, then drive
+    it with :meth:`start_task` / :meth:`submit_answer` / :meth:`drain_resumes` /
+    :meth:`tick` / :meth:`recover`. :meth:`serve` binds the live Claude invoker
+    and runs the maintenance loop.
+
+    Everything network/SDK is injected so the whole surface (minus the infinite
+    serve loop) is unit-testable with a fake transport and a stub clarify node.
+    """
+
+    def __init__(
+        self,
+        *,
+        db_path: Path | str,
+        transport: Transport,
+        build_clarify_node: ClarifyNodeFactory | None = None,
+        build_plan_node: PlanNodeFactory | None = None,
+        review_wiring: ReviewWiring | None = None,
+        build_checkpointer: CheckpointerFactory | None = None,
+        resume_queue: "queue.Queue[Any] | None" = None,
+        deadline_window: timedelta | None = None,
+        alarm_hook: AlarmHook | None = None,
+    ) -> None:
+        self._db_path = Path(db_path)
+        self._transport = transport
+        self._build_clarify_node = build_clarify_node or default_clarify_node_factory
+        # P2 wiring is OPT-IN: left None, the graph uses its P1 plan stub and no
+        # review loop (keeps the unit suite model-free). The production serve/start
+        # path injects default_plan_node_factory + default_review_wiring.
+        self._build_plan_node = build_plan_node
+        self._review_wiring = review_wiring
+        self._build_checkpointer = (
+            build_checkpointer or graph_mod.build_sqlite_checkpointer
+        )
+        self._resume_queue: "queue.Queue[Any]" = resume_queue or queue.Queue()
+        self._deadline_window = deadline_window or graph_mod.DEFAULT_CLARIFY_DEADLINE
+        self._alarm_hook = alarm_hook or self._default_alarm_hook
+
+        # Built by setup().
+        self._graph: Any = None
+        self._resume_worker: ResumeWorker | None = None
+        # Retains a context-manager checkpointer (production SQLite saver) so its
+        # __exit__ is not run early; held open for the daemon's lifetime.
+        self._checkpointer_cm: Any = None
+
+    # ------------------------------------------------------------------ #
+    # Accessors (the shared queue is the slack_listener handoff seam).
+    # ------------------------------------------------------------------ #
+
+    @property
+    def resume_queue(self) -> "queue.Queue[Any]":
+        """The shared resume-job queue (slack_listener enqueues, drainer drains)."""
+        return self._resume_queue
+
+    @property
+    def graph(self) -> Any:
+        """The compiled LangGraph app (``None`` until :meth:`setup`)."""
+        return self._graph
+
+    @property
+    def resume_worker(self) -> ResumeWorker | None:
+        """The single-flight resume worker (``None`` until :meth:`setup`)."""
+        return self._resume_worker
+
+    # ------------------------------------------------------------------ #
+    # Lifecycle wiring.
+    # ------------------------------------------------------------------ #
+
+    def setup(self) -> None:
+        """Build the durable graph + resume worker (§3.3 keystone composition).
+
+        Idempotent-friendly one-time wiring: ensures the ledger tables exist
+        (:func:`agent_team.db.init_db`), builds the SQLite checkpointer and the
+        graph with the injected live clarifier node
+        (:func:`agent_team.graph.build_graph`), and constructs the
+        :class:`ResumeWorker` over the compiled graph and a fresh durable
+        connection.
+
+        Does NOT bind the Claude invoker — that is ``serve``'s job, kept out of
+        ``setup`` so tests can build a coordinator with a stub clarify node and
+        never touch the SDK. The clarify node is built here (from the injected
+        factory) and handed to ``build_graph`` as ``live_clarify_node``.
+
+        The production :func:`agent_team.graph.build_sqlite_checkpointer` returns
+        a context manager (``SqliteSaver.from_conn_string``), not a bare saver,
+        so we enter it here to obtain the live saver and hold the context open
+        for the coordinator's lifetime (the process runs the saver until exit).
+        An already-unwrapped saver (the test in-memory factory) passes through.
+        """
+        init_db(self._db_path)
+
+        checkpointer = self._enter_checkpointer(self._build_checkpointer(self._db_path))
+        clarify_node = self._build_clarify_node()
+
+        # P2 (opt-in): a wrapped real planner + the bound GPT-4.1 review loop.
+        plan_node = self._build_plan_node() if self._build_plan_node else None
+        review_node: Any = None
+        route_review: Any = None
+        if self._review_wiring is not None:
+            review_node, route_review = self._review_wiring()
+
+        self._graph = graph_mod.build_graph(
+            checkpointer,
+            live_clarify_node=clarify_node,
+            live_plan_node=plan_node,
+            review_node=review_node,
+            route_review=route_review,
+        )
+
+        # The ResumeWorker is satisfied directly by the compiled LangGraph app
+        # (get_state / invoke); it gets its own durable connection so its ledger
+        # writes do not contend with notify/answer connections.
+        self._resume_worker = ResumeWorker(self._graph, connect(self._db_path))
+
+    def _enter_checkpointer(self, checkpointer: Any) -> Any:
+        """Unwrap a context-manager checkpointer into a live saver, or pass through.
+
+        ``SqliteSaver.from_conn_string`` (the production factory) is a context
+        manager yielding the saver; entering it gives the live saver and keeps
+        the underlying connection open for the daemon's lifetime (the saved
+        ``__exit__`` runs at process teardown). A plain ``BaseCheckpointSaver``
+        (the injected in-memory test factory) has no ``__enter__`` and is
+        returned unchanged.
+        """
+        enter = getattr(checkpointer, "__enter__", None)
+        if enter is None:
+            return checkpointer
+        saver = enter()
+        # Retain the CM so its __exit__ is not run early by the GC; the daemon
+        # holds the saver open until the process exits.
+        self._checkpointer_cm = checkpointer
+        return saver
+
+    # ------------------------------------------------------------------ #
+    # Intake.
+    # ------------------------------------------------------------------ #
+
+    def start_task(self, *, task_text: str, transport_name: str) -> str:
+        """Start one task: run to the first human gate, then notify (§3.3, §3.3.1).
+
+        Runs :func:`agent_team.graph.start_task` to the first clarifier
+        ``interrupt()``, reads the pending question payload
+        (:func:`agent_team.graph.pending_question`), and delivers it via
+        :func:`agent_team.responder.notify_question` (ledger row OPEN first, then
+        transport post). Returns the minted ``thread_id``.
+
+        **Intake-seed decision (P1).** ``agent_team.graph.start_task`` builds its
+        own INTAKE seed and accepts only ``thread_id`` / ``transport`` — it takes
+        no task-description argument, and a value pre-seeded onto the START
+        checkpoint via ``update_state`` is overwritten by its own seed invoke
+        (and a post-suspend ``update_state`` clears the pending interrupt, which
+        would break the human gate). So for P1 the ``task_text`` is intake
+        metadata held coordinator-side (logged) rather than written into
+        ``PipelineState``: the deterministic P1 clarifier does not consume a task
+        description anyway, and threading it into the graph state is a later phase
+        that extends the committed ``start_task`` seed contract. We keep it
+        minimal rather than reach past that contract or disturb the gate.
+        """
+        if self._graph is None:
+            raise RuntimeError("Coordinator.start_task called before setup()")
+
+        _LOG.info("start_task intake (transport=%s): %s", transport_name, task_text)
+        thread_id, _state = graph_mod.start_task(self._graph, transport=transport_name)
+
+        question = graph_mod.pending_question(self._graph, thread_id=thread_id)
+        if question is None:
+            # No human gate raised (e.g. the clarifier already cleared the bar
+            # from context alone, or a stub that does not interrupt). Nothing to
+            # notify; the task proceeds without a pending question.
+            _LOG.info("start_task %s produced no pending question", thread_id)
+            return thread_id
+
+        question_set = question["question_set"]
+        deadline = question.get("deadline") or self._default_deadline()
+
+        conn = connect(self._db_path)
+        try:
+            responder_mod.notify_question(
+                conn,
+                self._transport,
+                question_set,
+                deadline=deadline,
+            )
+        finally:
+            conn.close()
+        return thread_id
+
+    # ------------------------------------------------------------------ #
+    # Answer intake (the slack_listener calls this, or submit_answer directly).
+    # ------------------------------------------------------------------ #
+
+    def submit_answer(self, raw: Any) -> responder_mod.AnswerOutcome:
+        """Run the first-answer-wins compare-and-set and enqueue a resume (§3.3.1).
+
+        Normalizes ``raw`` through the transport, runs
+        :func:`agent_team.responder.submit_answer` (the single atomic
+        ``open → answered`` flip), and on the rowcount-1 win enqueues a
+        :class:`ResumeJob` onto the shared resume queue via
+        ``self._resume_queue.put``. The slack_listener calls this in production
+        (or calls ``responder.submit_answer`` directly with the same queue); the
+        shared queue is the coordination seam either way. Returns the
+        :class:`AnswerOutcome` so the caller can reply "already closed" on a
+        duplicate / late answer (``accepted=False``).
+        """
+        conn = connect(self._db_path)
+        try:
+            return responder_mod.submit_answer(
+                conn,
+                self._transport,
+                raw,
+                enqueue_resume=self._resume_queue.put,
+            )
+        finally:
+            conn.close()
+
+    # ------------------------------------------------------------------ #
+    # Resume drain (single-flight, turn-guarded via ResumeWorker).
+    # ------------------------------------------------------------------ #
+
+    def drain_resumes(self) -> list[ResumeResult]:
+        """Drain queued resume jobs through the turn-guarded worker (§3.3.1).
+
+        Pulls every currently-queued :class:`ResumeJob` and runs it through
+        :meth:`agent_team.resume_worker.ResumeWorker.resume`. Single-flight (per
+        ``thread_id``) and the turn guard live inside the worker, so a stale or
+        redelivered job supersedes-and-skips rather than double-applying. Drains
+        only what is already queued (a snapshot of ``qsize``) so a steady stream
+        of new answers cannot starve the caller; the next tick drains the rest.
+        Returns the per-job results for logging/ALARM.
+        """
+        if self._resume_worker is None:
+            raise RuntimeError("Coordinator.drain_resumes called before setup()")
+
+        results: list[ResumeResult] = []
+        for _ in range(self._resume_queue.qsize()):
+            try:
+                job = self._resume_queue.get_nowait()
+            except queue.Empty:
+                break
+            try:
+                results.append(
+                    self._resume_worker.resume(
+                        thread_id=job.thread_id,
+                        question_id=job.question_id,
+                        turn=job.turn,
+                        answer=job.answer,
+                    )
+                )
+            finally:
+                self._resume_queue.task_done()
+        return results
+
+    # ------------------------------------------------------------------ #
+    # Maintenance tick (deadline policy + drain).
+    # ------------------------------------------------------------------ #
+
+    def tick(self) -> list[ResumeResult]:
+        """One maintenance pass: deadline sweep + park policy, then drain (§3.3.1).
+
+        Runs :func:`agent_team.responder.deadline_sweep` to flip overdue ``open``
+        questions to ``expired`` (the deterministic answer-vs-expiry race), then
+        applies the park policy to each newly-expired id (raise the ALARM hook —
+        §6.6 "ALARM rather than spin"; the durable ledger row is already
+        ``expired``, which is the task's parked state for P1). Finally drains any
+        resume jobs that landed. Returns the drain results.
+        """
+        conn = connect(self._db_path)
+        try:
+            expired = responder_mod.deadline_sweep(conn)
+        finally:
+            conn.close()
+
+        for question_id in expired:
+            self._park(question_id)
+
+        return self.drain_resumes()
+
+    def _park(self, question_id: str) -> None:
+        """Apply the park policy to one expired question (§6.6 ALARM, not spin).
+
+        The deadline sweep already flipped the ledger row to ``expired`` (the
+        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.
+        """
+        self._alarm_hook(question_id)
+
+    @staticmethod
+    def _default_alarm_hook(question_id: str) -> None:
+        """Default park ALARM: log at WARNING (production injects a real emitter)."""
+        _LOG.warning(_PARK_ALARM_TEMPLATE, question_id)
+
+    # ------------------------------------------------------------------ #
+    # Startup recovery (§3.3.1 converge-after-reboot).
+    # ------------------------------------------------------------------ #
+
+    def recover(self) -> list[ResumeResult]:
+        """Startup sweep so a reboot converges from the durable ledger (§3.3.1).
+
+        Two halves of the §3.3.1 recovery:
+
+        1. Re-drive every ``answered`` ledger row whose graph is still
+           interrupted on its turn
+           (:meth:`agent_team.resume_worker.ResumeWorker.recover_pending_resumes`).
+           The turn guard makes this idempotent — a thread that already advanced
+           supersedes-and-skips, so converging cannot double-apply.
+        2. Re-deliver ``open`` rows that lack a ``channel_ref`` (a post lost to a
+           crash between the durable ledger write and the transport post): clear
+           any stale ref and re-post over the transport so the human gate is
+           actually reachable again (§3.3.1 "delivery / lost-post").
+
+        Returns the resume results from half (1) for logging/ALARM.
+        """
+        if self._resume_worker is None:
+            raise RuntimeError("Coordinator.recover called before setup()")
+
+        results = self._resume_worker.recover_pending_resumes()
+        self._redeliver_unposted_open_questions()
+        return results
+
+    def _redeliver_unposted_open_questions(self) -> None:
+        """Re-post ``open`` rows that never recorded a ``channel_ref`` (§3.3.1).
+
+        A crash between :func:`notify_question`'s durable ledger write and its
+        transport post leaves an ``open`` row with no ``channel_ref``. On reboot
+        we re-derive the pending interrupt payload from the live checkpoint and
+        re-post it, then record the fresh ref. Idempotent: a row already carrying
+        a ref is skipped, and a thread no longer interrupted (already advanced)
+        is skipped. The re-post reuses the committed
+        :func:`agent_team.responder.notify_question`, so the ledger-then-post
+        ordering and lost-post tolerance are identical to first delivery.
+
+        **Lost-update guard (RACE-REDELIVER-NONATOMIC-DELETE, CWE-362).** The
+        SELECT and the clear-then-reinsert are NOT one transaction, and the
+        answer compare-and-set writes on a *separate* connection. An answer that
+        lands between the SELECT and the clear flips the row ``open`` →
+        ``answered`` on that other connection; an unconditional DELETE would then
+        destroy the just-recorded human answer, dangle the enqueued ResumeJob,
+        and re-ask the human with a fresh ``open`` row. So the clear is a
+        *guarded* compare-and-set: a conditional DELETE re-checking
+        ``status='open' AND channel_ref IS NULL`` at WRITE time, run under
+        ``BEGIN IMMEDIATE`` on a private connection (mirroring schema.py's
+        ``_compare_and_set``). If its rowcount is 0 the row is no longer an
+        unposted-open question (it was answered/expired/advanced concurrently),
+        so re-delivery is SKIPPED — an accepted answer is never clobbered and the
+        human is never re-asked. Only a row STILL open-and-unposted at write time
+        is re-INSERTed and re-posted.
+        """
+        conn = connect(self._db_path)
+        try:
+            rows = conn.execute(
+                "SELECT question_id, thread_id, deadline_at FROM pending_questions "
+                "WHERE status='open' AND channel_ref IS NULL"
+            ).fetchall()
+            for row in rows:
+                thread_id = row["thread_id"]
+                question = graph_mod.pending_question(self._graph, thread_id=thread_id)
+                if question is None:
+                    # Graph already advanced past this gate; nothing to re-post.
+                    continue
+                # Guarded clear: re-check the unposted-open predicate atomically
+                # at WRITE time. If 0 rows are deleted, the row was answered /
+                # expired / advanced concurrently between the SELECT above and
+                # this write — SKIP re-delivery so an accepted answer is never
+                # destroyed and the human is never re-asked.
+                if not self._guarded_clear_unposted_open(row["question_id"]):
+                    continue
+                question_set = question["question_set"]
+                deadline = (
+                    row["deadline_at"]
+                    or question.get("deadline")
+                    or self._default_deadline()
+                )
+                responder_mod.notify_question(
+                    conn,
+                    self._transport,
+                    question_set,
+                    deadline=deadline,
+                )
+        finally:
+            conn.close()
+
+    # Bounded retry mirroring schema.py's _compare_and_set: if the write lock is
+    # still contended after busy_timeout elapses, retry a few times rather than
+    # surfacing a transient "database is locked" to the recovery sweep.
+    _CLEAR_RETRY_ATTEMPTS = 3
+    _CLEAR_RETRY_BACKOFF_S = 0.05
+
+    def _guarded_clear_unposted_open(self, question_id: str) -> bool:
+        """Conditionally DELETE an unposted-open row, atomically (CWE-362 guard).
+
+        The compare-and-set that closes the redeliver lost-update window. Runs the
+        predicate-guarded DELETE
+
+            DELETE FROM pending_questions
+            WHERE question_id=? AND status='open' AND channel_ref IS NULL
+
+        under ``BEGIN IMMEDIATE`` on a *private*, short-lived connection — exactly
+        the discipline schema.py's :func:`~agent_team.db.schema._compare_and_set`
+        uses for ``answer_question`` / ``expire_question``. Taking the write lock
+        up front and re-checking ``status='open' AND channel_ref IS NULL`` at
+        write time means a concurrent answer-CAS (on its own connection) either
+        ran first — in which case the row is no longer ``open`` and this DELETE
+        matches 0 rows — or runs after — in which case it sees the row already
+        gone and its own ``WHERE status='open'`` guard matches 0 rows. Either way
+        the answer is never lost.
+
+        Returns ``True`` iff exactly one row was deleted (the row was STILL
+        unposted-open, so re-delivery should proceed); ``False`` if 0 rows matched
+        (answered/expired/advanced concurrently → caller SKIPS re-delivery).
+
+        For an in-memory DB (no backing file to reopen) the write runs on the
+        passed-through connection; in-memory DBs are single-connection and not the
+        concurrent production path — the same fallback schema.py makes.
+        """
+        sql = (
+            "DELETE FROM pending_questions "
+            "WHERE question_id=? AND status='open' AND channel_ref IS NULL"
+        )
+        params = (question_id,)
+
+        probe = connect(self._db_path)
+        try:
+            db_file = getattr(probe, "agent_team_db_path", "") or None
+        finally:
+            probe.close()
+        if db_file is None:
+            return self._clear_once(connect(self._db_path), sql, params, close=True)
+
+        import time
+
+        last_err: Any = None
+        for attempt in range(self._CLEAR_RETRY_ATTEMPTS):
+            write = connect(Path(db_file))
+            try:
+                return self._clear_once(write, sql, params, close=False)
+            except Exception as err:  # noqa: BLE001 - re-raised below if not a lock
+                if "locked" not in str(err).lower():
+                    raise
+                last_err = err
+            finally:
+                write.close()
+            time.sleep(self._CLEAR_RETRY_BACKOFF_S * (attempt + 1))
+
+        assert last_err is not None  # loop only exits early via return or raise
+        raise last_err
+
+    @staticmethod
+    def _clear_once(
+        conn: Any, sql: str, params: tuple[Any, ...], *, close: bool
+    ) -> bool:
+        """Run one ``BEGIN IMMEDIATE`` conditional DELETE; rowcount==1 → True.
+
+        Mirrors schema.py's ``_cas_once``: ``BEGIN IMMEDIATE`` before the try so a
+        lock-acquisition error propagates to the retry loop with no transaction to
+        unwind; once open, any failure rolls back (best-effort) and re-raises.
+        """
+        try:
+            conn.execute("BEGIN IMMEDIATE")
+            try:
+                cur = conn.execute(sql, params)
+                changed = cur.rowcount == 1
+                conn.execute("COMMIT")
+                return changed
+            except BaseException:
+                try:
+                    conn.execute("ROLLBACK")
+                except Exception:  # noqa: BLE001 - best-effort unwind
+                    pass
+                raise
+        finally:
+            if close:
+                conn.close()
+
+    # ------------------------------------------------------------------ #
+    # Serve loop (production daemon entry).
+    # ------------------------------------------------------------------ #
+
+    def serve(self, *, poll_interval: timedelta | None = None) -> None:
+        """Run the live daemon: bind invoker, setup, recover, then tick forever.
+
+        The production entry. Binds the real Claude invoker
+        (:func:`agent_team.invoker.bind_subscription_invoker`) BEFORE
+        :meth:`setup` builds the clarifier node (so the node's
+        ``billing.claude_invoke`` calls hit the live subscription path), runs the
+        startup :meth:`recover` sweep, then loops calling :meth:`tick` on the
+        deadline cadence.
+
+        The actual Slack inbound feed is the slack_listener's job; the
+        coordinator exposes :meth:`submit_answer` and the shared
+        :attr:`resume_queue` for it. This loop owns only the deadline/recovery
+        maintenance cadence.
+        """
+        from agent_team.invoker import bind_subscription_invoker
+
+        bind_subscription_invoker()
+        self.setup()
+        self.recover()
+
+        interval = (poll_interval or DEFAULT_POLL_INTERVAL).total_seconds()
+        import time
+
+        while True:  # pragma: no cover - the infinite daemon loop
+            self.tick()
+            time.sleep(interval)
+
+    # ------------------------------------------------------------------ #
+    # Internals.
+    # ------------------------------------------------------------------ #
+
+    def _default_deadline(self) -> str:
+        """Compute a fresh ISO deadline from the configured window (§3.3.1)."""
+        from datetime import datetime, timezone
+
+        return (datetime.now(timezone.utc) + self._deadline_window).isoformat()

agent-team/agent_team/graph.py

@@ -39,6 +39,7 @@ shapes the interrupt payload that drives it.
 from __future__ import annotations
 
 import uuid
+from collections.abc import Callable
 from datetime import datetime, timedelta, timezone
 from pathlib import Path
 from typing import TYPE_CHECKING, Any
@@ -55,15 +56,20 @@ from agent_team.task_model import (
 from agent_team.transport import QuestionSet
 
 if TYPE_CHECKING:  # pragma: no cover - typing only
+    from contextlib import AbstractContextManager
+
     from langgraph.checkpoint.base import BaseCheckpointSaver
     from langgraph.graph.state import CompiledStateGraph
 
 __all__ = [
+    "BUILD_ROUTE",
     "CLARIFY",
     "DEFAULT_CLARIFY_DEADLINE",
     "INTAKE",
     "P1_PHASE_SEQUENCE",
+    "PARKED_ROUTE",
     "PLAN",
+    "REVIEW",
     "build_graph",
     "build_sqlite_checkpointer",
     "clarify_node",
@@ -83,6 +89,15 @@ __all__ = [
 INTAKE = "intake"
 CLARIFY = "clarify"
 PLAN = "plan"
+# P2 (review loop) node ids. REVIEW is the adversarial-review vertex; BUILD_ROUTE
+# and PARKED_ROUTE are the *route ids* the injected route function returns (they
+# mirror agent_team.nodes.review_loop.BUILD_NODE / PARKED_NODE by value, so the
+# conditional-edge map matches without graph.py importing review_loop). In P2
+# both terminate the graph (no builders yet); P3 replaces BUILD_ROUTE's target
+# with the real builders subgraph.
+REVIEW = "review"
+BUILD_ROUTE = "build"
+PARKED_ROUTE = "parked"
 
 # The P1 stage order (§7.1): intake -> clarify -> plan, then stop. Builders and
 # verifiers (BUILD/VERIFY) are deliberately NOT wired here — P1 ends at an
@@ -246,6 +261,11 @@ def plan_phase(state: PipelineState) -> dict[str, Any]:
 
 def build_graph(
     checkpointer: BaseCheckpointSaver | None = None,
+    *,
+    live_clarify_node: Callable[[PipelineState], PipelineState] | None = None,
+    live_plan_node: Callable[[PipelineState], PipelineState] | None = None,
+    review_node: Callable[[PipelineState], PipelineState] | None = None,
+    route_review: Callable[[PipelineState], str] | None = None,
 ) -> CompiledStateGraph:
     """Assemble + compile the P1 pipeline ``StateGraph`` (§3.3, §7.1).
 
@@ -256,29 +276,95 @@ def build_graph(
     The ``checkpointer`` is **injected**, never constructed here: the design's
     durable store is the SQLite checkpointer (D9), but pre-deploy scaffolding
     must not provision it, and tests inject an in-memory saver. Production wires
-    :func:`build_sqlite_checkpointer`. A checkpointer is required for the
+    the **entered** saver yielded by :func:`build_sqlite_checkpointer` (which
+    returns a context manager the caller must enter and hold, not a bare saver).
+    A checkpointer is required for the
     ``interrupt()``/``resume`` mechanic to work, so callers that pass ``None``
     get an uncheckpointed graph that can run straight-through but cannot
     suspend; the driver functions therefore require a checkpointed graph.
+
+    ``live_clarify_node`` is the **injected real clarifier** (P1a): the live
+    coordinator passes the Claude-backed multi-turn node built from
+    :func:`agent_team.nodes.clarifier.make_clarifier_node`, while tests and the
+    pre-deploy scaffold fall back to the deterministic single-turn
+    :func:`clarify_node` stub. Either node honours the same ``interrupt()``
+    suspend/resume contract, so the durable human gate is identical; only the
+    question authoring differs. Defaulting to the stub keeps the graph wiring
+    model-free and the existing tests unchanged.
+
+    ``live_plan_node`` / ``review_node`` / ``route_review`` wire **P2** (planner
+    + adversarial review loop). All are injected so this module stays decoupled
+    from the model + review code (the coordinator passes the wrapped
+    :func:`agent_team.nodes.planner.plan_node`, the
+    :func:`agent_team.nodes.review_loop.review_node`, and its
+    :func:`~agent_team.nodes.review_loop.route_after_review`):
+
+    * **P1 (default):** ``review_node`` is ``None`` -> ``plan -> END``. The plan
+      stage is the terminus (no review, no build), exactly as before.
+    * **P2:** ``review_node`` is given -> ``plan -> review -> {build|plan|parked}``.
+      The injected ``route_review`` reads the latest verdict and returns a route
+      id; the conditional-edge map sends ``"plan"`` back to the planner
+      (loop-back), and ``"build"`` / ``"parked"`` to ``END`` (P2 stops at an
+      approved-or-escalated plan; P3 will repoint ``"build"`` at the real
+      builders subgraph). The plan<->review cycle is bounded by the planner's
+      revision cap and the review round cap, so the loop always terminates.
+
+    ``review_node`` requires ``route_review`` (and a real ``live_plan_node`` that
+    advances to REVIEW); passing one without the other is a wiring error.
     """
+    clarify = live_clarify_node if live_clarify_node is not None else clarify_node
+    plan = live_plan_node if live_plan_node is not None else plan_node
+
+    if review_node is not None and route_review is None:
+        raise ValueError(
+            "build_graph: review_node requires route_review (the conditional-edge "
+            "function, e.g. review_loop.route_after_review)."
+        )
+
     builder: StateGraph = StateGraph(PipelineState)
     builder.add_node(INTAKE, intake_node)
-    builder.add_node(CLARIFY, clarify_node)
+    builder.add_node(CLARIFY, clarify)
-    builder.add_node(PLAN, plan_node)
+    builder.add_node(PLAN, plan)
 
     builder.add_edge(START, INTAKE)
     builder.add_edge(INTAKE, CLARIFY)
     builder.add_edge(CLARIFY, PLAN)
-    builder.add_edge(PLAN, END)
+
+    if review_node is None:
+        # P1: the plan stage is the terminus.
+        builder.add_edge(PLAN, END)
+    else:
+        # P2: plan -> review -> {loop-back to plan | END}.
+        builder.add_node(REVIEW, review_node)
+        builder.add_edge(PLAN, REVIEW)
+        builder.add_conditional_edges(
+            REVIEW,
+            route_review,
+            {BUILD_ROUTE: END, PLAN: PLAN, PARKED_ROUTE: END},
+        )
 
     if checkpointer is None:
         return builder.compile()
     return builder.compile(checkpointer=checkpointer)
 
 
-def build_sqlite_checkpointer(db_path: Path | str) -> BaseCheckpointSaver:
+def build_sqlite_checkpointer(
+    db_path: Path | str,
+) -> AbstractContextManager[BaseCheckpointSaver]:
     """Construct the production SQLite checkpointer over ``db_path`` (D9, §3.3).
 
+    Returns a **context manager**, not an entered saver: in
+    ``langgraph-checkpoint-sqlite`` ``SqliteSaver.from_conn_string`` is a
+    ``@contextmanager`` classmethod, so the caller MUST enter it (``with`` it,
+    or ``__enter__`` and retain it for the graph's lifetime) before passing the
+    yielded saver to :func:`build_graph`. The live coordinator owns that
+    lifecycle (it enters the CM at setup and holds it for the daemon's life);
+    passing the raw return value straight into ``build_graph`` would compile a
+    graph whose checkpointer is an un-entered CM and break ``get_state`` /
+    ``invoke`` at runtime. The earlier ``-> BaseCheckpointSaver`` annotation
+    mis-stated this contract (review FIX); the type now matches reality so a
+    direct caller cannot be silently misled.
+
     The import of ``langgraph.checkpoint.sqlite`` is deferred to call time so
     this module imports cleanly in environments where that optional package is
     not installed (pre-deploy scaffolding). The checkpointer creates its own

agent-team/agent_team/invoker.py

@@ -0,0 +1,300 @@
+"""Real Claude invokers for the ``claude_invoke`` billing seam (design §3.1).
+
+:mod:`agent_team.billing` owns mode selection and the subscription-mode env
+hygiene, but delegates the actual SDK call to a pluggable invoker bound via
+:func:`agent_team.billing.set_invoker`. This module supplies that invoker: a
+single function matching the ``billing.Invoker`` signature
+``(prompt, *, mode, **kw) -> ClaudeResult`` that dispatches on
+:class:`~agent_team.billing.BillingMode`:
+
+* ``SUBSCRIPTION`` — the R720 default. Runs the Claude Agent SDK headless over
+  the subscription OAuth token (``CLAUDE_CODE_OAUTH_TOKEN``), mirroring the
+  canonical pattern in ``security-review/run_headless.py``. ``billing`` has
+  already popped any stray ``ANTHROPIC_API_KEY`` for the duration of the call,
+  so we only assert the OAuth token is present.
+* ``API`` — a thin metered call through the ``anthropic`` SDK.
+* ``BEDROCK`` — the rare cross-family tiebreak path; not wired for P1, so it
+  raises :class:`NotImplementedError` honestly (a later config-flip wires it).
+
+Deferred-import rationale: neither ``claude_agent_sdk`` nor ``anthropic`` is
+installed in the test/Mac scaffolding environment, so importing either at
+module load would raise :class:`ModuleNotFoundError` and break a clean import.
+Following the deferred-import pattern of
+:func:`agent_team.graph.build_sqlite_checkpointer`, the SDK imports live inside
+the functions that actually call them and raise a clear :class:`RuntimeError`
+when the package is missing. The SDK callables are also injectable (``_query``,
+``_client``) so the real path stays unit-testable without the SDKs installed,
+mirroring how the codebase keeps SDK calls injectable (see
+:func:`agent_team.billing.set_invoker` and
+:func:`agent_team.resume_worker.build_resume_command`).
+"""
+
+from __future__ import annotations
+
+import asyncio
+from typing import Any, Callable
+
+from agent_team.billing import BillingMode, ClaudeResult, set_invoker
+
+__all__ = [
+    "API_MODEL",
+    "api_invoker",
+    "bind_invoker",
+    "bind_subscription_invoker",
+    "subscription_invoker",
+]
+
+# Metered model for the API path (the rare opt-in billing mode).
+API_MODEL = "claude-sonnet-4-6"
+
+# Default per-call agent budget for the headless subscription path, in USD.
+_DEFAULT_BUDGET_USD = 2.0
+_DEFAULT_MAX_TURNS = 40
+
+
+# --------------------------------------------------------------------------- #
+# Subscription path (Claude Agent SDK, headless OAuth)
+# --------------------------------------------------------------------------- #
+
+
+def _require_agent_sdk() -> Any:
+    """Import and return ``claude_agent_sdk`` or raise a clear RuntimeError.
+
+    Deferred so this module imports cleanly where the SDK is absent (the
+    Mac/test scaffold). Mirrors graph.build_sqlite_checkpointer.
+    """
+    try:
+        import claude_agent_sdk
+    except ImportError as exc:  # pragma: no cover - depends on optional dep
+        raise RuntimeError(
+            "claude_agent_sdk is unavailable; install it to use the "
+            "subscription billing path (the R720 default). Tests inject a fake "
+            "query via the _query parameter."
+        ) from exc
+    return claude_agent_sdk
+
+
+async def _collect_subscription_text(
+    prompt: str,
+    *,
+    max_turns: int,
+    budget_usd: float,
+    model: str | None,
+    _query: Callable[..., Any] | None = None,
+    _options_cls: Callable[..., Any] | None = None,
+) -> tuple[str, dict[str, Any], list[Any]]:
+    """Drive one headless Agent SDK turn; return (text, usage, raw_messages).
+
+    ``_query``/``_options_cls`` default to the real ``claude_agent_sdk``
+    callables (lazily imported) but are injectable so tests can supply a fake
+    async ``query`` without the SDK installed. Text extraction mirrors
+    ``run_headless.py``: prefer the terminal ``ResultMessage.result``, falling
+    back to concatenated ``AssistantMessage`` text blocks.
+    """
+    if _query is None or _options_cls is None:
+        sdk = _require_agent_sdk()
+        if _query is None:
+            _query = sdk.query
+        if _options_cls is None:
+            _options_cls = sdk.ClaudeAgentOptions
+
+    opts = _options_cls(
+        permission_mode="bypassPermissions",
+        setting_sources=[],  # hermetic: ignore user/project/local config + CLAUDE.md
+        model=model,
+        max_turns=max_turns,
+        max_budget_usd=budget_usd,
+    )
+
+    texts: list[str] = []
+    result_text: str | None = None
+    messages: list[Any] = []
+    usage: dict[str, Any] = {}
+    async for msg in _query(prompt=prompt, options=opts):
+        messages.append(msg)
+        name = type(msg).__name__
+        if name == "AssistantMessage":
+            for block in getattr(msg, "content", []) or []:
+                text = getattr(block, "text", None)
+                if text:
+                    texts.append(text)
+        elif name == "ResultMessage":
+            result_text = getattr(msg, "result", None)
+            cost = getattr(msg, "total_cost_usd", None)
+            if cost is not None:
+                usage["total_cost_usd"] = float(cost)
+            sdk_usage = getattr(msg, "usage", None)
+            if isinstance(sdk_usage, dict):
+                usage.update(sdk_usage)
+            elif sdk_usage is not None:
+                usage["usage"] = sdk_usage
+
+    return (result_text or "\n".join(texts)), usage, messages
+
+
+def subscription_invoker(
+    prompt: str,
+    *,
+    mode: BillingMode,
+    max_turns: int = _DEFAULT_MAX_TURNS,
+    budget_usd: float = _DEFAULT_BUDGET_USD,
+    model: str | None = None,
+    _query: Callable[..., Any] | None = None,
+    _options_cls: Callable[..., Any] | None = None,
+    **kw: Any,
+) -> ClaudeResult:
+    """Invoke Claude headless over the subscription OAuth token (§3.1).
+
+    Asserts ``CLAUDE_CODE_OAUTH_TOKEN`` is present (the metered key is already
+    popped by :func:`agent_team.billing.claude_invoke` in subscription mode) and
+    refuses to run without it, naming ``~/secrev.env`` as the source. The Agent
+    SDK ``query()`` coroutine is bridged to this sync seam with
+    :func:`asyncio.run`; the box path is synchronous, but we fail clearly rather
+    than silently if invoked from inside a running event loop.
+
+    ``_query``/``_options_cls`` are injection seams for tests; production leaves
+    them ``None`` so the real ``claude_agent_sdk`` callables are used.
+    """
+    import os
+
+    if not os.environ.get("CLAUDE_CODE_OAUTH_TOKEN"):
+        raise RuntimeError(
+            "subscription_invoker requires CLAUDE_CODE_OAUTH_TOKEN to be set "
+            "(source ~/secrev.env). Refusing to run the subscription OAuth path "
+            "without it."
+        )
+
+    coro = _collect_subscription_text(
+        prompt,
+        max_turns=max_turns,
+        budget_usd=budget_usd,
+        model=model,
+        _query=_query,
+        _options_cls=_options_cls,
+    )
+
+    try:
+        asyncio.get_running_loop()
+    except RuntimeError:
+        text, usage, messages = asyncio.run(coro)
+    else:  # pragma: no cover - the box path is synchronous
+        coro.close()
+        raise RuntimeError(
+            "subscription_invoker cannot bridge asyncio.run from within a "
+            "running event loop; call claude_invoke from synchronous code."
+        )
+
+    return ClaudeResult(text=text, mode=mode, usage=usage, raw=messages)
+
+
+# --------------------------------------------------------------------------- #
+# API path (anthropic SDK, metered)
+# --------------------------------------------------------------------------- #
+
+
+def _require_anthropic() -> Any:
+    """Import and return the ``anthropic`` module or raise a clear RuntimeError."""
+    try:
+        import anthropic
+    except ImportError as exc:  # pragma: no cover - depends on optional dep
+        raise RuntimeError(
+            "anthropic is unavailable; install it to use the API billing path. "
+            "Tests inject a fake client via the _client parameter."
+        ) from exc
+    return anthropic
+
+
+def _extract_text(message: Any) -> str:
+    """Join the text blocks of an anthropic Messages response."""
+    parts: list[str] = []
+    for block in getattr(message, "content", []) or []:
+        text = getattr(block, "text", None)
+        if text:
+            parts.append(text)
+    return "".join(parts)
+
+
+def api_invoker(
+    prompt: str,
+    *,
+    mode: BillingMode,
+    model: str = API_MODEL,
+    max_tokens: int = 4096,
+    _client: Any | None = None,
+    **kw: Any,
+) -> ClaudeResult:
+    """Invoke Claude through the metered ``anthropic`` SDK (§3.1, API mode).
+
+    ``_client`` is an injection seam for tests; production leaves it ``None`` so
+    a real ``anthropic.Anthropic()`` is constructed (reading
+    ``ANTHROPIC_API_KEY`` from the environment, as the SDK does by default).
+    """
+    if _client is None:
+        anthropic = _require_anthropic()
+        _client = anthropic.Anthropic()
+
+    message = _client.messages.create(
+        model=model,
+        max_tokens=max_tokens,
+        messages=[{"role": "user", "content": prompt}],
+    )
+
+    usage_obj = getattr(message, "usage", None)
+    if usage_obj is None:
+        usage: dict[str, Any] = {}
+    elif isinstance(usage_obj, dict):
+        usage = dict(usage_obj)
+    elif hasattr(usage_obj, "model_dump"):
+        usage = usage_obj.model_dump()
+    else:
+        usage = {
+            "input_tokens": getattr(usage_obj, "input_tokens", None),
+            "output_tokens": getattr(usage_obj, "output_tokens", None),
+        }
+
+    return ClaudeResult(
+        text=_extract_text(message), mode=mode, usage=usage, raw=message
+    )
+
+
+# --------------------------------------------------------------------------- #
+# Dispatch + binding
+# --------------------------------------------------------------------------- #
+
+
+def real_invoker(prompt: str, *, mode: BillingMode, **kw: Any) -> ClaudeResult:
+    """Dispatch to the per-mode real invoker (the ``billing.Invoker`` contract).
+
+    ``BEDROCK`` is the rare cross-family tiebreak path and is not wired for P1;
+    it raises :class:`NotImplementedError` honestly. Flipping it on is a later
+    config change, not a code rewrite of the seam.
+    """
+    if mode is BillingMode.SUBSCRIPTION:
+        return subscription_invoker(prompt, mode=mode, **kw)
+    if mode is BillingMode.API:
+        return api_invoker(prompt, mode=mode, **kw)
+    if mode is BillingMode.BEDROCK:
+        raise NotImplementedError(
+            "BEDROCK billing is the rare cross-family tiebreak path and is not "
+            "wired for P1; enable it later via config-flip once the cross-account "
+            "Bedrock transport is provisioned."
+        )
+    raise NotImplementedError(f"no invoker for billing mode {mode!r}")
+
+
+def bind_invoker(invoker: Callable[..., ClaudeResult] | None = None) -> None:
+    """Bind a real invoker into the billing seam in one line at startup.
+
+    Defaults to :func:`real_invoker` (mode-dispatching). Not called at import
+    time so importing this module has no global side effects.
+    """
+    set_invoker(invoker or real_invoker)
+
+
+def bind_subscription_invoker() -> None:
+    """Bind the mode-dispatching real invoker (subscription is the default mode).
+
+    Convenience for the common R720 startup: one call wires
+    :func:`agent_team.billing.claude_invoke` to the real Claude path.
+    """
+    set_invoker(real_invoker)

agent-team/agent_team/nodes/builders_llm.py

@@ -0,0 +1,374 @@
+"""DeepSeek-backed builders binding — the real §3.3 / §7.1 P3 build seam.
+
+:mod:`agent_team.nodes.builders` owns the Plane-2 builders *node* (the §3.3.2
+box-side trust-control-surface denylist + diff-integrity hash) but deliberately
+injects the diff-synthesis step behind an ``DiffBuilder`` seam so the leaf stays
+pure and unit-testable. Its committed default
+(:func:`agent_team.nodes.builders.default_diff_builder`) is a Claude-billing
+*stub* whose docstring (builders.py line ~164) notes the REAL implementation
+wires "DeepSeek (mechanical edits, via the local orchestrator)". This module is
+that real implementation.
+
+Per the locked design, builders are P3 mechanical edits and route to the
+orchestrator's ``fast_coder`` (DeepSeek), NOT to Claude. This module therefore
+does NOT call :func:`agent_team.billing.claude_invoke`; it calls the local
+orchestrator's ``fast_coder`` to produce the candidate diff.
+
+================================  SECURITY BOUNDARY  ========================
+Builders are P3 in the locked design and HARD-GATED: the live CI apply/verify
+trust boundary (§3.3.2) must clear ``/sh-security-review`` + GPT-4.1 cross-review
+BEFORE it goes live. This module is MODEL LOGIC ONLY and MUST stay INERT:
+
+  * It PROPOSES a candidate diff as DATA (a :class:`CandidateDiff` record). It
+    NEVER applies a patch, NEVER shells out to ``git``, NEVER writes to or
+    otherwise mutates the working tree / filesystem, and NEVER makes a live CI
+    call. Applying a diff is the GATED CI path — not this module's job.
+  * The only subprocess this module spawns is a read-only call to the local
+    orchestrator's ``run.py`` to ask ``fast_coder`` for diff TEXT. That
+    subprocess is a model invocation, not a patch application: its stdout is
+    parsed as untrusted data and returned; it touches nothing in the target
+    repo. There is no ``git apply``/``patch``/``git``/``write_text``/``open(...,
+    "w")`` path anywhere in this file — by construction, the builder cannot
+    mutate state.
+
+Because the model output is UNTRUSTED, parsing is defensive and FAILS SAFE: on
+unparseable output, an empty/whitespace diff, or any build error, the builder
+returns an EMPTY/NO-OP candidate marked ``failed`` (``ok is False``) so the
+downstream verifier / CI REJECTS it. It NEVER fabricates a "success" diff.
+============================================================================
+
+Wiring note (no node edit): this module is a standalone real binding. The
+node's injection point is its ``DiffBuilder`` seam — the coordinator should bind
+:func:`default_build` (adapted via :func:`as_diff_builder`) into
+:func:`agent_team.nodes.builders.builders_node` / ``build_candidate_diff`` at
+startup. That wiring edit is deliberately left to the coordinator; this module
+does not edit the node.
+"""
+
+from __future__ import annotations
+
+import subprocess
+from collections.abc import Callable, Mapping
+from dataclasses import dataclass
+from pathlib import Path
+from typing import Any
+
+from agent_team.state_store import compute_content_hash
+
+__all__ = [
+    "BuildCallable",
+    "CandidateDiff",
+    "as_diff_builder",
+    "build_candidate_diff",
+    "default_build",
+]
+
+# The injectable build seam: given the rendered build instruction (a string),
+# return the model's raw candidate-diff text. Tests pass a fake; the default
+# (:func:`default_build`) routes to the orchestrator's DeepSeek ``fast_coder``.
+BuildCallable = Callable[[str], str]
+
+# Default subprocess timeout (seconds) for the orchestrator fast_coder call.
+_DEFAULT_TIMEOUT_S = 600
+
+
+@dataclass
+class CandidateDiff:
+    """A proposed candidate diff emitted as DATA (never applied here).
+
+    This is the record the builders pipeline carries downstream. It mirrors the
+    fields :func:`agent_team.nodes.builders.build_candidate_diff` records on the
+    task — the unified-diff text plus its content-hash — and adds the explicit
+    fail-safe flags so an unparseable/failed build is propagated as a NO-OP the
+    verifier/CI rejects, rather than as a fabricated success.
+
+    Attributes:
+        diff: The candidate unified diff (empty string on a failed/no-op build).
+        diff_hash: Content hash of ``diff`` via
+            :func:`agent_team.state_store.compute_content_hash` (always computed,
+            including over the empty diff, so CI keys against it deterministically).
+        ok: ``True`` only when a non-empty, plausibly-unified diff was produced.
+        failed: ``True`` when the build failed or produced nothing usable (the
+            inverse of :attr:`ok`); kept explicit so a downstream check can read
+            either flag.
+        reason: Human-readable explanation when :attr:`failed`; empty when ``ok``.
+    """
+
+    diff: str
+    diff_hash: str
+    ok: bool
+    failed: bool
+    reason: str = ""
+
+    @classmethod
+    def success(cls, diff: str) -> "CandidateDiff":
+        """Build an ``ok`` candidate from a validated non-empty diff string."""
+        return cls(
+            diff=diff,
+            diff_hash=compute_content_hash(diff.encode("utf-8")),
+            ok=True,
+            failed=False,
+            reason="",
+        )
+
+    @classmethod
+    def no_op(cls, reason: str) -> "CandidateDiff":
+        """Build a FAILED no-op candidate (empty diff) the verifier/CI rejects.
+
+        The empty diff is still hashed so the record shape is uniform and CI's
+        hash check has a deterministic value to compare; the ``failed`` flag is
+        what makes the downstream reject it.
+        """
+        return cls(
+            diff="",
+            diff_hash=compute_content_hash(b""),
+            ok=False,
+            failed=True,
+            reason=reason,
+        )
+
+
+@dataclass
+class _OrchestratorRoute:
+    """Resolved location + runner for the local orchestrator ``run.py``.
+
+    Kept as a tiny dataclass (rather than module-level constants) so the default
+    build call resolves the orchestrator root lazily and a test could swap the
+    runner without importing the orchestrator. No orchestrator code is imported
+    at module top (mirrors :func:`agent_team.graph.build_sqlite_checkpointer`'s
+    deferred-import discipline).
+    """
+
+    root: Path
+    timeout_s: int = _DEFAULT_TIMEOUT_S
+
+
+def _orchestrator_root() -> Path:
+    """Resolve the orchestrator root (the dir holding ``run.py``).
+
+    This file lives at ``<root>/agent-team/agent_team/nodes/builders_llm.py``,
+    so the orchestrator root is ``parents[3]`` (nodes -> agent_team -> agent-team
+    -> <root>). Verified against the real tree: ``parents[2]`` is ``agent-team``,
+    not the root.
+    """
+    return Path(__file__).resolve().parents[3]
+
+
+def default_build(instruction: str, *, route: _OrchestratorRoute | None = None) -> str:
+    """Default :data:`BuildCallable`: route the build to DeepSeek ``fast_coder``.
+
+    Calls the local orchestrator out-of-process — ``python3 <root>/run.py
+    "<instruction>"`` — and returns its stdout. The orchestrator routes a
+    well-specified coding task to its ``fast_coder`` agent (DeepSeek); this is
+    the design's "DeepSeek mechanical edits, via the local orchestrator" path,
+    deliberately NOT :func:`agent_team.billing.claude_invoke`.
+
+    No orchestrator module is imported at module top (deferred, mirroring
+    :func:`agent_team.graph.build_sqlite_checkpointer`); the call is a plain
+    subprocess so this binding adds no import-time dependency on the
+    orchestrator's package graph.
+
+    SECURITY: this subprocess only ASKS the model for diff text — it is a model
+    invocation, not a patch application. It does not run ``git``, does not apply
+    anything, and does not touch the target repo. Its stdout is untrusted input
+    handed back to :func:`build_candidate_diff` for defensive parsing.
+    """
+    route = (
+        route if route is not None else _OrchestratorRoute(root=_orchestrator_root())
+    )
+    run_py = route.root / "run.py"
+    completed = subprocess.run(
+        ["python3", str(run_py), instruction],
+        capture_output=True,
+        text=True,
+        timeout=route.timeout_s,
+        check=True,
+        cwd=str(route.root),
+    )
+    return completed.stdout
+
+
+def _render_build_instruction(plan: Mapping[str, Any], state: Mapping[str, Any]) -> str:
+    """Render the approved plan into a mechanical-edit instruction for fast_coder.
+
+    Pure string assembly over the plan/state (no I/O) so the instruction shape is
+    directly unit-testable. The instruction tells the coder to emit ONLY a single
+    unified diff and to stay inside the declared scope — the box-side denylist in
+    :mod:`agent_team.nodes.builders` is the real enforcement, but reinforcing it
+    in the prompt keeps the model on-task.
+    """
+    title = str(plan.get("title") or plan.get("task") or "(untitled task)")
+    scope = plan.get("scope") or []
+    phases = plan.get("phases") or []
+    repo = ""
+    raw_repo = state.get("repo") if isinstance(state, Mapping) else None
+    if isinstance(raw_repo, str) and raw_repo.strip():
+        repo = raw_repo.strip()
+
+    scope_lines = "\n".join(f"  - {p}" for p in scope) or "  (no scope declared)"
+    phase_lines = (
+        "\n".join(f"  {i + 1}. {p}" for i, p in enumerate(phases)) or "  (none)"
+    )
+    sections = [
+        (
+            "You are performing a mechanical code edit. Implement the approved "
+            "plan below as a SINGLE unified diff in git format. Output ONLY the "
+            "diff — no prose, no explanation, no code fences. Touch ONLY files "
+            "within the declared scope. Do NOT modify CI workflows, IAM/policy "
+            "IaC, branch-protection, CODEOWNERS, or Dependabot config."
+        ),
+        "",
+        f"Title: {title}",
+    ]
+    if repo:
+        sections += [f"Repository: {repo}"]
+    sections += [
+        f"Declared scope (paths you may edit):\n{scope_lines}",
+        f"Phases:\n{phase_lines}",
+    ]
+    return "\n".join(sections)
+
+
+# A line is plausibly part of a unified diff if it opens a git/file/hunk header.
+# Used only to validate that the model returned a diff (not prose) and to strip
+# the orchestrator's framing lines (e.g. ``[retrieved: ...]``, ``[fast_coder]``)
+# that run.py prints before the result body. This is validation/extraction over
+# UNTRUSTED text — never application.
+_DIFF_HEADER_PREFIXES = (
+    "diff --git ",
+    "--- ",
+    "+++ ",
+    "@@ ",
+    "index ",
+    "rename from ",
+    "rename to ",
+    "copy from ",
+    "copy to ",
+    "new file mode ",
+    "deleted file mode ",
+    "old mode ",
+    "new mode ",
+)
+
+
+def _extract_diff(text: str) -> str | None:
+    """Extract a unified diff from UNTRUSTED model/orchestrator output, or ``None``.
+
+    The orchestrator's ``run.py`` prints framing lines (``[retrieved: ...]``, a
+    ``[route]`` line, a blank line) before the agent's result. We locate the
+    first real diff header (``diff --git`` / ``--- `` / ``@@ ``) and return from
+    there to the end, stripping a trailing code-fence if the model wrapped the
+    diff. Returns ``None`` when no diff header is present at all (prose-only /
+    empty output) so the caller fails SAFE to a no-op candidate. Pure text
+    inspection — it never executes or applies the diff.
+    """
+    if not isinstance(text, str) or not text.strip():
+        return None
+
+    lines = text.splitlines()
+    start: int | None = None
+    for idx, line in enumerate(lines):
+        stripped = line.strip()
+        # ``diff --git`` and a real ``--- a/...`` header are the strongest
+        # signals; a lone ``@@`` hunk header also anchors a body-only diff.
+        if (
+            stripped.startswith("diff --git ")
+            or line.startswith("--- ")
+            or stripped.startswith("@@ ")
+        ):
+            start = idx
+            break
+    if start is None:
+        return None
+
+    body_lines = lines[start:]
+    # Drop a trailing markdown fence if the model wrapped the diff in ```.
+    while body_lines and body_lines[-1].strip() in ("```", ""):
+        if body_lines[-1].strip() == "```":
+            body_lines.pop()
+            break
+        body_lines.pop()
+    diff = "\n".join(body_lines).strip()
+    if not diff:
+        return None
+    # Require at least one recognizable diff header line, so a stray ``--- ``
+    # inside prose cannot masquerade as a diff.
+    if not any(
+        any(ln.startswith(p) or ln.strip().startswith(p) for p in _DIFF_HEADER_PREFIXES)
+        for ln in diff.splitlines()
+    ):
+        return None
+    return diff
+
+
+def build_candidate_diff(
+    plan: Mapping[str, Any],
+    state: Mapping[str, Any] | None = None,
+    *,
+    build: BuildCallable | None = None,
+) -> CandidateDiff:
+    """Propose a candidate diff for ``plan`` via DeepSeek ``fast_coder`` (P3).
+
+    Renders the approved ``plan`` (+ optional ``state``) into a mechanical-edit
+    instruction, calls the injected ``build`` callable (default
+    :func:`default_build`, which routes to the orchestrator's DeepSeek
+    ``fast_coder``), defensively parses the UNTRUSTED result, and returns a
+    :class:`CandidateDiff` record.
+
+    FAIL SAFE (never fabricate success): if ``plan`` is not a mapping, the build
+    raises, or the output does not parse to a non-empty unified diff, this
+    returns ``CandidateDiff.no_op(reason)`` — an empty diff marked ``failed`` so
+    the verifier / CI rejects it. A valid diff yields ``CandidateDiff.success``.
+
+    INERT: this function only PROPOSES a diff as data. It does not apply it, run
+    ``git``, or write to the filesystem; applying is the gated CI path.
+    """
+    if not isinstance(plan, Mapping):
+        return CandidateDiff.no_op("approved plan must be a mapping")
+
+    instruction = _render_build_instruction(plan, state or {})
+    build_fn: BuildCallable = build if build is not None else default_build
+
+    try:
+        raw = build_fn(instruction)
+    except subprocess.TimeoutExpired:
+        return CandidateDiff.no_op("build timed out")
+    except subprocess.CalledProcessError as exc:
+        return CandidateDiff.no_op(f"build process failed (exit {exc.returncode})")
+    except Exception as exc:  # noqa: BLE001 - any builder failure must fail SAFE
+        return CandidateDiff.no_op(f"build error: {type(exc).__name__}")
+
+    if not isinstance(raw, str) or not raw.strip():
+        return CandidateDiff.no_op("builder produced empty output")
+
+    diff = _extract_diff(raw)
+    if diff is None:
+        return CandidateDiff.no_op("builder output is not a usable unified diff")
+
+    return CandidateDiff.success(diff)
+
+
+def as_diff_builder(
+    build: BuildCallable | None = None,
+) -> Callable[..., str]:
+    """Adapt this binding to the node's ``DiffBuilder`` seam (keyword signature).
+
+    :func:`agent_team.nodes.builders.build_candidate_diff` calls its injected
+    ``DiffBuilder`` as ``builder(plan=..., config=...)`` and expects a unified
+    -diff STRING back (it then hashes + denylist-scans). This adapter lets the
+    coordinator bind the real DeepSeek path there: it runs
+    :func:`build_candidate_diff` and returns the diff string on success.
+
+    On a failed/no-op build it returns an EMPTY string. The node treats an empty
+    diff as ``BuildError`` (its own fail-closed contract), so the adapter never
+    smuggles a fabricated success past the node either. (The richer
+    :class:`CandidateDiff` record path is available directly via
+    :func:`build_candidate_diff` for callers that want the explicit failed flag.)
+    """
+
+    def _builder(*, plan: Mapping[str, Any], config: Mapping[str, Any] | None) -> str:
+        state = config if isinstance(config, Mapping) else {}
+        candidate = build_candidate_diff(plan, state, build=build)
+        return candidate.diff
+
+    return _builder

agent-team/agent_team/nodes/clarifier_llm.py

@@ -0,0 +1,466 @@
+"""Claude-backed clarifier callables — the real §3.3 / §7.1 P1 bindings.
+
+:mod:`agent_team.nodes.clarifier` owns the *loop* (the LangGraph
+``interrupt()``/resume 98% gate) but deliberately injects the two reasoning
+seams so the loop stays pure and testable:
+
+* ``ConfidenceAssessor = Callable[[Sequence[object], PipelineState], float]``
+* ``QuestionGenerator = Callable[[Sequence[object], PipelineState], list[str]]``
+
+This module supplies the **real, Claude-backed** implementations of those two
+callables. It calls Claude only through the committed
+:func:`agent_team.billing.claude_invoke` seam (§3.1) — never a raw SDK — so the
+billing-mode hygiene and the budget ledger stay in one place.
+
+The naive binding is wasteful: the clarifier loop calls ``assess_confidence``
+and then ``generate_questions`` separately on the same turn, so two independent
+implementations would make **two** Claude calls per turn for what is really one
+reasoning step. :class:`ClaudeClarifier` instead makes **one** Claude call per
+turn and serves both methods from the memoized result. The memo is keyed on the
+Q&A history length, so a new answer (history grows) recomputes, while the
+back-to-back assess/generate pair within one turn reuses the same call.
+
+Defensive parsing is a hard requirement here because the model output is
+UNTRUSTED and this is the **human gate** (§3.3): a parse failure must *never*
+clear the gate. The parser fails SAFE — a missing/garbled confidence defaults to
+``0.0`` (so the loop keeps asking rather than falsely advancing to planning),
+and a missing question-set below threshold falls back to a single generic
+clarifying question (so the loop still has something to ask).
+"""
+
+from __future__ import annotations
+
+import hashlib
+import json
+import re
+from collections.abc import Callable, Sequence
+from typing import Any
+
+from agent_team.billing import ClaudeResult, claude_invoke
+from agent_team.nodes.clarifier import (
+    DEFAULT_CONFIDENCE_THRESHOLD,
+    ConfidenceAssessor,
+    QuestionGenerator,
+)
+from agent_team.task_model import PipelineState
+
+__all__ = [
+    "FALLBACK_QUESTION",
+    "ClaudeClarifier",
+    "build_claude_clarifier_callables",
+]
+
+# The signature the billing seam exposes: ``claude_invoke(prompt, *, mode=None,
+# config=None, **kw) -> ClaudeResult``. Injected so tests pass a fake, mirroring
+# the injection pattern used across this codebase (billing.set_invoker, the
+# clarifier loop's injected callables, etc.).
+ClaudeInvoke = Callable[..., ClaudeResult]
+
+# Used when the model is below the confidence bar but supplied no usable
+# question-set. The loop must always have something to ask rather than spin or
+# falsely advance, so we substitute a generic clarifier prompt.
+FALLBACK_QUESTION = (
+    "Could you share more about the goal, scope, and constraints of this task "
+    "so I can be sure I understand it well enough to plan?"
+)
+
+# Default system framing handed to Claude. Kept as a module constant so callers
+# can override via the ``system`` constructor hook without forking the class.
+_DEFAULT_SYSTEM = (
+    "You are the CLARIFIER stage of an agentic SDLC pipeline and the human "
+    "gate before any planning happens. Your job is to decide whether the "
+    "requirement is understood well enough to plan, drawing conceptually on "
+    "the repo, prior memory, and the engineering handbook. Be rigorous: only "
+    "report high confidence when the goal, scope, and constraints are "
+    "genuinely unambiguous."
+)
+
+
+def _turn_cache_key(
+    qa_history: Sequence[object], state: PipelineState
+) -> tuple[str, int, str]:
+    """Build the task-scoped memo key for one clarifier turn.
+
+    Binds the ``thread_id`` (task isolation), the history length (turn index),
+    and a content hash of the Q&A so far. The thread id is the load-bearing
+    part: one :class:`ClaudeClarifier` instance is shared by the long-lived
+    graph node across every task, so keying on length alone would let one
+    task's cached confidence satisfy another task's gate with no model call.
+    The content hash is belt-and-suspenders so an in-place edit of the same-
+    length history (should one ever occur) also invalidates the memo.
+    """
+    thread_id = str(state.get("thread_id", "") if isinstance(state, dict) else "")
+    try:
+        digest_src = json.dumps(list(qa_history), sort_keys=True, default=repr)
+    except (TypeError, ValueError):
+        digest_src = repr(list(qa_history))
+    content_hash = hashlib.sha1(digest_src.encode("utf-8")).hexdigest()
+    return (thread_id, len(qa_history), content_hash)
+
+
+class ClaudeClarifier:
+    """One Claude call per turn, serving both clarifier callables (§3.3, §7.1 P1).
+
+    Construct with an optional ``invoke`` callable (defaults to
+    :func:`agent_team.billing.claude_invoke`) so tests inject a fake and the
+    real wiring goes through the billing seam. ``model`` / ``config`` are passed
+    through to the invoker, and ``system`` overrides the prompt framing.
+
+    The single call per turn is memoized on a task-scoped key
+    (``thread_id`` + history length + content hash, see :func:`_turn_cache_key`):
+    calling :meth:`assess_confidence` then :meth:`generate_questions` for the
+    same turn of the same task reuses one Claude call; appending an answer (the
+    history grows) or a different task entering the shared node invalidates the
+    memo and the next assess triggers a fresh call. The thread-scoping is what
+    stops one task's cached confidence from clearing another task's human gate.
+
+    :meth:`assess_confidence` and :meth:`generate_questions` are bound methods
+    that match :data:`~agent_team.nodes.clarifier.ConfidenceAssessor` and
+    :data:`~agent_team.nodes.clarifier.QuestionGenerator` exactly, so they wire
+    straight into :func:`~agent_team.nodes.clarifier.make_clarifier_node`.
+    """
+
+    def __init__(
+        self,
+        *,
+        invoke: ClaudeInvoke | None = None,
+        model: str | None = None,
+        config: Any = None,
+        system: str = _DEFAULT_SYSTEM,
+        confidence_threshold: float = DEFAULT_CONFIDENCE_THRESHOLD,
+    ) -> None:
+        self._invoke: ClaudeInvoke = invoke if invoke is not None else claude_invoke
+        self._model = model
+        self._config = config
+        self._system = system
+        self._confidence_threshold = confidence_threshold
+        # Memo of the single per-turn call. The key is task-scoped, NOT just the
+        # history length: one ClaudeClarifier instance serves every task through
+        # the long-lived graph node, so a key of len(qa_history) alone would let
+        # one task's cached high confidence clear ANOTHER task's human gate with
+        # no Claude call (a fail-OPEN cross-task collision). The key therefore
+        # binds (thread_id, history-length, content-hash) so the memo isolates
+        # per task/thread and still recomputes when the Q&A changes.
+        self._cache_key: tuple[str, int, str] | None = None
+        self._cache: dict[str, Any] | None = None
+
+    # ------------------------------------------------------------------ #
+    # Public callables — exact ConfidenceAssessor / QuestionGenerator types.
+    # ------------------------------------------------------------------ #
+
+    def assess_confidence(
+        self, qa_history: Sequence[object], state: PipelineState
+    ) -> float:
+        """Return the current 0..1 confidence the requirement is understood.
+
+        Matches :data:`~agent_team.nodes.clarifier.ConfidenceAssessor`. Serves
+        the memoized per-turn Claude call; fails SAFE to ``0.0`` on any parse
+        trouble so a garbled response never clears the human gate.
+        """
+        return float(self._turn(qa_history, state)["confidence"])
+
+    def generate_questions(
+        self, qa_history: Sequence[object], state: PipelineState
+    ) -> list[str]:
+        """Return the next ordered question-set.
+
+        Matches :data:`~agent_team.nodes.clarifier.QuestionGenerator`. Reuses
+        the same memoized call as :meth:`assess_confidence` for this turn, and
+        always returns a non-empty list (the loop must have something to ask).
+        """
+        return list(self._turn(qa_history, state)["questions"])
+
+    # ------------------------------------------------------------------ #
+    # Internals: the single per-turn call + memo.
+    # ------------------------------------------------------------------ #
+
+    def _turn(
+        self, qa_history: Sequence[object], state: PipelineState
+    ) -> dict[str, Any]:
+        """Return the parsed result for this turn, making at most one Claude call.
+
+        Memoized on ``(thread_id, len(qa_history), content-hash)``: the
+        assess/generate pair within one turn of one task shares a call; once an
+        answer is appended (history grows) or a different task/thread enters the
+        shared node, the key changes and a fresh call is made. Keying on the
+        thread id is what prevents one task's cached confidence from clearing
+        another task's human gate (the fail-OPEN collision the review caught).
+        """
+        key = _turn_cache_key(qa_history, state)
+        if self._cache_key == key and self._cache is not None:
+            return self._cache
+
+        prompt = self._build_prompt(qa_history, state)
+        result = self._invoke(prompt, model=self._model, config=self._config)
+        parsed = self._parse(getattr(result, "text", ""))
+
+        self._cache_key = key
+        self._cache = parsed
+        return parsed
+
+    def _build_prompt(self, qa_history: Sequence[object], state: PipelineState) -> str:
+        """Assemble the clarifier prompt from the Q&A history and task state.
+
+        Pure string assembly over the graph state (§3.3) — no I/O — so the
+        prompt shape is directly unit-testable.
+        """
+        description = _task_description(state)
+        repo = _state_field(state, "repo")
+        context = _state_field(state, "context")
+        qa = _format_qa_history(qa_history)
+        threshold_pct = int(round(self._confidence_threshold * 100))
+
+        sections: list[str] = [
+            self._system,
+            "",
+            "## Task",
+            description or "(no task description provided)",
+        ]
+        if repo:
+            sections += ["", "## Repository", repo]
+        if context:
+            sections += ["", "## Additional context", context]
+        sections += [
+            "",
+            "## Clarifier Q&A so far (oldest first)",
+            qa or "(no questions answered yet)",
+            "",
+            "## Your job",
+            (
+                f"Decide whether you are at least {threshold_pct}% confident the "
+                "requirement is understood well enough to plan. If you are NOT, "
+                "produce the next ordered set of clarifying questions to ask the "
+                "human. Ask only what is genuinely needed; order them most "
+                "important first."
+            ),
+            "",
+            "## Output format",
+            (
+                "Respond with ONLY a strict JSON object and no prose outside it, "
+                'with keys: "confidence" (a float in [0, 1]), "questions" (a list '
+                "of strings; empty only when you are confident enough to plan), "
+                'and "rationale" (a short string). Example: '
+                '{"confidence": 0.42, "questions": ["..."], "rationale": "..."}'
+            ),
+        ]
+        return "\n".join(sections)
+
+    def _parse(self, text: str) -> dict[str, Any]:
+        """Parse the UNTRUSTED model reply into ``{confidence, questions, rationale}``.
+
+        Fails SAFE at every step (§3.3 human gate):
+
+        * confidence missing/unparseable -> ``0.0`` (keep asking, never clear
+          the gate on a garbled reply);
+        * confidence out of range -> clamped into ``[0, 1]``;
+        * questions missing/empty while below threshold -> a single generic
+          fallback question so the loop always has something to ask.
+
+        A parse error is swallowed into the fail-safe default rather than
+        raised, so a bad reply degrades to "ask again", never to "advance".
+        """
+        data = _extract_json_object(text)
+
+        confidence = _coerce_confidence(data.get("confidence") if data else None)
+        questions = _coerce_questions(data.get("questions") if data else None)
+        rationale = ""
+        if data is not None:
+            raw_rationale = data.get("rationale")
+            if isinstance(raw_rationale, str):
+                rationale = raw_rationale.strip()
+
+        if not questions and confidence < self._confidence_threshold:
+            # Below the bar but no usable question-set: substitute a generic
+            # clarifier so the loop still asks rather than spinning or advancing.
+            questions = [FALLBACK_QUESTION]
+
+        return {
+            "confidence": confidence,
+            "questions": questions,
+            "rationale": rationale,
+        }
+
+
+def build_claude_clarifier_callables(
+    *,
+    invoke: ClaudeInvoke | None = None,
+    model: str | None = None,
+    config: Any = None,
+    system: str = _DEFAULT_SYSTEM,
+    confidence_threshold: float = DEFAULT_CONFIDENCE_THRESHOLD,
+) -> tuple[ConfidenceAssessor, QuestionGenerator]:
+    """Build the ``(assess_confidence, generate_questions)`` pair for wiring.
+
+    Returns the two bound methods of a single shared :class:`ClaudeClarifier`,
+    ready to hand straight to
+    :func:`~agent_team.nodes.clarifier.make_clarifier_node`. Because both
+    callables share one instance, they share the per-turn memo, so the loop
+    makes one Claude call per turn rather than two.
+    """
+    clarifier = ClaudeClarifier(
+        invoke=invoke,
+        model=model,
+        config=config,
+        system=system,
+        confidence_threshold=confidence_threshold,
+    )
+    return clarifier.assess_confidence, clarifier.generate_questions
+
+
+# --------------------------------------------------------------------------- #
+# Module-level helpers (pure; no I/O).
+# --------------------------------------------------------------------------- #
+
+
+def _state_field(state: PipelineState, key: str) -> str:
+    """Pull a string field from the (untyped-extra) graph state, defensively."""
+    value = state.get(key)  # type: ignore[call-overload]
+    if isinstance(value, str) and value.strip():
+        return value.strip()
+    return ""
+
+
+def _task_description(state: PipelineState) -> str:
+    """Pull the task description out of the graph state (mirrors planner.py).
+
+    Looks in the conventional places (the ``plan`` dict, then a top-level
+    ``task``/``description`` key) and falls back to an empty string so a
+    malformed state surfaces as an empty prompt section, never a ``KeyError``.
+    """
+    plan = state.get("plan") or {}
+    if isinstance(plan, dict):
+        desc = plan.get("task") or plan.get("description")
+        if isinstance(desc, str) and desc.strip():
+            return desc.strip()
+    for key in ("task", "description"):
+        desc = _state_field(state, key)
+        if desc:
+            return desc
+    return ""
+
+
+def _format_qa_history(qa_history: Sequence[object]) -> str:
+    """Render the clarifier Q&A history (oldest first) into prompt text.
+
+    Each entry may be a ``{"question": ..., "answer": ...}`` mapping or a plain
+    string (the raw resume value the loop appends); both are handled so this
+    does not couple to a single record shape.
+    """
+    lines: list[str] = []
+    for idx, entry in enumerate(qa_history, start=1):
+        if isinstance(entry, dict):
+            question = str(entry.get("question", "")).strip()
+            answer = str(entry.get("answer", "")).strip()
+            if question or answer:
+                lines.append(f"{idx}. Q: {question}\n   A: {answer}")
+        else:
+            text = str(entry).strip()
+            if text:
+                lines.append(f"{idx}. {text}")
+    return "\n".join(lines)
+
+
+# A fenced ```json ... ``` block, if the model wrapped its JSON in Markdown.
+_FENCE_RE = re.compile(
+    r"```(?:json)?\s*\n?(?P<body>.*?)\n?\s*```",
+    flags=re.DOTALL | re.IGNORECASE,
+)
+
+
+def _extract_json_object(text: str) -> dict[str, Any] | None:
+    """Extract a JSON object from UNTRUSTED model output, or ``None``.
+
+    Tolerates the common ways a model deviates from "JSON only": a leading
+    apology or trailing prose, and ```json fences. Tries, in order, the whole
+    string, the contents of a fenced block, then the first ``{...}`` span found
+    by brace matching. Returns ``None`` (never raises) when nothing parses to a
+    JSON object, so the caller can fail SAFE.
+    """
+    if not isinstance(text, str) or not text.strip():
+        return None
+
+    candidates: list[str] = [text.strip()]
+
+    fence = _FENCE_RE.search(text)
+    if fence:
+        candidates.append(fence.group("body").strip())
+
+    span = _first_brace_span(text)
+    if span is not None:
+        candidates.append(span)
+
+    for candidate in candidates:
+        if not candidate:
+            continue
+        try:
+            parsed = json.loads(candidate)
+        except (json.JSONDecodeError, ValueError):
+            continue
+        if isinstance(parsed, dict):
+            return parsed
+    return None
+
+
+def _first_brace_span(text: str) -> str | None:
+    """Return the first balanced ``{...}`` span in ``text`` (string-aware)."""
+    start = text.find("{")
+    if start == -1:
+        return None
+    depth = 0
+    in_string = False
+    escaped = False
+    for idx in range(start, len(text)):
+        ch = text[idx]
+        if in_string:
+            if escaped:
+                escaped = False
+            elif ch == "\\":
+                escaped = True
+            elif ch == '"':
+                in_string = False
+            continue
+        if ch == '"':
+            in_string = True
+        elif ch == "{":
+            depth += 1
+        elif ch == "}":
+            depth -= 1
+            if depth == 0:
+                return text[start : idx + 1]
+    return None
+
+
+def _coerce_confidence(value: Any) -> float:
+    """Coerce the model's confidence into a clamped ``[0, 1]`` float.
+
+    Missing or unparseable -> ``0.0`` (fail SAFE: keep asking, never clear the
+    gate). Out-of-range values are clamped rather than rejected.
+    """
+    try:
+        confidence = float(value)
+    except (TypeError, ValueError):
+        return 0.0
+    if confidence != confidence:  # NaN guard
+        return 0.0
+    if confidence < 0.0:
+        return 0.0
+    if confidence > 1.0:
+        return 1.0
+    return confidence
+
+
+def _coerce_questions(value: Any) -> list[str]:
+    """Coerce the model's question-set into a clean list of non-empty strings.
+
+    Anything that is not a list of usable strings collapses to an empty list,
+    which the parser then fills with the generic fallback when below threshold.
+    """
+    if not isinstance(value, list):
+        return []
+    questions: list[str] = []
+    for item in value:
+        if isinstance(item, str):
+            text = item.strip()
+            if text:
+                questions.append(text)
+    return questions

agent-team/agent_team/nodes/review_loop.py

@@ -43,6 +43,7 @@ from __future__ import annotations
 
 import json
 import os
+import re
 import subprocess
 from dataclasses import dataclass
 from datetime import datetime, timezone
@@ -57,6 +58,7 @@ __all__ = [
     "ReviewOutcome",
     "ReviewResult",
     "ReviewVerdict",
+    "bind_review_node",
     "review_node",
     "route_after_review",
     "set_review_invoker",
@@ -82,21 +84,59 @@ _MAX_ROUNDS_ENV = "AGENT_TEAM_MAX_REVIEW_ROUNDS"
 _RUN_PY_CONFIG_KEY = "orchestrator_run_py"
 _DEFAULT_RUN_PY = os.path.expanduser("~/Documents/repositories/orchestrator/run.py")
 
-# Verdict tokens the reviewer output is scanned for. REQUEST_CHANGES wins on a
+# Config / env keys + default for the orchestrator subprocess timeout (seconds).
-# tie so an ambiguous review fails closed (loops back / escalates) rather than
+# Bounds the default shell-out so a hung run.py cannot stall the bounded review
-# advancing a plan the reviewer flagged.
+# loop. Mirrors the resolver in review_loop_llm so the two cannot drift.
-_APPROVE_TOKENS = ("APPROVE", "APPROVED", "LGTM", "NO BLOCKERS", "NO BLOCKING")
+_TIMEOUT_CONFIG_KEY = "review_timeout_seconds"
+_TIMEOUT_ENV = "AGENT_TEAM_REVIEW_TIMEOUT_SECONDS"
+_DEFAULT_TIMEOUT_SECONDS = 600.0
+
+# Sentinel returned by the default invoker when the orchestrator subprocess
+# times out. It parses (via parse_verdict) to REQUEST_CHANGES, so a hung run.py
+# fails CLOSED (loops back / escalates) instead of blocking the bounded loop.
+_TIMEOUT_VERDICT_TEXT = (
+    "REQUEST CHANGES: orchestrator review timed out (failing closed)."
+)
+
+# Verdict tokens the reviewer output is scanned for, matched on WORD BOUNDARIES
+# (not substrings). The change marker is the sh-plan-review rubric token
+# ``BLOCK`` (e.g. "BLOCK: ..."), matched as a whole word so it does NOT fire
+# inside benign prose like "no blockers" / "no blocking issues" (the substring
+# false-positive this guards against — those inflected words are deliberately
+# NOT change tokens). The former "NO BLOCKERS"/"NO BLOCKING" approve tokens
+# existed only to undo that substring false-positive; with word-boundary
+# matching they are unreachable (such bare prose is genuinely ambiguous and must
+# fail closed), so they are dropped. REQUEST_CHANGES still wins on a tie so an
+# ambiguous review fails closed (loops back / escalates) rather than advancing a
+# flagged plan.
+_APPROVE_TOKENS = ("APPROVE", "APPROVED", "LGTM")
 _CHANGES_TOKENS = (
     "REQUEST CHANGES",
     "REQUEST_CHANGES",
     "REQUESTCHANGES",
     "BLOCK",
-    "BLOCKING",
     "NEEDS CHANGES",
     "NEEDS WORK",
 )
 
 
+def _compile_token_pattern(tokens: tuple[str, ...]) -> re.Pattern[str]:
+    """Compile an alternation of ``tokens`` matched on word boundaries.
+
+    Word-boundary anchoring is what keeps ``BLOCK`` from matching inside
+    ``BLOCKERS``/``BLOCKING`` (the substring false-positive this guards against).
+    Tokens are sorted longest-first so a multi-word token (e.g. ``REQUEST
+    CHANGES``) is preferred over a shorter overlapping one.
+    """
+    ordered = sorted(tokens, key=len, reverse=True)
+    alternation = "|".join(re.escape(tok) for tok in ordered)
+    return re.compile(rf"\b(?:{alternation})\b", re.IGNORECASE)
+
+
+_APPROVE_RE = _compile_token_pattern(_APPROVE_TOKENS)
+_CHANGES_RE = _compile_token_pattern(_CHANGES_TOKENS)
+
+
 class ReviewVerdict(Enum):
     """The adversarial reviewer's verdict on a plan (design §3.3)."""
 
@@ -155,7 +195,13 @@ class ReviewResult:
 ReviewInvoker = Callable[..., str]
 
 
-def _orchestrator_invoker(prompt: str, *, run_py: str, **_kw: Any) -> str:
+def _orchestrator_invoker(
+    prompt: str,
+    *,
+    run_py: str,
+    config: Mapping[str, Any] | None = None,
+    **_kw: Any,
+) -> str:
     """Default reviewer: call the orchestrator's ``cross_reviewer`` (GPT-4.1).
 
     Invokes the local ``run.py`` with the review prompt. The orchestrator's
@@ -163,18 +209,28 @@ def _orchestrator_invoker(prompt: str, *, run_py: str, **_kw: Any) -> str:
     keeps the review cross-family (a different model than the Claude planner)
     and API-billed + LangSmith-traced per design §3.2. Returns the orchestrator's
     stdout (the reviewer's verdict + findings).
+
+    The call is bounded by :func:`_resolve_timeout`. If ``run.py`` hangs past the
+    timeout the subprocess is killed and a REQUEST_CHANGES sentinel is returned
+    (fail CLOSED) so a stuck review cannot block the bounded loop — rather than
+    raising, which would crash :func:`review_node` (it does not wrap the call).
     """
     if not os.path.exists(run_py):
         raise FileNotFoundError(
             f"orchestrator entry point not found: {run_py}; set "
             f"config[{_RUN_PY_CONFIG_KEY!r}] or rebind via set_review_invoker()."
         )
-    completed = subprocess.run(  # noqa: S603 - args are not shell-interpolated
+    try:
-        ["python3", run_py, prompt],
+        completed = subprocess.run(  # noqa: S603 - args are not shell-interpolated
-        capture_output=True,
+            ["python3", run_py, prompt],
-        text=True,
+            capture_output=True,
-        check=False,
+            text=True,
-    )
+            check=False,
+            timeout=_resolve_timeout(config),
+        )
+    except subprocess.TimeoutExpired:
+        # Hung run.py -> fail CLOSED (do not block the bounded loop).
+        return _TIMEOUT_VERDICT_TEXT
     if completed.returncode != 0:
         raise RuntimeError(
             "orchestrator review call failed "
@@ -226,6 +282,27 @@ def _resolve_max_rounds(config: Mapping[str, Any] | None) -> int:
     return value
 
 
+def _resolve_timeout(config: Mapping[str, Any] | None) -> float:
+    """Resolve the orchestrator subprocess timeout (seconds) from config/env/default.
+
+    A non-positive or non-numeric value falls back to the default so a
+    misconfigured knob cannot disable the bound. Mirrors the resolver in
+    :mod:`agent_team.nodes.review_loop_llm` so the two cannot drift.
+    """
+    raw: Any = None
+    if config is not None:
+        raw = config.get(_TIMEOUT_CONFIG_KEY)
+    if raw is None:
+        raw = os.environ.get(_TIMEOUT_ENV)
+    if raw is None:
+        return _DEFAULT_TIMEOUT_SECONDS
+    try:
+        value = float(raw)
+    except (TypeError, ValueError):
+        return _DEFAULT_TIMEOUT_SECONDS
+    return value if value > 0 else _DEFAULT_TIMEOUT_SECONDS
+
+
 def _resolve_run_py(config: Mapping[str, Any] | None) -> str:
     """Resolve the orchestrator ``run.py`` path from config, env, or default."""
     if config is not None:
@@ -242,17 +319,17 @@ def parse_verdict(text: str) -> ReviewVerdict:
     """Parse a :class:`ReviewVerdict` from the reviewer's free text.
 
     Scans for explicit ``REQUEST CHANGES`` / ``BLOCK`` tokens and ``APPROVE`` /
-    ``LGTM`` tokens (case-insensitive). The result **fails closed**: if a
+    ``LGTM`` tokens (case-insensitive, **word-boundary** matched so that prose
-    change-requesting token is present, or if neither token class is present
+    like "no blocking issues" inside an APPROVE does not trip a change token).
-    (an ambiguous / empty review), the verdict is ``REQUEST_CHANGES`` so an
+    The result **fails closed**: if a change-requesting token is present, or if
-    unclear review never silently advances a plan to the builders.
+    neither token class is present (an ambiguous / empty review), the verdict is
+    ``REQUEST_CHANGES`` so an unclear review never silently advances a plan to
+    the builders.
     """
-    haystack = (text or "").upper()
+    haystack = text or ""
-    has_changes = any(token in haystack for token in _CHANGES_TOKENS)
+    if _CHANGES_RE.search(haystack):
-    has_approve = any(token in haystack for token in _APPROVE_TOKENS)
-    if has_changes:
         return ReviewVerdict.REQUEST_CHANGES
-    if has_approve:
+    if _APPROVE_RE.search(haystack):
         return ReviewVerdict.APPROVE
     # Ambiguous / empty review -> fail closed.
     return ReviewVerdict.REQUEST_CHANGES
@@ -377,6 +454,28 @@ def review_node(
     return update
 
 
+def bind_review_node(
+    config: Mapping[str, Any] | None = None,
+) -> Callable[[PipelineState], PipelineState]:
+    """Return a **single-argument** review node bound to ``config`` (P2 wiring).
+
+    :func:`review_node` takes an optional ``config`` second argument. If it is
+    handed to LangGraph directly, LangGraph sees the ``config`` parameter and
+    injects its own ``RunnableConfig`` there, which (a) emits a typing
+    ``UserWarning`` and (b) means the task's ``max_review_rounds`` / timeout /
+    ``run_py`` overrides never reach the node. Wrapping it as a one-arg closure
+    over the intended ``config`` keeps the node free of a LangGraph-managed
+    ``config`` param (no warning, no injection) and threads the *real* review
+    config through. The coordinator passes the bound node to
+    :func:`agent_team.graph.build_graph` as ``review_node``.
+    """
+
+    def node(state: PipelineState) -> PipelineState:
+        return review_node(state, config)
+
+    return node
+
+
 def route_after_review(state: PipelineState) -> str:
     """LangGraph conditional-edge: next node after the review loop.
 

agent-team/agent_team/nodes/review_loop_llm.py

@@ -0,0 +1,292 @@
+"""GPT-4.1 cross-family review binding for the REVIEW_LOOP node (design §3.3, §7.1 P2).
+
+:mod:`agent_team.nodes.review_loop` owns the *loop* — the bounded
+approve / loop-back-to-planner / escalate-to-Adam state machine — but it
+deliberately routes the actual review call through an injectable
+:data:`~agent_team.nodes.review_loop.ReviewInvoker` seam (rebound via
+``set_review_invoker``) so the loop stays pure and unit-testable.
+
+This module supplies the **real** implementation of that seam: a single
+function, :func:`review_plan`, that takes the planner's phased plan plus the
+graph state and returns the node-contract verdict
+(:class:`~agent_team.nodes.review_loop.ReviewVerdict`) the loop consumes.
+
+Model routing — this node is GPT-4.1, NOT Claude
+------------------------------------------------
+Per the locked design the review loop runs an **independent, cross-family**
+review of the plan via the orchestrator's ``cross_reviewer`` agent
+(``gpt-4.1`` — a different model family than the Claude planner, so it catches
+different blind spots). The R720 coordinator "reuses the local rsync'd
+``orchestrator/run.py`` in place for non-Claude single-shots" (design §3.2), so
+the default review call is **not** routed through
+:func:`agent_team.billing.claude_invoke` (the *Claude* seam). It instead shells
+out to ``python3 <orchestrator_root>/run.py "<review task>"``, whose router
+sends adversarial-review tasks to ``cross_reviewer`` (GPT-4.1), keeping the call
+API-billed and LangSmith-traced.
+
+``<orchestrator_root>`` resolves to ``Path(__file__).resolve().parents[3]``
+(this file lives at ``<root>/agent-team/agent_team/nodes/review_loop_llm.py``,
+so parents[0]=nodes, [1]=agent_team, [2]=agent-team, [3]=<root>). The
+orchestrator package is **never imported at module load** — the subprocess path
+needs no import at all, and the optional in-process fallback
+(:func:`make_cross_reviewer_invoker`) imports ``models`` lazily, inside the
+call, mirroring the deferred-import discipline of
+``graph.build_sqlite_checkpointer``.
+
+The review callable is INJECTABLE (``review=`` parameter / the
+``PlanReviewer`` type) so tests pass a fake and never touch the network or a
+subprocess. The default is :func:`default_plan_reviewer`.
+
+Contract
+--------
+* **Input** to the review callable: a single string — the composed review task
+  (the plan plus prior-round context), built by
+  :func:`~agent_team.nodes.review_loop.build_review_prompt`.
+* **Output** of the review callable: the reviewer's verdict text (free-form),
+  which :func:`~agent_team.nodes.review_loop.parse_verdict` maps to a
+  :class:`~agent_team.nodes.review_loop.ReviewVerdict`.
+
+Fail-safe (UNTRUSTED output, never auto-approve on a bad review)
+----------------------------------------------------------------
+The reviewer's text is untrusted. Parsing is defensive and **fails closed**: if
+the call errors (subprocess failure, timeout, exception) or the output is empty
+/ unparseable / ambiguous, :func:`review_plan` returns
+:attr:`~agent_team.nodes.review_loop.ReviewVerdict.REQUEST_CHANGES` — the SAFE
+branch the loop treats as "do not auto-approve" (loop back to the planner, or
+escalate to Adam once the round cap is hit). A plan is **only ever** approved on
+an explicit, cleanly-parsed APPROVE verdict.
+
+Wiring (one-line injection point, added later — this module edits nothing)
+--------------------------------------------------------------------------
+At startup a leaf binds this real reviewer into the loop's seam by passing
+:func:`default_plan_reviewer` — which already matches the ``ReviewInvoker``
+``(prompt, **kw) -> str`` shape (it returns the reviewer's verdict *text*, which
+the loop's own :func:`~agent_team.nodes.review_loop.parse_verdict` maps to a
+verdict)::
+
+    from agent_team.nodes import review_loop
+    from agent_team.nodes.review_loop_llm import default_plan_reviewer
+    review_loop.set_review_invoker(default_plan_reviewer)
+
+This is a single line in the wiring module; this file does not edit the node.
+"""
+
+from __future__ import annotations
+
+import os
+import subprocess
+from collections.abc import Callable
+from pathlib import Path
+from typing import Any
+
+from agent_team.nodes.review_loop import (
+    ReviewVerdict,
+    build_review_prompt,
+    parse_verdict,
+)
+from agent_team.task_model import PipelineState
+
+__all__ = [
+    "PlanReviewer",
+    "default_plan_reviewer",
+    "make_cross_reviewer_invoker",
+    "make_run_py_invoker",
+    "resolve_orchestrator_root",
+    "resolve_run_py",
+    "review_plan",
+]
+
+# The injectable review callable: takes the composed review task (a string) and
+# returns the reviewer's verdict text (a string). The default shells out to the
+# orchestrator's cross_reviewer (GPT-4.1) via the local run.py. Tests rebind it
+# by passing review=<fake> to review_plan().
+PlanReviewer = Callable[..., str]
+
+# Config / env keys naming the orchestrator entry point (the local run.py) and a
+# subprocess timeout. Overridable for tests and non-default installs without
+# editing this module.
+_RUN_PY_CONFIG_KEY = "orchestrator_run_py"
+_RUN_PY_ENV = "AGENT_TEAM_ORCHESTRATOR_RUN_PY"
+_TIMEOUT_CONFIG_KEY = "review_timeout_seconds"
+_TIMEOUT_ENV = "AGENT_TEAM_REVIEW_TIMEOUT_SECONDS"
+_DEFAULT_TIMEOUT_SECONDS = 600.0
+
+
+def resolve_orchestrator_root() -> Path:
+    """Return the orchestrator root dir (the parent that holds ``run.py``).
+
+    This file lives at ``<root>/agent-team/agent_team/nodes/review_loop_llm.py``,
+    so the root is ``parents[3]`` of the resolved module path
+    (parents[0]=nodes, [1]=agent_team, [2]=agent-team, [3]=<root>). Resolved
+    lazily at call time — the orchestrator package itself is never imported here.
+    """
+    return Path(__file__).resolve().parents[3]
+
+
+def resolve_run_py(config: Any = None) -> str:
+    """Resolve the orchestrator ``run.py`` path from config, then env, then default.
+
+    The default is ``<orchestrator_root>/run.py`` (the rsync'd path the R720
+    coordinator reuses in place, design §3.2). Mirrors the resolution order used
+    by the loop node so the two cannot drift.
+    """
+    if isinstance(config, dict):
+        configured = config.get(_RUN_PY_CONFIG_KEY)
+        if configured:
+            return os.path.expanduser(str(configured))
+    env = os.environ.get(_RUN_PY_ENV)
+    if env:
+        return os.path.expanduser(env)
+    return str(resolve_orchestrator_root() / "run.py")
+
+
+def _resolve_timeout(config: Any = None) -> float:
+    """Resolve the subprocess timeout (seconds) from config, env, or default."""
+    raw: Any = None
+    if isinstance(config, dict):
+        raw = config.get(_TIMEOUT_CONFIG_KEY)
+    if raw is None:
+        raw = os.environ.get(_TIMEOUT_ENV)
+    if raw is None:
+        return _DEFAULT_TIMEOUT_SECONDS
+    try:
+        value = float(raw)
+    except (TypeError, ValueError):
+        return _DEFAULT_TIMEOUT_SECONDS
+    return value if value > 0 else _DEFAULT_TIMEOUT_SECONDS
+
+
+def make_run_py_invoker() -> PlanReviewer:
+    """Build the default reviewer: shell out to the orchestrator's ``run.py``.
+
+    Returns a callable ``(prompt, *, config=None, **kw) -> str`` that runs
+    ``python3 <run_py> "<prompt>"`` and returns its stdout (the reviewer's
+    verdict text). The orchestrator's router sends adversarial-review tasks to
+    ``cross_reviewer`` (GPT-4.1), keeping the review cross-family and
+    API-billed + LangSmith-traced (design §3.2). No orchestrator import is
+    needed for this path at all.
+
+    A missing ``run.py``, a non-zero exit, or a timeout raises — the caller
+    (:func:`review_plan`) turns any such error into the fail-safe
+    REQUEST_CHANGES verdict, so a broken review never auto-approves a plan.
+    """
+
+    def _invoke(prompt: str, *, config: Any = None, **_kw: Any) -> str:
+        run_py = resolve_run_py(config)
+        if not os.path.exists(run_py):
+            raise FileNotFoundError(
+                f"orchestrator entry point not found: {run_py}; set "
+                f"config[{_RUN_PY_CONFIG_KEY!r}] or pass review=<callable> to "
+                "review_plan()."
+            )
+        completed = subprocess.run(  # noqa: S603 - args are not shell-interpolated
+            ["python3", run_py, prompt],
+            capture_output=True,
+            text=True,
+            check=False,
+            timeout=_resolve_timeout(config),
+        )
+        if completed.returncode != 0:
+            raise RuntimeError(
+                "orchestrator review call failed "
+                f"(exit {completed.returncode}): {completed.stderr.strip()}"
+            )
+        return completed.stdout
+
+    return _invoke
+
+
+def make_cross_reviewer_invoker() -> PlanReviewer:
+    """Build an in-process reviewer that calls ``cross_reviewer`` (GPT-4.1) directly.
+
+    Optional alternative to :func:`make_run_py_invoker` for callers that would
+    rather invoke the model in-process than spawn ``run.py``. The orchestrator's
+    ``models`` module is imported **lazily, inside the call** (never at module
+    load), mirroring ``graph.build_sqlite_checkpointer``'s deferred-import
+    pattern, so importing this module never pulls in the orchestrator stack.
+
+    Like the subprocess path, any error propagates so :func:`review_plan` can
+    fail safe to REQUEST_CHANGES.
+    """
+
+    def _invoke(prompt: str, *, config: Any = None, **_kw: Any) -> str:
+        # Deferred import: keep the orchestrator package out of module import.
+        from models import get_cross_reviewer  # noqa: PLC0415
+
+        reviewer = get_cross_reviewer()
+        result = reviewer.invoke(prompt)
+        text = getattr(result, "content", result)
+        return text if isinstance(text, str) else str(text)
+
+    return _invoke
+
+
+# The default reviewer: subprocess to run.py (the design's "reuse run.py in
+# place" path). Built once; resolution of run.py / timeout still happens per
+# call so config and env overrides apply.
+default_plan_reviewer: PlanReviewer = make_run_py_invoker()
+
+
+def review_plan(
+    plan: Any = None,
+    state: PipelineState | None = None,
+    *,
+    prompt: str | None = None,
+    review: PlanReviewer | None = None,
+    config: Any = None,
+    **kw: Any,
+) -> ReviewVerdict:
+    """Run one cross-family (GPT-4.1) review of ``plan`` and return the verdict.
+
+    This is the REAL implementation of the loop's review seam. It composes the
+    review task, calls the injected ``review`` callable (default
+    :func:`default_plan_reviewer`, which shells out to the orchestrator's
+    ``cross_reviewer`` via ``run.py``), and maps the reviewer's free-form text to
+    a :class:`~agent_team.nodes.review_loop.ReviewVerdict` via the loop's own
+    :func:`~agent_team.nodes.review_loop.parse_verdict`.
+
+    Inputs are flexible so this slots in behind either calling convention:
+
+    * ``review_plan(plan, state)`` — compose the prompt from ``plan``/``state``
+      with :func:`~agent_team.nodes.review_loop.build_review_prompt`; or
+    * ``review_plan(prompt=...)`` — review an already-composed prompt (this is
+      the shape the ``ReviewInvoker`` seam hands in, so binding it as the
+      invoker is a one-liner).
+
+    FAIL SAFE: the reviewer output is UNTRUSTED. If the ``review`` call raises
+    (subprocess failure, timeout, any exception) or returns empty / non-string /
+    unparseable / ambiguous text, this returns
+    :attr:`~agent_team.nodes.review_loop.ReviewVerdict.REQUEST_CHANGES` — the
+    SAFE branch the loop treats as "do not auto-approve" (loop back, or escalate
+    to Adam at the round cap). A plan is approved **only** on an explicit,
+    cleanly-parsed APPROVE verdict; a failed or garbage review never approves.
+    """
+    reviewer = review if review is not None else default_plan_reviewer
+
+    task = prompt
+    if task is None:
+        # Compose the review task from the plan + prior-round context. Accept a
+        # bare plan dict by adapting it into the minimal state shape the prompt
+        # builder reads, so callers need not hand-build a full PipelineState.
+        review_state: PipelineState
+        if isinstance(state, dict):
+            review_state = state
+        elif isinstance(plan, dict):
+            review_state = {"plan": plan}  # type: ignore[assignment]
+        else:
+            # No usable plan/state to review -> fail safe, never auto-approve.
+            return ReviewVerdict.REQUEST_CHANGES
+        task = build_review_prompt(review_state)
+
+    try:
+        raw = reviewer(task, config=config, **kw)
+    except Exception:
+        # Any failure in the review call (subprocess error, timeout, bad import)
+        # -> fail safe. Never auto-approve a plan on a failed review.
+        return ReviewVerdict.REQUEST_CHANGES
+
+    text = raw if isinstance(raw, str) else "" if raw is None else str(raw)
+    # parse_verdict itself fails closed on empty / ambiguous text, but we route
+    # everything through it so the verdict tokens stay single-sourced in the
+    # loop node and the two cannot drift.
+    return parse_verdict(text)

agent-team/agent_team/nodes/verifier_llm.py

@@ -0,0 +1,494 @@
+"""Claude-backed verifier bindings — the real §3.3 / §3.3.2 P3 logic.
+
+:mod:`agent_team.nodes.verifier` owns the VERIFY-stage LangGraph node and the
+phase transitions, but it injects its reasoning seam (the fix-advisor,
+:data:`~agent_team.nodes.verifier.FixAdvisor`) so the loop stays pure and
+testable. This module supplies the **real** implementation of that seam, plus a
+thin pure-code verdict wrapper, mirroring how :mod:`clarifier_llm` backs the
+clarifier seams.
+
+The single load-bearing rule from design §3.3.2 boundary #4 is enforced
+STRUCTURALLY by the shape of this module, not by convention:
+
+    **The LLM verifier cannot declare green.** Pass/fail is owned by a pure-code
+    gate (:mod:`agent_team.ci_gate`) over the authenticated, patch-independent CI
+    Checks result (keyed to ``run_id`` + ``diff_hash``). The LLM verifier may
+    PROPOSE fixes but can NEVER flip the verdict to pass.
+
+So this module keeps two things rigorously SEPARATE:
+
+1. :func:`evaluate_verdict` — a PURE-CODE function that maps an authenticated CI
+   Checks result -> pass/fail. It is a thin compose over
+   :func:`agent_team.ci_gate.evaluate_ci_gate`; it reuses that gate verbatim and
+   does NOT reimplement or weaken it. It FAILS SAFE: a missing, ambiguous, or
+   unauthenticated CI result is never a pass.
+2. :class:`ClaudeFixProposer` — an optional, injectable LLM fix-PROPOSER
+   (Claude, via :func:`agent_team.billing.claude_invoke`). It is consulted ONLY
+   on a non-pass verdict to author advisory fix hints for the builders. Its
+   output is advisory DATA only; it is structurally incapable of changing the
+   verdict because the verdict is computed first, by the pure-code gate, and is
+   never read back from the proposer.
+
+Both halves meet in :func:`propose_for_failure`, which computes the verdict with
+the gate, and ONLY if that verdict is not a pass consults the proposer for a
+hint. The pass branch never touches the LLM at all.
+
+INERT / HARD-GATE NOTE (§3.3.2 P3): the verifier is hard-gated behind
+``/sh-security-review`` + a GPT-4.1 cross-review of the CI trust boundary before
+it goes live. This module authors the LOGIC ONLY and stays inert: it does NO
+live CI dispatch, NO network I/O, and NO filesystem mutation. The authenticated
+CI result is passed in as data (the caller fetches it via the read-only PAT),
+exactly as :func:`agent_team.ci_gate.evaluate_ci_gate` expects. The Claude call
+goes only through the committed billing seam and is injectable, so this module
+is fully unit-testable with no SDK or network.
+
+Defensive parsing is a hard requirement: the model output is UNTRUSTED. The
+proposer parser fails SAFE — a missing or garbled proposal degrades to an empty
+advisory hint and NEVER crashes, and (by construction) never affects the
+verdict.
+"""
+
+from __future__ import annotations
+
+import json
+import re
+from collections.abc import Callable, Mapping, Sequence
+from typing import Any
+
+from agent_team.billing import ClaudeResult, claude_invoke
+from agent_team.ci_gate import GateDecision, GateResult, evaluate_ci_gate
+
+__all__ = [
+    "FixProposal",
+    "ClaudeFixProposer",
+    "evaluate_verdict",
+    "propose_for_failure",
+    "build_fix_advisor",
+]
+
+# The signature the billing seam exposes: ``claude_invoke(prompt, *, mode=None,
+# config=None, **kw) -> ClaudeResult``. Injected so tests pass a fake, mirroring
+# the injection pattern used across this codebase (billing.set_invoker, the
+# clarifier callables, the verifier node's FixAdvisor seam).
+ClaudeInvoke = Callable[..., ClaudeResult]
+
+# System framing handed to Claude when authoring a fix hint. Kept as a module
+# constant so callers can override via the constructor without forking the class.
+_DEFAULT_SYSTEM = (
+    "You are the VERIFY stage of an agentic SDLC pipeline. A pure-code gate has "
+    "ALREADY decided this candidate diff did NOT pass CI; that decision is final "
+    "and is not yours to make or revisit. Your only job is to read the gate's "
+    "failure reasons and propose concrete, minimal fixes for the builders to "
+    "try next. You cannot declare the task green; only the authenticated CI "
+    "gate can."
+)
+
+
+def evaluate_verdict(
+    *,
+    candidate_diff: str,
+    ledger_hash: str | None,
+    ci_result: Mapping[str, Any] | None,
+    expected_run_id: str,
+    allowed_scope: Sequence[str] | None = None,
+) -> GateResult:
+    """Compute the pass/fail/block verdict from the authenticated CI result.
+
+    This is the §3.3.2 boundary #4 pass authority and the ONLY thing in this
+    module that can produce a :data:`~agent_team.ci_gate.GateDecision.PASS`. It
+    is a thin compose over :func:`agent_team.ci_gate.evaluate_ci_gate` — it
+    reuses that committed pure-code gate verbatim and does not reimplement,
+    relax, or second-guess any of its rules. The LLM is intentionally NOT a
+    parameter here: the verdict is derived SOLELY from the authenticated,
+    patch-independent CI Checks result (keyed to ``run_id`` + ``diff_hash``).
+
+    FAILS SAFE. Anything other than an unambiguous authenticated success is a
+    non-pass:
+
+    * a missing ``candidate_diff`` -> :data:`GateDecision.BLOCK` (nothing to
+      verify; refuse to proceed, never pass);
+    * a missing/``None`` ``ci_result`` -> ``BLOCK`` (no authenticated result;
+      the gate never passes without one);
+    * a run-id mismatch, hash mismatch, denylist hit, or ambiguous/unknown CI
+      conclusion -> ``BLOCK`` (per the gate);
+    * a recognised CI failure -> :data:`GateDecision.FAIL`;
+    * an authenticated ``success`` keyed to the expected run -> ``PASS``.
+
+    Returns the gate's :class:`~agent_team.ci_gate.GateResult` unchanged so the
+    decision stays auditable (its ``reasons`` quote the exact CI conclusion
+    consumed). Raises :class:`~agent_team.ci_gate.CiGateError` only on
+    structurally invalid inputs, exactly as the underlying gate does.
+    """
+    if not isinstance(candidate_diff, str):
+        # No diff to verify is itself a refuse-to-proceed (mirrors the verifier
+        # node): BLOCK rather than declare anything. Never a pass.
+        return GateResult(
+            decision=GateDecision.BLOCK,
+            reasons=["no candidate diff present to verify"],
+            run_id=expected_run_id if isinstance(expected_run_id, str) else None,
+            diff_hash=ledger_hash,
+            ci_conclusion=None,
+        )
+
+    return evaluate_ci_gate(
+        candidate_diff=candidate_diff,
+        ledger_hash=ledger_hash,
+        ci_result=ci_result,
+        expected_run_id=expected_run_id,
+        allowed_scope=allowed_scope,
+    )
+
+
+class FixProposal:
+    """An advisory fix proposal authored by the LLM (DATA, never a verdict).
+
+    Carries only suggestions for the builders: a free-text ``hint`` and an
+    optional ordered list of ``suggestions``. It deliberately has NO notion of
+    pass/fail and exposes no way to express one — it is impossible to encode a
+    "this passed" signal here, which is what structurally guarantees the LLM
+    cannot declare green (§3.3.2 boundary #4). The verdict is computed entirely
+    separately by :func:`evaluate_verdict`.
+    """
+
+    __slots__ = ("hint", "suggestions")
+
+    def __init__(self, hint: str = "", suggestions: list[str] | None = None) -> None:
+        self.hint = hint
+        self.suggestions = list(suggestions) if suggestions else []
+
+    def __bool__(self) -> bool:
+        return bool(self.hint or self.suggestions)
+
+    def __eq__(self, other: object) -> bool:
+        if not isinstance(other, FixProposal):
+            return NotImplemented
+        return self.hint == other.hint and self.suggestions == other.suggestions
+
+    def __repr__(self) -> str:
+        return f"FixProposal(hint={self.hint!r}, suggestions={self.suggestions!r})"
+
+    def as_hint(self) -> str:
+        """Render this proposal as a single advisory hint string for builders."""
+        parts: list[str] = []
+        if self.hint:
+            parts.append(self.hint)
+        for idx, suggestion in enumerate(self.suggestions, start=1):
+            parts.append(f"{idx}. {suggestion}")
+        return "\n".join(parts)
+
+
+# An empty proposal — the fail-safe result whenever the model is unwired,
+# errors, or returns garbage. It changes nothing and asserts nothing.
+_EMPTY_PROPOSAL = FixProposal()
+
+
+class ClaudeFixProposer:
+    """Claude-backed fix PROPOSER — advisory only, never a verdict (§3.3.2 P3).
+
+    Construct with an optional ``invoke`` callable (defaults to
+    :func:`agent_team.billing.claude_invoke`) so tests inject a fake and the
+    real wiring goes through the billing seam. ``model`` / ``config`` are passed
+    through to the invoker, and ``system`` overrides the prompt framing.
+
+    The proposer is consulted ONLY on a non-pass :class:`GateResult` to author a
+    next-fix hint from the *failure* reasons. It returns a :class:`FixProposal`,
+    which is pure advisory DATA — it carries no pass/fail and cannot influence
+    the verdict, which is computed independently by :func:`evaluate_verdict`.
+
+    Every failure mode degrades to an empty proposal rather than raising: an
+    unbound/throwing invoker, a non-string reply, or unparseable JSON all yield
+    :data:`_EMPTY_PROPOSAL`. A garbage proposal therefore never crashes the
+    pipeline and never changes the verdict.
+    """
+
+    def __init__(
+        self,
+        *,
+        invoke: ClaudeInvoke | None = None,
+        model: str | None = None,
+        config: Any = None,
+        system: str = _DEFAULT_SYSTEM,
+    ) -> None:
+        self._invoke: ClaudeInvoke = invoke if invoke is not None else claude_invoke
+        self._model = model
+        self._config = config
+        self._system = system
+
+    def propose(
+        self,
+        gate_result: GateResult,
+        state: Mapping[str, Any] | None = None,
+    ) -> FixProposal:
+        """Return an advisory :class:`FixProposal` for a non-pass gate result.
+
+        On a :data:`GateDecision.PASS` this returns an empty proposal WITHOUT
+        calling the model: the LLM is never consulted on success, structurally
+        keeping it off the happy path. On any other decision it asks Claude for
+        fix suggestions and parses the reply defensively, failing SAFE to an
+        empty proposal on any trouble (unbound invoker, non-string reply, bad
+        JSON). It never raises and never returns anything that could read as a
+        verdict.
+        """
+        if gate_result.decision is GateDecision.PASS:
+            # The gate already passed; the LLM has no role here. Never consulted.
+            return _EMPTY_PROPOSAL
+
+        prompt = self._build_prompt(gate_result, state or {})
+        try:
+            result = self._invoke(prompt, model=self._model, config=self._config)
+            text = getattr(result, "text", "")
+        except Exception:
+            # An unwired or throwing invoker must not crash VERIFY; the verdict
+            # already stands and the builders simply loop back without a hint.
+            return _EMPTY_PROPOSAL
+        return self._parse(text)
+
+    # The proposer matches the verifier node's FixAdvisor seam:
+    # ``(GateResult, Mapping) -> str``. Returning the rendered hint string keeps
+    # the advisory output as plain DATA the node appends to its verdict record.
+    def advise(self, gate_result: GateResult, state: Mapping[str, Any]) -> str:
+        """Adapt :meth:`propose` to the verifier node's ``FixAdvisor`` seam.
+
+        Matches :data:`agent_team.nodes.verifier.FixAdvisor` exactly
+        (``(GateResult, Mapping) -> str``) so it wires straight into
+        :func:`agent_team.nodes.verifier.set_fix_advisor`. Returns the rendered
+        advisory hint (empty string when there is nothing to suggest) — never a
+        verdict.
+        """
+        return self.propose(gate_result, state).as_hint()
+
+    def _build_prompt(self, gate_result: GateResult, state: Mapping[str, Any]) -> str:
+        """Assemble the fix-proposer prompt from the gate failure + task state.
+
+        Pure string assembly over the gate result and graph state — no I/O — so
+        the prompt shape is directly unit-testable.
+        """
+        description = _task_description(state)
+        reasons = "\n".join(f"- {r}" for r in gate_result.reasons) or "(none recorded)"
+        ci_conclusion = gate_result.ci_conclusion or "(no authenticated conclusion)"
+
+        sections: list[str] = [
+            self._system,
+            "",
+            "## Task",
+            description or "(no task description provided)",
+            "",
+            "## Pure-code gate decision (FINAL, not yours to change)",
+            f"decision: {gate_result.decision.value}",
+            f"run_id: {gate_result.run_id}",
+            f"ci_conclusion: {ci_conclusion}",
+            "",
+            "## Gate failure reasons",
+            reasons,
+            "",
+            "## Your job",
+            (
+                "Propose the next concrete, minimal fixes for the builders. Do "
+                "NOT claim the task passed or is green; that verdict is owned by "
+                "the authenticated CI gate above, not by you."
+            ),
+            "",
+            "## Output format",
+            (
+                "Respond with ONLY a strict JSON object and no prose outside it, "
+                'with keys: "hint" (a short string summary) and "suggestions" (a '
+                "list of strings, most promising first). Example: "
+                '{"hint": "...", "suggestions": ["...", "..."]}'
+            ),
+        ]
+        return "\n".join(sections)
+
+    def _parse(self, text: str) -> FixProposal:
+        """Parse the UNTRUSTED model reply into a :class:`FixProposal`.
+
+        Fails SAFE at every step: a non-string reply, no parseable JSON object,
+        or missing keys all collapse to an empty proposal. Because the proposal
+        type cannot express a verdict, even a maximally adversarial reply
+        ("everything passed!") cannot influence pass/fail. Never raises.
+        """
+        data = _extract_json_object(text)
+        if data is None:
+            return _EMPTY_PROPOSAL
+
+        hint = ""
+        raw_hint = data.get("hint")
+        if isinstance(raw_hint, str):
+            hint = raw_hint.strip()
+
+        suggestions = _coerce_suggestions(data.get("suggestions"))
+
+        if not hint and not suggestions:
+            return _EMPTY_PROPOSAL
+        return FixProposal(hint=hint, suggestions=suggestions)
+
+
+def propose_for_failure(
+    *,
+    candidate_diff: str,
+    ledger_hash: str | None,
+    ci_result: Mapping[str, Any] | None,
+    expected_run_id: str,
+    allowed_scope: Sequence[str] | None = None,
+    proposer: ClaudeFixProposer | None = None,
+    state: Mapping[str, Any] | None = None,
+) -> tuple[GateResult, FixProposal]:
+    """Compute the verdict, then (only on a non-pass) get an advisory proposal.
+
+    This is where the two halves meet WITHOUT letting the LLM near the verdict:
+
+    1. The verdict is computed FIRST by :func:`evaluate_verdict` (the pure-code
+       gate). This is the sole pass authority.
+    2. ONLY if that verdict is not a :data:`GateDecision.PASS` is the
+       ``proposer`` consulted for an advisory :class:`FixProposal`. On a pass,
+       the proposer is never called and an empty proposal is returned.
+
+    The returned ``GateResult`` is exactly what the gate produced — the proposal
+    is never read back into it — so a garbage or "this passed!" LLM reply cannot
+    flip a failing verdict to pass. Returns ``(gate_result, proposal)``.
+    """
+    gate_result = evaluate_verdict(
+        candidate_diff=candidate_diff,
+        ledger_hash=ledger_hash,
+        ci_result=ci_result,
+        expected_run_id=expected_run_id,
+        allowed_scope=allowed_scope,
+    )
+
+    if gate_result.decision is GateDecision.PASS:
+        return gate_result, _EMPTY_PROPOSAL
+
+    active_proposer = proposer if proposer is not None else ClaudeFixProposer()
+    proposal = active_proposer.propose(gate_result, state)
+    return gate_result, proposal
+
+
+def build_fix_advisor(
+    *,
+    invoke: ClaudeInvoke | None = None,
+    model: str | None = None,
+    config: Any = None,
+    system: str = _DEFAULT_SYSTEM,
+) -> Callable[[GateResult, Mapping[str, Any]], str]:
+    """Build the ``FixAdvisor`` callable for wiring into the verifier node.
+
+    Returns the bound :meth:`ClaudeFixProposer.advise` of a shared proposer,
+    ready to hand to :func:`agent_team.nodes.verifier.set_fix_advisor`. This is
+    the documented injection point: the verifier node never imports this module
+    directly — a leaf calls ``set_fix_advisor(build_fix_advisor(...))`` once at
+    startup, keeping the node dependency-free and structurally guaranteeing the
+    advisor is only ever consulted on a gate failure (the node never calls it on
+    a PASS).
+    """
+    proposer = ClaudeFixProposer(
+        invoke=invoke, model=model, config=config, system=system
+    )
+    return proposer.advise
+
+
+# --------------------------------------------------------------------------- #
+# Module-level helpers (pure; no I/O). Mirror clarifier_llm's defensive parsers.
+# --------------------------------------------------------------------------- #
+
+
+def _task_description(state: Mapping[str, Any]) -> str:
+    """Pull the task description out of the graph state, defensively.
+
+    Looks in the conventional places (the ``plan`` dict, then a top-level
+    ``task``/``description`` key) and falls back to an empty string so a
+    malformed state surfaces as an empty prompt section, never a ``KeyError``.
+    """
+    plan = state.get("plan") or {}
+    if isinstance(plan, dict):
+        desc = plan.get("task") or plan.get("description")
+        if isinstance(desc, str) and desc.strip():
+            return desc.strip()
+    for key in ("task", "description"):
+        value = state.get(key)
+        if isinstance(value, str) and value.strip():
+            return value.strip()
+    return ""
+
+
+def _coerce_suggestions(value: Any) -> list[str]:
+    """Coerce the model's suggestion list into clean non-empty strings.
+
+    Anything that is not a list of usable strings collapses to an empty list.
+    """
+    if not isinstance(value, list):
+        return []
+    out: list[str] = []
+    for item in value:
+        if isinstance(item, str):
+            text = item.strip()
+            if text:
+                out.append(text)
+    return out
+
+
+# A fenced ```json ... ``` block, if the model wrapped its JSON in Markdown.
+_FENCE_RE = re.compile(
+    r"```(?:json)?\s*\n?(?P<body>.*?)\n?\s*```",
+    flags=re.DOTALL | re.IGNORECASE,
+)
+
+
+def _extract_json_object(text: str) -> dict[str, Any] | None:
+    """Extract a JSON object from UNTRUSTED model output, or ``None``.
+
+    Tolerates a leading apology or trailing prose and ```json fences. Tries, in
+    order, the whole string, the contents of a fenced block, then the first
+    ``{...}`` span found by brace matching. Returns ``None`` (never raises) when
+    nothing parses to a JSON object, so the caller can fail SAFE.
+    """
+    if not isinstance(text, str) or not text.strip():
+        return None
+
+    candidates: list[str] = [text.strip()]
+
+    fence = _FENCE_RE.search(text)
+    if fence:
+        candidates.append(fence.group("body").strip())
+
+    span = _first_brace_span(text)
+    if span is not None:
+        candidates.append(span)
+
+    for candidate in candidates:
+        if not candidate:
+            continue
+        try:
+            parsed = json.loads(candidate)
+        except (json.JSONDecodeError, ValueError):
+            continue
+        if isinstance(parsed, dict):
+            return parsed
+    return None
+
+
+def _first_brace_span(text: str) -> str | None:
+    """Return the first balanced ``{...}`` span in ``text`` (string-aware)."""
+    start = text.find("{")
+    if start == -1:
+        return None
+    depth = 0
+    in_string = False
+    escaped = False
+    for idx in range(start, len(text)):
+        ch = text[idx]
+        if in_string:
+            if escaped:
+                escaped = False
+            elif ch == "\\":
+                escaped = True
+            elif ch == '"':
+                in_string = False
+            continue
+        if ch == '"':
+            in_string = True
+        elif ch == "{":
+            depth += 1
+        elif ch == "}":
+            depth -= 1
+            if depth == 0:
+                return text[start : idx + 1]
+    return None

agent-team/agent_team/transport/slack_listener.py

@@ -0,0 +1,385 @@
+"""Socket Mode inbound Slack listener for the human-gate responder (design §3.3.1).
+
+This is the inbound counterpart to :class:`~agent_team.transport.slack_adapter.SlackTransport`.
+The adapter posts question-sets outbound; this listener receives Adam's answers
+and drives them into the durable first-answer-wins compare-and-set:
+
+    inbound Slack event
+        -> SlackTransport.parse_answer (normalize to question_id/answer/via)
+        -> submit_answer (atomic UPDATE ... WHERE status='open')
+        -> enqueue_resume(job)   [only if accepted]
+
+The listener does NOT resume the LangGraph graph itself; its sole job is
+normalize -> submit -> enqueue. The turn-guarded :class:`ResumeWorker` (owned by
+the coordinator) consumes the enqueued :class:`~agent_team.responder.ResumeJob`.
+
+Transport choice — Socket Mode (NOT a public webhook). The R720 box is VPN-only,
+so there is no public HTTPS endpoint to expose. Slack's Socket Mode opens an
+*outbound* WebSocket from the box to Slack and authenticates with an app-level
+token; events arrive over that authenticated socket. This is the only place the
+network/SDK is touched, and the SDK import is deferred (``slack_bolt`` /
+``slack_sdk`` are not installed in the Mac/test env), so this module imports
+cleanly without them and :meth:`SlackListener.handle_event` is fully unit
+testable with no socket.
+
+SECURITY
+    This module handles inbound UNTRUSTED Slack input plus auth. The trust
+    boundary is enforced by THREE independent layers, all of which must hold for
+    an inbound event to mutate the ledger:
+
+    * (a) Socket Mode app-token authentication on the socket. Under Socket Mode
+      there is no inbound HTTP request, so there is no ``X-Slack-Signature`` to
+      verify; the transport itself is authenticated by the app-level token used
+      to open the outbound WebSocket (only a holder of that token can establish
+      the socket and receive events at all).
+
+    * (b) An explicit owner allowlist on the SENDER (AUTHZ-01, CWE-862). The
+      trust model is single-owner (Adam): only an authorized Slack user id may
+      answer/steer the autonomous pipeline. ``handle_event`` extracts the
+      inbound sender's Slack user id and rejects the event (``return None``,
+      WITHOUT calling :func:`~agent_team.responder.submit_answer`) unless that id
+      is in the configured ``owner_ids`` allowlist. This FAILS CLOSED: if the
+      allowlist is empty / unconfigured, EVERY answer is rejected, and if no
+      sender id can be recovered the event is treated as unauthorized. Socket
+      membership alone is NOT authorization — any member of a channel the app is
+      in could otherwise win the first-answer-wins race. This layer is the fix
+      for the prior (insufficient) assumption that "maps to an open row" was
+      itself authorization.
+
+    * (c) The open-status compare-and-set as ANTI-REPLAY (not authorization).
+      An authorized event's embedded ``question_id`` only has effect if it maps
+      to a real, still-``open`` ledger row, because
+      :func:`~agent_team.responder.submit_answer` runs
+      ``UPDATE ... WHERE question_id=? AND status='open'``. A replayed or stale
+      ``question_id`` for a closed / expired / superseded / nonexistent row
+      loses that compare-and-set (rowcount 0) and is a no-op
+      (``accepted=False``) — it can never resume a graph or overwrite an
+      existing answer. First-answer-wins also neutralizes duplicate redelivery.
+      This is anti-replay AFTER authorization, never a substitute for it.
+
+    * Answers are DATA, never code. The answer value is extracted by the
+      transport and stored verbatim as JSON (``json.dumps`` in the responder).
+      This module never ``eval``s, executes, interpolates, or otherwise
+      interprets answer content — it is treated purely as opaque payload data.
+
+    * Defensive event filtering. ``handle_event`` validates that the payload is
+      a mapping carrying a recoverable ``question_id`` before doing any work, and
+      swallows the :class:`ValueError` that :meth:`SlackTransport.parse_answer`
+      raises for an unrecoverable id. Unrelated / malformed events are
+      logged-and-ignored (``return None``) rather than crashing the listen loop,
+      so a hostile or noisy event stream cannot take the listener down.
+"""
+
+from __future__ import annotations
+
+import logging
+from collections.abc import Mapping
+from pathlib import Path
+from typing import Any
+
+from agent_team.db.schema import connect
+from agent_team.responder import AnswerOutcome, EnqueueResume, submit_answer
+from agent_team.transport.slack_adapter import SlackTransport
+
+__all__ = [
+    "SlackListener",
+]
+
+_LOG = logging.getLogger(__name__)
+
+# Inbound Slack event ``type`` values that can carry an answer for the human
+# gate: an interactive Block Kit callback (button / select), a thread reply or
+# mention message, or a slash-command invocation. Anything else (presence
+# changes, channel joins, reactions, ...) is ignored. ``parse_answer`` does the
+# real question_id recovery; this is a cheap first filter so unrelated events
+# never reach it.
+_ANSWER_BEARING_TYPES: frozenset[str] = frozenset(
+    {
+        "block_actions",
+        "message",
+        "app_mention",
+        "slash_commands",
+        "view_submission",
+    }
+)
+
+
+class SlackListener:
+    """Socket Mode inbound listener that drives answers into the responder.
+
+    Constructed with the injected collaborators so it is fully unit-testable
+    with no SDK and no socket:
+
+    * ``transport`` — the :class:`SlackTransport` whose ``parse_answer``
+      normalizes an inbound payload to ``(question_id, answer, via)``;
+    * ``db_path`` — the agent-team SQLite file; a fresh connection is opened per
+      event (and closed) so the per-event compare-and-set is isolated;
+    * ``enqueue_resume`` — the coordinator's resume-queue ``put`` callable; an
+      accepted answer hands its :class:`~agent_team.responder.ResumeJob` to it;
+    * ``app_token`` / ``bot_token`` — optional injected Slack tokens used only by
+      :meth:`serve` to open the Socket Mode connection. Never required for
+      :meth:`handle_event`.
+    * ``owner_ids`` — the allowlist of authorized Slack user ids (the
+      single-owner trust model, AUTHZ-01). Only a sender whose id is in this set
+      may answer. If ``None``/empty the listener FAILS CLOSED and rejects every
+      answer; :meth:`serve` sources it from ``AGENT_TEAM_SLACK_OWNER_IDS`` when
+      not injected.
+
+    The listener never resumes the graph; it only normalizes, submits, and
+    enqueues. See the module SECURITY note for the trust boundary.
+    """
+
+    def __init__(
+        self,
+        transport: SlackTransport,
+        db_path: Path | str,
+        enqueue_resume: EnqueueResume,
+        *,
+        app_token: str | None = None,
+        bot_token: str | None = None,
+        owner_ids: set[str] | None = None,
+    ) -> None:
+        self._transport = transport
+        self._db_path = Path(db_path)
+        self._enqueue_resume = enqueue_resume
+        self._app_token = app_token
+        self._bot_token = bot_token
+        # The owner allowlist (AUTHZ-01). An empty set is the fail-closed default:
+        # an unconfigured deploy rejects every answer.
+        self._owner_ids: set[str] = set(owner_ids) if owner_ids else set()
+
+    def handle_event(self, raw_payload: Any) -> AnswerOutcome | None:
+        """Normalize + submit one inbound event; return its outcome or ``None``.
+
+        Steps:
+
+        1. Defensively validate the payload is a mapping for an answer-bearing
+           event type that carries a recoverable ``question_id``. Unrelated or
+           malformed events are logged and ignored (``return None``) — never
+           raised — so the listen loop cannot be crashed by a hostile or noisy
+           event.
+        1b. AUTHORIZE THE SENDER (AUTHZ-01, fail-closed). Extract the inbound
+           sender's Slack user id and require it to be in the configured owner
+           allowlist BEFORE any ledger work. If the allowlist is unconfigured,
+           or no sender id can be recovered, or the sender is not an owner, the
+           event is rejected (``return None``, ``submit_answer`` is NOT called).
+        2. Open a per-event ledger connection, run
+           :func:`~agent_team.responder.submit_answer` (the atomic
+           first-answer-wins compare-and-set), then close the connection.
+        3. If the answer was accepted (it was the first valid answer for a
+           still-``open`` ledger row), hand the resulting
+           :class:`~agent_team.responder.ResumeJob` to the injected
+           ``enqueue_resume`` via the responder; a duplicate / late / forged id
+           yields ``accepted=False`` and is a no-op.
+
+        Returns the :class:`~agent_team.responder.AnswerOutcome` from
+        ``submit_answer``, or ``None`` if the event was not an answer we act on.
+        """
+        if not isinstance(raw_payload, Mapping):
+            _LOG.debug("ignoring non-mapping Slack event: %r", type(raw_payload))
+            return None
+
+        event_type = raw_payload.get("type")
+        if event_type is not None and event_type not in _ANSWER_BEARING_TYPES:
+            _LOG.debug("ignoring non-answer Slack event type: %r", event_type)
+            return None
+
+        # AUTHZ-01 (CWE-862), fail-closed: only an allowlisted owner may answer.
+        # Resolve the question_id first (best-effort) so a rejection log names the
+        # question without leaking answer content. parse_answer's id recovery is
+        # the same one submit_answer uses; an unrecoverable id is handled below.
+        if not self._is_authorized(raw_payload):
+            return None
+
+        # ``submit_answer`` calls ``transport.parse_answer`` internally, which
+        # raises ValueError when no question_id is recoverable. Wrap the whole
+        # submit so a malformed / unrelated event is logged-and-ignored rather
+        # than crashing the loop. The enqueue itself happens inside
+        # ``submit_answer`` (only on accept), so it is covered by this guard too.
+        conn = connect(self._db_path)
+        try:
+            outcome = submit_answer(
+                conn,
+                self._transport,
+                raw_payload,
+                enqueue_resume=self._enqueue_resume,
+            )
+        except ValueError as exc:
+            # Unrecoverable / forged-shape payload: parse_answer rejected it.
+            # This is expected for unrelated chatter on the channel; ignore it.
+            _LOG.debug("ignoring Slack event with no recoverable answer: %s", exc)
+            return None
+        finally:
+            conn.close()
+
+        if outcome.accepted:
+            _LOG.info("accepted Slack answer for question_id=%s", outcome.question_id)
+        else:
+            _LOG.info(
+                "ignored Slack answer for question_id=%s (not open: duplicate / "
+                "late / forged id loses the compare-and-set)",
+                outcome.question_id,
+            )
+        return outcome
+
+    def _is_authorized(self, raw_payload: Mapping[str, Any]) -> bool:
+        """Return ``True`` iff the payload's sender is an allowlisted owner.
+
+        Fails closed (AUTHZ-01, CWE-862):
+
+        * If the allowlist is empty / unconfigured, reject EVERY answer and log a
+          warning naming ``AGENT_TEAM_SLACK_OWNER_IDS`` so the misprovisioning is
+          obvious. An unconfigured deploy must accept answers from no one.
+        * If no sender id can be recovered, treat the event as unauthorized.
+        * If the sender id is not in the allowlist, reject it.
+
+        Never logs the answer content or any token — only the (best-effort)
+        question id and the offending sender id, which are non-secret routing
+        identifiers.
+        """
+        if not self._owner_ids:
+            _LOG.warning(
+                "rejecting Slack answer: owner allowlist is unconfigured "
+                "(set AGENT_TEAM_SLACK_OWNER_IDS); the listener fails closed and "
+                "accepts answers from no one until it is provisioned"
+            )
+            return False
+
+        sender_id = _extract_sender_id(raw_payload)
+        if sender_id is None:
+            _LOG.warning(
+                "rejecting Slack answer: no recoverable sender id in payload "
+                "(treated as unauthorized)"
+            )
+            return False
+
+        if sender_id not in self._owner_ids:
+            # Log the rejection WITHOUT the answer content or any token.
+            _LOG.warning(
+                "rejecting Slack answer for question_id=%s: unauthorized sender %r "
+                "(not in owner allowlist)",
+                _safe_question_id(self._transport, raw_payload),
+                sender_id,
+            )
+            return False
+
+        return True
+
+    def serve(self) -> None:  # pragma: no cover - live socket, not unit-tested
+        """Open the Socket Mode connection and forward events to ``handle_event``.
+
+        Lazily imports ``slack_bolt`` (deferred so this module imports cleanly
+        without the SDK, mirroring ``graph.build_sqlite_checkpointer``), wires a
+        handler that forwards every inbound event to :meth:`handle_event`, and
+        blocks on the Socket Mode handler. This is the ONLY method that touches
+        the network and is intentionally not unit-tested against a live socket;
+        :meth:`handle_event` carries all the testable logic.
+
+        Also sources the owner allowlist from ``AGENT_TEAM_SLACK_OWNER_IDS``
+        (comma-separated Slack user ids) when one was not injected, so the
+        production entry is allowlist-aware. The listener still FAILS CLOSED if
+        the env var is unset/empty — :meth:`handle_event` rejects every answer.
+
+        Raises :class:`RuntimeError` if the SDK package or the required tokens
+        are missing, so a misconfigured deploy fails loudly rather than silently
+        never receiving answers.
+        """
+        # Deferred import (mirrors the SDK import discipline): keep ``os`` out of
+        # the module's import-time surface so this stays cleanly importable.
+        if not self._owner_ids:
+            import os
+
+            raw = os.environ.get("AGENT_TEAM_SLACK_OWNER_IDS", "")
+            self._owner_ids = {uid.strip() for uid in raw.split(",") if uid.strip()}
+
+        if not self._app_token or not self._bot_token:
+            raise RuntimeError(
+                "SlackListener.serve requires both an app-level token "
+                "(xapp-, Socket Mode) and a bot token (xoxb-); inject them via "
+                "SlackListener(..., app_token=..., bot_token=...)."
+            )
+
+        try:
+            from slack_bolt import App
+            from slack_bolt.adapter.socket_mode import SocketModeHandler
+        except ImportError as exc:
+            raise RuntimeError(
+                "slack_bolt is unavailable; install 'slack-bolt' to run the "
+                "Socket Mode listener (SlackListener.serve). Tests exercise "
+                "handle_event directly with no SDK."
+            ) from exc
+
+        app = App(token=self._bot_token)
+
+        # Block Kit interactions, messages, mentions, and slash commands all
+        # funnel through the same normalize -> submit -> enqueue path. Slack Bolt
+        # dispatches by event family, so register the relevant ones and forward
+        # the raw body unchanged; handle_event does the filtering + parsing.
+        def _forward(body: Mapping[str, Any]) -> None:
+            self.handle_event(body)
+
+        @app.action({})  # any block_actions interaction
+        def _on_action(ack: Any, body: Mapping[str, Any]) -> None:
+            ack()
+            _forward(body)
+
+        @app.event("message")
+        def _on_message(body: Mapping[str, Any]) -> None:
+            _forward(body)
+
+        @app.event("app_mention")
+        def _on_mention(body: Mapping[str, Any]) -> None:
+            _forward(body)
+
+        SocketModeHandler(app, self._app_token).start()
+
+
+def _extract_sender_id(raw_payload: Mapping[str, Any]) -> str | None:
+    """Recover the inbound sender's Slack user id from any supported shape.
+
+    Handles the inbound payload shapes defensively (AUTHZ-01):
+
+    * interactive ``block_actions`` / view submissions: ``payload["user"]["id"]``;
+    * Events API message / mention: ``payload["event"]["user"]`` (the event
+      envelope) or a top-level ``payload["user"]`` (which may itself be the id
+      string or a ``{"id": ...}`` mapping);
+    * slash command: ``payload["user_id"]``.
+
+    Returns the non-empty user id string, or ``None`` if no sender id can be
+    recovered (the caller treats that as unauthorized — fail closed).
+    """
+    # Interactive / top-level ``user``: a mapping ({"id": ...}) or a bare id str.
+    user = raw_payload.get("user")
+    if isinstance(user, Mapping):
+        uid = user.get("id")
+        if uid:
+            return str(uid)
+    elif isinstance(user, str) and user:
+        return user
+
+    # Events API envelope: the inner event carries the author's user id.
+    event = raw_payload.get("event")
+    if isinstance(event, Mapping):
+        uid = event.get("user")
+        if uid:
+            return str(uid)
+
+    # Slash command shape.
+    user_id = raw_payload.get("user_id")
+    if user_id:
+        return str(user_id)
+
+    return None
+
+
+def _safe_question_id(transport: SlackTransport, raw_payload: Mapping[str, Any]) -> str:
+    """Best-effort recover the question id for a rejection log, never raising.
+
+    Used only to name the question in an unauthorized-sender warning. Returns a
+    placeholder if the id is unrecoverable; never logs answer content or tokens.
+    """
+    try:
+        question_id, _answer, _via = transport.parse_answer(raw_payload)
+    except Exception:  # noqa: BLE001 — logging path must never raise
+        return "<unknown>"
+    return question_id

agent-team/agent_team/transport/slack_live.py

@@ -0,0 +1,142 @@
+"""Live ``slack_sdk``-backed Slack poster (design §3.3.1, §7.1 P1 — Slack first).
+
+The :mod:`agent_team.transport.slack_adapter` module ships the §3.3.1 transport
+contract with a dependency-injected ``poster`` seam: the adapter renders the
+question-set into a message dict and hands it to a
+``SlackPoster = Callable[[dict[str, Any]], Mapping[str, Any]]`` whose job is to
+perform the real ``chat.postMessage`` and return a response carrying the message
+``ts``. The foundation's default poster refuses the network so nothing ships
+provisioned; this module supplies the **production** poster, backed by
+``slack_sdk.WebClient``, that the P1 (Slack first) live wiring injects.
+
+Deferred import (mirrors :func:`agent_team.graph.build_sqlite_checkpointer`):
+``slack_sdk`` is an optional dependency that may be absent in pre-deploy /
+test environments, so this module imports cleanly without it. The import is
+deferred to the moment a live client is actually constructed, and a missing
+package raises a clear :class:`RuntimeError` so a misconfigured deploy fails
+loudly rather than silently.
+
+Message-dict to ``chat.postMessage`` mapping
+--------------------------------------------
+The adapter's message dict (see ``SlackTransport.post_question``) carries
+``channel``, ``callback_id``, ``text``, ``blocks`` and ``metadata``. Slack's
+``chat.postMessage`` Web API method does **not** accept a top-level
+``callback_id`` keyword argument (``callback_id`` is a legacy attachment /
+interactive-component field, not a message-post parameter), so passing it
+through verbatim would raise a ``TypeError`` / Slack ``invalid_arguments``.
+
+The durable inbound key is therefore carried by ``metadata`` instead: the
+adapter embeds ``question_id`` under ``metadata.event_payload.question_id``, and
+``slack_adapter._extract_question_id`` reads exactly that path off an inbound
+message. ``chat.postMessage`` *does* accept ``metadata``, so forwarding it
+preserves the inbound mapping. The poster consequently **drops** ``callback_id``
+from the postMessage kwargs and forwards only the parameters the Web API
+accepts (``channel``, ``text``, ``blocks``, ``metadata``), letting ``metadata``
+do the question-id round-trip the adapter relies on.
+"""
+
+from __future__ import annotations
+
+import os
+from collections.abc import Mapping
+from typing import Any
+
+from agent_team.transport.slack_adapter import SlackPoster, SlackTransport
+
+__all__ = [
+    "build_live_slack_transport",
+    "build_slack_poster",
+]
+
+# Top-level ``chat.postMessage`` keyword arguments the live poster forwards.
+# ``callback_id`` is deliberately excluded: it is not a postMessage parameter,
+# and the durable inbound key lives in ``metadata.event_payload`` instead.
+_POST_MESSAGE_KEYS = ("channel", "text", "blocks", "metadata")
+
+
+def build_slack_poster(token: str | None = None, *, client: Any = None) -> SlackPoster:
+    """Build a live ``slack_sdk``-backed :data:`SlackPoster` (§3.3.1, P1).
+
+    The returned callable accepts the adapter's rendered message dict, performs
+    a ``chat.postMessage``, and returns the response as a mapping carrying the
+    message ``ts`` so ``SlackTransport._extract_ts`` can record the
+    ``channel_ref``.
+
+    ``client`` (optional) injects a pre-built Slack client for testability; any
+    object exposing ``chat_postMessage(**kwargs)`` works. When omitted, a
+    ``slack_sdk.WebClient`` is constructed lazily from ``token`` (falling back to
+    the ``SLACK_BOT_TOKEN`` environment variable). The ``slack_sdk`` import is
+    deferred so this module imports cleanly without the optional package; a
+    missing package or a missing token raises a clear :class:`RuntimeError`.
+
+    The poster maps the adapter's message dict to the Web API's accepted
+    parameters: it forwards ``channel``, ``text``, ``blocks`` and ``metadata``
+    and **drops** ``callback_id`` (not a ``chat.postMessage`` parameter — the
+    ``question_id`` round-trips via ``metadata.event_payload`` instead). See the
+    module docstring for the full rationale.
+    """
+    if client is None:
+        client = _build_web_client(token)
+
+    def _poster(message: dict[str, Any]) -> Mapping[str, Any]:
+        kwargs = {key: message[key] for key in _POST_MESSAGE_KEYS if key in message}
+        response = client.chat_postMessage(**kwargs)
+        return _as_mapping(response)
+
+    return _poster
+
+
+def build_live_slack_transport(
+    channel: str, token: str | None = None, *, client: Any = None
+) -> SlackTransport:
+    """Build a :class:`SlackTransport` wired to a live ``slack_sdk`` poster.
+
+    Convenience constructor for the P1 live coordinator: equivalent to
+    ``SlackTransport(channel, poster=build_slack_poster(token, client=client))``.
+    See :func:`build_slack_poster` for the token / client / deferred-import
+    semantics.
+    """
+    return SlackTransport(channel, poster=build_slack_poster(token, client=client))
+
+
+def _build_web_client(token: str | None) -> Any:
+    """Lazily construct a ``slack_sdk.WebClient`` (deferred optional import).
+
+    Raises a clear :class:`RuntimeError` if ``slack_sdk`` is not installed or no
+    token is resolvable (neither ``token`` nor ``SLACK_BOT_TOKEN``), so a
+    misconfigured deploy fails loudly rather than silently.
+    """
+    try:
+        from slack_sdk import WebClient
+    except ImportError as exc:  # pragma: no cover - depends on optional dep
+        raise RuntimeError(
+            "slack_sdk is unavailable; install the 'slack_sdk' package to build "
+            "a live Slack poster (P1), or inject a 'client' for testing."
+        ) from exc
+
+    resolved = token or os.environ.get("SLACK_BOT_TOKEN")
+    if not resolved:
+        raise RuntimeError(
+            "No Slack bot token available; pass 'token' or set the "
+            "SLACK_BOT_TOKEN environment variable to build a live Slack poster."
+        )
+    return WebClient(token=resolved)
+
+
+def _as_mapping(response: Any) -> Mapping[str, Any]:
+    """Coerce a ``chat_postMessage`` response to a plain mapping.
+
+    ``slack_sdk`` returns a ``SlackResponse`` exposing the payload via ``.data``;
+    if a test injects a client returning a bare mapping, accept it as-is. The
+    result must carry ``ts`` so ``SlackTransport._extract_ts`` recovers the
+    ``channel_ref``.
+    """
+    if isinstance(response, Mapping):
+        return response
+    data = getattr(response, "data", None)
+    if isinstance(data, Mapping):
+        return data
+    raise TypeError(
+        "Slack chat_postMessage returned an unsupported response; expected a "
+        f"mapping or an object with a mapping '.data', got {type(response)!r}"
+    )

agent-team/run-team.py

@@ -72,6 +72,12 @@ from agent_team.db.schema import (  # noqa: E402  (path bootstrap must precede)
     reopen_question,
     supersede_question,
 )
+from agent_team.transport.base import Transport  # noqa: E402  (path bootstrap)
+
+# Transport choices the start/serve commands accept (§3.3.1 D10). Only ``slack``
+# has a live adapter wired for the P1 CLI; the others are accepted for forward
+# compatibility and gated in _build_transport.
+_TRANSPORT_CHOICES: tuple[str, ...] = ("slack", "github", "claude_code")
 
 __all__ = [
     "build_parser",
@@ -470,6 +476,121 @@ def _cmd_supersede(args: argparse.Namespace, *, out: Any) -> int:
     return 0
 
 
+def _build_coordinator(args: argparse.Namespace) -> Any:
+    """Construct a :class:`Coordinator` for the ``start`` / ``serve`` commands.
+
+    The live transport is built LAZILY here (never at import) so ``run-team.py``
+    imports, ``--help``, and the ledger subcommands all work with no Slack token
+    present. ``--dry-run`` substitutes a non-posting transport so an operator can
+    drive intake without a token (the ledger row is still written; only the
+    transport post is a no-op).
+
+    The coordinator is imported inside this function for the same reason: pulling
+    in the live runtime (and its optional SDK-adjacent deps) must not happen just
+    to render ``--help`` or run a read-only ledger command.
+    """
+    from agent_team.coordinator import (
+        Coordinator,
+        default_plan_node_factory,
+        default_review_wiring,
+    )
+
+    transport = _build_transport(args)
+    # Production runs the full P2 graph: the wrapped real planner + the bound
+    # GPT-4.1 review loop (Plane-2 depth-first). These factories are lazy and
+    # only build/bind the model seams when a task actually runs.
+    return Coordinator(
+        db_path=args.db,
+        transport=transport,
+        build_plan_node=default_plan_node_factory,
+        review_wiring=default_review_wiring,
+    )
+
+
+def _build_transport(args: argparse.Namespace) -> Any:
+    """Build the transport for a coordinator command (lazy; token-tolerant).
+
+    ``--dry-run`` (or any transport in dry-run) yields a non-posting transport so
+    intake works without credentials. Otherwise the live Slack transport is
+    constructed lazily from ``SLACK_BOT_TOKEN`` / ``SLACK_CHANNEL``; GitHub and
+    Claude-Code live transports are not wired for the P1 CLI surface and raise a
+    clear error rather than pretending to post.
+    """
+    if getattr(args, "dry_run", False):
+        return _DryRunTransport()
+    if args.transport == "slack":
+        from agent_team.transport.slack_live import build_live_slack_transport
+
+        channel = os.environ.get("SLACK_CHANNEL", "")
+        return build_live_slack_transport(channel)
+    raise SystemExit(
+        f"live transport '{args.transport}' is not wired for the run-team CLI; "
+        "use --transport slack, or --dry-run for a no-token dry run"
+    )
+
+
+class _DryRunTransport(Transport):
+    """A non-posting transport for ``--dry-run`` intake (no token, no Slack).
+
+    ``post_question`` records nothing on a real channel — it returns a synthetic
+    ``channel_ref`` so :func:`agent_team.responder.notify_question` still writes
+    and stamps the durable ledger row (the durable seam is exercised; only the
+    side-effecting post is skipped). ``parse_answer`` is unused by the CLI path
+    but implemented so the ABC is concrete.
+    """
+
+    def post_question(
+        self,
+        *,
+        thread_id: str,
+        question_id: str,
+        turn: int,
+        question_set: Any,
+        deadline: str,
+    ) -> str:
+        print(
+            f"[dry-run] would post question {question_id} (turn {turn}) "
+            f"for thread {thread_id}",
+            file=sys.stderr,
+        )
+        return f"dry-run:{question_id}"
+
+    def parse_answer(self, raw: Any) -> tuple[str, Any, str]:
+        raise NotImplementedError("dry-run transport does not parse answers")
+
+
+def _cmd_start(args: argparse.Namespace, *, out: Any) -> int:
+    """Intake: start one task and run it to the first human gate (§3.3, §3.3.1).
+
+    Builds a :class:`Coordinator` (transport from the lazy factory; ``--dry-run``
+    posts nowhere), runs ``setup`` + ``start_task``, and prints the minted
+    ``thread_id``. The clarifier question-set is delivered over the chosen
+    transport (or no-op under ``--dry-run``); the durable ledger row is written
+    either way.
+    """
+    coordinator = _build_coordinator(args)
+    coordinator.setup()
+    thread_id = coordinator.start_task(
+        task_text=args.task, transport_name=args.transport
+    )
+    print(thread_id, file=out)
+    return 0
+
+
+def _cmd_serve(args: argparse.Namespace, *, out: Any) -> int:
+    """Run the coordinator daemon loop (binds the live invoker; §7.1 P1).
+
+    Delegates to :meth:`agent_team.coordinator.Coordinator.serve`, which binds
+    the real Claude subscription invoker, runs the startup recovery sweep, then
+    loops on the deadline cadence. The Slack inbound feed is the slack_listener's
+    job; this command owns the maintenance loop. Runs until interrupted.
+    """
+    coordinator = _build_coordinator(args)
+    print("agent-team coordinator starting (Ctrl-C to stop)", file=out)
+    coordinator.serve()
+    return 0  # pragma: no cover - serve() loops until interrupted
+
+
 def _cmd_force_resume(args: argparse.Namespace, *, out: Any) -> int:
     """Force-resume a parked task's question (destructive; audit-logged).
 
@@ -666,6 +787,47 @@ def build_parser() -> argparse.ArgumentParser:
     )
     p_resume.set_defaults(func=_cmd_force_resume)
 
+    p_start = sub.add_parser(
+        "start",
+        help="intake: start one task and run it to the first human gate",
+    )
+    p_start.add_argument(
+        "--task",
+        required=True,
+        help="the task description (intake text) to run through the pipeline",
+    )
+    p_start.add_argument(
+        "--transport",
+        choices=_TRANSPORT_CHOICES,
+        default="slack",
+        help="channel for delivering clarifier questions (default: slack)",
+    )
+    p_start.add_argument(
+        "--dry-run",
+        action="store_true",
+        dest="dry_run",
+        help="use a non-posting transport (no token needed; ledger still written)",
+    )
+    p_start.set_defaults(func=_cmd_start)
+
+    p_serve = sub.add_parser(
+        "serve",
+        help="run the coordinator daemon (binds invoker, runs the loop)",
+    )
+    p_serve.add_argument(
+        "--transport",
+        choices=_TRANSPORT_CHOICES,
+        default="slack",
+        help="channel for delivering clarifier questions (default: slack)",
+    )
+    p_serve.add_argument(
+        "--dry-run",
+        action="store_true",
+        dest="dry_run",
+        help="use a non-posting transport (no token needed)",
+    )
+    p_serve.set_defaults(func=_cmd_serve)
+
     return parser
 
 

agent-team/systemd/agent-team-coordinator.service

@@ -0,0 +1,54 @@
+# agent-team-coordinator.service - R720 Plane-2 coordinator daemon (sh-secrev VM, user adam).
+#
+# Long-running coordinator for the agent-team SDLC pipeline. Unlike the
+# sea-haven-secrev sweep (a oneshot driven by a timer), this is an always-on
+# service: it serves the LangGraph coordinator, the durable pending_questions
+# ledger, and the Slack Socket Mode inbound listener that answers clarifier
+# questions. ExecStart runs the `serve` subcommand of the operator CLI.
+#
+# Install (on the VM, as root):
+#   sudo cp agent-team-coordinator.service /etc/systemd/system/
+#   sudo systemctl daemon-reload
+#   sudo systemctl enable --now agent-team-coordinator.service
+#   systemctl status agent-team-coordinator.service
+#   journalctl -u agent-team-coordinator.service -e -f
+#
+# Secrets come from the EnvironmentFile (leading '-' = optional, no failure if
+# absent), ~/secrev.env (mode 600, NOT in git):
+#   CLAUDE_CODE_OAUTH_TOKEN  -> subscription OAuth (from `claude setup-token`).
+#                               A raw ANTHROPIC_API_KEY must NOT be set on this
+#                               box; it would silently win and meter to API
+#                               rates. The billing seam pops it defensively.
+#   SLACK_BOT_TOKEN          -> xoxb- bot token (chat:write) - posts questions.
+#   SLACK_APP_TOKEN          -> xapp- app-level token (connections:write) -
+#                               REQUIRED for Socket Mode; opens the inbound
+#                               WebSocket that receives answers. Without it the
+#                               coordinator can post but never hear replies.
+#   SLACK_CHANNEL_ID         -> target channel for clarifier questions.
+
+[Unit]
+Description=Sea Haven agent-team Plane-2 coordinator daemon
+After=network-online.target
+Wants=network-online.target
+
+[Service]
+Type=simple
+User=adam
+WorkingDirectory=/home/adam/orchestrator/agent-team
+EnvironmentFile=-/home/adam/secrev.env
+ExecStart=/usr/bin/env python3 run-team.py serve
+Restart=on-failure
+RestartSec=5
+# Hardening - matches the level the sea-haven-secrev unit relies on, scoped for a
+# long-running daemon that must READ ~/secrev.env and WRITE the local ledger.
+NoNewPrivileges=true
+ProtectSystem=full
+# ProtectHome cannot be `true`: the daemon reads /home/adam/secrev.env and writes
+# the ledger under the working dir. read-only home + an explicit RW carve-out for
+# the state/ dir keeps the rest of $HOME unreadable/unwritable to the service.
+ProtectHome=read-only
+ReadWritePaths=/home/adam/orchestrator/agent-team/state
+Nice=10
+
+[Install]
+WantedBy=multi-user.target

agent-team/tests/test_builders_llm.py

@@ -0,0 +1,354 @@
+"""Unit tests for agent_team.nodes.builders_llm (§3.3, §7.1 P3).
+
+The DeepSeek-backed builders binding is exercised with a FAKE ``build`` callable
+that returns canned text — no network, no subprocess. The load-bearing
+properties under test:
+
+* **Clean import.** The module imports without importing the orchestrator at
+  module top (the orchestrator package is not importable from this tree).
+* **Happy path.** A fake build returning a valid unified diff yields an ``ok``
+  :class:`CandidateDiff` with the right diff and a real content hash.
+* **Fail SAFE.** Garbage / empty / prose-only model output yields a FAILED
+  no-op candidate (empty diff, ``ok is False``), never a fabricated success.
+  A build that raises also fails safe.
+* **Inert boundary.** The module exposes no patch-applying / git / fs-write
+  function — by construction it cannot mutate the repo.
+"""
+
+from __future__ import annotations
+
+import inspect
+import sys
+from pathlib import Path
+from typing import Any
+
+import pytest
+
+from agent_team.nodes import builders, builders_llm
+from agent_team.nodes.builders import BuildError, builders_node
+from agent_team.nodes.builders_llm import (
+    CandidateDiff,
+    as_diff_builder,
+    build_candidate_diff,
+    default_build,
+)
+from agent_team.state_store import compute_content_hash
+
+# A minimal but realistic unified diff the fake build can return.
+_VALID_DIFF = (
+    "diff --git a/agent_team/example.py b/agent_team/example.py\n"
+    "--- a/agent_team/example.py\n"
+    "+++ b/agent_team/example.py\n"
+    "@@ -1,2 +1,2 @@\n"
+    "-old = 1\n"
+    "+new = 2\n"
+)
+
+_PLAN = {
+    "title": "Add a thing",
+    "scope": ["agent_team/"],
+    "phases": ["edit example.py"],
+}
+
+
+# --------------------------------------------------------------------------- #
+# Import hygiene
+# --------------------------------------------------------------------------- #
+
+
+def test_module_imports_without_orchestrator_at_top() -> None:
+    """The module imports cleanly with NO orchestrator import at module top.
+
+    Parses the module's own top-level import statements (AST) and asserts none of
+    them pull in the orchestrator's top-level modules — the real DeepSeek route
+    defers to a subprocess, mirroring graph.build_sqlite_checkpointer's deferred
+    import. (We inspect the AST rather than reload the module, so the
+    ``CandidateDiff`` identity used by other tests stays stable.)
+    """
+    import ast
+
+    tree = ast.parse(inspect.getsource(builders_llm))
+    orchestrator_mods = {"graph", "agents", "run", "models", "tools", "retriever"}
+    top_level_imports: set[str] = set()
+    for node in tree.body:  # module body only -> top-level imports
+        if isinstance(node, ast.Import):
+            top_level_imports.update(alias.name.split(".")[0] for alias in node.names)
+        elif isinstance(node, ast.ImportFrom) and node.module:
+            top_level_imports.add(node.module.split(".")[0])
+
+    leaked = top_level_imports & orchestrator_mods
+    assert not leaked, f"builders_llm imports orchestrator modules at top: {leaked}"
+    # And the module imports cleanly (already imported above).
+    assert builders_llm.build_candidate_diff is build_candidate_diff
+    assert "agent_team.nodes.builders_llm" in sys.modules
+
+
+# --------------------------------------------------------------------------- #
+# Happy path
+# --------------------------------------------------------------------------- #
+
+
+def test_valid_diff_yields_ok_candidate() -> None:
+    calls: list[str] = []
+
+    def fake_build(instruction: str) -> str:
+        calls.append(instruction)
+        return _VALID_DIFF
+
+    candidate = build_candidate_diff(_PLAN, {"repo": "demo"}, build=fake_build)
+
+    assert isinstance(candidate, CandidateDiff)
+    assert candidate.ok is True
+    assert candidate.failed is False
+    assert candidate.reason == ""
+    assert candidate.diff == _VALID_DIFF.strip()
+    assert candidate.diff_hash == compute_content_hash(candidate.diff.encode("utf-8"))
+    # The instruction was rendered from the plan and handed to the builder.
+    assert calls and "Add a thing" in calls[0]
+    assert "unified diff" in calls[0]
+
+
+def test_orchestrator_framing_lines_are_stripped() -> None:
+    """run.py prints framing lines before the result; they must be stripped."""
+    framed = "[retrieved: none]\n[fast_coder]\n\n" + _VALID_DIFF
+
+    candidate = build_candidate_diff(_PLAN, build=lambda _i: framed)
+
+    assert candidate.ok is True
+    assert candidate.diff.startswith("diff --git ")
+    assert "[fast_coder]" not in candidate.diff
+    assert "[retrieved" not in candidate.diff
+
+
+def test_fenced_diff_is_unwrapped() -> None:
+    fenced = "Here is the diff:\n```diff\n" + _VALID_DIFF + "```\n"
+
+    candidate = build_candidate_diff(_PLAN, build=lambda _i: fenced)
+
+    assert candidate.ok is True
+    assert candidate.diff.endswith("+new = 2")
+    assert "```" not in candidate.diff
+
+
+# --------------------------------------------------------------------------- #
+# Fail SAFE — never a fabricated success
+# --------------------------------------------------------------------------- #
+
+
+def test_garbage_output_yields_failed_no_op() -> None:
+    candidate = build_candidate_diff(
+        _PLAN, build=lambda _i: "Sure! I cannot produce a diff right now."
+    )
+
+    assert candidate.ok is False
+    assert candidate.failed is True
+    assert candidate.diff == ""
+    assert candidate.reason
+    assert candidate.diff_hash == compute_content_hash(b"")
+
+
+def test_empty_output_yields_failed_no_op() -> None:
+    candidate = build_candidate_diff(_PLAN, build=lambda _i: "   \n  ")
+
+    assert candidate.ok is False
+    assert candidate.failed is True
+    assert candidate.diff == ""
+
+
+def test_build_exception_fails_safe() -> None:
+    def boom(_instruction: str) -> str:
+        raise RuntimeError("model exploded")
+
+    candidate = build_candidate_diff(_PLAN, build=boom)
+
+    assert candidate.ok is False
+    assert candidate.failed is True
+    assert candidate.diff == ""
+    assert "build error" in candidate.reason
+
+
+def test_non_mapping_plan_fails_safe() -> None:
+    candidate = build_candidate_diff("not a plan", build=lambda _i: _VALID_DIFF)  # type: ignore[arg-type]
+
+    assert candidate.ok is False
+    assert candidate.failed is True
+    assert candidate.diff == ""
+
+
+# --------------------------------------------------------------------------- #
+# DiffBuilder adapter (node seam parity)
+# --------------------------------------------------------------------------- #
+
+
+def test_as_diff_builder_returns_string_on_success() -> None:
+    builder = as_diff_builder(build=lambda _i: _VALID_DIFF)
+    diff = builder(plan=_PLAN, config={"repo": "demo"})
+    assert isinstance(diff, str)
+    assert diff.startswith("diff --git ")
+
+
+def test_as_diff_builder_returns_empty_on_failure() -> None:
+    """A failed build must surface as an empty string (node's fail-closed input)."""
+    builder = as_diff_builder(build=lambda _i: "no diff here")
+    diff = builder(plan=_PLAN, config=None)
+    assert diff == ""
+
+
+# --------------------------------------------------------------------------- #
+# Inert / no-apply boundary
+# --------------------------------------------------------------------------- #
+
+
+def test_module_exposes_no_apply_or_fs_mutation_function() -> None:
+    """No public callable hints at applying a patch, git, or writing files."""
+    forbidden_tokens = (
+        "apply",
+        "git",
+        "commit",
+        "push",
+        "write",
+        "mutat",
+        "patch",
+        "checkout",
+        "remove",
+        "delete",
+    )
+    public = [
+        name
+        for name in dir(builders_llm)
+        if not name.startswith("_") and callable(getattr(builders_llm, name))
+    ]
+    for name in public:
+        lowered = name.lower()
+        for token in forbidden_tokens:
+            assert token not in lowered, (
+                f"public callable {name!r} suggests a mutation/apply path"
+            )
+
+
+def test_source_has_no_patch_application_or_fs_write_paths() -> None:
+    """Static guard: NO executable call applies a patch, runs git, or writes files.
+
+    Inspects the AST (so the SECURITY-BOUNDARY docstring's mentions of what the
+    module does NOT do are ignored) and asserts no call/attribute names a
+    git/patch/apply/fs-mutation primitive. The only subprocess permitted is the
+    read-only ``subprocess.run`` model call to the orchestrator.
+    """
+    import ast
+
+    src = inspect.getsource(builders_llm)
+    tree = ast.parse(src)
+
+    banned_attrs = {
+        "Popen",
+        "write_text",
+        "write_bytes",
+        "unlink",
+        "rmtree",
+        "remove",
+        "mkdir",
+        "rename",
+        "replace",
+    }
+    banned_names = {"open"}
+    subprocess_attrs: set[str] = set()
+
+    for node in ast.walk(tree):
+        if isinstance(node, ast.Attribute):
+            assert node.attr not in banned_attrs, (
+                f"module calls a banned fs/git primitive: .{node.attr}"
+            )
+            if isinstance(node.value, ast.Name) and node.value.id == "subprocess":
+                subprocess_attrs.add(node.attr)
+        if isinstance(node, ast.Name):
+            assert node.id not in banned_names, (
+                f"module references a banned builtin: {node.id}"
+            )
+
+    # The only subprocess primitives used are the read-only ``run`` call plus the
+    # exception types caught around it — never Popen/call/etc. that could shell a
+    # patch-apply.
+    assert subprocess_attrs <= {
+        "run",
+        "TimeoutExpired",
+        "CalledProcessError",
+    }, f"module uses unexpected subprocess primitives: {subprocess_attrs}"
+
+
+def test_default_build_invokes_run_py_as_list_argv(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """``default_build`` shells ``run.py`` via list-form argv (no shell) + maps stdout.
+
+    Executes the subprocess path (not just AST-checks it): monkeypatches
+    ``subprocess.run`` to capture the invocation and return canned stdout. The
+    argv MUST be the list form ``["python3", <root>/run.py, <instruction>]`` so
+    the instruction can never be interpreted by a shell (no ``shell=True``), and
+    the return value is the subprocess stdout verbatim.
+    """
+    captured: dict[str, Any] = {}
+
+    class _FakeCompleted:
+        stdout = "DIFF-FROM-SUBPROCESS"
+
+    def _fake_run(argv: Any, **kwargs: Any) -> _FakeCompleted:
+        captured["argv"] = argv
+        captured["kwargs"] = kwargs
+        return _FakeCompleted()
+
+    monkeypatch.setattr(builders_llm.subprocess, "run", _fake_run)
+
+    root = Path("/tmp/fake-orchestrator-root")
+    route = builders_llm._OrchestratorRoute(root=root)
+    out = default_build("do the edit", route=route)
+
+    assert out == "DIFF-FROM-SUBPROCESS"
+    # List-form argv (no shell): exactly python3, the run.py path, the instruction.
+    assert captured["argv"] == ["python3", str(root / "run.py"), "do the edit"]
+    # No shell=True anywhere in the call (defense against shell injection).
+    assert captured["kwargs"].get("shell", False) is False
+
+
+def test_as_diff_builder_empty_raises_build_error_in_real_node() -> None:
+    """A failed build surfaces as the real ``builders_node``'s BuildError.
+
+    Integration across the seam boundary: ``as_diff_builder`` adapts a failing
+    build ("no diff here" has no diff header -> empty candidate -> empty string),
+    and the REAL ``agent_team.nodes.builders.builders_node`` raises its own
+    ``BuildError`` on the empty-diff path rather than emitting a fabricated diff.
+    """
+    builder = as_diff_builder(build=lambda _i: "no diff here")
+    state = {"plan": dict(_PLAN)}
+
+    with pytest.raises(BuildError, match="empty candidate diff"):
+        builders_node(state, builder=builder)  # type: ignore[arg-type]
+
+    # Sanity: the node + error are the foundation's, not a redefinition.
+    assert builders_node.__module__ == builders.__name__
+
+
+def test_default_build_is_the_injection_default() -> None:
+    """The default build seam is default_build (the DeepSeek/orchestrator route)."""
+    sig = inspect.signature(build_candidate_diff)
+    assert sig.parameters["build"].default is None
+    # default_build is what gets used when build is None — assert it's callable
+    # and routes to a subprocess to run.py (string check, no execution).
+    src = inspect.getsource(default_build)
+    assert "run.py" in src
+    assert "subprocess.run" in src
+
+    # builders are DeepSeek (orchestrator fast_coder), NOT Claude: assert the
+    # module never CALLS billing.claude_invoke (AST, so docstring mentions of the
+    # "NOT claude_invoke" contrast don't trip the check).
+    import ast
+
+    tree = ast.parse(inspect.getsource(builders_llm))
+    called = set()
+    for node in ast.walk(tree):
+        if isinstance(node, ast.Call):
+            fn = node.func
+            if isinstance(fn, ast.Name):
+                called.add(fn.id)
+            elif isinstance(fn, ast.Attribute):
+                called.add(fn.attr)
+    assert "claude_invoke" not in called

agent-team/tests/test_clarifier_llm.py

@@ -0,0 +1,335 @@
+"""Unit tests for agent_team.nodes.clarifier_llm (§3.3, §7.1 P1).
+
+The Claude-backed clarifier callables are exercised with a FAKE invoke that
+returns canned :class:`~agent_team.billing.ClaudeResult` text — no network. The
+load-bearing properties under test:
+
+* **One call per turn (memoization).** ``assess_confidence`` then
+  ``generate_questions`` on the same turn must reuse a single Claude call.
+* **Fail SAFE (the human gate).** A garbled / non-JSON reply must yield
+  confidence ``0.0`` (never >= the 0.98 bar) and a non-empty fallback question.
+* **Defensive parsing.** ```json fences and surrounding prose still parse, and
+  out-of-range confidence is clamped to ``[0, 1]``.
+* **Integration smoke.** The callables wire into the real
+  :func:`~agent_team.nodes.clarifier.make_clarifier_node` and clear the gate
+  once confidence rises across turns.
+"""
+
+from __future__ import annotations
+
+import json
+from typing import Any
+
+from langgraph.checkpoint.memory import MemorySaver
+from langgraph.graph import END, START, StateGraph
+from langgraph.types import Command
+
+from agent_team.billing import BillingMode, ClaudeResult
+from agent_team.nodes.clarifier import (
+    DEFAULT_CONFIDENCE_THRESHOLD,
+    make_clarifier_node,
+)
+from agent_team.nodes.clarifier_llm import (
+    FALLBACK_QUESTION,
+    ClaudeClarifier,
+    build_claude_clarifier_callables,
+)
+from agent_team.task_model import Phase, PipelineState, TaskStatus
+
+# --------------------------------------------------------------------------- #
+# Fakes / helpers
+# --------------------------------------------------------------------------- #
+
+
+class _FakeInvoke:
+    """A fake billing.claude_invoke that returns canned text and counts calls.
+
+    ``replies`` may be a single string (returned every call) or a list of
+    strings (consumed one per call, last one repeating) so a test can simulate
+    rising confidence across turns.
+    """
+
+    def __init__(self, replies: str | list[str]) -> None:
+        self._replies = [replies] if isinstance(replies, str) else list(replies)
+        self.calls: list[dict[str, Any]] = []
+
+    def __call__(self, prompt: str, **kw: Any) -> ClaudeResult:
+        idx = min(len(self.calls), len(self._replies) - 1)
+        text = self._replies[idx]
+        self.calls.append({"prompt": prompt, "kw": kw})
+        return ClaudeResult(text=text, mode=BillingMode.SUBSCRIPTION)
+
+
+def _state(**overrides: Any) -> PipelineState:
+    base: PipelineState = PipelineState(
+        thread_id="t-1",
+        status=TaskStatus.ACTIVE.value,
+        current_phase=Phase.CLARIFY.value,
+        qa_history=[],
+        transport="slack",
+    )
+    base.update(overrides)  # type: ignore[typeddict-item]
+    return base
+
+
+def _json(confidence: Any, questions: Any, rationale: str = "because") -> str:
+    return json.dumps(
+        {"confidence": confidence, "questions": questions, "rationale": rationale}
+    )
+
+
+# --------------------------------------------------------------------------- #
+# High confidence: assess returns ~value AND the call is reused (memoization).
+# --------------------------------------------------------------------------- #
+
+
+def test_high_confidence_parsed() -> None:
+    fake = _FakeInvoke(_json(0.99, []))
+    clar = ClaudeClarifier(invoke=fake)
+    assert clar.assess_confidence([], _state()) == 0.99
+
+
+def test_single_call_per_turn_memoized() -> None:
+    fake = _FakeInvoke(_json(0.99, []))
+    clar = ClaudeClarifier(invoke=fake)
+    qa: list[object] = []
+
+    # Both methods called for the same turn -> exactly ONE Claude call.
+    conf = clar.assess_confidence(qa, _state())
+    questions = clar.generate_questions(qa, _state())
+
+    assert conf == 0.99
+    assert questions == []  # confident, no questions needed
+    assert len(fake.calls) == 1
+
+
+def test_memo_recomputes_when_history_grows() -> None:
+    fake = _FakeInvoke([_json(0.10, ["q1"]), _json(0.99, [])])
+    clar = ClaudeClarifier(invoke=fake)
+
+    # Turn 0: one answer-less call.
+    assert clar.assess_confidence([], _state()) == 0.10
+    assert clar.generate_questions([], _state()) == ["q1"]
+    assert len(fake.calls) == 1
+
+    # Turn 1: history grew -> a fresh call, now confident.
+    assert clar.assess_confidence(["a1"], _state()) == 0.99
+    assert clar.generate_questions(["a1"], _state()) == []
+    assert len(fake.calls) == 2
+
+
+def test_memo_isolates_across_tasks_no_cross_gate_clear() -> None:
+    """A second task at the same history length must NOT reuse task A's memo.
+
+    Regression for the fail-OPEN collision: one ClaudeClarifier instance serves
+    every task through the shared graph node, so keying the memo on history
+    length alone would let task A's cached 0.99 clear task B's human gate with
+    no model call. Keying on thread_id forces a fresh assessment for task B.
+    """
+    fake = _FakeInvoke([_json(0.99, []), _json(0.10, ["need more from B"])])
+    clar = ClaudeClarifier(invoke=fake)
+
+    # Task A (thread t-A), empty history -> confident, cached.
+    assert clar.assess_confidence([], _state(thread_id="t-A")) == 0.99
+    assert len(fake.calls) == 1
+
+    # Task B (thread t-B), SAME empty history/length -> must re-assess, NOT
+    # inherit A's cache, so its low confidence holds and the gate stays shut.
+    assert clar.assess_confidence([], _state(thread_id="t-B")) == 0.10
+    assert clar.generate_questions([], _state(thread_id="t-B")) == ["need more from B"]
+    assert len(fake.calls) == 2  # a real second call happened for task B
+
+
+# --------------------------------------------------------------------------- #
+# Low confidence: below the bar, questions are returned.
+# --------------------------------------------------------------------------- #
+
+
+def test_low_confidence_returns_questions() -> None:
+    fake = _FakeInvoke(_json(0.40, ["What is the scope?", "Which repo?"]))
+    clar = ClaudeClarifier(invoke=fake)
+
+    assert clar.assess_confidence([], _state()) < DEFAULT_CONFIDENCE_THRESHOLD
+    assert clar.generate_questions([], _state()) == [
+        "What is the scope?",
+        "Which repo?",
+    ]
+    assert len(fake.calls) == 1
+
+
+# --------------------------------------------------------------------------- #
+# Malformed output: fail SAFE (0.0 confidence, non-empty fallback questions).
+# --------------------------------------------------------------------------- #
+
+
+def test_malformed_output_fails_safe() -> None:
+    fake = _FakeInvoke("I'm sorry, I cannot help with that. <no json here>")
+    clar = ClaudeClarifier(invoke=fake)
+
+    assert clar.assess_confidence([], _state()) == 0.0
+    questions = clar.generate_questions([], _state())
+    assert questions == [FALLBACK_QUESTION]
+    assert questions  # non-empty
+
+
+def test_garbage_never_clears_the_gate() -> None:
+    # The critical safety property: garbage must never read >= 0.98.
+    for garbage in ["", "   ", "not json", "{broken", "[1,2,3]", "null", "42"]:
+        clar = ClaudeClarifier(invoke=_FakeInvoke(garbage))
+        conf = clar.assess_confidence([], _state())
+        assert conf < DEFAULT_CONFIDENCE_THRESHOLD
+        assert conf == 0.0
+        assert clar.generate_questions([], _state()) == [FALLBACK_QUESTION]
+
+
+def test_missing_confidence_key_defaults_zero() -> None:
+    fake = _FakeInvoke(json.dumps({"questions": ["q?"], "rationale": "x"}))
+    clar = ClaudeClarifier(invoke=fake)
+    assert clar.assess_confidence([], _state()) == 0.0
+    # Questions present in the reply are kept as-is.
+    assert clar.generate_questions([], _state()) == ["q?"]
+
+
+def test_low_confidence_empty_questions_gets_fallback() -> None:
+    # Below the bar but model gave no questions -> generic fallback so the loop
+    # always has something to ask.
+    fake = _FakeInvoke(_json(0.20, []))
+    clar = ClaudeClarifier(invoke=fake)
+    assert clar.generate_questions([], _state()) == [FALLBACK_QUESTION]
+
+
+# --------------------------------------------------------------------------- #
+# Defensive parsing: fences and surrounding prose still parse.
+# --------------------------------------------------------------------------- #
+
+
+def test_json_in_code_fence_is_parsed() -> None:
+    fenced = "```json\n" + _json(0.97, ["q?"]) + "\n```"
+    clar = ClaudeClarifier(invoke=_FakeInvoke(fenced))
+    assert clar.assess_confidence([], _state()) == 0.97
+    assert clar.generate_questions([], _state()) == ["q?"]
+
+
+def test_json_wrapped_in_prose_is_parsed() -> None:
+    prose = (
+        "Sure! Here is my assessment:\n"
+        + _json(0.55, ["Clarify the deadline?"])
+        + "\nLet me know if that helps."
+    )
+    clar = ClaudeClarifier(invoke=_FakeInvoke(prose))
+    assert clar.assess_confidence([], _state()) == 0.55
+    assert clar.generate_questions([], _state()) == ["Clarify the deadline?"]
+
+
+def test_plain_json_fence_without_lang_is_parsed() -> None:
+    fenced = "```\n" + _json(0.33, ["q?"]) + "\n```"
+    clar = ClaudeClarifier(invoke=_FakeInvoke(fenced))
+    assert clar.assess_confidence([], _state()) == 0.33
+
+
+# --------------------------------------------------------------------------- #
+# Confidence clamping into [0, 1].
+# --------------------------------------------------------------------------- #
+
+
+def test_confidence_above_one_is_clamped() -> None:
+    clar = ClaudeClarifier(invoke=_FakeInvoke(_json(1.5, [])))
+    assert clar.assess_confidence([], _state()) == 1.0
+
+
+def test_confidence_below_zero_is_clamped() -> None:
+    clar = ClaudeClarifier(invoke=_FakeInvoke(_json(-0.2, ["q?"])))
+    assert clar.assess_confidence([], _state()) == 0.0
+
+
+def test_confidence_as_string_is_coerced() -> None:
+    clar = ClaudeClarifier(invoke=_FakeInvoke(_json("0.88", ["q?"])))
+    assert clar.assess_confidence([], _state()) == 0.88
+
+
+# --------------------------------------------------------------------------- #
+# Prompt assembly pulls task/repo/context out of state.
+# --------------------------------------------------------------------------- #
+
+
+def test_prompt_includes_task_repo_and_qa() -> None:
+    fake = _FakeInvoke(_json(0.99, []))
+    clar = ClaudeClarifier(invoke=fake)
+    state = _state(task="Add a webhook verifier", repo="agent-team")
+    clar.assess_confidence(["prior answer"], state)
+
+    prompt = fake.calls[0]["prompt"]
+    assert "Add a webhook verifier" in prompt
+    assert "agent-team" in prompt
+    assert "prior answer" in prompt
+
+
+# --------------------------------------------------------------------------- #
+# Factory returns the exact ConfidenceAssessor / QuestionGenerator pair.
+# --------------------------------------------------------------------------- #
+
+
+def test_factory_returns_shared_memoized_pair() -> None:
+    fake = _FakeInvoke(_json(0.45, ["q?"]))
+    assess, generate = build_claude_clarifier_callables(invoke=fake)
+
+    # Both come from one shared instance -> one call serves both this turn.
+    assert assess([], _state()) == 0.45
+    assert generate([], _state()) == ["q?"]
+    assert len(fake.calls) == 1
+
+
+# --------------------------------------------------------------------------- #
+# Integration smoke: wire into the real make_clarifier_node, gate clears.
+# --------------------------------------------------------------------------- #
+
+
+def _build_app(node):
+    graph = StateGraph(PipelineState)
+    graph.add_node("clarify", node)
+    graph.add_edge(START, "clarify")
+    graph.add_edge("clarify", END)
+    return graph.compile(checkpointer=MemorySaver())
+
+
+def test_node_clears_gate_when_confidence_rises() -> None:
+    # Turn 0 (no answers): low confidence, asks. Turn 1 (one answer): confident.
+    fake = _FakeInvoke([_json(0.20, ["What is the goal?"]), _json(0.99, [])])
+    assess, generate = build_claude_clarifier_callables(invoke=fake)
+    node = make_clarifier_node(assess_confidence=assess, generate_questions=generate)
+    app = _build_app(node)
+    cfg = {"configurable": {"thread_id": "t-1"}}
+
+    first = app.invoke(_state(thread_id="t-1"), cfg)
+    assert "__interrupt__" in first  # suspended on the question-set
+
+    final = app.invoke(Command(resume="ship feature X"), cfg)
+    assert "__interrupt__" not in final
+    assert final["qa_history"] == ["ship feature X"]
+    assert final["current_phase"] == Phase.PLAN.value
+    assert final["status"] == TaskStatus.ACTIVE.value
+
+
+def test_node_parks_when_garbage_never_clears_gate() -> None:
+    # A model that only ever emits garbage must NEVER open the gate; the loop
+    # asks until the turn cap and parks (human gate stays shut).
+    fake = _FakeInvoke("garbage, no json")
+    assess, generate = build_claude_clarifier_callables(invoke=fake)
+    from agent_team.nodes.clarifier import ClarifierConfig
+
+    node = make_clarifier_node(
+        assess_confidence=assess,
+        generate_questions=generate,
+        config=ClarifierConfig(max_turns=2),
+    )
+    app = _build_app(node)
+    cfg = {"configurable": {"thread_id": "t-1"}}
+
+    assert "__interrupt__" in app.invoke(_state(thread_id="t-1"), cfg)
+    assert "__interrupt__" in app.invoke(Command(resume="a1"), cfg)
+    final = app.invoke(Command(resume="a2"), cfg)
+
+    assert "__interrupt__" not in final
+    assert final["current_phase"] == Phase.PARKED.value
+    assert final["status"] == TaskStatus.PARKED.value
+    assert final["current_phase"] != Phase.PLAN.value

agent-team/tests/test_coordinator.py

@@ -0,0 +1,604 @@
+"""Unit tests for the COORDINATOR daemon (design §3.3, §3.3.1, §7.1 P1).
+
+The :class:`agent_team.coordinator.Coordinator` is the live keystone that ties
+the Plane-2 pipeline into one process. These tests exercise its full surface —
+``setup`` / ``start_task`` / ``submit_answer`` / ``drain_resumes`` / ``tick`` /
+``recover`` — with NO Claude SDK and NO Slack: the transport is a fake and the
+clarify node is a stub, both injected, so the durable composition is proven
+deterministically.
+
+Every test injects:
+
+* an in-memory LangGraph checkpointer (``build_checkpointer``) so ``setup`` needs
+  no SQLite checkpointer package and the suspend/resume mechanic is real;
+* the deterministic single-turn :func:`agent_team.graph.clarify_node` stub as the
+  ``build_clarify_node`` so the real Claude clarifier is never built or called.
+"""
+
+from __future__ import annotations
+
+import queue
+from datetime import timedelta
+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, default_clarify_node_factory
+from agent_team.db.schema import connect, init_db
+from agent_team.resume_worker import ResumeOutcome
+from agent_team.transport.base import QuestionSet, Transport
+
+
+# --------------------------------------------------------------------------- #
+# Test doubles
+# --------------------------------------------------------------------------- #
+
+
+class FakeTransport(Transport):
+    """A record-only transport: no Slack, no network (§3.3.1 injection seam).
+
+    ``post_question`` appends to ``posted`` and returns a deterministic
+    ``channel_ref`` embedding the ``question_id`` (mirroring the real contract).
+    ``parse_answer`` reads a plain dict ``{"question_id", "answer", "via"}`` so
+    tests can submit answers without a Slack payload. ``fail_post`` makes the
+    post raise once, to exercise the lost-post path.
+    """
+
+    def __init__(self, *, fail_post: bool = False) -> None:
+        self.posted: list[QuestionSet] = []
+        self.fail_post = fail_post
+
+    def post_question(
+        self,
+        *,
+        thread_id: str,
+        question_id: str,
+        turn: int,
+        question_set: QuestionSet,
+        deadline: str,
+    ) -> str:
+        if self.fail_post:
+            raise RuntimeError("simulated transport post failure")
+        self.posted.append(question_set)
+        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 _make_coordinator(
+    db_path: Path,
+    *,
+    transport: Transport | None = None,
+    resume_queue: "queue.Queue[Any] | None" = None,
+    deadline_window: timedelta | None = None,
+    alarm_hook: Any = None,
+) -> Coordinator:
+    """Build a Coordinator wired with an in-memory saver + the stub clarify node."""
+    saver = _Saver()
+    return Coordinator(
+        db_path=db_path,
+        transport=transport or FakeTransport(),
+        build_clarify_node=lambda: graph_mod.clarify_node,
+        build_checkpointer=lambda _path: saver,
+        resume_queue=resume_queue,
+        deadline_window=deadline_window,
+        alarm_hook=alarm_hook,
+    )
+
+
+@pytest.fixture()
+def db_path(tmp_path: Path) -> Path:
+    path = tmp_path / "state" / "agent_team.sqlite"
+    init_db(path)
+    return path
+
+
+def _row(db_path: Path, question_id: str) -> dict[str, Any] | None:
+    conn = connect(db_path)
+    try:
+        row = conn.execute(
+            "SELECT * FROM pending_questions WHERE question_id=?",
+            (question_id,),
+        ).fetchone()
+    finally:
+        conn.close()
+    return dict(row) if row is not None else None
+
+
+def _only_open_row(db_path: Path) -> dict[str, Any]:
+    conn = connect(db_path)
+    try:
+        rows = conn.execute(
+            "SELECT * FROM pending_questions WHERE status='open'"
+        ).fetchall()
+    finally:
+        conn.close()
+    assert len(rows) == 1
+    return dict(rows[0])
+
+
+def _all_rows(db_path: Path, *, status: str) -> list[dict[str, Any]]:
+    conn = connect(db_path)
+    try:
+        rows = conn.execute(
+            "SELECT * FROM pending_questions WHERE status=?",
+            (status,),
+        ).fetchall()
+    finally:
+        conn.close()
+    return [dict(r) for r in rows]
+
+
+# --------------------------------------------------------------------------- #
+# setup
+# --------------------------------------------------------------------------- #
+
+
+def test_setup_builds_graph_and_resume_worker(db_path: Path) -> None:
+    coord = _make_coordinator(db_path)
+    assert coord.graph is None
+    assert coord.resume_worker is None
+    coord.setup()
+    assert coord.graph is not None
+    assert coord.resume_worker is not None
+
+
+def test_setup_initializes_ledger_tables(tmp_path: Path) -> None:
+    # init_db is invoked by setup even if the DB did not pre-exist.
+    db = tmp_path / "fresh" / "agent_team.sqlite"
+    coord = _make_coordinator(db)
+    coord.setup()
+    conn = connect(db)
+    try:
+        names = {
+            r["name"]
+            for r in conn.execute(
+                "SELECT name FROM sqlite_master WHERE type='table'"
+            ).fetchall()
+        }
+    finally:
+        conn.close()
+    assert "pending_questions" in names
+
+
+def test_start_task_before_setup_raises(db_path: Path) -> None:
+    coord = _make_coordinator(db_path)
+    with pytest.raises(RuntimeError):
+        coord.start_task(task_text="x", transport_name="slack")
+
+
+# --------------------------------------------------------------------------- #
+# start_task — run to the human gate + notify
+# --------------------------------------------------------------------------- #
+
+
+def test_start_task_suspends_and_writes_open_ledger_row(db_path: Path) -> None:
+    transport = FakeTransport()
+    coord = _make_coordinator(db_path, transport=transport)
+    coord.setup()
+
+    thread_id = coord.start_task(task_text="build a thing", transport_name="slack")
+    assert thread_id
+
+    # The clarifier suspended: a pending interrupt exists on the live checkpoint.
+    pending = graph_mod.pending_question(coord.graph, thread_id=thread_id)
+    assert pending is not None
+    assert pending["thread_id"] == thread_id
+
+    # notify_question wrote the ledger row OPEN and posted, recording a ref.
+    row = _only_open_row(db_path)
+    assert row["thread_id"] == thread_id
+    assert row["status"] == "open"
+    assert row["channel_ref"] == f"fake:{row['question_id']}"
+    assert len(transport.posted) == 1
+
+
+def test_start_task_lost_post_leaves_open_row_without_ref(db_path: Path) -> None:
+    # A failed transport post is recoverable: the row stays open with no ref.
+    transport = FakeTransport(fail_post=True)
+    coord = _make_coordinator(db_path, transport=transport)
+    coord.setup()
+    coord.start_task(task_text="x", transport_name="slack")
+    row = _only_open_row(db_path)
+    assert row["status"] == "open"
+    assert row["channel_ref"] is None
+
+
+# --------------------------------------------------------------------------- #
+# submit_answer — first-answer-wins + enqueue resume
+# --------------------------------------------------------------------------- #
+
+
+def test_submit_answer_accepts_and_enqueues(db_path: Path) -> None:
+    q: "queue.Queue[Any]" = queue.Queue()
+    transport = FakeTransport()
+    coord = _make_coordinator(db_path, transport=transport, resume_queue=q)
+    coord.setup()
+    thread_id = coord.start_task(task_text="x", transport_name="slack")
+    qid = _only_open_row(db_path)["question_id"]
+
+    outcome = coord.submit_answer(
+        {"question_id": qid, "answer": "the answer", "via": "fake:U1"}
+    )
+    assert outcome.accepted is True
+    assert outcome.question_id == qid
+    assert q.qsize() == 1
+    job = q.get_nowait()
+    assert job.thread_id == thread_id
+    assert job.question_id == qid
+    assert _row(db_path, qid)["status"] == "answered"
+
+
+def test_submit_duplicate_answer_is_rejected(db_path: Path) -> None:
+    coord = _make_coordinator(db_path)
+    coord.setup()
+    coord.start_task(task_text="x", transport_name="slack")
+    qid = _only_open_row(db_path)["question_id"]
+
+    first = coord.submit_answer({"question_id": qid, "answer": "a", "via": "v"})
+    second = coord.submit_answer({"question_id": qid, "answer": "b", "via": "v"})
+    assert first.accepted is True
+    assert second.accepted is False  # first-answer-wins; late answer ignored
+
+
+# --------------------------------------------------------------------------- #
+# drain_resumes — single-flight, turn-guarded via ResumeWorker
+# --------------------------------------------------------------------------- #
+
+
+def test_drain_resumes_advances_graph_to_done(db_path: Path) -> None:
+    coord = _make_coordinator(db_path)
+    coord.setup()
+    thread_id = coord.start_task(task_text="x", transport_name="slack")
+    qid = _only_open_row(db_path)["question_id"]
+    coord.submit_answer({"question_id": qid, "answer": "go", "via": "v"})
+
+    results = coord.drain_resumes()
+    assert len(results) == 1
+    assert results[0].outcome is ResumeOutcome.RESUMED
+
+    # The graph ran through PLAN to its P1 terminus (DONE).
+    state = graph_mod.get_pipeline_state(coord.graph, thread_id=thread_id)
+    assert state["status"] == "done"
+
+
+def test_drain_resumes_before_setup_raises(db_path: Path) -> None:
+    coord = _make_coordinator(db_path)
+    with pytest.raises(RuntimeError):
+        coord.drain_resumes()
+
+
+def test_drain_resumes_empty_queue_is_noop(db_path: Path) -> None:
+    coord = _make_coordinator(db_path)
+    coord.setup()
+    assert coord.drain_resumes() == []
+
+
+# --------------------------------------------------------------------------- #
+# tick — deadline sweep + park ALARM + drain
+# --------------------------------------------------------------------------- #
+
+
+def test_tick_parks_expired_question_and_alarms(db_path: Path) -> None:
+    alarmed: list[str] = []
+    coord = _make_coordinator(db_path, alarm_hook=alarmed.append)
+    coord.setup()
+    coord.start_task(task_text="x", transport_name="slack")
+    qid = _only_open_row(db_path)["question_id"]
+
+    # Force the deadline into the past so the sweep marks it overdue.
+    conn = connect(db_path)
+    try:
+        conn.execute(
+            "UPDATE pending_questions SET deadline_at=? WHERE question_id=?",
+            ("2000-01-01T00:00:00+00:00", qid),
+        )
+    finally:
+        conn.close()
+
+    coord.tick()
+    # Deadline policy flipped it to expired (parked state) and raised the ALARM.
+    assert _row(db_path, qid)["status"] == "expired"
+    assert alarmed == [qid]
+
+
+def test_tick_drains_pending_resume(db_path: Path) -> None:
+    coord = _make_coordinator(db_path)
+    coord.setup()
+    thread_id = coord.start_task(task_text="x", transport_name="slack")
+    qid = _only_open_row(db_path)["question_id"]
+    coord.submit_answer({"question_id": qid, "answer": "go", "via": "v"})
+
+    results = coord.tick()
+    assert len(results) == 1
+    assert results[0].outcome is ResumeOutcome.RESUMED
+    state = graph_mod.get_pipeline_state(coord.graph, thread_id=thread_id)
+    assert state["status"] == "done"
+
+
+# --------------------------------------------------------------------------- #
+# recover — startup convergence (§3.3.1)
+# --------------------------------------------------------------------------- #
+
+
+def test_recover_redrives_answered_row(db_path: Path) -> None:
+    # An answer that landed before a "crash" (no drain) is re-driven on recover.
+    coord = _make_coordinator(db_path)
+    coord.setup()
+    thread_id = coord.start_task(task_text="x", transport_name="slack")
+    qid = _only_open_row(db_path)["question_id"]
+    coord.submit_answer({"question_id": qid, "answer": "go", "via": "v"})
+    # Simulate a crash: the resume job is lost from the queue without draining.
+    coord.resume_queue.get_nowait()
+
+    results = coord.recover()
+    assert any(r.outcome is ResumeOutcome.RESUMED for r in results)
+    state = graph_mod.get_pipeline_state(coord.graph, thread_id=thread_id)
+    assert state["status"] == "done"
+
+
+def test_recover_reposts_open_row_lacking_channel_ref(db_path: Path) -> None:
+    # First delivery's post is lost (row open, no ref); recover re-posts it.
+    transport = FakeTransport(fail_post=True)
+    coord = _make_coordinator(db_path, transport=transport)
+    coord.setup()
+    coord.start_task(task_text="x", transport_name="slack")
+    row = _only_open_row(db_path)
+    assert row["channel_ref"] is None
+
+    # The transport now succeeds; recovery should re-post and record a ref.
+    transport.fail_post = False
+    coord.recover()
+
+    reposted = _only_open_row(db_path)
+    assert reposted["channel_ref"] is not None
+    assert len(transport.posted) == 1
+
+
+def test_redeliver_race_guard_preserves_concurrently_answered_row(
+    db_path: Path,
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """RACE-REDELIVER-NONATOMIC-DELETE (CWE-362): an answer landing in the window
+    BETWEEN the redeliver SELECT and the clear-write must survive.
+
+    This is the exact lost-update window the finding describes. The row is
+    open-and-null-ref at SELECT time (so it is selected for re-delivery), but an
+    answer flips it open → answered on a *separate* connection before the clear
+    runs. The injection point is :func:`graph_mod.pending_question`, which the
+    redeliver loop calls per-row AFTER the SELECT and BEFORE the clear-write —
+    monkeypatching it to land the answer reproduces the concurrent CAS precisely
+    in that window.
+
+    With the old UNCONDITIONAL ``DELETE ... WHERE question_id=?`` the now-answered
+    row would be destroyed (answer lost, ResumeJob dangling, human re-asked). The
+    guarded conditional DELETE re-checks ``status='open' AND channel_ref IS NULL``
+    at write time, matches 0 rows, and SKIPS — so the answered row survives with
+    its answer intact and no fresh open question is re-posted.
+    """
+    from agent_team.db import schema as schema_mod
+    from agent_team import coordinator as coordinator_mod
+
+    # Lost post: open row with no channel_ref (selected by the redeliver SELECT).
+    transport = FakeTransport(fail_post=True)
+    coord = _make_coordinator(db_path, transport=transport)
+    coord.setup()
+    coord.start_task(task_text="x", transport_name="slack")
+    row = _only_open_row(db_path)
+    qid = row["question_id"]
+    assert row["channel_ref"] is None
+
+    # The transport now succeeds — so any re-post would be observable as a fresh
+    # open row + a posted question_set (the clobber symptom we must NOT see).
+    transport.fail_post = False
+
+    real_pending = graph_mod.pending_question
+    landed: dict[str, bool] = {"done": False}
+
+    def _pending_then_answer(graph: Any, *, thread_id: str) -> Any:
+        """Return the live pending payload, then land the answer in the window.
+
+        The redeliver loop calls this between the SELECT and the guarded clear,
+        so landing the open->answered CAS here (on its own connection) is exactly
+        the concurrent answer the finding races against.
+        """
+        result = real_pending(graph, thread_id=thread_id)
+        if not landed["done"]:
+            landed["done"] = True
+            conn = connect(db_path)
+            try:
+                assert schema_mod.answer_question(
+                    conn,
+                    question_id=qid,
+                    answer_json='{"answer": "human said go"}',
+                    answered_via="fake:U1",
+                )
+            finally:
+                conn.close()
+        return result
+
+    monkeypatch.setattr(
+        coordinator_mod.graph_mod, "pending_question", _pending_then_answer
+    )
+
+    # Drive the redeliver sweep (recover's half-2) with the race injected.
+    coord._redeliver_unposted_open_questions()
+
+    # The concurrently-answered row was NOT deleted; its answer_json is intact.
+    after = _row(db_path, qid)
+    assert after is not None, (
+        "guarded clear must not delete the concurrently-answered row"
+    )
+    assert after["status"] == "answered"
+    assert after["answer_json"] == '{"answer": "human said go"}'
+
+    # NO fresh open question was created and NO re-post happened (skip, not re-ask).
+    assert _all_rows(db_path, status="open") == []
+    assert transport.posted == []
+
+
+def test_redeliver_happy_path_reposts_genuinely_unposted_open_row(
+    db_path: Path,
+) -> None:
+    """The guard preserves the normal case: a genuinely lost post (row still
+    open, null ref) IS re-delivered — re-posted and a fresh channel_ref recorded.
+    """
+    transport = FakeTransport(fail_post=True)
+    coord = _make_coordinator(db_path, transport=transport)
+    coord.setup()
+    coord.start_task(task_text="x", transport_name="slack")
+    assert _only_open_row(db_path)["channel_ref"] is None
+
+    # Post now succeeds; the genuinely unposted-open row is re-delivered.
+    transport.fail_post = False
+    coord._redeliver_unposted_open_questions()
+
+    reposted = _only_open_row(db_path)
+    assert reposted["channel_ref"] is not None
+    assert len(transport.posted) == 1
+
+
+def test_guarded_clear_uses_status_and_ref_predicate_rowcount_skip(
+    db_path: Path,
+) -> None:
+    """The guarded write is conditioned on ``status='open' AND channel_ref IS
+    NULL``: it returns False (rowcount 0 → skip) for a row that is not an
+    unposted-open question, and True only for a row that still is.
+    """
+    transport = FakeTransport(fail_post=True)
+    coord = _make_coordinator(db_path, transport=transport)
+    coord.setup()
+    coord.start_task(task_text="x", transport_name="slack")
+    qid = _only_open_row(db_path)["question_id"]
+
+    # A row that is open BUT already has a channel_ref must not be cleared.
+    conn = connect(db_path)
+    try:
+        conn.execute(
+            "UPDATE pending_questions SET channel_ref='fake:ref' WHERE question_id=?",
+            (qid,),
+        )
+    finally:
+        conn.close()
+    assert coord._guarded_clear_unposted_open(qid) is False
+    assert _row(db_path, qid) is not None  # untouched
+
+    # Reset to the genuine unposted-open state: now the predicate matches.
+    conn = connect(db_path)
+    try:
+        conn.execute(
+            "UPDATE pending_questions SET channel_ref=NULL WHERE question_id=?",
+            (qid,),
+        )
+    finally:
+        conn.close()
+    assert coord._guarded_clear_unposted_open(qid) is True
+    assert _row(db_path, qid) is None  # deleted, ready for re-insert
+
+
+def test_recover_before_setup_raises(db_path: Path) -> None:
+    coord = _make_coordinator(db_path)
+    with pytest.raises(RuntimeError):
+        coord.recover()
+
+
+# --------------------------------------------------------------------------- #
+# default_clarify_node_factory — lazy leaves compose into a callable node
+# --------------------------------------------------------------------------- #
+
+
+def test_default_clarify_node_factory_returns_callable_node() -> None:
+    """The factory's lazy imports resolve and the live clarifier leaves compose.
+
+    This is the real (Claude-backed) node the coordinator hands to ``build_graph``
+    when no stub is injected. We only assert it constructs into a callable — the
+    billing seam is unbound and the node is NOT invoked here, so no Claude call
+    is made; this proves the deferred imports + composition wire up cleanly.
+    """
+    node = default_clarify_node_factory()
+    assert callable(node)
+
+
+# --------------------------------------------------------------------------- #
+# P2 wiring — wrapped planner (fail-safe) + bound review loop
+# --------------------------------------------------------------------------- #
+
+
+def test_default_plan_node_factory_passes_through_real_plan(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """A clean planner reply is returned verbatim by the wrapper."""
+    from agent_team.coordinator import default_plan_node_factory
+    from agent_team.nodes import planner
+
+    delta = {"plan": {"phases": ["x"]}, "current_phase": "review", "status": "active"}
+    monkeypatch.setattr(planner, "plan_node", lambda state, config=None: delta)
+    node = default_plan_node_factory()
+    assert node({"thread_id": "t-1"}) == delta
+
+
+def test_default_plan_node_factory_parks_on_planner_error(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """A PlannerError is caught and turned into a PARKED delta, never raised."""
+    from agent_team.coordinator import default_plan_node_factory
+    from agent_team.nodes import planner
+
+    def _boom(state: Any, config: Any = None) -> Any:
+        raise planner.PlannerError("unparseable plan")
+
+    monkeypatch.setattr(planner, "plan_node", _boom)
+    node = default_plan_node_factory()
+    out = node({"thread_id": "t-1"})
+    assert out["status"] == "parked"
+    assert out["current_phase"] == "parked"
+
+
+def test_default_review_wiring_binds_and_returns_node_and_router() -> None:
+    """The wiring binds the GPT-4.1 invoker and returns a single-arg node + router."""
+    from agent_team.coordinator import default_review_wiring
+    from agent_team.nodes import review_loop
+
+    saved = review_loop._review_invoker
+    try:
+        node, router = default_review_wiring()
+        assert callable(node) and callable(router)
+        # The review node is single-arg (no LangGraph config injection).
+        import inspect
+
+        assert len(inspect.signature(node).parameters) == 1
+        assert review_loop._review_invoker is not saved  # invoker was bound
+    finally:
+        review_loop._review_invoker = saved
+
+
+def test_setup_with_p2_factories_builds_a_review_node(db_path: Path) -> None:
+    """Injecting the P2 factories compiles a graph that includes the review vertex."""
+    from agent_team.nodes import review_loop
+
+    saved = review_loop._review_invoker
+    saver = _Saver()
+    try:
+        coord = Coordinator(
+            db_path=db_path,
+            transport=FakeTransport(),
+            build_clarify_node=lambda: graph_mod.clarify_node,
+            build_plan_node=lambda: lambda state: {"current_phase": "review"},
+            review_wiring=lambda: (
+                review_loop.bind_review_node(),
+                review_loop.route_after_review,
+            ),
+            build_checkpointer=lambda _path: saver,
+        )
+        coord.setup()
+        assert graph_mod.REVIEW in coord.graph.get_graph().nodes
+    finally:
+        review_loop._review_invoker = saved

agent-team/tests/test_graph.py

@@ -281,9 +281,89 @@ def test_build_sqlite_checkpointer_builds_when_dep_present(tmp_path) -> None:
     # If the optional package IS installed, the factory returns a checkpointer
     # over the DB path. Skip cleanly where it's absent (pre-deploy scaffolding).
     pytest.importorskip("langgraph.checkpoint.sqlite")
-    saver = build_sqlite_checkpointer(tmp_path / "nested" / "state.db")
+    cm = build_sqlite_checkpointer(tmp_path / "nested" / "state.db")
-    assert saver is not None
+    assert cm is not None
     assert (tmp_path / "nested").is_dir()
+    # Contract: the factory returns a CONTEXT MANAGER (SqliteSaver.from_conn_string
+    # is a @contextmanager), so callers must enter it. Pin that here so a future
+    # change that returns a bare/un-entered object is caught (review FIX).
+    assert hasattr(cm, "__enter__") and hasattr(cm, "__exit__")
+    with cm as saver:
+        # The entered object is the real saver the graph compiles against.
+        assert hasattr(saver, "get_next_version")
+
+
+# --- P2 review-loop wiring. -------------------------------------------------
+
+
+def _p2_plan_stub(state: PipelineState) -> PipelineState:
+    """Stand-in for the real planner: emit a plan and advance to REVIEW.
+
+    Mirrors planner.plan_node's contract (sets ``plan`` + phase REVIEW) without a
+    model call, so the P2 graph topology + the review loop can be driven in a
+    unit test. The revision index tracks prior review rounds.
+    """
+    revisions = len(state.get("review_verdicts") or [])
+    return PipelineState(
+        plan={"phases": ["P1"], "revision": revisions},
+        current_phase=Phase.REVIEW.value,
+        status=TaskStatus.ACTIVE.value,
+    )
+
+
+def _p2_graph(review_text: str):
+    """Compile a P2 graph whose review invoker returns ``review_text``."""
+    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,
+    )
+
+
+@pytest.fixture
+def restore_review_invoker():
+    """Save/restore the review-loop module-global invoker around a test."""
+    from agent_team.nodes import review_loop
+
+    saved = review_loop._review_invoker
+    yield
+    review_loop._review_invoker = saved
+
+
+def test_build_graph_review_node_requires_route() -> None:
+    from agent_team.nodes import review_loop
+
+    with pytest.raises(ValueError, match="route_review"):
+        build_graph(review_node=review_loop.review_node)
+
+
+def test_p2_graph_approve_terminates(restore_review_invoker) -> None:
+    # clarify(stub) -> plan(stub->REVIEW) -> review(APPROVE) -> END.
+    graph = _p2_graph("VERDICT: APPROVE\nlooks solid")
+    thread_id, _ = start_task(graph, transport="slack")
+    final = resume_task(graph, thread_id=thread_id, answer="scope is X")
+
+    # The review node advanced an APPROVED plan toward BUILD and the graph ended.
+    assert final["current_phase"] == Phase.BUILD.value
+    assert len(final["review_verdicts"]) == 1
+
+
+def test_p2_graph_loops_then_escalates_on_persistent_changes(
+    restore_review_invoker,
+) -> None:
+    # A reviewer that never approves loops plan<->review until the round cap,
+    # then escalates (parks) rather than spinning. Default cap is 3 rounds.
+    graph = _p2_graph("VERDICT: REQUEST CHANGES\nstill not ready")
+    thread_id, _ = start_task(graph, transport="slack")
+    final = resume_task(graph, thread_id=thread_id, answer="scope is X")
+
+    assert final["current_phase"] == Phase.PARKED.value
+    assert final["status"] == TaskStatus.PARKED.value
+    assert len(final["review_verdicts"]) == 3  # looped to the cap, then escalated
 
 
 # --- Module import hygiene. -------------------------------------------------

agent-team/tests/test_invoker.py

@@ -0,0 +1,255 @@
+"""Unit tests for agent_team.invoker (§3.1) — all mocked, no network/SDK.
+
+These tests prove the module imports cleanly without ``claude_agent_sdk`` or
+``anthropic`` installed, and exercise each billing path through the injected
+SDK seams (``_query``/``_options_cls``/``_client``) so nothing real is called.
+"""
+
+from __future__ import annotations
+
+import pytest
+
+from agent_team import billing, invoker
+from agent_team.billing import BillingMode, ClaudeResult
+
+
+@pytest.fixture(autouse=True)
+def _restore_invoker():
+    """Restore the module-global billing invoker after each test."""
+    original = billing._invoker
+    yield
+    billing._invoker = original
+
+
+# --------------------------------------------------------------------------- #
+# Fakes mirroring the Agent SDK / anthropic message shapes
+# --------------------------------------------------------------------------- #
+
+
+class _FakeTextBlock:
+    def __init__(self, text: str) -> None:
+        self.text = text
+
+
+# Class names mirror the real Agent SDK message types — the invoker dispatches
+# on ``type(msg).__name__``, so these MUST be named AssistantMessage /
+# ResultMessage to be recognised.
+class AssistantMessage:
+    def __init__(self, text: str) -> None:
+        self.content = [_FakeTextBlock(text)]
+
+
+class ResultMessage:
+    def __init__(self, result: str, cost: float = 0.42) -> None:
+        self.result = result
+        self.total_cost_usd = cost
+        self.usage = {"input_tokens": 11, "output_tokens": 7}
+
+
+def _fake_options(**kwargs):
+    """Stand-in for ClaudeAgentOptions: just record the kwargs."""
+    return dict(kwargs)
+
+
+def _make_fake_query(messages):
+    """Build an async ``query(prompt=..., options=...)`` yielding ``messages``."""
+
+    async def _query(*, prompt, options):
+        for msg in messages:
+            yield msg
+
+    return _query
+
+
+class _FakeAnthropicUsage:
+    def __init__(self) -> None:
+        self.input_tokens = 12
+        self.output_tokens = 5
+
+    def model_dump(self) -> dict:
+        return {"input_tokens": self.input_tokens, "output_tokens": self.output_tokens}
+
+
+class _FakeAnthropicMessage:
+    def __init__(self, text: str) -> None:
+        self.content = [_FakeTextBlock(text)]
+        self.usage = _FakeAnthropicUsage()
+
+
+class _FakeMessages:
+    def __init__(self, text: str) -> None:
+        self._text = text
+        self.calls: list[dict] = []
+
+    def create(self, **kwargs):
+        self.calls.append(kwargs)
+        return _FakeAnthropicMessage(self._text)
+
+
+class _FakeAnthropicClient:
+    def __init__(self, text: str) -> None:
+        self.messages = _FakeMessages(text)
+
+
+# --------------------------------------------------------------------------- #
+# Clean import without the SDKs
+# --------------------------------------------------------------------------- #
+
+
+def test_module_imports_without_sdks() -> None:
+    import importlib
+    import sys
+
+    # claude_agent_sdk / anthropic are not installed in this env.
+    assert "claude_agent_sdk" not in sys.modules
+    mod = importlib.import_module("agent_team.invoker")
+    assert hasattr(mod, "subscription_invoker")
+    assert hasattr(mod, "api_invoker")
+
+
+# --------------------------------------------------------------------------- #
+# Subscription path
+# --------------------------------------------------------------------------- #
+
+
+def test_subscription_invoker_returns_result(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", "oauth-tok")
+    messages = [
+        AssistantMessage("partial "),
+        ResultMessage("final answer"),
+    ]
+    result = invoker.subscription_invoker(
+        "hello",
+        mode=BillingMode.SUBSCRIPTION,
+        _query=_make_fake_query(messages),
+        _options_cls=_fake_options,
+    )
+    assert isinstance(result, ClaudeResult)
+    assert result.text == "final answer"
+    assert result.mode is BillingMode.SUBSCRIPTION
+    # usage populated from ResultMessage cost + usage dict.
+    assert result.usage["total_cost_usd"] == pytest.approx(0.42)
+    assert result.usage["input_tokens"] == 11
+    # raw carries the collected message stream.
+    assert result.raw == messages
+
+
+def test_subscription_invoker_falls_back_to_assistant_text(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", "oauth-tok")
+    messages = [AssistantMessage("a"), AssistantMessage("b")]
+    result = invoker.subscription_invoker(
+        "hi",
+        mode=BillingMode.SUBSCRIPTION,
+        _query=_make_fake_query(messages),
+        _options_cls=_fake_options,
+    )
+    assert result.text == "a\nb"
+
+
+def test_subscription_invoker_requires_oauth_token(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    monkeypatch.delenv("CLAUDE_CODE_OAUTH_TOKEN", raising=False)
+    with pytest.raises(RuntimeError, match="secrev.env"):
+        invoker.subscription_invoker(
+            "hi",
+            mode=BillingMode.SUBSCRIPTION,
+            _query=_make_fake_query([]),
+            _options_cls=_fake_options,
+        )
+
+
+# --------------------------------------------------------------------------- #
+# API path
+# --------------------------------------------------------------------------- #
+
+
+def test_api_invoker_returns_result() -> None:
+    client = _FakeAnthropicClient("api text")
+    result = invoker.api_invoker("ask", mode=BillingMode.API, _client=client)
+    assert result.text == "api text"
+    assert result.mode is BillingMode.API
+    assert result.usage == {"input_tokens": 12, "output_tokens": 5}
+    # The model + prompt were threaded into the SDK call.
+    assert client.messages.calls[0]["model"] == invoker.API_MODEL
+    assert client.messages.calls[0]["messages"] == [{"role": "user", "content": "ask"}]
+
+
+# --------------------------------------------------------------------------- #
+# Bedrock path
+# --------------------------------------------------------------------------- #
+
+
+def test_bedrock_raises_not_implemented() -> None:
+    with pytest.raises(NotImplementedError, match="BEDROCK"):
+        invoker.real_invoker("hi", mode=BillingMode.BEDROCK)
+
+
+# --------------------------------------------------------------------------- #
+# real_invoker dispatch — SUBSCRIPTION + API branches forward kwargs to the leaf
+# --------------------------------------------------------------------------- #
+
+
+def test_real_invoker_dispatches_subscription_branch(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """``real_invoker`` routes SUBSCRIPTION to ``subscription_invoker`` (kwargs fwd)."""
+    monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", "oauth-tok")
+    messages = [AssistantMessage("partial "), ResultMessage("sub final")]
+    result = invoker.real_invoker(
+        "hello",
+        mode=BillingMode.SUBSCRIPTION,
+        _query=_make_fake_query(messages),
+        _options_cls=_fake_options,
+    )
+    assert isinstance(result, ClaudeResult)
+    assert result.mode is BillingMode.SUBSCRIPTION
+    assert result.text == "sub final"  # the ResultMessage leaf ran
+    assert result.raw == messages
+
+
+def test_real_invoker_dispatches_api_branch() -> None:
+    """``real_invoker`` routes API to ``api_invoker`` with the injected client."""
+    client = _FakeAnthropicClient("api branch text")
+    result = invoker.real_invoker("ask", mode=BillingMode.API, _client=client)
+    assert result.mode is BillingMode.API
+    assert result.text == "api branch text"  # the anthropic-client leaf ran
+    # The prompt was threaded through to the injected client.
+    assert client.messages.calls[0]["messages"] == [{"role": "user", "content": "ask"}]
+
+
+# --------------------------------------------------------------------------- #
+# Binding into the billing seam
+# --------------------------------------------------------------------------- #
+
+
+def test_bind_subscription_invoker_sets_billing_invoker() -> None:
+    invoker.bind_subscription_invoker()
+    assert billing._invoker is invoker.real_invoker
+
+
+def test_bound_invoker_drives_claude_invoke(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    # Bind a fake invoker so claude_invoke routes through it end to end.
+    captured: dict = {}
+
+    def fake(prompt: str, *, mode: BillingMode, **kw):
+        captured["prompt"] = prompt
+        captured["mode"] = mode
+        return ClaudeResult(text="routed", mode=mode)
+
+    invoker.bind_invoker(fake)
+    result = billing.claude_invoke("q", mode=BillingMode.API)
+    assert result.text == "routed"
+    assert captured["mode"] is BillingMode.API
+    assert captured["prompt"] == "q"
+
+
+def test_bind_invoker_defaults_to_real_invoker() -> None:
+    invoker.bind_invoker()
+    assert billing._invoker is invoker.real_invoker

agent-team/tests/test_review_loop.py

@@ -94,6 +94,35 @@ def test_parse_verdict_request_changes_wins_on_conflict() -> None:
     assert parse_verdict(text) is ReviewVerdict.REQUEST_CHANGES
 
 
+def test_parse_verdict_approve_with_no_blockers_prose() -> None:
+    # Regression: "no blockers" / "no blocking" prose inside an APPROVE must not
+    # trip the BLOCK change-token (substring false-positive). Word-boundary
+    # matching keeps these as APPROVE.
+    assert parse_verdict("Approved, no blockers.") is ReviewVerdict.APPROVE
+    assert (
+        parse_verdict("VERDICT: APPROVE — no blocking issues found")
+        is ReviewVerdict.APPROVE
+    )
+    assert parse_verdict("LGTM, found no blockers") is ReviewVerdict.APPROVE
+
+
+def test_parse_verdict_real_block_token_requests_changes() -> None:
+    # A real, standalone BLOCK verdict token (rubric vocabulary) -> REQUEST_CHANGES.
+    assert parse_verdict("BLOCK: unsafe IAM policy") is ReviewVerdict.REQUEST_CHANGES
+    assert (
+        parse_verdict("VERDICT: REQUEST CHANGES\nthis is a BLOCK")
+        is ReviewVerdict.REQUEST_CHANGES
+    )
+
+
+def test_parse_verdict_bare_no_blockers_prose_fails_closed() -> None:
+    # "no blockers" with NO explicit APPROVE/LGTM token is genuinely ambiguous
+    # and must fail closed (the dropped NO BLOCKERS approve token is unreachable).
+    # Note these inflected words ("blockers"/"blocking") are NOT change tokens.
+    assert parse_verdict("no blockers") is ReviewVerdict.REQUEST_CHANGES
+    assert parse_verdict("no blocking issues") is ReviewVerdict.REQUEST_CHANGES
+
+
 # --------------------------------------------------------------------------- #
 # build_review_prompt
 # --------------------------------------------------------------------------- #
@@ -350,3 +379,64 @@ def test_review_result_to_dict_round_trips_fields() -> None:
 def test_default_invoker_missing_run_py_raises() -> None:
     with pytest.raises(FileNotFoundError):
         review_loop._orchestrator_invoker("prompt", run_py="/nonexistent/path/run.py")
+
+
+def test_default_invoker_passes_timeout_to_subprocess(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    # The default shell-out must pass a bounded timeout to subprocess.run.
+    captured: dict[str, Any] = {}
+
+    class _Completed:
+        returncode = 0
+        stdout = "VERDICT: APPROVE"
+        stderr = ""
+
+    def _fake_run(args: list[str], **kw: Any) -> _Completed:
+        captured["kw"] = kw
+        return _Completed()
+
+    monkeypatch.setattr(review_loop.subprocess, "run", _fake_run)
+    monkeypatch.setattr(review_loop.os.path, "exists", lambda _p: True)
+    out = review_loop._orchestrator_invoker(
+        "prompt", run_py="/tmp/run.py", config={"review_timeout_seconds": 12}
+    )
+    assert "APPROVE" in out
+    assert captured["kw"]["timeout"] == 12.0
+
+
+def test_default_invoker_timeout_fails_closed(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    # A hung run.py (TimeoutExpired) must fail CLOSED: return text that parses to
+    # REQUEST_CHANGES rather than raising and crashing review_node.
+    import subprocess as _sp
+
+    def _raise_timeout(args: list[str], **kw: Any):
+        raise _sp.TimeoutExpired(cmd=args, timeout=kw.get("timeout", 1))
+
+    monkeypatch.setattr(review_loop.subprocess, "run", _raise_timeout)
+    monkeypatch.setattr(review_loop.os.path, "exists", lambda _p: True)
+    out = review_loop._orchestrator_invoker("prompt", run_py="/tmp/run.py")
+    assert parse_verdict(out) is ReviewVerdict.REQUEST_CHANGES
+
+
+def test_review_node_survives_timeout_fail_closed(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    # End-to-end: a hung default invoker makes review_node loop back / escalate,
+    # never approve, and never raise.
+    import subprocess as _sp
+
+    def _raise_timeout(args: list[str], **kw: Any):
+        raise _sp.TimeoutExpired(cmd=args, timeout=kw.get("timeout", 1))
+
+    monkeypatch.setattr(review_loop.subprocess, "run", _raise_timeout)
+    monkeypatch.setattr(review_loop.os.path, "exists", lambda _p: True)
+    # Use the real default invoker (not a test fake).
+    set_review_invoker(review_loop._orchestrator_invoker)
+    update = review_node(_state(), config={"max_review_rounds": 3})
+    assert (
+        update["review_verdicts"][-1]["verdict"] == ReviewVerdict.REQUEST_CHANGES.value
+    )
+    assert update["current_phase"] == Phase.PLAN.value

agent-team/tests/test_review_loop_llm.py

@@ -0,0 +1,229 @@
+"""Unit tests for agent_team.nodes.review_loop_llm (§3.3, §7.1 P2).
+
+The real GPT-4.1 cross-family review binding is exercised with a FAKE review
+callable that returns canned verdict text — no network, no subprocess. The
+load-bearing properties under test:
+
+* **No orchestrator import at module load.** Importing this module must not pull
+  in the orchestrator package (``models`` / ``graph``); the default reviewer
+  shells out / imports lazily.
+* **Clear approve -> APPROVE.** An injected fake returning an explicit APPROVE
+  verdict maps to the node-contract ``ReviewVerdict.APPROVE`` (proceed).
+* **Changes requested -> REQUEST_CHANGES.** The loop-back / escalate verdict.
+* **Fail SAFE.** A review call that raises, or returns garbage / empty / a
+  non-string, maps to ``REQUEST_CHANGES`` — never an auto-approve.
+* **Routing facts.** The default reviewer resolves the orchestrator root at
+  ``parents[3]`` and a ``run.py`` next to it, and is bound as the default.
+"""
+
+from __future__ import annotations
+
+import subprocess
+import sys
+from typing import Any
+
+import pytest
+
+from agent_team.nodes.review_loop import ReviewVerdict
+from agent_team.nodes.review_loop_llm import (
+    default_plan_reviewer,
+    make_run_py_invoker,
+    resolve_orchestrator_root,
+    resolve_run_py,
+    review_plan,
+)
+
+# --------------------------------------------------------------------------- #
+# Fakes / helpers
+# --------------------------------------------------------------------------- #
+
+
+class _FakeReview:
+    """A fake plan reviewer that returns canned text and records its calls.
+
+    ``reply`` is the verdict text returned every call. ``raises`` (if set) is
+    raised instead, to simulate a failed review call.
+    """
+
+    def __init__(self, reply: Any = "", *, raises: BaseException | None = None) -> None:
+        self._reply = reply
+        self._raises = raises
+        self.calls: list[dict[str, Any]] = []
+
+    def __call__(self, prompt: str, **kw: Any) -> Any:
+        self.calls.append({"prompt": prompt, "kw": kw})
+        if self._raises is not None:
+            raise self._raises
+        return self._reply
+
+
+_PLAN = {"task": "ship a thing", "phases": [{"name": "P1"}, {"name": "P2"}]}
+
+
+def _state() -> dict[str, Any]:
+    return {"plan": _PLAN, "review_verdicts": []}
+
+
+# --------------------------------------------------------------------------- #
+# Module import hygiene
+# --------------------------------------------------------------------------- #
+
+
+def test_module_imports_without_orchestrator() -> None:
+    """Importing the module must not import the orchestrator package."""
+    # The module is already imported at top, but assert the orchestrator stack
+    # did not get pulled in as a side effect of importing it.
+    assert "models" not in sys.modules
+    assert "graph" not in sys.modules
+
+
+# --------------------------------------------------------------------------- #
+# Verdict mapping
+# --------------------------------------------------------------------------- #
+
+
+def test_clear_approve_maps_to_approve() -> None:
+    """An injected fake returning a clear APPROVE verdict -> ReviewVerdict.APPROVE."""
+    fake = _FakeReview("VERDICT: APPROVE\nThe plan is sound and ready to build.")
+    verdict = review_plan(_PLAN, _state(), review=fake)
+    assert verdict is ReviewVerdict.APPROVE
+
+
+def test_changes_requested_maps_to_request_changes() -> None:
+    """An injected fake returning changes-requested -> ReviewVerdict.REQUEST_CHANGES."""
+    fake = _FakeReview("VERDICT: REQUEST CHANGES\nPhase ordering is wrong.")
+    verdict = review_plan(_PLAN, _state(), review=fake)
+    assert verdict is ReviewVerdict.REQUEST_CHANGES
+
+
+def test_block_token_maps_to_request_changes() -> None:
+    """A BLOCK verdict (sh-plan-review vocabulary) -> REQUEST_CHANGES."""
+    fake = _FakeReview("BLOCK: missing rollback phase.")
+    assert review_plan(_PLAN, _state(), review=fake) is ReviewVerdict.REQUEST_CHANGES
+
+
+def test_prompt_only_calling_convention() -> None:
+    """review_plan(prompt=...) reviews an already-composed prompt (the seam shape)."""
+    fake = _FakeReview("APPROVE")
+    verdict = review_plan(prompt="pre-composed review task", review=fake)
+    assert verdict is ReviewVerdict.APPROVE
+    assert fake.calls[0]["prompt"] == "pre-composed review task"
+
+
+def test_prompt_is_composed_from_plan_when_not_supplied() -> None:
+    """With no prompt, the plan text is embedded in the composed review task."""
+    fake = _FakeReview("APPROVE")
+    review_plan(_PLAN, _state(), review=fake)
+    sent = fake.calls[0]["prompt"]
+    assert "ship a thing" in sent
+
+
+# --------------------------------------------------------------------------- #
+# Fail-safe (UNTRUSTED output, never auto-approve)
+# --------------------------------------------------------------------------- #
+
+
+def test_review_call_raising_fails_safe() -> None:
+    """A review call that raises -> REQUEST_CHANGES, never an auto-approve."""
+    fake = _FakeReview(raises=RuntimeError("orchestrator exploded"))
+    assert review_plan(_PLAN, _state(), review=fake) is ReviewVerdict.REQUEST_CHANGES
+
+
+def test_garbage_output_fails_safe() -> None:
+    """Unparseable / ambiguous reviewer text -> REQUEST_CHANGES."""
+    fake = _FakeReview("lorem ipsum dolor sit amet, nothing verdict-like here")
+    assert review_plan(_PLAN, _state(), review=fake) is ReviewVerdict.REQUEST_CHANGES
+
+
+def test_empty_output_fails_safe() -> None:
+    """Empty reviewer output -> REQUEST_CHANGES (fail closed)."""
+    fake = _FakeReview("")
+    assert review_plan(_PLAN, _state(), review=fake) is ReviewVerdict.REQUEST_CHANGES
+
+
+def test_non_string_output_fails_safe() -> None:
+    """A non-string (e.g. None) reviewer output never auto-approves."""
+    fake = _FakeReview(None)
+    assert review_plan(_PLAN, _state(), review=fake) is ReviewVerdict.REQUEST_CHANGES
+
+
+def test_conflicting_tokens_fail_closed() -> None:
+    """When both APPROVE and REQUEST CHANGES appear, fail closed (changes wins)."""
+    fake = _FakeReview("APPROVE in spirit but REQUEST CHANGES on phase 2.")
+    assert review_plan(_PLAN, _state(), review=fake) is ReviewVerdict.REQUEST_CHANGES
+
+
+def test_no_plan_no_prompt_fails_safe() -> None:
+    """No usable plan/state/prompt to review -> REQUEST_CHANGES, no review call."""
+    fake = _FakeReview("APPROVE")
+    verdict = review_plan(plan="not-a-dict", state=None, review=fake)
+    assert verdict is ReviewVerdict.REQUEST_CHANGES
+    assert fake.calls == []
+
+
+# --------------------------------------------------------------------------- #
+# Default routing to GPT-4.1 cross_reviewer (no network: monkeypatched)
+# --------------------------------------------------------------------------- #
+
+
+def test_orchestrator_root_resolves_to_run_py_parent() -> None:
+    """The default reviewer resolves the orchestrator root holding run.py."""
+    root = resolve_orchestrator_root()
+    # run.py lives next to the resolved root.
+    assert resolve_run_py() == str(root / "run.py")
+
+
+def test_default_reviewer_is_bound() -> None:
+    """The module default reviewer is the run.py subprocess invoker."""
+    assert callable(default_plan_reviewer)
+
+
+def test_default_path_invokes_run_py(monkeypatch: pytest.MonkeyPatch) -> None:
+    """The default reviewer shells out to ``python3 <run_py> "<prompt>"``."""
+    captured: dict[str, Any] = {}
+
+    class _Completed:
+        returncode = 0
+        stdout = "VERDICT: APPROVE\nlgtm"
+        stderr = ""
+
+    def _fake_run(args: list[str], **kw: Any) -> _Completed:
+        captured["args"] = args
+        return _Completed()
+
+    monkeypatch.setattr(subprocess, "run", _fake_run)
+    # Point run.py resolution at a path that exists so the existence check passes.
+    monkeypatch.setenv("AGENT_TEAM_ORCHESTRATOR_RUN_PY", __file__)
+
+    invoker = make_run_py_invoker()
+    out = invoker("review this plan")
+    assert "APPROVE" in out
+    assert captured["args"][0] == "python3"
+    assert captured["args"][1] == __file__
+    assert captured["args"][2] == "review this plan"
+
+
+def test_default_path_nonzero_exit_propagates_to_fail_safe(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """A non-zero run.py exit makes review_plan fail safe to REQUEST_CHANGES."""
+
+    class _Completed:
+        returncode = 1
+        stdout = ""
+        stderr = "boom"
+
+    monkeypatch.setattr(subprocess, "run", lambda *a, **k: _Completed())
+    monkeypatch.setenv("AGENT_TEAM_ORCHESTRATOR_RUN_PY", __file__)
+
+    verdict = review_plan(_PLAN, _state())  # uses the default reviewer
+    assert verdict is ReviewVerdict.REQUEST_CHANGES
+
+
+def test_default_path_missing_run_py_fails_safe(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """A missing run.py makes the default review fail safe, not auto-approve."""
+    monkeypatch.setenv("AGENT_TEAM_ORCHESTRATOR_RUN_PY", "/nonexistent/path/to/run.py")
+    verdict = review_plan(_PLAN, _state())
+    assert verdict is ReviewVerdict.REQUEST_CHANGES

agent-team/tests/test_run_team.py

@@ -13,11 +13,13 @@ compare-and-set, and read-only commands needing no confirmation.
 
 from __future__ import annotations
 
+import argparse
 import importlib.util
 import io
 import json
 from pathlib import Path
 from types import ModuleType
+from typing import Any
 
 import pytest
 
@@ -625,3 +627,109 @@ def test_unwritable_audit_path_aborts_before_mutation(
     code, _ = _run(cli, db_path, bad_audit, "expire", "q1", "--confirm")
     assert code == 1  # clean failure, not an uncaught traceback
     assert _status_of(db_path, "q1") == "open"  # NOT mutated — no trail, no change
+
+
+# --------------------------------------------------------------------------- #
+# start / serve coordinator commands + transport factory (lazy, token-tolerant)
+# --------------------------------------------------------------------------- #
+
+
+class _FakeCoordinator:
+    """Records setup()/start_task() so the ``start`` CLI boundary is testable."""
+
+    instances: list[_FakeCoordinator] = []
+
+    def __init__(
+        self,
+        *,
+        db_path: Any,
+        transport: Any,
+        build_plan_node: Any = None,
+        review_wiring: Any = None,
+    ) -> None:
+        self.db_path = db_path
+        self.transport = transport
+        # The production CLI opts the coordinator into the P2 graph by injecting
+        # these factories; record them so the wiring is asserted, not ignored.
+        self.build_plan_node = build_plan_node
+        self.review_wiring = review_wiring
+        self.setup_called = False
+        self.start_kwargs: dict[str, Any] | None = None
+        _FakeCoordinator.instances.append(self)
+
+    def setup(self) -> None:
+        self.setup_called = True
+
+    def start_task(self, *, task_text: str, transport_name: str) -> str:
+        self.start_kwargs = {"task_text": task_text, "transport_name": transport_name}
+        return "thread-minted-42"
+
+
+def test_start_runs_setup_and_start_task_and_prints_thread_id(
+    cli: ModuleType,
+    db_path: Path,
+    audit_log: Path,
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """``start --dry-run`` builds a coordinator, runs setup + start_task, prints id.
+
+    ``_build_coordinator`` imports ``Coordinator`` lazily from
+    ``agent_team.coordinator``, so patching it there intercepts construction.
+    ``--dry-run`` means no Slack token is required (the real
+    ``_build_transport`` returns a ``_DryRunTransport``).
+    """
+    _FakeCoordinator.instances.clear()
+    monkeypatch.setattr(
+        "agent_team.coordinator.Coordinator", _FakeCoordinator, raising=True
+    )
+
+    code, out = _run(
+        cli, db_path, audit_log, "start", "--dry-run", "--task", "do the thing"
+    )
+
+    assert code == 0
+    assert len(_FakeCoordinator.instances) == 1
+    coord = _FakeCoordinator.instances[0]
+    assert coord.setup_called is True
+    assert coord.start_kwargs == {
+        "task_text": "do the thing",
+        "transport_name": "slack",
+    }
+    # The minted thread_id is printed to the captured stdout.
+    assert out.strip() == "thread-minted-42"
+    # --dry-run substitutes the non-posting transport (no token needed).
+    assert isinstance(coord.transport, cli._DryRunTransport)
+    # The production CLI opts into the full P2 graph: planner + review factories
+    # are injected (callables), not left at the P1-stub default of None.
+    assert callable(coord.build_plan_node)
+    assert callable(coord.review_wiring)
+
+
+def test_build_transport_live_github_raises_system_exit(cli: ModuleType) -> None:
+    """A non-slack live transport is not wired and raises a clear SystemExit."""
+    args = argparse.Namespace(dry_run=False, transport="github")
+    with pytest.raises(SystemExit, match="is not wired for the run-team CLI"):
+        cli._build_transport(args)
+
+
+def test_build_transport_live_claude_code_raises_system_exit(cli: ModuleType) -> None:
+    """claude_code is likewise un-wired for the P1 CLI surface."""
+    args = argparse.Namespace(dry_run=False, transport="claude_code")
+    with pytest.raises(SystemExit, match="is not wired for the run-team CLI"):
+        cli._build_transport(args)
+
+
+def test_build_transport_dry_run_returns_dry_run_transport(cli: ModuleType) -> None:
+    """``dry_run=True`` yields a _DryRunTransport whose post returns a synthetic ref."""
+    args = argparse.Namespace(dry_run=True, transport="slack")
+    transport = cli._build_transport(args)
+
+    assert isinstance(transport, cli._DryRunTransport)
+    ref = transport.post_question(
+        thread_id="t1",
+        question_id="q1",
+        turn=0,
+        question_set=None,
+        deadline="2026-06-18T00:00:00+00:00",
+    )
+    assert ref == "dry-run:q1"

agent-team/tests/test_slack_listener.py

@@ -0,0 +1,454 @@
+"""Unit tests for agent_team.transport.slack_listener (§3.3.1).
+
+All mocked — no network, no slack_sdk / slack_bolt. Covers:
+
+* the module imports cleanly without the Slack SDK installed;
+* handle_event on a valid interactive payload from an AUTHORIZED owner accepts
+  and enqueues a resume job;
+* AUTHZ-01: a non-owner sender is rejected (submit_answer + enqueue NOT called,
+  ledger row stays open); an unconfigured allowlist rejects every answer
+  (fail-closed); a payload with no recoverable sender id is rejected;
+* a duplicate event for the same question_id is a no-op (first-answer-wins) and
+  does NOT enqueue again;
+* an unrelated / malformed event (no recoverable question_id) is ignored without
+  crashing and never enqueues;
+* a forged question_id for a nonexistent / closed row is a no-op (accepted=False)
+  — the anti-replay layer (the responder's WHERE status='open' CAS).
+
+The accept / duplicate / forged cases drive a REAL on-disk SQLite ledger (the
+foundation ``init_db`` / ``connect`` + a seeded open question via
+``notify_question``) so the actual compare-and-set runs.
+"""
+
+from __future__ import annotations
+
+import importlib
+from pathlib import Path
+from typing import Any
+
+import pytest
+
+from agent_team.db.schema import connect, init_db
+from agent_team.responder import ResumeJob, notify_question
+from agent_team.transport.base import QuestionSet, Transport
+from agent_team.transport.slack_adapter import SlackTransport, build_callback_id
+from agent_team.transport.slack_listener import SlackListener
+
+
+# ---------------------------------------------------------------------------
+# Fixtures + helpers.
+# ---------------------------------------------------------------------------
+
+# The single authorized owner id used across the accept-path tests (AUTHZ-01).
+OWNER_ID = "U_OWNER"
+
+
+@pytest.fixture
+def db_path(tmp_path: Path) -> Path:
+    """An initialized on-disk ledger DB file (foundation schema)."""
+    path = tmp_path / "agent-team.db"
+    init_db(path)
+    return path
+
+
+class RecordingQueue:
+    """Captures the resume jobs the listener enqueues."""
+
+    def __init__(self) -> None:
+        self.jobs: list[ResumeJob] = []
+
+    def __call__(self, job: ResumeJob) -> None:
+        self.jobs.append(job)
+
+
+def _seed_open_question(
+    db_path: Path,
+    *,
+    thread_id: str = "t1",
+    question_id: str = "q1",
+    turn: int = 0,
+) -> None:
+    """Insert a real ``open`` ledger row via notify_question (no network post)."""
+    conn = connect(db_path)
+    try:
+        # An injected poster that returns a ts ref; never reaches the network.
+        transport = SlackTransport(
+            channel="C123", poster=lambda _msg: {"ts": "1700000000.000100"}
+        )
+        qs = QuestionSet(
+            thread_id=thread_id,
+            question_id=question_id,
+            turn=turn,
+            questions=["proceed?"],
+            context={"repo": "x"},
+        )
+        notify_question(conn, transport, qs, deadline="2026-06-18T00:00:00+00:00")
+    finally:
+        conn.close()
+
+
+def _interactive_payload(
+    question_id: str,
+    value: str = "approve",
+    *,
+    sender_id: str | None = OWNER_ID,
+) -> dict[str, Any]:
+    """A minimal Slack ``block_actions`` payload carrying ``question_id``.
+
+    Carries the interactive sender id (``user.id``) so the AUTHZ-01 allowlist
+    check can run. Pass ``sender_id=None`` to omit the sender entirely (the
+    no-recoverable-sender case).
+    """
+    payload: dict[str, Any] = {
+        "type": "block_actions",
+        "callback_id": build_callback_id(question_id),
+        "actions": [{"action_id": "answer", "value": value}],
+    }
+    if sender_id is not None:
+        payload["user"] = {"id": sender_id}
+    return payload
+
+
+def _listener(
+    db_path: Path,
+    enqueue: Any,
+    *,
+    owner_ids: set[str] | None = frozenset({OWNER_ID}),
+) -> SlackListener:
+    """Construct a listener, authorized for ``OWNER_ID`` by default.
+
+    Pass ``owner_ids=None`` (or an empty set) to exercise the fail-closed
+    unconfigured-allowlist path.
+    """
+    return SlackListener(
+        SlackTransport(channel="C123"),
+        db_path,
+        enqueue,
+        owner_ids=set(owner_ids) if owner_ids else None,
+    )
+
+
+def _row_status(db_path: Path, question_id: str) -> str | None:
+    """Return the ledger ``status`` for ``question_id`` (or ``None`` if absent)."""
+    conn = connect(db_path)
+    try:
+        row = conn.execute(
+            "SELECT status FROM pending_questions WHERE question_id=?",
+            (question_id,),
+        ).fetchone()
+    finally:
+        conn.close()
+    return None if row is None else str(row["status"])
+
+
+# ---------------------------------------------------------------------------
+# Import cleanliness (no SDK).
+# ---------------------------------------------------------------------------
+
+
+def test_module_imports_without_slack_sdk(monkeypatch: pytest.MonkeyPatch) -> None:
+    """Reloading the module + constructing a listener never imports the SDK.
+
+    Mirrors ``tests/test_slack_live.py``: monkeypatch ``builtins.__import__`` to
+    raise ImportError for any ``slack_sdk`` / ``slack_bolt`` import, then reload
+    the module under test. This proves the SDK import is genuinely deferred (it
+    is touched only in :meth:`SlackListener.serve`, never at module import or
+    listener construction time) — independent of any prior ``sys.modules``
+    state, unlike a global-state precondition that is merely order-dependent.
+    """
+    import builtins
+
+    import agent_team.transport.slack_listener as slack_listener_module
+
+    real_import = builtins.__import__
+
+    def _blocked_import(name: str, *args: Any, **kwargs: Any) -> Any:
+        if (
+            name == "slack_sdk"
+            or name.startswith("slack_sdk.")
+            or name == "slack_bolt"
+            or name.startswith("slack_bolt.")
+        ):
+            raise ImportError(f"{name} is blocked for this test")
+        return real_import(name, *args, **kwargs)
+
+    monkeypatch.setattr(builtins, "__import__", _blocked_import)
+
+    module = importlib.reload(slack_listener_module)
+
+    # The reload succeeded with the SDK blocked, and the listener is
+    # constructible without ever importing slack_sdk / slack_bolt.
+    listener = module.SlackListener(
+        SlackTransport(channel="C123"), Path(":memory:"), lambda _job: None
+    )
+    assert isinstance(listener, module.SlackListener)
+
+
+# ---------------------------------------------------------------------------
+# handle_event — accept + enqueue (real CAS).
+# ---------------------------------------------------------------------------
+
+
+def test_handle_event_accepts_and_enqueues(db_path: Path) -> None:
+    _seed_open_question(db_path, question_id="q1", thread_id="t1", turn=0)
+    queue = RecordingQueue()
+    listener = _listener(db_path, queue)
+
+    outcome = listener.handle_event(_interactive_payload("q1", value="approve"))
+
+    assert outcome is not None
+    assert outcome.accepted is True
+    assert outcome.question_id == "q1"
+    assert len(queue.jobs) == 1
+    job = queue.jobs[0]
+    assert job.thread_id == "t1"
+    assert job.question_id == "q1"
+    assert job.turn == 0
+    assert job.answer == "approve"
+
+
+# ---------------------------------------------------------------------------
+# handle_event — AUTHZ-01 owner allowlist (CWE-862), fail-closed.
+# ---------------------------------------------------------------------------
+
+
+def test_handle_event_rejects_non_owner_sender(db_path: Path) -> None:
+    """A sender not in the owner allowlist is rejected; the open row is untouched.
+
+    The payload carries a valid, recoverable question_id mapping to a real open
+    ledger row, so ONLY the sender-identity check stands between the attacker and
+    the first-answer-wins CAS. Prove submit_answer is never reached: no enqueue,
+    and the seeded row stays ``open`` in the real sqlite ledger.
+    """
+    _seed_open_question(db_path, question_id="q1", thread_id="t1", turn=0)
+    queue = RecordingQueue()
+    listener = _listener(db_path, queue, owner_ids={OWNER_ID})
+
+    payload = _interactive_payload("q1", value="approve", sender_id="U_INTRUDER")
+    outcome = listener.handle_event(payload)
+
+    assert outcome is None
+    assert queue.jobs == []
+    # The CAS never ran: the row is still open (submit_answer was not called).
+    assert _row_status(db_path, "q1") == "open"
+
+
+def test_handle_event_rejects_when_allowlist_unconfigured(db_path: Path) -> None:
+    """Fail-closed: with no owner allowlist, EVERY answer is rejected.
+
+    Even a valid question_id from an otherwise-plausible sender is rejected so an
+    unprovisioned deploy accepts answers from no one.
+    """
+    _seed_open_question(db_path, question_id="q1")
+    queue = RecordingQueue()
+    listener = _listener(db_path, queue, owner_ids=None)  # unconfigured
+
+    outcome = listener.handle_event(
+        _interactive_payload("q1", value="approve", sender_id=OWNER_ID)
+    )
+
+    assert outcome is None
+    assert queue.jobs == []
+    assert _row_status(db_path, "q1") == "open"
+
+
+def test_handle_event_rejects_empty_allowlist(db_path: Path) -> None:
+    """An explicitly empty allowlist is also fail-closed (rejects everything)."""
+    _seed_open_question(db_path, question_id="q1")
+    queue = RecordingQueue()
+    listener = _listener(db_path, queue, owner_ids=set())
+
+    outcome = listener.handle_event(_interactive_payload("q1", sender_id=OWNER_ID))
+
+    assert outcome is None
+    assert queue.jobs == []
+    assert _row_status(db_path, "q1") == "open"
+
+
+def test_handle_event_rejects_payload_without_sender_id(db_path: Path) -> None:
+    """A payload from which no sender id can be recovered is rejected (unauthorized)."""
+    _seed_open_question(db_path, question_id="q1")
+    queue = RecordingQueue()
+    listener = _listener(db_path, queue, owner_ids={OWNER_ID})
+
+    # sender_id=None omits ``user`` entirely; no event/user_id either.
+    outcome = listener.handle_event(_interactive_payload("q1", sender_id=None))
+
+    assert outcome is None
+    assert queue.jobs == []
+    assert _row_status(db_path, "q1") == "open"
+
+
+def test_handle_event_accepts_events_api_owner_sender(db_path: Path) -> None:
+    """An Events API message shape resolves the sender via ``event.user``."""
+    _seed_open_question(db_path, question_id="q1", thread_id="t1", turn=0)
+    queue = RecordingQueue()
+    listener = _listener(db_path, queue, owner_ids={OWNER_ID})
+
+    payload = {
+        "type": "message",
+        "callback_id": build_callback_id("q1"),
+        "answer": "approve",
+        "event": {"user": OWNER_ID, "type": "message"},
+    }
+    outcome = listener.handle_event(payload)
+
+    assert outcome is not None and outcome.accepted is True
+    assert len(queue.jobs) == 1
+
+
+def test_handle_event_rejects_events_api_non_owner(db_path: Path) -> None:
+    """An Events API message from a non-owner ``event.user`` is rejected."""
+    _seed_open_question(db_path, question_id="q1")
+    queue = RecordingQueue()
+    listener = _listener(db_path, queue, owner_ids={OWNER_ID})
+
+    payload = {
+        "type": "message",
+        "callback_id": build_callback_id("q1"),
+        "answer": "approve",
+        "event": {"user": "U_INTRUDER", "type": "message"},
+    }
+    outcome = listener.handle_event(payload)
+
+    assert outcome is None
+    assert queue.jobs == []
+    assert _row_status(db_path, "q1") == "open"
+
+
+def test_handle_event_accepts_slash_command_owner_sender(db_path: Path) -> None:
+    """A slash-command shape resolves the sender via ``user_id``."""
+    _seed_open_question(db_path, question_id="q1", thread_id="t1", turn=0)
+    queue = RecordingQueue()
+    listener = _listener(db_path, queue, owner_ids={OWNER_ID})
+
+    payload = {
+        "type": "slash_commands",
+        "callback_id": build_callback_id("q1"),
+        "text": "approve",
+        "user_id": OWNER_ID,
+    }
+    outcome = listener.handle_event(payload)
+
+    assert outcome is not None and outcome.accepted is True
+    assert len(queue.jobs) == 1
+
+
+# ---------------------------------------------------------------------------
+# handle_event — duplicate event (first-answer-wins).
+# ---------------------------------------------------------------------------
+
+
+def test_handle_event_duplicate_is_noop(db_path: Path) -> None:
+    """Second answer for the same question_id loses the CAS; no re-enqueue."""
+    _seed_open_question(db_path, question_id="q1")
+    queue = RecordingQueue()
+    listener = _listener(db_path, queue)
+
+    first = listener.handle_event(_interactive_payload("q1", value="approve"))
+    second = listener.handle_event(_interactive_payload("q1", value="reject"))
+
+    assert first is not None and first.accepted is True
+    assert second is not None and second.accepted is False
+    # First-answer-wins: only the first answer enqueued a resume job.
+    assert len(queue.jobs) == 1
+    assert queue.jobs[0].answer == "approve"
+
+
+# ---------------------------------------------------------------------------
+# handle_event — unrelated / malformed event is ignored.
+# ---------------------------------------------------------------------------
+
+
+def test_handle_event_ignores_non_mapping(db_path: Path) -> None:
+    queue = RecordingQueue()
+    listener = _listener(db_path, queue)
+
+    assert listener.handle_event("not a mapping") is None
+    assert listener.handle_event(None) is None
+    assert queue.jobs == []
+
+
+def test_handle_event_ignores_unrelated_event_type(db_path: Path) -> None:
+    """An event of a type we never act on is filtered before parsing."""
+    queue = RecordingQueue()
+    listener = _listener(db_path, queue)
+
+    # A reaction event carries no question_id and is not answer-bearing.
+    outcome = listener.handle_event({"type": "reaction_added", "reaction": "thumbsup"})
+
+    assert outcome is None
+    assert queue.jobs == []
+
+
+def test_handle_event_ignores_payload_without_question_id(db_path: Path) -> None:
+    """An answer-bearing type with no recoverable question_id is logged-ignored."""
+    queue = RecordingQueue()
+    listener = _listener(db_path, queue)
+
+    # A message with no callback_id / metadata / question_id: parse_answer raises
+    # ValueError, which handle_event swallows.
+    outcome = listener.handle_event(
+        {"type": "message", "text": "just chatting", "channel": "C123"}
+    )
+
+    assert outcome is None
+    assert queue.jobs == []
+
+
+# ---------------------------------------------------------------------------
+# handle_event — forged question_id (trust boundary held by the CAS).
+# ---------------------------------------------------------------------------
+
+
+def test_handle_event_forged_question_id_is_noop(db_path: Path) -> None:
+    """A well-formed payload whose question_id matches no open row is a no-op.
+
+    The id is recoverable (so parse_answer succeeds), but it maps to no
+    ``open`` ledger row, so the responder's ``WHERE status='open'``
+    compare-and-set returns rowcount 0 → accepted=False. This is the documented
+    trust boundary: a forged / replayed id cannot resume a graph.
+    """
+    # Note: NO row seeded for this id.
+    queue = RecordingQueue()
+    listener = _listener(db_path, queue)
+
+    outcome = listener.handle_event(_interactive_payload("forged-qid"))
+
+    assert outcome is not None
+    assert outcome.accepted is False
+    assert outcome.question_id == "forged-qid"
+    assert queue.jobs == []
+
+
+def test_handle_event_closed_row_is_noop(db_path: Path) -> None:
+    """A second submit after the row is already answered loses the CAS too."""
+    _seed_open_question(db_path, question_id="q1")
+    queue = RecordingQueue()
+    listener = _listener(db_path, queue)
+
+    listener.handle_event(_interactive_payload("q1"))  # closes the row
+    queue.jobs.clear()
+
+    # Row is now 'answered'; a fresh forged event for it is a no-op.
+    outcome = listener.handle_event(_interactive_payload("q1", value="late"))
+    assert outcome is not None
+    assert outcome.accepted is False
+    assert queue.jobs == []
+
+
+# ---------------------------------------------------------------------------
+# serve — token guard (no live socket).
+# ---------------------------------------------------------------------------
+
+
+def test_serve_requires_tokens(db_path: Path) -> None:
+    """serve raises a clear RuntimeError when tokens are missing."""
+    listener = _listener(db_path, RecordingQueue())
+    with pytest.raises(RuntimeError, match="app-level token"):
+        listener.serve()
+
+
+def test_real_slack_transport_is_a_transport() -> None:
+    """Sanity: the injected SlackTransport is the contract the listener expects."""
+    assert isinstance(SlackTransport(channel="C123"), Transport)

agent-team/tests/test_slack_live.py

@@ -0,0 +1,269 @@
+"""Unit tests for agent_team.transport.slack_live (§3.3.1, §7.1 P1).
+
+The live poster is the production ``slack_sdk`` backing for the §3.3.1 injected
+``SlackPoster`` seam. These tests prove the contract entirely with mocks (no
+network, and ``slack_sdk`` itself is never required): the poster maps the
+adapter's message dict to the ``chat.postMessage`` parameters Slack accepts,
+the ``ts`` round-trips as the ``channel_ref`` through a real ``SlackTransport``,
+``callback_id`` is dropped (not a valid postMessage param) while ``metadata``
+carries the durable ``question_id``, and a missing package / token or a client
+failure fails loudly.
+"""
+
+from __future__ import annotations
+
+import importlib
+from typing import Any
+
+import pytest
+
+from agent_team.transport.base import QuestionSet
+from agent_team.transport.slack_adapter import SlackPostError, SlackTransport
+from agent_team.transport.slack_live import (
+    build_live_slack_transport,
+    build_slack_poster,
+)
+
+
+# --------------------------------------------------------------------------- #
+# Test doubles                                                                #
+# --------------------------------------------------------------------------- #
+
+
+class _FakeWebClient:
+    """A fake ``slack_sdk.WebClient`` recording ``chat_postMessage`` kwargs."""
+
+    def __init__(self, response: dict[str, Any] | None = None) -> None:
+        self.response = (
+            response if response is not None else {"ts": "169.1", "ok": True}
+        )
+        self.calls: list[dict[str, Any]] = []
+
+    def chat_postMessage(self, **kwargs: Any) -> dict[str, Any]:
+        self.calls.append(kwargs)
+        return self.response
+
+
+class _DataResponse:
+    """A ``slack_sdk.SlackResponse``-like object exposing the payload via ``.data``."""
+
+    def __init__(self, data: dict[str, Any]) -> None:
+        self.data = data
+
+
+class _SlackApiErrorLike(Exception):
+    """Stands in for ``slack_sdk.errors.SlackApiError`` (no slack_sdk needed)."""
+
+
+class _FailingClient:
+    """A fake client whose ``chat_postMessage`` raises a Slack-API-like error."""
+
+    def chat_postMessage(self, **kwargs: Any) -> dict[str, Any]:
+        raise _SlackApiErrorLike("the_dog_ate_it")
+
+
+def _question_set() -> QuestionSet:
+    return QuestionSet(
+        thread_id="task-7",
+        question_id="q-42",
+        turn=1,
+        questions=["Ship it?"],
+        context={"repo": "agent-team"},
+    )
+
+
+# --------------------------------------------------------------------------- #
+# Clean import without slack_sdk                                              #
+# --------------------------------------------------------------------------- #
+
+
+def test_module_imports_without_slack_sdk(monkeypatch: pytest.MonkeyPatch) -> None:
+    """The module imports cleanly even when ``slack_sdk`` cannot be imported."""
+    import builtins
+
+    real_import = builtins.__import__
+
+    def _blocked_import(name: str, *args: Any, **kwargs: Any) -> Any:
+        if name == "slack_sdk" or name.startswith("slack_sdk."):
+            raise ImportError("slack_sdk is blocked for this test")
+        return real_import(name, *args, **kwargs)
+
+    monkeypatch.setattr(builtins, "__import__", _blocked_import)
+
+    module = importlib.reload(
+        importlib.import_module("agent_team.transport.slack_live")
+    )
+    assert hasattr(module, "build_slack_poster")
+    assert hasattr(module, "build_live_slack_transport")
+
+
+# --------------------------------------------------------------------------- #
+# Happy path: injected fake client                                           #
+# --------------------------------------------------------------------------- #
+
+
+def test_poster_returns_mapping_with_ts() -> None:
+    """The poster forwards to the client and returns a mapping carrying ``ts``."""
+    client = _FakeWebClient()
+    poster = build_slack_poster(client=client)
+
+    result = poster(
+        {
+            "channel": "C123",
+            "callback_id": "shq:q-42",
+            "text": "hi",
+            "blocks": [],
+            "metadata": {"event_type": "agent_team_question"},
+        }
+    )
+
+    assert result["ts"] == "169.1"
+
+
+def test_post_question_round_trips_ts_as_channel_ref() -> None:
+    """Wired through a real ``SlackTransport``, ``ts`` becomes the channel_ref."""
+    client = _FakeWebClient()
+    transport = SlackTransport("C123", poster=build_slack_poster(client=client))
+
+    channel_ref = transport.post_question(
+        thread_id="task-7",
+        question_id="q-42",
+        turn=1,
+        question_set=_question_set(),
+        deadline="2026-06-18T00:00:00Z",
+    )
+
+    assert channel_ref == "169.1"
+
+
+def test_callback_id_dropped_metadata_carries_question_id() -> None:
+    """``callback_id`` is not sent; ``metadata`` carries the durable question_id."""
+    client = _FakeWebClient()
+    transport = SlackTransport("C123", poster=build_slack_poster(client=client))
+
+    transport.post_question(
+        thread_id="task-7",
+        question_id="q-42",
+        turn=1,
+        question_set=_question_set(),
+        deadline="2026-06-18T00:00:00Z",
+    )
+
+    assert len(client.calls) == 1
+    kwargs = client.calls[0]
+    # callback_id is NOT a valid chat.postMessage parameter and must be dropped.
+    assert "callback_id" not in kwargs
+    # metadata IS forwarded and carries the durable inbound question_id.
+    assert kwargs["metadata"]["event_payload"]["question_id"] == "q-42"
+    # the accepted parameters are forwarded.
+    assert kwargs["channel"] == "C123"
+    assert "text" in kwargs
+    assert "blocks" in kwargs
+
+
+def test_convenience_transport_factory() -> None:
+    """``build_live_slack_transport`` wires the live poster onto a transport."""
+    client = _FakeWebClient()
+    transport = build_live_slack_transport("C123", client=client)
+
+    channel_ref = transport.post_question(
+        thread_id="task-7",
+        question_id="q-42",
+        turn=1,
+        question_set=_question_set(),
+        deadline="2026-06-18T00:00:00Z",
+    )
+
+    assert channel_ref == "169.1"
+
+
+def test_poster_accepts_slack_response_with_data_attr() -> None:
+    """A ``SlackResponse``-like object is coerced via its ``.data`` mapping."""
+    client = _FakeWebClient(response=None)
+    client.response = _DataResponse({"ts": "169.1", "ok": True})  # type: ignore[assignment]
+    poster = build_slack_poster(client=client)
+
+    result = poster({"channel": "C123", "text": "hi"})
+
+    assert result["ts"] == "169.1"
+
+
+class _UnsupportedResponseClient:
+    """A fake client returning neither a mapping nor an object with ``.data``."""
+
+    def chat_postMessage(self, **kwargs: Any) -> Any:
+        return object()
+
+
+def test_poster_unsupported_response_raises_type_error() -> None:
+    """A response that is neither a mapping nor has a mapping ``.data`` is fatal.
+
+    Exercises the ``_as_mapping`` guard: a bare object (no ``ts``, no ``.data``)
+    cannot yield a ``channel_ref``, so the poster raises ``TypeError`` with the
+    guard's "unsupported response" message rather than silently dropping the ts.
+    """
+    poster = build_slack_poster(client=_UnsupportedResponseClient())
+
+    with pytest.raises(TypeError, match="unsupported response"):
+        poster({"channel": "C123", "text": "hi"})
+
+
+# --------------------------------------------------------------------------- #
+# Failure modes                                                              #
+# --------------------------------------------------------------------------- #
+
+
+def test_missing_token_raises_runtime_error(monkeypatch: pytest.MonkeyPatch) -> None:
+    """No token and no SLACK_BOT_TOKEN raises a clear RuntimeError.
+
+    Stub ``slack_sdk`` into ``sys.modules`` so the deferred import SUCCEEDS and
+    the no-token branch is what's under test. Without this the test is
+    environment-dependent: where slack_sdk is absent (CI) the import-missing
+    error fires first and masks the token check (local-vs-CI drift).
+    """
+    import sys
+    from types import ModuleType
+
+    fake = ModuleType("slack_sdk")
+    fake.WebClient = lambda **kw: None  # type: ignore[attr-defined]
+    monkeypatch.setitem(sys.modules, "slack_sdk", fake)
+    monkeypatch.delenv("SLACK_BOT_TOKEN", raising=False)
+
+    with pytest.raises(RuntimeError, match="Slack bot token"):
+        build_slack_poster()
+
+
+def test_missing_package_raises_runtime_error(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """A missing ``slack_sdk`` package raises a clear RuntimeError."""
+    import builtins
+
+    real_import = builtins.__import__
+
+    def _blocked_import(name: str, *args: Any, **kwargs: Any) -> Any:
+        if name == "slack_sdk" or name.startswith("slack_sdk."):
+            raise ImportError("slack_sdk is blocked for this test")
+        return real_import(name, *args, **kwargs)
+
+    monkeypatch.setattr(builtins, "__import__", _blocked_import)
+    monkeypatch.setenv("SLACK_BOT_TOKEN", "xoxb-present")
+
+    with pytest.raises(RuntimeError, match="slack_sdk is unavailable"):
+        build_slack_poster()
+
+
+def test_client_failure_surfaces_as_slack_post_error() -> None:
+    """A SlackApiError-like client failure surfaces as ``SlackPostError``."""
+    transport = SlackTransport(
+        "C123", poster=build_slack_poster(client=_FailingClient())
+    )
+
+    with pytest.raises(SlackPostError):
+        transport.post_question(
+            thread_id="task-7",
+            question_id="q-42",
+            turn=1,
+            question_set=_question_set(),
+            deadline="2026-06-18T00:00:00Z",
+        )

agent-team/tests/test_verifier_llm.py

@@ -0,0 +1,324 @@
+"""Unit tests for agent_team.nodes.verifier_llm (§3.3, §3.3.2 P3).
+
+These exercise the REAL verifier binding with a FAKE invoke that returns canned
+:class:`~agent_team.billing.ClaudeResult` text — no network, no CI dispatch, no
+filesystem mutation (the module is inert by design, pending the P3 hard gate).
+
+The load-bearing properties under test (all from §3.3.2 boundary #4):
+
+* **Pure-code pass authority.** An authenticated "all checks passed" CI result
+  yields a PASS verdict that comes from :mod:`agent_team.ci_gate`, not the LLM.
+* **Fail safe.** A failed / missing / unauthenticated / run-id-mismatched CI
+  result is never a pass, regardless of any LLM proposal.
+* **The LLM cannot declare green (the §3.3.2 invariant).** Even an adversarial
+  proposer screaming "everything passed" cannot flip a failing verdict to pass.
+* **Advisory only + no crash.** The fix-proposer returns suggestions as DATA,
+  and garbage / unbound / throwing model output never crashes and never changes
+  the verdict.
+"""
+
+from __future__ import annotations
+
+from typing import Any
+
+from agent_team.billing import BillingMode, ClaudeResult
+from agent_team.ci_gate import GateDecision, GateResult
+from agent_team.nodes.verifier_llm import (
+    ClaudeFixProposer,
+    FixProposal,
+    build_fix_advisor,
+    evaluate_verdict,
+    propose_for_failure,
+)
+from agent_team.state_store import compute_content_hash
+
+# --------------------------------------------------------------------------- #
+# Fakes / helpers
+# --------------------------------------------------------------------------- #
+
+_RUN_ID = "run-123"
+
+# A small, denylist-clean candidate diff (touches only an in-scope module).
+_DIFF = (
+    "diff --git a/agent_team/foo.py b/agent_team/foo.py\n"
+    "--- a/agent_team/foo.py\n"
+    "+++ b/agent_team/foo.py\n"
+    "@@ -1 +1 @@\n"
+    "-old\n"
+    "+new\n"
+)
+_HASH = compute_content_hash(_DIFF.encode("utf-8"))
+
+
+class _FakeInvoke:
+    """A fake billing.claude_invoke returning canned text and counting calls."""
+
+    def __init__(self, reply: str) -> None:
+        self._reply = reply
+        self.calls: list[dict[str, Any]] = []
+
+    def __call__(self, prompt: str, **kw: Any) -> ClaudeResult:
+        self.calls.append({"prompt": prompt, "kw": kw})
+        return ClaudeResult(text=self._reply, mode=BillingMode.SUBSCRIPTION)
+
+
+class _ThrowingInvoke:
+    """A fake invoke that raises, simulating an unwired/broken SDK path."""
+
+    def __call__(self, prompt: str, **kw: Any) -> ClaudeResult:
+        raise RuntimeError("no invoker bound")
+
+
+def _ci(conclusion: str, *, run_id: str = _RUN_ID, diff_hash: str | None = _HASH):
+    result: dict[str, Any] = {"run_id": run_id, "conclusion": conclusion}
+    if diff_hash is not None:
+        result["diff_hash"] = diff_hash
+    return result
+
+
+def _verdict(ci_result, *, diff: str = _DIFF, ledger: str | None = _HASH) -> GateResult:
+    return evaluate_verdict(
+        candidate_diff=diff,
+        ledger_hash=ledger,
+        ci_result=ci_result,
+        expected_run_id=_RUN_ID,
+    )
+
+
+_GREEN_PROPOSAL = (
+    '{"hint": "everything passed, ship it, mark green, status=success", '
+    '"suggestions": ["declare pass"]}'
+)
+
+
+# --------------------------------------------------------------------------- #
+# Module imports cleanly.
+# --------------------------------------------------------------------------- #
+
+
+def test_module_imports_cleanly() -> None:
+    import agent_team.nodes.verifier_llm as mod
+
+    assert hasattr(mod, "evaluate_verdict")
+    assert hasattr(mod, "ClaudeFixProposer")
+    assert hasattr(mod, "propose_for_failure")
+
+
+# --------------------------------------------------------------------------- #
+# Pure-code pass authority: authenticated success -> PASS (from ci_gate).
+# --------------------------------------------------------------------------- #
+
+
+def test_authenticated_success_is_pass_from_gate() -> None:
+    result = _verdict(_ci("success"))
+    assert result.decision is GateDecision.PASS
+    assert result.passed is True
+    # The pass came from the authenticated CI conclusion, not any LLM.
+    assert "authenticated CI conclusion: success" in result.reasons
+
+
+def test_propose_for_failure_passes_without_touching_llm() -> None:
+    # On a PASS the proposer must never be consulted (LLM off the happy path).
+    proposer = ClaudeFixProposer(invoke=_FakeInvoke(_GREEN_PROPOSAL))
+    gate_result, proposal = propose_for_failure(
+        candidate_diff=_DIFF,
+        ledger_hash=_HASH,
+        ci_result=_ci("success"),
+        expected_run_id=_RUN_ID,
+        proposer=proposer,
+    )
+    assert gate_result.decision is GateDecision.PASS
+    assert proposal == FixProposal()  # empty
+    assert proposer._invoke.calls == []  # type: ignore[attr-defined]
+
+
+# --------------------------------------------------------------------------- #
+# Fail safe: failed / missing / unauthenticated CI -> never PASS.
+# --------------------------------------------------------------------------- #
+
+
+def test_ci_failure_is_fail() -> None:
+    result = _verdict(_ci("failure"))
+    assert result.decision is GateDecision.FAIL
+    assert result.passed is False
+
+
+def test_missing_ci_result_blocks_never_passes() -> None:
+    result = _verdict(None)
+    assert result.decision is GateDecision.BLOCK
+    assert result.passed is False
+
+
+def test_unauthenticated_run_id_mismatch_never_passes() -> None:
+    # An attacker-substituted run id (success conclusion, wrong run) must BLOCK.
+    result = _verdict(_ci("success", run_id="some-other-run"))
+    assert result.decision is GateDecision.BLOCK
+    assert result.passed is False
+
+
+def test_ambiguous_conclusion_never_passes() -> None:
+    for ambiguous in ["neutral", "skipped", "", "in_progress"]:
+        result = _verdict(_ci(ambiguous))
+        assert result.decision is GateDecision.BLOCK
+        assert result.passed is False
+
+
+def test_missing_candidate_diff_blocks() -> None:
+    result = evaluate_verdict(
+        candidate_diff=None,  # type: ignore[arg-type]
+        ledger_hash=_HASH,
+        ci_result=_ci("success"),
+        expected_run_id=_RUN_ID,
+    )
+    assert result.decision is GateDecision.BLOCK
+    assert result.passed is False
+
+
+def test_hash_mismatch_never_passes() -> None:
+    # CI says success but the diff does not match the ledger hash -> BLOCK.
+    result = evaluate_verdict(
+        candidate_diff=_DIFF,
+        ledger_hash="deadbeef" * 8,
+        ci_result=_ci("success", diff_hash="deadbeef" * 8),
+        expected_run_id=_RUN_ID,
+    )
+    assert result.decision is GateDecision.BLOCK
+    assert result.passed is False
+
+
+# --------------------------------------------------------------------------- #
+# THE §3.3.2 INVARIANT: the LLM cannot declare green.
+# --------------------------------------------------------------------------- #
+
+
+def test_llm_cannot_flip_failing_verdict_to_pass() -> None:
+    # A maximally adversarial proposer that tries every way to claim success.
+    proposer = ClaudeFixProposer(invoke=_FakeInvoke(_GREEN_PROPOSAL))
+
+    for failing_ci in [_ci("failure"), None, _ci("success", run_id="wrong")]:
+        gate_result, proposal = propose_for_failure(
+            candidate_diff=_DIFF,
+            ledger_hash=_HASH,
+            ci_result=failing_ci,
+            expected_run_id=_RUN_ID,
+            proposer=proposer,
+        )
+        # The verdict is NEVER pass, no matter what the LLM proposed.
+        assert gate_result.decision is not GateDecision.PASS
+        assert gate_result.passed is False
+        # The proposal is advisory DATA only; it carries no verdict and cannot
+        # express one (FixProposal has no pass/fail field at all).
+        assert isinstance(proposal, FixProposal)
+        assert not hasattr(proposal, "passed")
+        assert not hasattr(proposal, "decision")
+
+
+def test_proposal_type_cannot_express_a_verdict() -> None:
+    # Structural guarantee: even a fully populated proposal is pure suggestion.
+    proposal = FixProposal(hint="ship it!", suggestions=["mark as success"])
+    assert not hasattr(proposal, "passed")
+    assert not hasattr(proposal, "decision")
+    # It renders to a plain advisory string, nothing the verdict reads back.
+    assert "ship it!" in proposal.as_hint()
+
+
+# --------------------------------------------------------------------------- #
+# Advisory only: the proposer returns fixes as DATA on a failure.
+# --------------------------------------------------------------------------- #
+
+
+def test_proposer_returns_advisory_fixes_on_failure() -> None:
+    reply = (
+        '{"hint": "the lint step failed", '
+        '"suggestions": ["run ruff format", "fix the import order"]}'
+    )
+    proposer = ClaudeFixProposer(invoke=_FakeInvoke(reply))
+    failing = _verdict(_ci("failure"))
+
+    proposal = proposer.propose(failing, {})
+    assert proposal.hint == "the lint step failed"
+    assert proposal.suggestions == ["run ruff format", "fix the import order"]
+    assert "run ruff format" in proposal.as_hint()
+
+
+def test_proposer_not_consulted_on_pass() -> None:
+    proposer = ClaudeFixProposer(invoke=_FakeInvoke('{"hint": "x"}'))
+    passing = _verdict(_ci("success"))
+    proposal = proposer.propose(passing, {})
+    assert proposal == FixProposal()
+    assert proposer._invoke.calls == []  # type: ignore[attr-defined]
+
+
+def test_advise_matches_fix_advisor_seam() -> None:
+    # build_fix_advisor returns a (GateResult, Mapping) -> str callable, the
+    # exact verifier-node FixAdvisor seam.
+    advisor = build_fix_advisor(invoke=_FakeInvoke('{"hint": "fix it"}'))
+    failing = _verdict(_ci("failure"))
+    hint = advisor(failing, {})
+    assert isinstance(hint, str)
+    assert "fix it" in hint
+    # On a PASS the advisor yields no hint (and never calls the model).
+    assert advisor(_verdict(_ci("success")), {}) == ""
+
+
+# --------------------------------------------------------------------------- #
+# Garbage / unbound model output: no crash, verdict unchanged.
+# --------------------------------------------------------------------------- #
+
+
+def test_garbage_proposal_does_not_crash_and_verdict_unchanged() -> None:
+    for garbage in ["", "   ", "not json", "{broken", "null", "42", "[1,2,3]"]:
+        proposer = ClaudeFixProposer(invoke=_FakeInvoke(garbage))
+        gate_result, proposal = propose_for_failure(
+            candidate_diff=_DIFF,
+            ledger_hash=_HASH,
+            ci_result=_ci("failure"),
+            expected_run_id=_RUN_ID,
+            proposer=proposer,
+        )
+        # No crash, empty advisory, verdict still FAIL.
+        assert proposal == FixProposal()
+        assert gate_result.decision is GateDecision.FAIL
+        assert gate_result.passed is False
+
+
+def test_unbound_or_throwing_invoke_fails_safe() -> None:
+    proposer = ClaudeFixProposer(invoke=_ThrowingInvoke())
+    failing = _verdict(_ci("failure"))
+    # A throwing invoker degrades to an empty proposal rather than crashing.
+    proposal = proposer.propose(failing, {})
+    assert proposal == FixProposal()
+
+
+def test_garbage_cannot_flip_to_pass() -> None:
+    # Combine the two invariants: garbage AND a failing verdict -> still fail.
+    proposer = ClaudeFixProposer(invoke=_FakeInvoke("total nonsense, no json"))
+    gate_result, proposal = propose_for_failure(
+        candidate_diff=_DIFF,
+        ledger_hash=_HASH,
+        ci_result=_ci("failure"),
+        expected_run_id=_RUN_ID,
+        proposer=proposer,
+    )
+    assert gate_result.decision is GateDecision.FAIL
+    assert proposal == FixProposal()
+
+
+# --------------------------------------------------------------------------- #
+# Defensive parsing: fenced / prose-wrapped JSON still parses.
+# --------------------------------------------------------------------------- #
+
+
+def test_fenced_json_proposal_parses() -> None:
+    fenced = '```json\n{"hint": "h", "suggestions": ["s"]}\n```'
+    proposer = ClaudeFixProposer(invoke=_FakeInvoke(fenced))
+    proposal = proposer.propose(_verdict(_ci("failure")), {})
+    assert proposal.hint == "h"
+    assert proposal.suggestions == ["s"]
+
+
+def test_prose_wrapped_json_proposal_parses() -> None:
+    prose = 'Sure, here you go:\n{"hint": "do x"}\nHope that helps.'
+    proposer = ClaudeFixProposer(invoke=_FakeInvoke(prose))
+    proposal = proposer.propose(_verdict(_ci("failure")), {})
+    assert proposal.hint == "do x"