diff --git a/agent-team/DEPLOY-R720.md b/agent-team/DEPLOY-R720.md new file mode 100644 index 0000000..18cc9a7 --- /dev/null +++ b/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 # 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 # 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 # 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 --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. diff --git a/agent-team/agent_team/coordinator.py b/agent-team/agent_team/coordinator.py new file mode 100644 index 0000000..79d8ed5 --- /dev/null +++ b/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() diff --git a/agent-team/agent_team/graph.py b/agent-team/agent_team/graph.py index eeda2f9..8f6ee4a 100644 --- a/agent-team/agent_team/graph.py +++ b/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(PLAN, plan_node) + builder.add_node(CLARIFY, clarify) + 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 diff --git a/agent-team/agent_team/invoker.py b/agent-team/agent_team/invoker.py new file mode 100644 index 0000000..58d11b3 --- /dev/null +++ b/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) diff --git a/agent-team/agent_team/nodes/builders_llm.py b/agent-team/agent_team/nodes/builders_llm.py new file mode 100644 index 0000000..30701d6 --- /dev/null +++ b/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 ``/agent-team/agent_team/nodes/builders_llm.py``, + so the orchestrator root is ``parents[3]`` (nodes -> agent_team -> agent-team + -> ). 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 /run.py + ""`` — 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 diff --git a/agent-team/agent_team/nodes/clarifier_llm.py b/agent-team/agent_team/nodes/clarifier_llm.py new file mode 100644 index 0000000..cd6604d --- /dev/null +++ b/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.*?)\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 diff --git a/agent-team/agent_team/nodes/review_loop.py b/agent-team/agent_team/nodes/review_loop.py index daba0d7..d61e959 100644 --- a/agent-team/agent_team/nodes/review_loop.py +++ b/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 -# tie so an ambiguous review fails closed (loops back / escalates) rather than -# advancing a plan the reviewer flagged. -_APPROVE_TOKENS = ("APPROVE", "APPROVED", "LGTM", "NO BLOCKERS", "NO BLOCKING") +# Config / env keys + default for the orchestrator subprocess timeout (seconds). +# Bounds the default shell-out so a hung run.py cannot stall the bounded review +# loop. Mirrors the resolver in review_loop_llm so the two cannot drift. +_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 - ["python3", run_py, prompt], - capture_output=True, - text=True, - check=False, - ) + try: + 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), + ) + 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 - change-requesting token is present, or if 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. + ``LGTM`` tokens (case-insensitive, **word-boundary** matched so that prose + like "no blocking issues" inside an APPROVE does not trip a change token). + The result **fails closed**: if a change-requesting token is present, or if + 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() - has_changes = any(token in haystack for token in _CHANGES_TOKENS) - has_approve = any(token in haystack for token in _APPROVE_TOKENS) - if has_changes: + haystack = text or "" + if _CHANGES_RE.search(haystack): 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. diff --git a/agent-team/agent_team/nodes/review_loop_llm.py b/agent-team/agent_team/nodes/review_loop_llm.py new file mode 100644 index 0000000..1a9a8ed --- /dev/null +++ b/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 /run.py ""``, whose router +sends adversarial-review tasks to ``cross_reviewer`` (GPT-4.1), keeping the call +API-billed and LangSmith-traced. + +```` resolves to ``Path(__file__).resolve().parents[3]`` +(this file lives at ``/agent-team/agent_team/nodes/review_loop_llm.py``, +so parents[0]=nodes, [1]=agent_team, [2]=agent-team, [3]=). 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= 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 ``/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]=). 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 ``/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 ""`` 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= 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) diff --git a/agent-team/agent_team/nodes/verifier_llm.py b/agent-team/agent_team/nodes/verifier_llm.py new file mode 100644 index 0000000..9039806 --- /dev/null +++ b/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.*?)\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 diff --git a/agent-team/agent_team/transport/slack_listener.py b/agent-team/agent_team/transport/slack_listener.py new file mode 100644 index 0000000..b592a56 --- /dev/null +++ b/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 "" + return question_id diff --git a/agent-team/agent_team/transport/slack_live.py b/agent-team/agent_team/transport/slack_live.py new file mode 100644 index 0000000..9f9868e --- /dev/null +++ b/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}" + ) diff --git a/agent-team/run-team.py b/agent-team/run-team.py index 71e895b..6907483 100644 --- a/agent-team/run-team.py +++ b/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 diff --git a/agent-team/systemd/agent-team-coordinator.service b/agent-team/systemd/agent-team-coordinator.service new file mode 100644 index 0000000..20d25d2 --- /dev/null +++ b/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 diff --git a/agent-team/tests/test_builders_llm.py b/agent-team/tests/test_builders_llm.py new file mode 100644 index 0000000..55bb366 --- /dev/null +++ b/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", /run.py, ]`` 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 diff --git a/agent-team/tests/test_clarifier_llm.py b/agent-team/tests/test_clarifier_llm.py new file mode 100644 index 0000000..1448709 --- /dev/null +++ b/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. ") + 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 diff --git a/agent-team/tests/test_coordinator.py b/agent-team/tests/test_coordinator.py new file mode 100644 index 0000000..d0d67c5 --- /dev/null +++ b/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 diff --git a/agent-team/tests/test_graph.py b/agent-team/tests/test_graph.py index fa6ed27..c13e718 100644 --- a/agent-team/tests/test_graph.py +++ b/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") - assert saver is not None + cm = build_sqlite_checkpointer(tmp_path / "nested" / "state.db") + 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. ------------------------------------------------- diff --git a/agent-team/tests/test_invoker.py b/agent-team/tests/test_invoker.py new file mode 100644 index 0000000..b1873ce --- /dev/null +++ b/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 diff --git a/agent-team/tests/test_review_loop.py b/agent-team/tests/test_review_loop.py index 8666321..5870d54 100644 --- a/agent-team/tests/test_review_loop.py +++ b/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 diff --git a/agent-team/tests/test_review_loop_llm.py b/agent-team/tests/test_review_loop_llm.py new file mode 100644 index 0000000..de068a9 --- /dev/null +++ b/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 ""``.""" + 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 diff --git a/agent-team/tests/test_run_team.py b/agent-team/tests/test_run_team.py index 1530a81..196741e 100644 --- a/agent-team/tests/test_run_team.py +++ b/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" diff --git a/agent-team/tests/test_slack_listener.py b/agent-team/tests/test_slack_listener.py new file mode 100644 index 0000000..10eaf85 --- /dev/null +++ b/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) diff --git a/agent-team/tests/test_slack_live.py b/agent-team/tests/test_slack_live.py new file mode 100644 index 0000000..6d3fbf8 --- /dev/null +++ b/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", + ) diff --git a/agent-team/tests/test_verifier_llm.py b/agent-team/tests/test_verifier_llm.py new file mode 100644 index 0000000..10d215d --- /dev/null +++ b/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"