fix(plan): make approve_plan idempotent, dispatch before persisting, fix comment count

SH-128-03: approve_plan set status APPROVED before dispatching the follow-up
run and had no already-approved guard, so a failed dispatch left the plan stuck
approved-but-undispatched and a double-submit double-dispatched + double-posted
the Slack notice. And the Slack notice counted len(comments) including empty
comments _format_comments filters out.

- Return 409 when the plan is already approved (idempotent double-click/retry).
- Dispatch the implementation run BEFORE persisting APPROVED so a dispatch
  failure leaves the plan re-approvable. _dispatch_followup passes plan_mode
  explicitly, so the run is unaffected by the reorder.
- Count only non-empty comments in the Slack approval notice.

Fixed here (not on #128) because #128's approve_plan is rewritten on this
branch; #129 inherits it. Adds tests for the 409, the filtered count, and the
dispatch-before-status ordering.

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"
 

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