open-swe/tests/test_plan_review.py
seahaven-openswe[bot] f2ba89e515
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
2026-07-08 18:42:36 -04:00

619 lines
21 KiB
Python

from __future__ import annotations
from typing import Any
import pytest
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
assert dashboard_plan_url("abc-123") == "https://example.test/agents/abc-123/plan"
def test_dashboard_plan_url_none_without_thread() -> None:
from agent.utils.dashboard_links import dashboard_plan_url
assert dashboard_plan_url("") is None
def test_format_comments_numbers_and_skips_blank() -> None:
from agent.dashboard.plan_api import _format_comments
text = _format_comments(
[
{"author": "alice", "body": "add a docstring"},
{"author": "bob", "body": "looks good"},
{"author": "carol", "body": " "}, # blank → skipped
]
)
assert "1. alice: add a docstring" in text
assert "2. bob: looks good" in text
assert "carol" not in text
def test_format_comments_empty() -> None:
from agent.dashboard.plan_api import _format_comments
assert _format_comments([]) == ""
def test_plan_comment_helpers_exported() -> None:
from agent.dashboard import plan_store
assert plan_store.PLAN_COMMENTS_NAMESPACE == ["plan", "comments"]
assert callable(plan_store.add_plan_comment)
assert callable(plan_store.list_plan_comments)
assert callable(plan_store.delete_plan_comment)
assert callable(plan_store.clear_plan_comments)
def _fake_client(store: Any) -> Any:
return type("C", (), {"store": store})()
async def test_list_plan_comments_swallows_errors_by_default(
monkeypatch: pytest.MonkeyPatch,
) -> None:
from agent.dashboard import plan_store
class _Store:
async def search_items(self, *a: Any, **k: Any) -> Any:
raise RuntimeError("boom")
monkeypatch.setattr(plan_store, "_client", lambda: _fake_client(_Store()))
assert await plan_store.list_plan_comments("t") == []
async def test_list_plan_comments_raises_with_flag(monkeypatch: pytest.MonkeyPatch) -> None:
from agent.dashboard import plan_store
class _Store:
async def search_items(self, *a: Any, **k: Any) -> Any:
raise RuntimeError("boom")
monkeypatch.setattr(plan_store, "_client", lambda: _fake_client(_Store()))
with pytest.raises(RuntimeError):
await plan_store.list_plan_comments("t", raise_on_error=True)
async def test_clear_plan_comments_deletes_each(monkeypatch: pytest.MonkeyPatch) -> None:
from agent.dashboard import plan_store
deleted: list[str] = []
class _Store:
async def search_items(self, *a: Any, **k: Any) -> Any:
return {"items": [{"value": {"id": "a"}}, {"value": {"id": "b"}}]}
async def delete_item(self, _ns: Any, key: str) -> None:
deleted.append(key)
monkeypatch.setattr(plan_store, "_client", lambda: _fake_client(_Store()))
await plan_store.clear_plan_comments("t")
assert deleted == ["a", "b"]
async def test_save_plan_requires_run_context() -> None:
from agent.tools.save_plan import save_plan
# No LangGraph run context → no thread_id → graceful error, not a crash.
result = await save_plan("/workspace/plans/2026-07-08-test-plan.md")
assert result["success"] is False
assert "thread_id" in result["error"]
async def test_save_plan_rejects_empty_path() -> None:
from agent.tools.save_plan import save_plan
result = await save_plan(" ")
assert result["success"] is False
assert "empty" in result["error"]
async def test_save_plan_rejects_non_markdown_path() -> None:
from agent.tools.save_plan import save_plan
result = await save_plan("/workspace/plans/plan.txt")
assert result["success"] is False
assert "Markdown" in result["error"]
async def test_save_plan_rejects_markdown_outside_plans_dir() -> None:
from agent.tools.save_plan import save_plan
result = await save_plan("/workspace/plan.md")
assert result["success"] is False
assert "/workspace/plans" in result["error"]
async def test_save_plan_reads_markdown_file_from_sandbox(
monkeypatch: pytest.MonkeyPatch,
) -> None:
import importlib
save_plan_tool = importlib.import_module("agent.tools.save_plan")
saved: dict[str, Any] = {}
reads: list[tuple[str, int, int]] = []
class _Backend:
async def aread(self, file_path: str, offset: int = 0, limit: int = 2000) -> dict[str, Any]:
reads.append((file_path, offset, limit))
return {"file_data": {"encoding": "utf-8", "content": "# Plan\n\nDo it.\n"}}
async def fake_backend(thread_id: str) -> _Backend:
assert thread_id == "thread-1"
return _Backend()
async def fake_save_content(
thread_id: str, *, markdown: str, status: str, plan_file_path: str | None = None
) -> None:
saved.update(
thread_id=thread_id, markdown=markdown, status=status, plan_file_path=plan_file_path
)
monkeypatch.setattr(
save_plan_tool,
"get_config",
lambda: {"configurable": {"thread_id": "thread-1"}},
)
monkeypatch.setattr(save_plan_tool, "get_sandbox_backend", fake_backend)
monkeypatch.setattr(save_plan_tool, "save_plan_content", fake_save_content)
result = await save_plan_tool.save_plan("/workspace/plans/2026-07-08-test-plan.md")
assert result == {"success": True, "path": "/workspace/plans/2026-07-08-test-plan.md"}
assert reads == [
("/workspace/plans/2026-07-08-test-plan.md", 0, save_plan_tool._MAX_PLAN_LINES)
]
assert saved == {
"thread_id": "thread-1",
"markdown": "# Plan\n\nDo it.",
"status": "ready",
"plan_file_path": "/workspace/plans/2026-07-08-test-plan.md",
}
def test_plan_routes_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
assert "/dashboard/api/plan/{thread_id}/approve" in paths
assert "/dashboard/api/plan/{thread_id}/reject" in paths
assert "/dashboard/api/plan/{thread_id}/comments" in paths
assert "/dashboard/api/plan/{thread_id}/comments/{comment_id}" in paths
assert "/dashboard/api/plan/yjs/{thread_id}" not in paths
def test_save_plan_exported_and_wired() -> None:
from agent.tools import save_plan
assert callable(save_plan)
def test_plan_status_constants() -> None:
from agent.dashboard import plan_store
assert plan_store.PLAN_STATUS_READY == "ready"
assert plan_store.PLAN_STATUS_PLANNING == "planning"
assert plan_store.PLAN_STATUS_APPROVED == "approved"
assert plan_store.PLAN_STATUS_REVISING == "revising"
def test_plan_file_path_for_thread_uses_plans_dir_and_slug() -> None:
from agent.dashboard import plan_store
path = plan_store.plan_file_path_for_thread("Thread ABC/123")
assert path.startswith("/workspace/plans/")
assert path.endswith("-thread-abc-123.md")
def test_http_request_excluded_in_plan_mode() -> None:
from agent.server import PLAN_MODE_EXCLUDED_TOOLS
assert "http_request" in PLAN_MODE_EXCLUDED_TOOLS
def test_file_edit_tools_available_in_plan_mode_for_plan_file() -> None:
from agent.server import PLAN_MODE_EXCLUDED_TOOLS
assert "write_file" not in PLAN_MODE_EXCLUDED_TOOLS
assert "edit_file" not in PLAN_MODE_EXCLUDED_TOOLS
class _FakeReq:
def __init__(self, tools: list[Any], state: dict[str, Any]) -> None:
self.tools = tools
self.state = state
def override(self, **kw: Any) -> _FakeReq:
return _FakeReq(kw.get("tools", self.tools), self.state)
def _names(req: _FakeReq) -> set[str]:
return {t["name"] for t in req.tools}
def test_plan_mode_middleware_initial_always_filters() -> None:
from agent.middleware import PlanModeMiddleware
mw = PlanModeMiddleware(excluded=frozenset({"write_file"}), initial=True)
req = _FakeReq([{"name": "read_file"}, {"name": "write_file"}], {})
assert _names(mw._filter(req)) == {"read_file"}
def test_plan_mode_middleware_self_activation_via_state() -> None:
from agent.middleware import PlanModeMiddleware
mw = PlanModeMiddleware(excluded=frozenset({"write_file"}), initial=False)
# Plan mode not yet active: nothing filtered.
off = _FakeReq([{"name": "read_file"}, {"name": "write_file"}], {})
assert _names(mw._filter(off)) == {"read_file", "write_file"}
# After enter_plan_mode sets state: the next request is filtered.
on = _FakeReq([{"name": "read_file"}, {"name": "write_file"}], {"plan_mode": True})
assert _names(mw._filter(on)) == {"read_file"}
def test_plan_approved_slack_text_mentions_comments_actor_and_start() -> None:
from agent.dashboard.plan_api import _plan_approved_slack_text
text = _plan_approved_slack_text(3, "Alice")
assert text == "Plan approved with 3 comments by Alice\nbeginning implementation"
async def test_approve_plan_posts_slack_approval_notice(monkeypatch: pytest.MonkeyPatch) -> None:
from agent.dashboard import plan_api
metadata = {
"github_login": "alice",
"source_context": {"slack_thread": {"channel_id": "C1", "thread_ts": "1700000000.0001"}},
}
posted: 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": "# 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 = None
) -> None:
posted["status"] = {"status": status, "plan_mode": plan_mode}
async def fake_dispatch_followup(
thread_id: str, md: dict, text: str, *, plan_mode: bool
) -> None:
posted["dispatch"] = {"text": text, "plan_mode": plan_mode}
async def fake_post_slack_thread_reply(channel_id: str, thread_ts: str, text: str) -> bool:
posted["slack"] = {"channel_id": channel_id, "thread_ts": thread_ts, "text": text}
return True
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)
monkeypatch.setattr(plan_api, "post_slack_thread_reply", fake_post_slack_thread_reply)
result = await plan_api.approve_plan("tid", session={"sub": "u1", "name": "Alice"})
assert result == {"status": plan_api.PLAN_STATUS_APPROVED}
assert posted["dispatch"]["plan_mode"] is False
assert posted["slack"] == {
"channel_id": "C1",
"thread_ts": "1700000000.0001",
"text": "Plan approved with 1 comments by Alice\nbeginning implementation",
}
async def test_set_plan_status_preserves_plan_file_path(monkeypatch: pytest.MonkeyPatch) -> None:
from agent.dashboard import plan_store
existing = {
"markdown": "# Plan",
"status": "ready",
"plan_file_path": "/workspace/plans/foo.md",
}
saved: dict[str, Any] = {}
class _Store:
async def get_item(self, *a: Any, **k: Any) -> Any:
return {"value": existing}
async def put_item(self, namespace: Any, key: str, value: Any, *a: Any, **k: Any) -> None:
saved.update(value)
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, "_merge_thread_metadata", fake_merge)
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