adam/orchestrator
Archived
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

agent-team Plane-2: bind P1+P2 to real models, live transport, coordinator #12

Merged
amoussa1229 merged 6 commits from feature/agent-team-plane2-p1-p2 into main 2026-06-18 17:29:03 +00:00
Conversation 0 Commits 6 Files changed 24 +6896 -28
24 changed files with 6896 additions and 28 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

202
agent-team/DEPLOY-R720.md Normal file
View file

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

710
agent-team/agent_team/coordinator.py Normal file
View file

@ -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()

96
agent-team/agent_team/graph.py
View file

@ -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

300
agent-team/agent_team/invoker.py Normal file
View file

@ -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)

374
agent-team/agent_team/nodes/builders_llm.py Normal file
View file

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

466
agent-team/agent_team/nodes/clarifier_llm.py Normal file
View file

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

141
agent-team/agent_team/nodes/review_loop.py
View file

@ -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.

292
agent-team/agent_team/nodes/review_loop_llm.py Normal file
View file

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

494
agent-team/agent_team/nodes/verifier_llm.py Normal file
View file

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

385
agent-team/agent_team/transport/slack_listener.py Normal file
View file

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

142
agent-team/agent_team/transport/slack_live.py Normal file
View file

@ -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}"
)

162
agent-team/run-team.py
View file

@ -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

54
agent-team/systemd/agent-team-coordinator.service Normal file
View file

@ -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

354
agent-team/tests/test_builders_llm.py Normal file
View file

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

335
agent-team/tests/test_clarifier_llm.py Normal file
View file

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

604
agent-team/tests/test_coordinator.py Normal file
View file

@ -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

84
agent-team/tests/test_graph.py
View file

@ -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. -------------------------------------------------

255
agent-team/tests/test_invoker.py Normal file
View file

@ -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

90
agent-team/tests/test_review_loop.py
View file

@ -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

229
agent-team/tests/test_review_loop_llm.py Normal file
View file

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

108
agent-team/tests/test_run_team.py
View file

@ -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"

454
agent-team/tests/test_slack_listener.py Normal file
View file

@ -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)

269
agent-team/tests/test_slack_live.py Normal file
View file

@ -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",
)

324
agent-team/tests/test_verifier_llm.py Normal file
View file

@ -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"