diff --git a/agent/dashboard/plan_api.py b/agent/dashboard/plan_api.py index bde2a5dd..c5a9c2e4 100644 --- a/agent/dashboard/plan_api.py +++ b/agent/dashboard/plan_api.py @@ -26,12 +26,16 @@ from ..utils.slack import post_slack_thread_reply from .oauth import require_same_origin_for_mutations, require_session from .plan_store import ( PLAN_STATUS_APPROVED, + PLAN_STATUS_CANCELLED, + PLAN_STATUS_READY, PLAN_STATUS_REVISING, add_plan_comment, delete_plan_comment, get_plan_content, list_plan_comments, + save_plan_content, set_plan_status, + write_plan_to_sandbox, ) from .thread_api import ( _repo_config_from_metadata, @@ -54,6 +58,10 @@ class CommentBody(BaseModel): body: str +class PlanUpdate(BaseModel): + markdown: str + + async def _thread_metadata(thread_id: str) -> dict[str, Any]: client = get_client() try: @@ -88,6 +96,32 @@ async def get_plan(thread_id: str, session: dict[str, Any] = _SESSION_DEP) -> di } +@plan_router.put("/{thread_id}") +async def update_plan( + thread_id: str, body: PlanUpdate, session: dict[str, Any] = _SESSION_DEP +) -> dict[str, Any]: + """Owner-only manual edit of the plan markdown. + + Re-publishes the edited plan as ``ready`` (and mirrors it into the sandbox + ``plan.md``) while preserving reviewer comments, so the owner can refine the + plan before approving it.""" + metadata = await _thread_metadata(thread_id) + if not _user_owns_thread(metadata, session["sub"], session.get("email")): + raise HTTPException(403, "only the plan owner can edit the plan") + markdown = body.markdown.strip() + if not markdown: + raise HTTPException(422, "plan markdown cannot be empty") + content = await get_plan_content(thread_id) or {} + status = content.get("status") or metadata.get("plan_status") or "planning" + if status in (PLAN_STATUS_APPROVED, PLAN_STATUS_CANCELLED): + raise HTTPException(409, f"cannot edit a {status} plan") + await save_plan_content( + thread_id, markdown=markdown, status=PLAN_STATUS_READY, clear_comments=False + ) + await write_plan_to_sandbox(thread_id, markdown) + return {"status": PLAN_STATUS_READY, "markdown": markdown} + + @plan_router.get("/{thread_id}/comments") async def get_plan_comments( thread_id: str, session: dict[str, Any] = _SESSION_DEP @@ -138,21 +172,40 @@ async def approve_plan(thread_id: str, session: dict[str, Any] = _SESSION_DEP) - metadata = await _thread_metadata(thread_id) if not _user_owns_thread(metadata, session["sub"], session.get("email")): raise HTTPException(403, "only the plan owner can approve") - # Read comments BEFORE mutating state: a store failure here aborts the - # decision (500) rather than dispatching the run without the feedback. + # Read the published plan + comments BEFORE mutating state: a store failure + # here aborts the decision (500) rather than dispatching without them. The + # 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 feedback: + if plan_markdown: + text = ( + "The plan has been approved. Implement it now as described below:\n\n" + plan_markdown + ) + if feedback: + text += f"\n\nAlso take this reviewer feedback into account:\n\n{feedback}" + elif feedback: text = ( "The plan has been approved. Implement it now, taking this reviewer " f"feedback into account:\n\n{feedback}" ) 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} @@ -187,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 74804d38..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 @@ -285,12 +289,19 @@ async def test_approve_plan_posts_slack_approval_notice(monkeypatch: pytest.Monk def fake_user_owns_thread(md: dict, sub: str, email: str | None) -> bool: return True + 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 [{"author": "bob", "body": "tweak this"}] - async def fake_set_plan_status(thread_id: str, status: str, *, plan_mode: bool) -> None: + async def fake_set_plan_status( + thread_id: str, status: str, *, plan_mode: bool | None = None + ) -> None: posted["status"] = {"status": status, "plan_mode": plan_mode} async def fake_dispatch_followup( @@ -304,6 +315,7 @@ async def test_approve_plan_posts_slack_approval_notice(monkeypatch: pytest.Monk monkeypatch.setattr(plan_api, "_thread_metadata", fake_thread_metadata) monkeypatch.setattr(plan_api, "_user_owns_thread", fake_user_owns_thread) + 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", fake_set_plan_status) monkeypatch.setattr(plan_api, "_dispatch_followup", fake_dispatch_followup) @@ -320,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 @@ -346,3 +483,266 @@ async def test_set_plan_status_preserves_plan_file_path(monkeypatch: pytest.Monk await plan_store.set_plan_status("t", plan_store.PLAN_STATUS_REVISING, plan_mode=True) assert saved["plan_file_path"] == "/workspace/plans/foo.md" assert saved["status"] == plan_store.PLAN_STATUS_REVISING + + +# --- manual plan editing ------------------------------------------------- + + +async def test_save_plan_content_clear_comments_flag(monkeypatch: pytest.MonkeyPatch) -> None: + from agent.dashboard import plan_store + + cleared: list[str] = [] + + class _Store: + async def put_item(self, *a: Any, **k: Any) -> None: + return None + + async def fake_clear(thread_id: str) -> None: + cleared.append(thread_id) + + async def fake_merge(thread_id: str, metadata: dict[str, Any]) -> None: + return None + + monkeypatch.setattr(plan_store, "_client", lambda: _fake_client(_Store())) + monkeypatch.setattr(plan_store, "clear_plan_comments", fake_clear) + monkeypatch.setattr(plan_store, "_merge_thread_metadata", fake_merge) + + # A manual edit keeps reviewer comments; the agent's republish clears them. + await plan_store.save_plan_content("t", markdown="x", clear_comments=False) + assert cleared == [] + await plan_store.save_plan_content("t", markdown="x") + assert cleared == ["t"] + + +def _patch_update_plan_deps( + monkeypatch: pytest.MonkeyPatch, + *, + metadata: dict[str, Any], + owner: bool, + content: dict[str, Any], + saved: dict[str, Any], + sandbox: dict[str, Any], +) -> None: + from agent.dashboard import plan_api + + async def fake_meta(thread_id: str) -> dict[str, Any]: + return metadata + + async def fake_get_content(thread_id: str) -> dict[str, Any]: + return content + + async def fake_save( + thread_id: str, *, markdown: str, status: str, clear_comments: bool = True + ) -> None: + saved.update(markdown=markdown, status=status, clear_comments=clear_comments) + + async def fake_write(thread_id: str, c: str) -> str: + sandbox["content"] = c + return "plan.md" + + monkeypatch.setattr(plan_api, "_thread_metadata", fake_meta) + monkeypatch.setattr(plan_api, "_user_owns_thread", lambda *a, **k: owner) + monkeypatch.setattr(plan_api, "get_plan_content", fake_get_content) + monkeypatch.setattr(plan_api, "save_plan_content", fake_save) + monkeypatch.setattr(plan_api, "write_plan_to_sandbox", fake_write) + + +async def test_update_plan_owner_saves_and_mirrors_sandbox( + monkeypatch: pytest.MonkeyPatch, +) -> None: + from agent.dashboard import plan_api + + saved: dict[str, Any] = {} + sandbox: dict[str, Any] = {} + _patch_update_plan_deps( + monkeypatch, + metadata={"plan_status": "ready"}, + owner=True, + content={"markdown": "old", "status": "ready"}, + saved=saved, + sandbox=sandbox, + ) + + result = await plan_api.update_plan( + "tid", + plan_api.PlanUpdate(markdown=" # Edited plan "), + session={"sub": "u1"}, + ) + + assert result == {"status": "ready", "markdown": "# Edited plan"} + assert saved["markdown"] == "# Edited plan" + assert saved["status"] == "ready" + assert saved["clear_comments"] is False + assert sandbox["content"] == "# Edited plan" + + +async def test_update_plan_rejects_non_owner(monkeypatch: pytest.MonkeyPatch) -> None: + from fastapi import HTTPException + + from agent.dashboard import plan_api + + _patch_update_plan_deps( + monkeypatch, + metadata={}, + owner=False, + content={}, + saved={}, + sandbox={}, + ) + with pytest.raises(HTTPException, match="only the plan owner"): + await plan_api.update_plan("tid", plan_api.PlanUpdate(markdown="x"), session={"sub": "u1"}) + + +async def test_update_plan_rejects_empty_markdown(monkeypatch: pytest.MonkeyPatch) -> None: + from fastapi import HTTPException + + from agent.dashboard import plan_api + + _patch_update_plan_deps( + monkeypatch, + metadata={}, + owner=True, + content={}, + saved={}, + sandbox={}, + ) + with pytest.raises(HTTPException, match="plan markdown cannot be empty"): + await plan_api.update_plan( + "tid", plan_api.PlanUpdate(markdown=" "), session={"sub": "u1"} + ) + + +async def test_update_plan_rejects_approved_plan(monkeypatch: pytest.MonkeyPatch) -> None: + from fastapi import HTTPException + + from agent.dashboard import plan_api + + _patch_update_plan_deps( + monkeypatch, + metadata={}, + owner=True, + content={"status": "approved"}, + saved={}, + sandbox={}, + ) + with pytest.raises(HTTPException, match="cannot edit a approved plan"): + await plan_api.update_plan("tid", plan_api.PlanUpdate(markdown="x"), session={"sub": "u1"}) + + +async def test_update_plan_rejects_cancelled_plan(monkeypatch: pytest.MonkeyPatch) -> None: + from fastapi import HTTPException + + from agent.dashboard import plan_api + + _patch_update_plan_deps( + monkeypatch, + metadata={}, + owner=True, + content={"status": "cancelled"}, + saved={}, + sandbox={}, + ) + with pytest.raises(HTTPException, match="cannot edit a cancelled plan"): + await plan_api.update_plan("tid", plan_api.PlanUpdate(markdown="x"), session={"sub": "u1"}) + + +async def test_approve_plan_reads_published_content_strictly( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Approve reads plan + comments with raise_on_error; failure aborts.""" + from agent.dashboard import plan_api + + metadata = {"github_login": "alice"} + + async def fake_thread_metadata(thread_id: str) -> dict[str, Any]: + return metadata + + def fake_user_owns_thread(md: dict, sub: str, email: str | None) -> bool: + return True + + async def fake_get_plan_content( + thread_id: str, *, raise_on_error: bool = False + ) -> dict[str, Any] | None: + if raise_on_error: + raise RuntimeError("store is down") + return None + + async def fake_list_plan_comments( + thread_id: str, *, raise_on_error: bool = False + ) -> list[dict]: + return [] + + monkeypatch.setattr(plan_api, "_thread_metadata", fake_thread_metadata) + monkeypatch.setattr(plan_api, "_user_owns_thread", fake_user_owns_thread) + monkeypatch.setattr(plan_api, "get_plan_content", fake_get_plan_content) + monkeypatch.setattr(plan_api, "list_plan_comments", fake_list_plan_comments) + + with pytest.raises(RuntimeError, match="store is down"): + await plan_api.approve_plan("tid", session={"sub": "u1", "name": "Alice"}) + + +async def test_approve_plan_hands_edited_plan_to_agent( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """When the owner edited the plan, the edited markdown is dispatched.""" + from agent.dashboard import plan_api + + metadata = {"github_login": "alice"} + dispatched: dict[str, Any] = {} + + async def fake_thread_metadata(thread_id: str) -> dict[str, Any]: + return metadata + + def fake_user_owns_thread(md: dict, sub: str, email: str | None) -> bool: + return True + + async def fake_get_plan_content( + thread_id: str, *, raise_on_error: bool = False + ) -> dict[str, Any] | None: + return {"markdown": "# Owner edits", "status": "ready"} + + async def fake_list_plan_comments( + thread_id: str, *, raise_on_error: bool = False + ) -> list[dict]: + return [{"author": "bob", "body": "looks good"}] + + async def fake_set_plan_status( + thread_id: str, status: str, *, plan_mode: bool | None = None + ) -> None: + dispatched["status_set"] = status + + async def fake_dispatch_followup( + thread_id: str, md: dict, text: str, *, plan_mode: bool + ) -> None: + dispatched["text"] = text + dispatched["plan_mode"] = plan_mode + + monkeypatch.setattr(plan_api, "_thread_metadata", fake_thread_metadata) + monkeypatch.setattr(plan_api, "_user_owns_thread", fake_user_owns_thread) + 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", fake_set_plan_status) + + async def fake_maybe_post_slack(*a: Any, **k: Any) -> None: + return None + + monkeypatch.setattr(plan_api, "_dispatch_followup", fake_dispatch_followup) + monkeypatch.setattr(plan_api, "_maybe_post_plan_approved_to_slack", fake_maybe_post_slack) + + await plan_api.approve_plan("tid", session={"sub": "u1", "name": "Alice"}) + + assert "# Owner edits" in dispatched["text"] + assert "looks good" in dispatched["text"] + + +def test_plan_update_route_registered() -> None: + from agent.webapp import app + + paths = set() + for route in app.routes: + if hasattr(route, "path"): + paths.add(route.path) + included = getattr(route, "original_router", None) + if included is not None: + paths.update(r.path for r in included.routes if hasattr(r, "path")) + assert "/dashboard/api/plan/{thread_id}" in paths diff --git a/ui/src/components/agents/PlanReview.tsx b/ui/src/components/agents/PlanReview.tsx index c8ec2815..95b60923 100644 --- a/ui/src/components/agents/PlanReview.tsx +++ b/ui/src/components/agents/PlanReview.tsx @@ -8,6 +8,7 @@ import { deletePlanComment, getPlanComments, rejectPlan, + updatePlan, } from "@/lib/plan" import { Button } from "@/components/ui/button" import { Markdown } from "@/components/agents/ported" @@ -57,6 +58,50 @@ export function PlanReview({ plan }: { plan: PlanData }) { const [busy, setBusy] = useState<"approve" | "reject" | null>(null) const [error, setError] = useState(null) const [copied, setCopied] = useState(false) + // Locally track the displayed markdown so a manual edit shows immediately; the + // route's query stops polling once a plan exists, so the prop won't refetch. + const [markdown, setMarkdown] = useState(plan.markdown) + const [editing, setEditing] = useState(false) + const [editDraft, setEditDraft] = useState(plan.markdown) + const [saving, setSaving] = useState(false) + + // Reflect external plan updates (e.g. an agent revision) while not editing. + useEffect(() => { + if (!editing) setMarkdown(plan.markdown) + }, [plan.markdown, editing]) + + const canEdit = + plan.isOwner && plan.status !== "approved" && plan.status !== "cancelled" + + const startEditing = useCallback(() => { + setEditDraft(markdown) + setEditing(true) + setError(null) + }, [markdown]) + + const cancelEditing = useCallback(() => { + setEditing(false) + setError(null) + }, []) + + const saveEdit = useCallback(async () => { + const next = editDraft.trim() + if (!next) { + setError("The plan cannot be empty.") + return + } + setSaving(true) + setError(null) + try { + const result = await updatePlan(plan.threadId, next) + setMarkdown(result.markdown) + setEditing(false) + } catch (e) { + setError((e as Error).message) + } finally { + setSaving(false) + } + }, [editDraft, plan.threadId]) // Poll so reviewers see each other's comments without a realtime transport. useEffect(() => { @@ -131,13 +176,13 @@ export function PlanReview({ plan }: { plan: PlanData }) { const copyPlan = useCallback(async () => { setError(null) - if (await copyToClipboard(plan.markdown)) { + if (await copyToClipboard(markdown)) { setCopied(true) window.setTimeout(() => setCopied(false), 1500) } else { setError("Couldn't copy the plan to the clipboard.") } - }, [plan.markdown]) + }, [markdown]) return (
void copyPlan()} > {copied ? "Copied!" : "Copy markdown"} + {canEdit && ( + + )} {plan.isOwner && (