diff --git a/agent/dashboard/plan_api.py b/agent/dashboard/plan_api.py index 866e1500..c5a9c2e4 100644 --- a/agent/dashboard/plan_api.py +++ b/agent/dashboard/plan_api.py @@ -177,10 +177,14 @@ async def approve_plan(thread_id: str, session: dict[str, Any] = _SESSION_DEP) - # published markdown may have been edited by the owner, so it is the # source of truth handed to the agent (not its own stale history). content = await get_plan_content(thread_id, raise_on_error=True) + status = (content or {}).get("status") or metadata.get("plan_status") or "planning" + if status == PLAN_STATUS_APPROVED: + # Idempotent: a repeat approve (double-click / retry) must not dispatch a + # second implementation run or post a duplicate Slack notice. + raise HTTPException(409, "plan is already approved") plan_markdown = (content or {}).get("markdown", "") comments = await list_plan_comments(thread_id, raise_on_error=True) feedback = _format_comments(comments) - await set_plan_status(thread_id, PLAN_STATUS_APPROVED, plan_mode=False) if plan_markdown: text = ( "The plan has been approved. Implement it now as described below:\n\n" + plan_markdown @@ -194,9 +198,14 @@ async def approve_plan(thread_id: str, session: dict[str, Any] = _SESSION_DEP) - ) else: text = "The plan has been approved. Implement it now as described in the plan." + # Dispatch the implementation run BEFORE persisting APPROVED: if the dispatch + # fails, the plan stays re-approvable rather than stuck approved-but-undispatched. await _dispatch_followup(thread_id, metadata, text, plan_mode=False) + await set_plan_status(thread_id, PLAN_STATUS_APPROVED, plan_mode=False) await _maybe_post_plan_approved_to_slack( - metadata, comment_count=len(comments), actor=_approval_actor_name(session) + metadata, + comment_count=_substantive_comment_count(comments), + actor=_approval_actor_name(session), ) return {"status": PLAN_STATUS_APPROVED} @@ -231,6 +240,12 @@ def _format_comments(comments: list[dict[str, Any]]) -> str: return "\n".join(lines) +def _substantive_comment_count(comments: list[dict[str, Any]]) -> int: + """Count only non-empty comments — the same ones _format_comments feeds the + agent — so the Slack approval notice doesn't overstate the comment count.""" + return sum(1 for c in comments if str(c.get("body", "")).strip()) + + def _approval_actor_name(session: dict[str, Any]) -> str: return str(session.get("name") or session.get("sub") or "User").strip() or "User" diff --git a/tests/test_plan_review.py b/tests/test_plan_review.py index 647cf6a8..98d73d06 100644 --- a/tests/test_plan_review.py +++ b/tests/test_plan_review.py @@ -5,6 +5,10 @@ from typing import Any import pytest +async def _async_noop(*args: Any, **kwargs: Any) -> None: + return None + + def test_dashboard_plan_url_uses_plan_path(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv("DASHBOARD_BASE_URL", "https://example.test") from agent.utils.dashboard_links import dashboard_plan_url @@ -328,6 +332,131 @@ async def test_approve_plan_posts_slack_approval_notice(monkeypatch: pytest.Monk } +async def test_approve_plan_is_idempotent_when_already_approved( + monkeypatch: pytest.MonkeyPatch, +) -> None: + from fastapi import HTTPException + + from agent.dashboard import plan_api + + calls: list[str] = [] + + async def fake_thread_metadata(thread_id: str) -> dict[str, Any]: + return {"github_login": "alice"} + + async def fake_get_plan_content( + thread_id: str, *, raise_on_error: bool = False + ) -> dict[str, Any] | None: + return {"markdown": "# Plan", "status": plan_api.PLAN_STATUS_APPROVED} + + async def fake_dispatch_followup(*a: Any, **k: Any) -> None: + calls.append("dispatch") + + async def fake_set_plan_status(*a: Any, **k: Any) -> None: + calls.append("set_status") + + monkeypatch.setattr(plan_api, "_thread_metadata", fake_thread_metadata) + monkeypatch.setattr(plan_api, "_user_owns_thread", lambda *a, **k: True) + monkeypatch.setattr(plan_api, "get_plan_content", fake_get_plan_content) + monkeypatch.setattr(plan_api, "_dispatch_followup", fake_dispatch_followup) + monkeypatch.setattr(plan_api, "set_plan_status", fake_set_plan_status) + + with pytest.raises(HTTPException) as exc: + await plan_api.approve_plan("tid", session={"sub": "u1", "name": "Alice"}) + + assert exc.value.status_code == 409 + # A repeat approve must neither dispatch a second run nor re-persist state. + assert calls == [] + + +async def test_approve_plan_slack_notice_counts_only_substantive_comments( + monkeypatch: pytest.MonkeyPatch, +) -> None: + from agent.dashboard import plan_api + + posted: dict[str, Any] = {} + + async def fake_thread_metadata(thread_id: str) -> dict[str, Any]: + return { + "github_login": "alice", + "source_context": {"slack_thread": {"channel_id": "C1", "thread_ts": "1700000000.1"}}, + } + + async def fake_get_plan_content( + thread_id: str, *, raise_on_error: bool = False + ) -> dict[str, Any] | None: + return {"markdown": "# Plan", "status": "ready"} + + async def fake_list_plan_comments( + thread_id: str, *, raise_on_error: bool = False + ) -> list[dict]: + # Two real comments plus an empty and a whitespace-only one that + # _format_comments filters out — the notice must count only the two. + return [ + {"author": "bob", "body": "tweak this"}, + {"author": "sys", "body": ""}, + {"author": "sys", "body": " "}, + {"author": "cara", "body": "and this"}, + ] + + async def fake_post_slack_thread_reply(channel_id: str, thread_ts: str, text: str) -> bool: + posted["text"] = text + return True + + monkeypatch.setattr(plan_api, "_thread_metadata", fake_thread_metadata) + monkeypatch.setattr(plan_api, "_user_owns_thread", lambda *a, **k: True) + monkeypatch.setattr(plan_api, "get_plan_content", fake_get_plan_content) + monkeypatch.setattr(plan_api, "list_plan_comments", fake_list_plan_comments) + monkeypatch.setattr(plan_api, "set_plan_status", _async_noop) + monkeypatch.setattr(plan_api, "_dispatch_followup", _async_noop) + monkeypatch.setattr(plan_api, "post_slack_thread_reply", fake_post_slack_thread_reply) + + await plan_api.approve_plan("tid", session={"sub": "u1", "name": "Alice"}) + + assert posted["text"] == "Plan approved with 2 comments by Alice\nbeginning implementation" + + +async def test_approve_plan_dispatches_before_persisting_approved( + monkeypatch: pytest.MonkeyPatch, +) -> None: + from agent.dashboard import plan_api + + order: list[str] = [] + + async def fake_thread_metadata(thread_id: str) -> dict[str, Any]: + return {"github_login": "alice"} + + async def fake_get_plan_content( + thread_id: str, *, raise_on_error: bool = False + ) -> dict[str, Any] | None: + return {"markdown": "# Plan", "status": "ready"} + + async def fake_list_plan_comments( + thread_id: str, *, raise_on_error: bool = False + ) -> list[dict]: + return [] + + async def fake_dispatch_followup(*a: Any, **k: Any) -> None: + order.append("dispatch") + + async def fake_set_plan_status(*a: Any, **k: Any) -> None: + order.append("set_status") + + monkeypatch.setattr(plan_api, "_thread_metadata", fake_thread_metadata) + monkeypatch.setattr(plan_api, "_user_owns_thread", lambda *a, **k: True) + monkeypatch.setattr(plan_api, "get_plan_content", fake_get_plan_content) + monkeypatch.setattr(plan_api, "list_plan_comments", fake_list_plan_comments) + monkeypatch.setattr(plan_api, "_dispatch_followup", fake_dispatch_followup) + monkeypatch.setattr(plan_api, "set_plan_status", fake_set_plan_status) + monkeypatch.setattr(plan_api, "_maybe_post_plan_approved_to_slack", _async_noop) + + await plan_api.approve_plan("tid", session={"sub": "u1", "name": "Alice"}) + + # Dispatch must precede the APPROVED write so a dispatch failure leaves the + # plan re-approvable rather than stuck approved-but-undispatched. + assert order == ["dispatch", "set_status"] + + async def test_set_plan_status_preserves_plan_file_path(monkeypatch: pytest.MonkeyPatch) -> None: from agent.dashboard import plan_store