fix(agent-team): centralize safe decision mapping + remove free-text abandon hair-trigger
Security-review follow-up (LOGIC-01/02/05, all confirmed correctness). - New transport-neutral `decisions.normalize_decision(raw, *, allow_abandon)` is the single source of truth: approve-allowlist→approve; abandon-allowlist→abandon ONLY when allow_abandon; everything else (prose, empty, abandon-verbs when disallowed) → request_changes with the full reply as notes; idempotent on an already-formed decision dict. slack_adapter.map_plan_decision is now a thin wrapper (default allow_abandon=True, no caller churn). - LOGIC-01/02: graph._parse_decision now delegates to normalize_decision (was: any unrecognized verb → abandon → FAILED). The graph is now the universal safe backstop, so EVERY writer that bypassed the listener mapping — operator CLI answer_on_behalf (raw), Coordinator.submit_answer (raw), the recovery sweep — loops back on prose instead of silently FAILing the task. Explicit abandon still abandons (preserves the confirmed-button path). - LOGIC-05: the Slack FREE-TEXT reply path maps with allow_abandon=False, so a bare "cancel"/"stop"/"abandon" typed in-thread → request_changes (never terminal abandon); abandon stays reachable only via the confirm-guarded button. Tests: graph unrecognized→loops-back (not FAILED), operator raw-prose→request_ changes, free-text destructive verbs→request_changes vs button→abandon, normalizer idempotency. 1484 passed.
This commit is contained in:
parent
f46d691e36
commit
48a81c0802
7 changed files with 356 additions and 73 deletions
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()}
|
||||
|
|
@ -380,7 +380,9 @@ def plan_gate_node(state: PipelineState) -> PipelineState:
|
|||
the human ``notes`` folded into ``review_verdicts`` as a synthetic
|
||||
REQUEST_CHANGES verdict so the planner's ``_format_review_feedback`` reads
|
||||
it on the re-plan;
|
||||
* ``abandon`` (or any unrecognized decision) -> terminal ``FAILED``.
|
||||
* ``abandon`` (an explicit abandon only) -> terminal ``FAILED``. Any
|
||||
UNRECOGNIZED input is mapped to ``request_changes`` by ``_parse_decision``
|
||||
(LOGIC-01/02), so it loops back rather than silently failing the task.
|
||||
"""
|
||||
prior_visits = int(state.get("plan_gate_visits", 0) or 0)
|
||||
|
||||
|
|
@ -441,7 +443,9 @@ def _apply_plan_decision(
|
|||
``decision`` is the value passed to ``Command(resume=...)`` — the contract
|
||||
``{"decision": ..., "notes": ...}``. A bare string is tolerated as the
|
||||
decision verb. The default for an unrecognized/empty decision is the SAFE
|
||||
direction: terminal ``FAILED`` (never an accidental approve).
|
||||
direction: ``request_changes`` carrying the reply as notes (never an
|
||||
accidental approve, and — per LOGIC-01/02 — never a silent terminal abandon).
|
||||
Only an EXPLICIT ``abandon`` reaches the terminal FAILED branch.
|
||||
"""
|
||||
verb, notes = _parse_decision(decision)
|
||||
now = _utc_now_iso()
|
||||
|
|
@ -478,7 +482,8 @@ def _apply_plan_decision(
|
|||
updated_at=now,
|
||||
)
|
||||
|
||||
# abandon (or any unrecognized decision) -> terminal FAILED.
|
||||
# abandon (an EXPLICIT abandon verb only; unrecognized input was already
|
||||
# mapped to request_changes by _parse_decision) -> terminal FAILED.
|
||||
return PipelineState(
|
||||
status=TaskStatus.FAILED.value,
|
||||
current_phase=_phase_value(Phase.PARKED),
|
||||
|
|
@ -489,24 +494,30 @@ def _apply_plan_decision(
|
|||
|
||||
|
||||
def _parse_decision(decision: Any) -> tuple[str, str]:
|
||||
"""Normalize a resume decision into ``(verb, notes)`` (B2a).
|
||||
"""Normalize a resume decision into ``(verb, notes)`` (B2a, LOGIC-01/02).
|
||||
|
||||
Accepts the ``{"decision": ..., "notes": ...}`` mapping or a bare string. The
|
||||
verb is lowercased/stripped and mapped to one of ``approve`` /
|
||||
``request_changes`` / ``abandon``; anything unrecognized maps to ``abandon``
|
||||
(the SAFE terminal direction — never an accidental approve).
|
||||
The UNIVERSAL SAFE BACKSTOP. A plan-gate decision row can be written by
|
||||
several paths — the Slack listener (which pre-maps), the operator CLI
|
||||
(``answer_on_behalf`` → ``answer_question``, RAW), ``Coordinator.submit_answer``
|
||||
(RAW), and the recovery sweep — and whatever was stored reaches this parser
|
||||
on resume. Delegating to the transport-neutral
|
||||
:func:`agent_team.decisions.normalize_decision` (with ``allow_abandon=True``)
|
||||
makes EVERY writer fail safe at this single chokepoint:
|
||||
|
||||
* an explicit ``approve`` / ``request_changes`` / ``abandon`` (string or the
|
||||
``{"decision","notes"}`` dict) is honoured as-is (idempotent on a dict);
|
||||
* an explicit ``abandon`` still abandons → terminal FAILED (preserves the
|
||||
confirm-guarded Abandon BUTTON path);
|
||||
* anything UNRECOGNIZED — change-request prose, an empty/blank reply, a verb
|
||||
embedded in a sentence — maps to ``request_changes`` carrying the full
|
||||
reply as ``notes``, NOT to ``abandon``. Previously the unrecognized case
|
||||
fell to ``abandon`` → terminal FAILED, silently failing an operator (or any
|
||||
non-listener writer) who answered with prose; this is the LOGIC-01/02 fix.
|
||||
"""
|
||||
raw_decision: Any = ""
|
||||
notes = ""
|
||||
if isinstance(decision, dict):
|
||||
raw_decision = decision.get("decision", "")
|
||||
notes = str(decision.get("notes", "") or "")
|
||||
else:
|
||||
raw_decision = decision
|
||||
verb = str(raw_decision or "").strip().lower()
|
||||
if verb in {"approve", "request_changes", "abandon"}:
|
||||
return verb, notes
|
||||
return "abandon", notes
|
||||
from agent_team.decisions import normalize_decision
|
||||
|
||||
result = normalize_decision(decision, allow_abandon=True)
|
||||
return result["decision"], result["notes"]
|
||||
|
||||
|
||||
def route_after_plan_gate(state: PipelineState) -> str:
|
||||
|
|
|
|||
|
|
@ -27,6 +27,15 @@ from __future__ import annotations
|
|||
from collections.abc import Callable, Mapping, Sequence
|
||||
from typing import Any
|
||||
|
||||
from agent_team.decisions import (
|
||||
ABANDON_VERBS as _ABANDON_VERBS,
|
||||
)
|
||||
from agent_team.decisions import (
|
||||
APPROVE_VERBS as _APPROVE_VERBS,
|
||||
)
|
||||
from agent_team.decisions import (
|
||||
normalize_decision,
|
||||
)
|
||||
from agent_team.transport.base import (
|
||||
NormalizedAnswer,
|
||||
QuestionSet,
|
||||
|
|
@ -69,18 +78,16 @@ CALLBACK_ID_PREFIX = "shq"
|
|||
# can route kind-aware decisions WITHOUT importing the graph module.
|
||||
PLAN_DECISION_KIND = "plan_decision"
|
||||
|
||||
# Plan-decision verb allowlists (B3). A reply / button-value to a
|
||||
# ``kind == 'plan_decision'`` question is normalized (lowercase + strip) and
|
||||
# matched against these allowlists. Anything ELSE — arbitrary change-request
|
||||
# prose — maps to ``request_changes`` carrying the FULL original reply as
|
||||
# ``notes`` (the safe default; NEVER an accidental approve or abandon). See
|
||||
# :func:`map_plan_decision`.
|
||||
PLAN_DECISION_APPROVE_VERBS: frozenset[str] = frozenset(
|
||||
{"approve", "approved", "yes", "ok", "lgtm", "ship"}
|
||||
)
|
||||
PLAN_DECISION_ABANDON_VERBS: frozenset[str] = frozenset(
|
||||
{"abandon", "reject", "cancel", "stop", "kill"}
|
||||
)
|
||||
# Plan-decision verb allowlists (B3). Re-exported from the transport-neutral
|
||||
# :mod:`agent_team.decisions` home (the single source of truth) under their
|
||||
# historical names so existing importers/tests are unchanged. A reply /
|
||||
# button-value to a ``kind == 'plan_decision'`` question is normalized (lowercase
|
||||
# + strip) and matched against these allowlists. Anything ELSE — arbitrary
|
||||
# change-request prose — maps to ``request_changes`` carrying the FULL original
|
||||
# reply as ``notes`` (the safe default; NEVER an accidental approve or abandon).
|
||||
# See :func:`map_plan_decision` / :func:`agent_team.decisions.normalize_decision`.
|
||||
PLAN_DECISION_APPROVE_VERBS: frozenset[str] = _APPROVE_VERBS
|
||||
PLAN_DECISION_ABANDON_VERBS: frozenset[str] = _ABANDON_VERBS
|
||||
|
||||
# Block Kit action_id namespace for the three plan-gate buttons. Each carries
|
||||
# the decision verb; ``request_changes`` opens a modal for free-form notes.
|
||||
|
|
@ -90,38 +97,31 @@ PLAN_DECISION_REQUEST_CHANGES_ACTION = f"{PLAN_DECISION_ACTION_PREFIX}:request_c
|
|||
PLAN_DECISION_ABANDON_ACTION = f"{PLAN_DECISION_ACTION_PREFIX}:abandon"
|
||||
|
||||
|
||||
def map_plan_decision(raw_answer: Any) -> dict[str, str]:
|
||||
def map_plan_decision(raw_answer: Any, *, allow_abandon: bool = True) -> dict[str, str]:
|
||||
"""Map a raw plan-gate reply to a structured ``{"decision", "notes"}`` dict.
|
||||
|
||||
THE LOAD-BEARING B3 SAFETY MAPPING. The graph's ``_parse_decision`` maps any
|
||||
unrecognized verb to ``abandon`` → terminal FAILED, so free-text change
|
||||
notes (e.g. "use pytest fixtures instead") would silently FAIL the task if
|
||||
they reached the graph unmapped. This normalizes BEFORE the graph sees it:
|
||||
Thin wrapper over the transport-neutral
|
||||
:func:`agent_team.decisions.normalize_decision` (the single source of truth),
|
||||
kept so existing importers/tests are unchanged. THE LOAD-BEARING B3 SAFETY
|
||||
MAPPING normalizes a reply BEFORE it reaches the graph so arbitrary change
|
||||
prose becomes ``request_changes`` (carrying the full original reply as
|
||||
``notes``) rather than an accidental approve/abandon.
|
||||
|
||||
* normalize the text (``str`` → lowercase → strip);
|
||||
* **approve** iff in :data:`PLAN_DECISION_APPROVE_VERBS`
|
||||
(``approve/approved/yes/ok/lgtm/ship``), with ``notes=""``;
|
||||
* **abandon** iff in :data:`PLAN_DECISION_ABANDON_VERBS`
|
||||
(``abandon/reject/cancel/stop/kill``), with ``notes=""``;
|
||||
* **everything else → ``request_changes`` with the FULL original reply as
|
||||
``notes``** — the safe default. Arbitrary prose is a change request, never
|
||||
an accidental approve or abandon. Empty / whitespace-only input also maps
|
||||
to ``request_changes`` (with empty notes): a blank reply is treated as a
|
||||
benign no-op change request, never a destructive abandon.
|
||||
``allow_abandon`` (LOGIC-05) distinguishes a CONFIRMED destructive intent
|
||||
from free text:
|
||||
|
||||
* ``allow_abandon=True`` (the default, used by the confirm-dialog-guarded
|
||||
Block Kit Abandon BUTTON and explicit-id paths) — a bare abandon-verb in
|
||||
:data:`PLAN_DECISION_ABANDON_VERBS` maps to ``abandon``.
|
||||
* ``allow_abandon=False`` (FREE-TEXT thread replies) — those SAME verbs are
|
||||
NOT honoured as a destructive abandon; they fall through to
|
||||
``request_changes`` so a casually-typed "cancel" never terminally fails a
|
||||
task with no confirmation. Only the confirmed button can abandon.
|
||||
|
||||
The answer is opaque DATA throughout — never executed or interpreted beyond
|
||||
this verb match. Returns the dict the graph's ``_parse_decision`` consumes.
|
||||
the verb match. Returns the dict the graph's decision parser consumes.
|
||||
"""
|
||||
original = "" if raw_answer is None else str(raw_answer)
|
||||
verb = original.strip().lower()
|
||||
if verb in PLAN_DECISION_APPROVE_VERBS:
|
||||
return {"decision": "approve", "notes": ""}
|
||||
if verb in PLAN_DECISION_ABANDON_VERBS:
|
||||
return {"decision": "abandon", "notes": ""}
|
||||
# Everything else (including empty/whitespace) → request_changes, carrying
|
||||
# the full ORIGINAL reply (not the lowercased form) so the notes preserve
|
||||
# the human's exact wording. Never an accidental abandon.
|
||||
return {"decision": "request_changes", "notes": original.strip()}
|
||||
return normalize_decision(raw_answer, allow_abandon=allow_abandon)
|
||||
|
||||
|
||||
# Type of the network seam: given the rendered Slack message kwargs, perform the
|
||||
|
|
|
|||
|
|
@ -550,13 +550,27 @@ class SlackListener:
|
|||
a row whose ``kind == 'plan_decision'`` — in BOTH the explicit-id and
|
||||
thread-reply paths — the raw answer is mapped to a structured decision
|
||||
``{"decision","notes"}`` via :func:`map_plan_decision` BEFORE it reaches
|
||||
the graph. Without this, the graph's decision parser maps any
|
||||
unrecognized verb to ``abandon`` → terminal FAILED, so arbitrary
|
||||
change-request prose ("use pytest fixtures instead") would silently FAIL
|
||||
the task instead of requesting changes. ``approve``/``abandon`` map to
|
||||
the named verb; EVERYTHING else maps to ``request_changes`` carrying the
|
||||
full original reply as notes (never an accidental abandon). A
|
||||
``clarify`` row passes through UNCHANGED (its free text IS the answer).
|
||||
the graph, with ``approve`` mapped to the named verb and EVERYTHING
|
||||
non-verb mapped to ``request_changes`` carrying the full original reply
|
||||
as notes. A ``clarify`` row passes through UNCHANGED (its free text IS
|
||||
the answer).
|
||||
|
||||
FREE-TEXT vs BUTTON ABANDON (LOGIC-05). The two paths differ ONLY in
|
||||
whether a destructive verb may abandon:
|
||||
|
||||
* **explicit-id path** (the confirm-dialog-guarded Block Kit BUTTONS and
|
||||
the request-changes modal) calls the mapper with ``allow_abandon=True``
|
||||
— a confirmed Abandon button honours the ``abandon`` verb;
|
||||
* **thread-reply path** (raw free text) calls it with
|
||||
``allow_abandon=False`` — a bare destructive verb typed in-thread
|
||||
("cancel"/"abandon"/...) maps to ``request_changes``, NOT a terminal
|
||||
abandon, so a casually-typed word never silently FAILS a task. Only the
|
||||
confirmed button can abandon.
|
||||
|
||||
Without this normalization the graph's parser would still fail safe
|
||||
(it maps unrecognized → request_changes), but pre-mapping at the
|
||||
transport keeps the listener's free-text guard authoritative for the
|
||||
confirm distinction.
|
||||
|
||||
Returns the payload to submit, or ``None`` when no question can be
|
||||
resolved (the caller ignores the event without crashing). Re-raises
|
||||
|
|
@ -576,9 +590,13 @@ class SlackListener:
|
|||
# the {"decision","notes"} dict; otherwise pass through unchanged.
|
||||
kind = self._question_kind(conn, question_id)
|
||||
if kind == PLAN_DECISION_KIND:
|
||||
# Explicit-id path = the confirm-guarded Block Kit BUTTONS
|
||||
# (Approve / Abandon — Abandon behind a confirm dialog) and the
|
||||
# request-changes modal submit. These are CONFIRMED destructive
|
||||
# intents, so an abandon verb is honoured: allow_abandon=True.
|
||||
return {
|
||||
"question_id": question_id,
|
||||
"answer": map_plan_decision(answer),
|
||||
"answer": map_plan_decision(answer, allow_abandon=True),
|
||||
}
|
||||
return raw_payload
|
||||
|
||||
|
|
@ -605,13 +623,20 @@ class SlackListener:
|
|||
|
||||
text = event.get("text")
|
||||
answer = text.strip() if isinstance(text, str) else text
|
||||
# Kind-aware normalization (B3): a free-text reply to a plan-decision row
|
||||
# Kind-aware normalization (B3): a FREE-TEXT reply to a plan-decision row
|
||||
# — arbitrary change-request prose — is mapped to a structured decision
|
||||
# (request_changes carrying the full reply as notes; NEVER an accidental
|
||||
# abandon). A clarify row's free text passes through unchanged: the text
|
||||
# IS the answer.
|
||||
#
|
||||
# LOGIC-05: free text NEVER abandons. ``allow_abandon=False`` so a bare
|
||||
# destructive verb typed in-thread ("cancel"/"stop"/"abandon"/...) maps
|
||||
# to request_changes (carrying that text as notes), NOT a terminal
|
||||
# abandon. Only the confirm-dialog-guarded Block Kit Abandon BUTTON
|
||||
# (the explicit-id path above, allow_abandon=True) can terminally
|
||||
# abandon the task.
|
||||
if kind == PLAN_DECISION_KIND:
|
||||
answer = map_plan_decision(answer)
|
||||
answer = map_plan_decision(answer, allow_abandon=False)
|
||||
# Synthesize an explicit payload the transport already understands
|
||||
# (bare question_id + answer). The answer is opaque DATA — stored
|
||||
# verbatim as JSON by the responder, never interpreted.
|
||||
|
|
|
|||
|
|
@ -656,13 +656,67 @@ def test_plan_gate_abandon_fails(restore_review_invoker) -> None:
|
|||
assert final["status"] == TaskStatus.FAILED.value
|
||||
|
||||
|
||||
def test_plan_gate_unrecognized_decision_fails_safe(restore_review_invoker) -> None:
|
||||
# An unrecognized decision must NOT accidentally approve — it fails terminal.
|
||||
def test_plan_gate_unrecognized_decision_loops_back_not_failed(
|
||||
restore_review_invoker,
|
||||
) -> None:
|
||||
# LOGIC-01/02: an UNRECOGNIZED decision must NOT silently FAIL the task (the
|
||||
# old behavior). The graph backstop maps it to request_changes — it loops
|
||||
# back to the planner (and, with a reviewer that never approves, re-suspends
|
||||
# on the gate), NEVER terminal FAILED, and NEVER an accidental approve.
|
||||
from agent_team.graph import pending_question
|
||||
|
||||
graph = _plan_gate_graph("VERDICT: REQUEST CHANGES\nnope")
|
||||
thread_id, _ = _drive_to_plan_gate(graph)
|
||||
final = resume_task(graph, thread_id=thread_id, answer={"decision": "huh?"})
|
||||
|
||||
assert final["status"] == TaskStatus.FAILED.value
|
||||
assert final["status"] != TaskStatus.FAILED.value
|
||||
# request_changes loops plan->review and re-suspends on the gate (reviewer
|
||||
# never approves): the task is still pending a human decision, not terminal.
|
||||
assert pending_question(graph, thread_id=thread_id) is not None
|
||||
|
||||
|
||||
def test_plan_gate_operator_raw_prose_loops_back_not_failed(
|
||||
restore_review_invoker,
|
||||
) -> None:
|
||||
# LOGIC-01/02 (operator-CLI-style RAW write): a non-listener writer (operator
|
||||
# CLI / Coordinator.submit_answer) stores RAW change-request prose on a
|
||||
# plan_decision row — it never passes through the Slack listener's pre-map.
|
||||
# The graph backstop (_parse_decision via normalize_decision) maps it to
|
||||
# request_changes carrying the full prose as notes, so it loops back to the
|
||||
# planner rather than the old silent abandon -> terminal FAILED.
|
||||
from agent_team.graph import pending_question
|
||||
from agent_team.nodes.planner import _format_review_feedback
|
||||
|
||||
captured: dict[str, object] = {}
|
||||
|
||||
def capturing_plan(state):
|
||||
captured["feedback"] = _format_review_feedback(
|
||||
list(state.get("review_verdicts") or [])
|
||||
)
|
||||
return _p2_plan_stub(state)
|
||||
|
||||
from agent_team.nodes import review_loop
|
||||
|
||||
review_loop.set_review_invoker(lambda prompt, **kw: "VERDICT: REQUEST CHANGES\nno")
|
||||
graph = build_graph(
|
||||
checkpointer=_Saver(),
|
||||
live_plan_node=capturing_plan,
|
||||
review_node=review_loop.bind_review_node(),
|
||||
route_review=review_loop.route_after_review,
|
||||
plan_gate=True,
|
||||
)
|
||||
thread_id, _ = _drive_to_plan_gate(graph)
|
||||
|
||||
# A bare RAW string (exactly what answer_question stores for an operator who
|
||||
# typed prose) resumes the gate. It must NOT fail the task.
|
||||
final = resume_task(
|
||||
graph, thread_id=thread_id, answer="please use pytest fixtures instead"
|
||||
)
|
||||
|
||||
assert final["status"] != TaskStatus.FAILED.value
|
||||
assert pending_question(graph, thread_id=thread_id) is not None
|
||||
# The raw prose was folded into the re-plan as request_changes notes.
|
||||
assert "pytest fixtures" in str(captured.get("feedback", ""))
|
||||
|
||||
|
||||
def test_plan_gate_request_changes_terminates_at_ceiling(
|
||||
|
|
|
|||
|
|
@ -481,6 +481,80 @@ def test_map_plan_decision_verb_with_trailing_text_is_request_changes() -> None:
|
|||
assert result["notes"] == raw
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# normalize_decision — shared transport-neutral normalizer (decisions.py) #
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
|
||||
@pytest.mark.parametrize("verb", ["abandon", "reject", "cancel", "stop", "kill"])
|
||||
def test_normalize_decision_free_text_destructive_verb_not_abandon(verb: str) -> None:
|
||||
"""LOGIC-05: with allow_abandon=False, destructive verbs -> request_changes.
|
||||
|
||||
A bare destructive verb (the free-text path) is NOT a confirmed abandon; it
|
||||
carries the verb as notes and requests changes instead of terminally
|
||||
failing.
|
||||
"""
|
||||
from agent_team.decisions import normalize_decision
|
||||
|
||||
result = normalize_decision(verb, allow_abandon=False)
|
||||
assert result["decision"] == "request_changes"
|
||||
assert result["decision"] != "abandon"
|
||||
assert result["notes"] == verb
|
||||
|
||||
|
||||
@pytest.mark.parametrize("verb", ["abandon", "reject", "cancel", "stop", "kill"])
|
||||
def test_normalize_decision_button_destructive_verb_abandons(verb: str) -> None:
|
||||
"""With allow_abandon=True (confirmed button), destructive verbs abandon."""
|
||||
from agent_team.decisions import normalize_decision
|
||||
|
||||
result = normalize_decision(verb, allow_abandon=True)
|
||||
assert result == {"decision": "abandon", "notes": ""}
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"decision",
|
||||
[
|
||||
{"decision": "approve", "notes": ""},
|
||||
{"decision": "request_changes", "notes": "do X"},
|
||||
{"decision": "abandon", "notes": ""},
|
||||
],
|
||||
)
|
||||
@pytest.mark.parametrize("allow_abandon", [True, False])
|
||||
def test_normalize_decision_idempotent_on_decision_dict(
|
||||
decision: dict, allow_abandon: bool
|
||||
) -> None:
|
||||
"""An already-formed valid decision dict passes through normalized, un-remapped.
|
||||
|
||||
Idempotency: re-normalizing a dict that already carries a valid decision
|
||||
returns it unchanged (notes preserved). An explicit abandon dict is honoured
|
||||
even with allow_abandon=False — it is an explicit decision, not free text.
|
||||
"""
|
||||
from agent_team.decisions import normalize_decision
|
||||
|
||||
result = normalize_decision(decision, allow_abandon=allow_abandon)
|
||||
assert result == decision
|
||||
# Double-application is stable.
|
||||
assert normalize_decision(result, allow_abandon=allow_abandon) == decision
|
||||
|
||||
|
||||
def test_normalize_decision_idempotent_normalizes_casing() -> None:
|
||||
"""A decision dict with odd casing/whitespace is normalized (not re-mapped)."""
|
||||
from agent_team.decisions import normalize_decision
|
||||
|
||||
result = normalize_decision(
|
||||
{"decision": " Request_Changes ", "notes": "Keep Casing"},
|
||||
allow_abandon=True,
|
||||
)
|
||||
assert result == {"decision": "request_changes", "notes": "Keep Casing"}
|
||||
|
||||
|
||||
def test_map_plan_decision_free_text_abandon_verb_requests_changes() -> None:
|
||||
"""The wrapper threads allow_abandon: free-text 'cancel' -> request_changes."""
|
||||
result = map_plan_decision("cancel", allow_abandon=False)
|
||||
assert result["decision"] == "request_changes"
|
||||
assert result["notes"] == "cancel"
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# B3 — gate decision blocks (the three buttons) #
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
|
|
|||
|
|
@ -960,17 +960,29 @@ def test_plan_decision_free_text_approve_maps_to_approve(db_path: Path) -> None:
|
|||
assert answer == {"decision": "approve", "notes": ""}
|
||||
|
||||
|
||||
def test_plan_decision_free_text_abandon_maps_to_abandon(db_path: Path) -> None:
|
||||
@pytest.mark.parametrize("verb", ["cancel", "stop", "abandon", "kill", "reject"])
|
||||
def test_plan_decision_free_text_destructive_verb_maps_to_request_changes(
|
||||
db_path: Path, verb: str
|
||||
) -> None:
|
||||
"""LOGIC-05: a bare destructive verb typed as FREE TEXT must NOT abandon.
|
||||
|
||||
The Block Kit Abandon button is guarded by a confirm dialog; a casually-typed
|
||||
"cancel"/"stop"/"abandon" in a thread is NOT a confirmed destructive intent.
|
||||
It must map to request_changes (carrying the verb as notes), never a terminal
|
||||
abandon. Only the confirmed button path (allow_abandon=True) abandons.
|
||||
"""
|
||||
_seed_plan_decision_question(
|
||||
db_path, question_id="q1", channel_ref=SEED_CHANNEL_REF
|
||||
)
|
||||
queue = RecordingQueue()
|
||||
listener = _listener(db_path, queue, owner_ids={OWNER_ID})
|
||||
|
||||
listener.handle_event(_events_api_reply(text="cancel", thread_ts=SEED_CHANNEL_REF))
|
||||
listener.handle_event(_events_api_reply(text=verb, thread_ts=SEED_CHANNEL_REF))
|
||||
|
||||
answer = queue.jobs[0].answer
|
||||
assert answer == {"decision": "abandon", "notes": ""}
|
||||
assert answer["decision"] == "request_changes"
|
||||
assert answer["decision"] != "abandon"
|
||||
assert answer["notes"] == verb
|
||||
|
||||
|
||||
def test_clarify_free_text_passes_through_unchanged_regression(db_path: Path) -> None:
|
||||
|
|
|
|||
Reference in a new issue