From 403b91328797639a000503338c97fe37958eed3f Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Tue, 23 Jun 2026 16:42:27 -0400 Subject: [PATCH] fix(agent-team): thread lifecycle milestones + present the plan in Slack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps surfaced by a live /new-task (task 9bce78ad): the plan was produced and approved, but the "plan ready" notice posted top-level (not in the task thread) and contained no plan to review. 1. THREADING — `run-team.py` `_build_notifiers` exposed `notify(message)` with no `thread_ts`. The coordinator's `_emit` calls `notify(message, thread_ts=root)`; that raised TypeError, and `_emit`'s fallback re-posted TOP-LEVEL. So every lifecycle milestone (plan-ready / parked / failed) landed unthreaded, despite the coordinator computing the root ts. (The clarifier QUESTION threaded fine — different path.) Fix: the sink now accepts and forwards `thread_ts` into the chat.postMessage payload (build_slack_poster already forwards the key). 2. PRESENTATION — the plan-ready milestone was a bare one-liner. It now posts a CONDENSED plan (summary + numbered phase names; step detail stays on the status dashboard) via new `Coordinator._summarize_plan`, so the plan is actually reviewable in-thread. Tests: notify sink forwards thread_ts (and omits it for top-level); condensed plan renders summary + phase names (not steps); malformed plan falls back; plan-ready milestone threads under the task root AND carries the plan. 1193 pass. --- agent-team/agent_team/coordinator.py | 41 ++++++++++++++++- agent-team/run-team.py | 13 +++++- agent-team/tests/test_coordinator.py | 67 ++++++++++++++++++++++++++++ agent-team/tests/test_run_team.py | 44 ++++++++++++++++++ 4 files changed, 162 insertions(+), 3 deletions(-) diff --git a/agent-team/agent_team/coordinator.py b/agent-team/agent_team/coordinator.py index 14a5134..53e67ca 100644 --- a/agent-team/agent_team/coordinator.py +++ b/agent-team/agent_team/coordinator.py @@ -995,11 +995,50 @@ class Coordinator: thread_ts=root_ts, ) else: + # Plan approved + settled at the P2 terminus. PRESENT the plan + # (condensed) so the human can actually review it in-thread, not + # just a "ready" notice with nothing to look at. self._emit( - f"✅ {label} — plan ready for review (phase: {phase}).", + f"✅ {label} — plan ready for review (phase: {phase}).\n" + f"{self._summarize_plan(values)}", thread_ts=root_ts, ) + @staticmethod + def _summarize_plan(values: "dict[str, Any]") -> str: + """Condensed, Slack-friendly view of the approved plan (summary + phases). + + Posts the plan ``summary`` (collapsed + truncated) plus the numbered + phase NAMES — enough to review/approve the shape in-thread without + dumping every step. Falls back to a bare line if the plan is malformed + (the milestone must still post). Step detail lives on the status + dashboard / a follow-up; this is the at-a-glance review view. + """ + plan = values.get("plan") + if not isinstance(plan, dict): + return "• (plan unavailable to summarize)" + lines: list[str] = [] + summary = " ".join(str(plan.get("summary") or "").split()) + if summary: + if len(summary) > 350: + summary = summary[:350] + "…" + lines.append(f"• Summary: {summary}") + phases = plan.get("phases") or [] + if isinstance(phases, list) and phases: + names = [ + str(p.get("name")).strip() + for p in phases + if isinstance(p, dict) and str(p.get("name") or "").strip() + ] + if names: + lines.append(f"• Phases ({len(names)}):") + lines += [f" {i}. {n}" for i, n in enumerate(names, start=1)] + lines.append( + "• Reply in this thread to steer, or re-assign with changes. " + "(Full step detail: status dashboard.)" + ) + return "\n".join(lines) + @staticmethod def _summarize_blocker(values: "dict[str, Any]") -> str: """Human-readable reason a task parked, from the last review verdict. diff --git a/agent-team/run-team.py b/agent-team/run-team.py index d699217..58326f3 100644 --- a/agent-team/run-team.py +++ b/agent-team/run-team.py @@ -544,8 +544,17 @@ def _build_notifiers( ) return None, None - def notify(message: str) -> None: - poster({"channel": channel, "text": message}) + def notify(message: str, thread_ts: str | None = None) -> None: + # `thread_ts` is the one-thread-per-task root ts: the coordinator's + # `_emit` passes it so lifecycle milestones (plan-ready / parked / + # failed) thread under the task's "📥 Task received" root. Without this + # param `_emit` hit a TypeError and silently fell back to a TOP-LEVEL + # post, so every milestone landed unthreaded. build_slack_poster already + # forwards a `thread_ts` payload key to chat.postMessage. + payload = {"channel": channel, "text": message} + if thread_ts: + payload["thread_ts"] = thread_ts + poster(payload) def alarm_hook(question_id: str) -> None: _LOG.warning("park ALARM: clarifier question %s expired", question_id) diff --git a/agent-team/tests/test_coordinator.py b/agent-team/tests/test_coordinator.py index f3470de..15afb11 100644 --- a/agent-team/tests/test_coordinator.py +++ b/agent-team/tests/test_coordinator.py @@ -409,6 +409,73 @@ def test_drain_resumes_one_failing_task_does_not_block_others(db_path: Path) -> assert graph_mod.get_pipeline_state(coord.graph, thread_id=t2)["status"] == "failed" +# --------------------------------------------------------------------------- # +# plan presentation + lifecycle-milestone threading +# --------------------------------------------------------------------------- # + + +def test_summarize_plan_condensed_summary_and_phase_names() -> None: + """The condensed plan view renders the summary + numbered phase NAMES only.""" + values = { + "plan": { + "summary": "Add a hermetic smoke test.", + "phases": [ + {"name": "Audit infra", "steps": ["look at tests/", "read conftest"]}, + { + "name": "Write test_smoke.py", + "steps": ["import checks", "graph build"], + }, + {"name": "Lint + commit", "steps": ["ruff", "pytest"]}, + ], + } + } + out = Coordinator._summarize_plan(values) + assert "Summary: Add a hermetic smoke test." in out + assert "Phases (3):" in out + assert "1. Audit infra" in out + assert "2. Write test_smoke.py" in out + assert "3. Lint + commit" in out + # Condensed: step detail is NOT dumped. + assert "read conftest" not in out + assert "ruff" not in out + + +def test_summarize_plan_malformed_falls_back_without_crashing() -> None: + assert "unavailable" in Coordinator._summarize_plan({"plan": None}) + assert "unavailable" in Coordinator._summarize_plan({}) + + +def test_plan_ready_milestone_threads_under_root_and_presents_plan( + db_path: Path, +) -> None: + """A settled approved plan posts the plan (condensed) THREADED under the root. + + Covers both fixes: the milestone is delivered with the task's + ``slack_thread_ts`` (threading) and now carries the plan summary, not just a + bare "ready" line (presentation). + """ + posted: list[tuple[str, str | None]] = [] + coord = _make_coordinator( + db_path, + notify=lambda message, thread_ts=None: posted.append((message, thread_ts)), + ) + coord.setup() + coord.start_task( + task_text="add a smoke test", transport_name="slack", slack_thread_ts="ROOT.TS" + ) + qid = _only_open_row(db_path)["question_id"] + coord.submit_answer({"question_id": qid, "answer": "go", "via": "v"}) + results = coord.drain_resumes() + assert results[0].outcome is ResumeOutcome.RESUMED + + coord._post_resume_followups(results) + ready = [p for p in posted if "plan ready for review" in p[0]] + assert len(ready) == 1 + message, thread_ts = ready[0] + assert thread_ts == "ROOT.TS" # threaded under the task root, not top-level + assert "Summary:" in message # the plan is actually presented + + # --------------------------------------------------------------------------- # # tick — deadline sweep + park ALARM + drain # --------------------------------------------------------------------------- # diff --git a/agent-team/tests/test_run_team.py b/agent-team/tests/test_run_team.py index 2826bc5..ad6de38 100644 --- a/agent-team/tests/test_run_team.py +++ b/agent-team/tests/test_run_team.py @@ -1033,3 +1033,47 @@ def test_fix_non_fixable_finding_returns_one( ) assert rc == 1 assert "FIX NOT PLANNED" in out.getvalue() + + +# --------------------------------------------------------------------------- # +# _build_notifiers — the Slack lifecycle-milestone sink (one-thread-per-task) +# --------------------------------------------------------------------------- # + + +def test_notify_sink_forwards_thread_ts( + cli: ModuleType, monkeypatch: pytest.MonkeyPatch +) -> None: + """The notify sink must pass `thread_ts` so milestones thread under the task. + + Regression: the sink was `def notify(message)` with no `thread_ts`, so the + coordinator's `_emit(message, thread_ts=root)` hit a TypeError and silently + fell back to a TOP-LEVEL post — every plan-ready/parked/failed milestone + landed unthreaded. build_slack_poster already forwards a `thread_ts` payload + key, so the only gap was this wrapper. + """ + captured: list[dict] = [] + monkeypatch.setenv("SLACK_CHANNEL_ID", "C123") + # _build_notifiers imports build_slack_poster from slack_live at call time. + import agent_team.transport.slack_live as slack_live + + monkeypatch.setattr( + slack_live, + "build_slack_poster", + lambda: lambda payload: captured.append(payload), + ) + + args = argparse.Namespace(dry_run=False, transport="slack") + notify, _alarm = cli._build_notifiers(args) + assert notify is not None + + notify("threaded milestone", thread_ts="ROOT.TS") + notify("top-level milestone") + + assert captured[0] == { + "channel": "C123", + "text": "threaded milestone", + "thread_ts": "ROOT.TS", + } + # No thread_ts when none is given (top-level post, not a broken key). + assert "thread_ts" not in captured[1] + assert captured[1] == {"channel": "C123", "text": "top-level milestone"} -- 2.50.1