From f2ba89e515d71ee0318ec7335486fb7a01cfcfd0 Mon Sep 17 00:00:00 2001 From: "seahaven-openswe[bot]" <296972425+seahaven-openswe[bot]@users.noreply.github.com> Date: Wed, 8 Jul 2026 21:19:23 +0000 Subject: [PATCH] =?UTF-8?q?feat:=20editable=20plan=20mode=20=E2=80=94=20ow?= =?UTF-8?q?ner=20hand-edits=20the=20plan=20before=20approval=20(#1610,=20#?= =?UTF-8?q?80)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a PUT /dashboard/api/plan/{thread_id} endpoint and PlanReview UI edit mode so the thread owner can refine the published plan markdown by hand. The edited markdown is re-published as "ready" (preserving reviewer comments), mirrored into the sandbox plan.md, and handed to the agent as the source of truth on approve. Approve now reads the published plan content strictly (raise_on_error=True) so a transient store failure aborts instead of silently dropping an edited plan, matching the comment-read contract. The banner-overlap fix (collapsed git-panel clearing the "Review plan" link) was already ported in #128; this picks up the remaining edit-mode pieces. Refs #80 --- agent/dashboard/plan_api.py | 50 ++++- tests/test_plan_review.py | 273 +++++++++++++++++++++++- ui/src/components/agents/PlanReview.tsx | 92 +++++++- ui/src/lib/plan.ts | 10 + 4 files changed, 416 insertions(+), 9 deletions(-) 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 && (