diff --git a/agent-team/agent_team/decisions.py b/agent-team/agent_team/decisions.py new file mode 100644 index 0000000..7a8a205 --- /dev/null +++ b/agent-team/agent_team/decisions.py @@ -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()} diff --git a/agent-team/agent_team/graph.py b/agent-team/agent_team/graph.py index d176f8e..3615030 100644 --- a/agent-team/agent_team/graph.py +++ b/agent-team/agent_team/graph.py @@ -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: diff --git a/agent-team/agent_team/transport/slack_adapter.py b/agent-team/agent_team/transport/slack_adapter.py index e458625..f654da0 100644 --- a/agent-team/agent_team/transport/slack_adapter.py +++ b/agent-team/agent_team/transport/slack_adapter.py @@ -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 diff --git a/agent-team/agent_team/transport/slack_listener.py b/agent-team/agent_team/transport/slack_listener.py index cd900b4..c37fdc1 100644 --- a/agent-team/agent_team/transport/slack_listener.py +++ b/agent-team/agent_team/transport/slack_listener.py @@ -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. diff --git a/agent-team/tests/test_graph.py b/agent-team/tests/test_graph.py index 52fa8ae..d7a13ae 100644 --- a/agent-team/tests/test_graph.py +++ b/agent-team/tests/test_graph.py @@ -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( diff --git a/agent-team/tests/test_slack_adapter.py b/agent-team/tests/test_slack_adapter.py index 4d10e2e..449b790 100644 --- a/agent-team/tests/test_slack_adapter.py +++ b/agent-team/tests/test_slack_adapter.py @@ -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) # # --------------------------------------------------------------------------- # diff --git a/agent-team/tests/test_slack_listener.py b/agent-team/tests/test_slack_listener.py index ed1e7a1..9550703 100644 --- a/agent-team/tests/test_slack_listener.py +++ b/agent-team/tests/test_slack_listener.py @@ -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: