diff --git a/agent/dashboard/plan_api.py b/agent/dashboard/plan_api.py index bde2a5dd..866e1500 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,12 +172,22 @@ 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) + 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}" diff --git a/tests/test_plan_review.py b/tests/test_plan_review.py index 74804d38..647cf6a8 100644 --- a/tests/test_plan_review.py +++ b/tests/test_plan_review.py @@ -285,12 +285,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 +311,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) @@ -346,3 +354,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 && (