mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-09-30 10:23:14 +00:00
* feat: editable plan mode — owner hand-edits the plan before approval (#1610, #80) 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 * 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. --------- Co-authored-by: seahaven-openswe[bot] <296972425+seahaven-openswe[bot]@users.noreply.github.com> Co-authored-by: Adam Moussa <adam@seahavenind.com>
This commit is contained in:
parent
b3b0274403
commit
a53a96d37b
4 changed files with 562 additions and 11 deletions
|
|
@ -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"
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<string | null>(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 (
|
||||
<div
|
||||
|
|
@ -167,11 +212,21 @@ export function PlanReview({ plan }: { plan: PlanData }) {
|
|||
<Button
|
||||
data-testid="copy-plan"
|
||||
variant="secondary"
|
||||
disabled={!plan.markdown.trim()}
|
||||
disabled={!markdown.trim()}
|
||||
onClick={() => void copyPlan()}
|
||||
>
|
||||
{copied ? "Copied!" : "Copy markdown"}
|
||||
</Button>
|
||||
{canEdit && (
|
||||
<Button
|
||||
data-testid="edit-plan"
|
||||
variant="secondary"
|
||||
disabled={busy !== null || decision !== null}
|
||||
onClick={startEditing}
|
||||
>
|
||||
Edit
|
||||
</Button>
|
||||
)}
|
||||
{plan.isOwner && (
|
||||
<Button
|
||||
data-testid="approve-plan"
|
||||
|
|
@ -205,8 +260,35 @@ export function PlanReview({ plan }: { plan: PlanData }) {
|
|||
data-testid="plan-document"
|
||||
data-color-scheme={resolvedTheme}
|
||||
>
|
||||
{plan.markdown.trim() ? (
|
||||
<Markdown content={plan.markdown} />
|
||||
{editing ? (
|
||||
<div className="flex h-full flex-col gap-3">
|
||||
<textarea
|
||||
data-testid="plan-edit-textarea"
|
||||
value={editDraft}
|
||||
onChange={(e) => setEditDraft(e.target.value)}
|
||||
className="min-h-0 flex-1 resize-none rounded-md border border-[var(--ui-border)] bg-[var(--ui-bg)] p-3 font-mono text-sm text-[var(--ui-text)] outline-none focus:border-[var(--ui-accent)]"
|
||||
placeholder="Write the plan in Markdown…"
|
||||
/>
|
||||
<div className="flex shrink-0 justify-end gap-2">
|
||||
<Button
|
||||
data-testid="plan-edit-cancel"
|
||||
variant="secondary"
|
||||
disabled={saving}
|
||||
onClick={cancelEditing}
|
||||
>
|
||||
Cancel
|
||||
</Button>
|
||||
<Button
|
||||
data-testid="plan-edit-save"
|
||||
disabled={saving || !editDraft.trim()}
|
||||
onClick={() => void saveEdit()}
|
||||
>
|
||||
{saving ? "Saving…" : "Save"}
|
||||
</Button>
|
||||
</div>
|
||||
</div>
|
||||
) : markdown.trim() ? (
|
||||
<Markdown content={markdown} />
|
||||
) : (
|
||||
<p className="text-sm text-[var(--ui-text-dim)]">
|
||||
The plan hasn't been written yet.
|
||||
|
|
|
|||
|
|
@ -113,6 +113,16 @@ export function deletePlanComment(
|
|||
)
|
||||
}
|
||||
|
||||
export function updatePlan(
|
||||
threadId: string,
|
||||
markdown: string
|
||||
): Promise<{ status: PlanStatus; markdown: string }> {
|
||||
return req(`/plan/${encodeURIComponent(threadId)}`, {
|
||||
method: "PUT",
|
||||
body: JSON.stringify({ markdown }),
|
||||
})
|
||||
}
|
||||
|
||||
export function approvePlan(threadId: string): Promise<{ status: string }> {
|
||||
return req(`/plan/${encodeURIComponent(threadId)}/approve`, {
|
||||
method: "POST",
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue