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