mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-07 16:19:09 +00:00
feat(plan): publish plans from sandbox files
Re-land upstream #1635 onto the modular structure, completing the partially-ported change so dev is internally consistent. save_plan now takes a plan_file_path, reads the agent-authored Markdown file from /workspace/plans/ (validating extension/location/UTF-8/size) and publishes it, instead of taking a plan_markdown string. Removes write_file/edit_file from PLAN_MODE_EXCLUDED_TOOLS so the agent can author the plan file, updates enter_plan_mode/reject_plan guidance and the e2e fake LLM. Skips the #1610-only update_plan hunk (not on dev). Refs #80, upstream #1635
This commit is contained in:
parent
80c9545f24
commit
3b477e7798
7 changed files with 199 additions and 53 deletions
|
|
@ -166,7 +166,8 @@ async def reject_plan(thread_id: str, session: dict[str, Any] = _SESSION_DEP) ->
|
||||||
await set_plan_status(thread_id, PLAN_STATUS_REVISING, plan_mode=True)
|
await set_plan_status(thread_id, PLAN_STATUS_REVISING, plan_mode=True)
|
||||||
text = (
|
text = (
|
||||||
"The plan needs changes before implementation. Address this reviewer "
|
"The plan needs changes before implementation. Address this reviewer "
|
||||||
"feedback and publish an updated plan with the save_plan tool:\n\n"
|
"feedback in the existing Markdown file under /workspace/plans/, then "
|
||||||
|
"publish an updated plan with the save_plan tool:\n\n"
|
||||||
f"{feedback or '(no specific comments were left)'}"
|
f"{feedback or '(no specific comments were left)'}"
|
||||||
)
|
)
|
||||||
await _dispatch_followup(thread_id, metadata, text, plan_mode=True)
|
await _dispatch_followup(thread_id, metadata, text, plan_mode=True)
|
||||||
|
|
|
||||||
|
|
@ -619,8 +619,6 @@ MODEL_CALL_RECURSION_LIMIT = 5_000
|
||||||
# intent.
|
# intent.
|
||||||
PLAN_MODE_EXCLUDED_TOOLS: frozenset[str] = frozenset(
|
PLAN_MODE_EXCLUDED_TOOLS: frozenset[str] = frozenset(
|
||||||
{
|
{
|
||||||
"write_file",
|
|
||||||
"edit_file",
|
|
||||||
"task",
|
"task",
|
||||||
"http_request",
|
"http_request",
|
||||||
"open_pull_request",
|
"open_pull_request",
|
||||||
|
|
|
||||||
|
|
@ -15,10 +15,11 @@ from ..dashboard.plan_store import PLAN_STATUS_PLANNING, set_plan_status
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
_ENTERED_MESSAGE = (
|
_ENTERED_MESSAGE = (
|
||||||
"Plan mode is active. Stay read-only: research the codebase, then record your "
|
"Plan mode is active. Stay read-only for the target repo: research the codebase, "
|
||||||
"implementation plan with the `save_plan` tool (it publishes the plan to the "
|
"create or edit a dated, concise plan file under `/workspace/plans/`, then publish "
|
||||||
"review page) and share the plan-review link in the source channel. Do not edit "
|
"it with the `save_plan` tool and share the plan-review link in the source channel. "
|
||||||
"files, commit, push, or open a PR — wait for the user to approve the plan."
|
"Do not edit repo files, commit, push, or open a PR — wait for the user to approve "
|
||||||
|
"the plan."
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -31,11 +32,12 @@ async def enter_plan_mode(tool_call_id: Annotated[str, InjectedToolCallId]) -> C
|
||||||
NOT triggered by the word "plan" appearing in the request; use your
|
NOT triggered by the word "plan" appearing in the request; use your
|
||||||
judgment about whether planning is genuinely warranted.
|
judgment about whether planning is genuinely warranted.
|
||||||
|
|
||||||
Once activated, stay read-only: research the codebase, then record your plan
|
Once activated, stay read-only for the target repo: research the codebase,
|
||||||
with the ``save_plan`` tool (it publishes the plan to the review page) and
|
create or edit a dated, concise Markdown plan outside any repo (for example,
|
||||||
share the plan-review link with the user. Do not edit files, commit, push,
|
``/workspace/plans/YYYY-MM-DD-short-task-slug.md``), then publish it with
|
||||||
or open a PR — the user reviews the plan and approves it before you
|
the ``save_plan`` tool and share the plan-review link with the user. Do not
|
||||||
implement.
|
edit repo files, commit, push, or open a PR — the user reviews the plan and
|
||||||
|
approves it before you implement.
|
||||||
"""
|
"""
|
||||||
thread_id = _thread_id_from_config()
|
thread_id = _thread_id_from_config()
|
||||||
if thread_id:
|
if thread_id:
|
||||||
|
|
|
||||||
|
|
@ -1,47 +1,59 @@
|
||||||
"""Tool: ``save_plan``. Record the implementation plan for review.
|
"""Tool: ``save_plan``. Publish the sandbox plan file for review.
|
||||||
|
|
||||||
Writes the plan as a real ``plan.md`` file in the sandbox (the artifact the
|
Reads the Markdown plan file the agent created in the sandbox and publishes it to
|
||||||
agent produces and can re-read) and publishes it to the plan-review page, where
|
the plan-review page, where the user and collaborators read it, comment inline,
|
||||||
the user and collaborators read it, comment inline, and approve or request
|
and approve or request changes. Available in plan mode (it does not modify the
|
||||||
changes. Available in plan mode (it does not modify the repository under review).
|
repository under review).
|
||||||
"""
|
"""
|
||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import logging
|
import logging
|
||||||
|
from collections.abc import Mapping
|
||||||
from typing import Any
|
from typing import Any
|
||||||
|
|
||||||
from langgraph.config import get_config
|
from langgraph.config import get_config
|
||||||
|
|
||||||
from ..dashboard.plan_store import PLAN_STATUS_READY, save_plan_content
|
from ..dashboard.plan_store import PLAN_FILE_DIRECTORY, PLAN_STATUS_READY, save_plan_content
|
||||||
|
from ..utils.sandbox_state import get_sandbox_backend
|
||||||
|
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
PLAN_FILE_PATH = "plan.md"
|
_MAX_PLAN_LINES = 20_000
|
||||||
|
_MARKDOWN_EXTENSIONS = (".md", ".markdown")
|
||||||
|
|
||||||
|
|
||||||
async def save_plan(plan_markdown: str) -> dict[str, Any]:
|
async def save_plan(plan_file_path: str) -> dict[str, Any]:
|
||||||
"""Write your implementation plan as a markdown file and publish it for review.
|
"""Publish a Markdown plan file from the sandbox for review.
|
||||||
|
|
||||||
Use this in plan mode once your plan is ready. The plan is saved as
|
Use this in plan mode once your plan is ready. First create a Markdown file
|
||||||
``plan.md`` in the sandbox and published to the plan-review page linked in
|
under ``/workspace/plans/`` using a dated, descriptive filename, then pass
|
||||||
the conversation, where the user (the owner) and any collaborators can read
|
that file path here. The file contents are published to the plan-review page
|
||||||
it, leave inline comments, and then approve it or request changes. Call it
|
linked in the conversation, where the user (the owner) and any collaborators
|
||||||
again to overwrite the plan with a revised version when addressing feedback.
|
can read it, leave inline comments, and then approve it or request changes.
|
||||||
|
Call it again to publish a revised file when addressing feedback.
|
||||||
|
|
||||||
Write the plan in standard Markdown — headings, bullet/numbered lists, and
|
Write the plan in standard Markdown — headings, bullet/numbered lists, and
|
||||||
fenced code blocks all render. Structure it clearly (overview, files to
|
fenced code blocks all render. Keep it concise and high level, focusing on
|
||||||
change, ordered steps, risks).
|
approach, decisions/tradeoffs, risks, and verification; avoid file/function
|
||||||
|
details unless they are unusually tricky or controversial.
|
||||||
|
|
||||||
Args:
|
Args:
|
||||||
plan_markdown: The full plan, as a Markdown document.
|
plan_file_path: Path to the Markdown plan file in the sandbox.
|
||||||
|
|
||||||
Returns:
|
Returns:
|
||||||
``{success: True, path}`` on success, or ``{success: False, error}``.
|
``{success: True, path}`` on success, or ``{success: False, error}``.
|
||||||
"""
|
"""
|
||||||
content = plan_markdown.strip()
|
if not isinstance(plan_file_path, str):
|
||||||
if not content:
|
return {"success": False, "error": "plan_file_path must be a string"}
|
||||||
return {"success": False, "error": "plan_markdown cannot be empty"}
|
path = plan_file_path.strip()
|
||||||
|
if not path:
|
||||||
|
return {"success": False, "error": "plan_file_path cannot be empty"}
|
||||||
|
if not _is_markdown_path(path):
|
||||||
|
return {
|
||||||
|
"success": False,
|
||||||
|
"error": f"plan_file_path must point to a Markdown file in {PLAN_FILE_DIRECTORY}",
|
||||||
|
}
|
||||||
|
|
||||||
try:
|
try:
|
||||||
config = get_config()
|
config = get_config()
|
||||||
|
|
@ -53,28 +65,52 @@ async def save_plan(plan_markdown: str) -> dict[str, Any]:
|
||||||
return {"success": False, "error": "no thread_id in run config"}
|
return {"success": False, "error": "no thread_id in run config"}
|
||||||
|
|
||||||
try:
|
try:
|
||||||
path = await _save(str(thread_id), content)
|
content = (await _read_plan_file(str(thread_id), path)).strip()
|
||||||
|
if not content:
|
||||||
|
return {"success": False, "error": "plan file cannot be empty"}
|
||||||
|
await _save(str(thread_id), content, path)
|
||||||
except Exception as exc: # noqa: BLE001
|
except Exception as exc: # noqa: BLE001
|
||||||
logger.exception("save_plan failed for thread %s", thread_id)
|
logger.exception("save_plan failed for thread %s", thread_id)
|
||||||
return {"success": False, "error": f"failed to save plan: {exc}"}
|
return {"success": False, "error": f"failed to save plan: {exc}"}
|
||||||
return {"success": True, "path": path}
|
return {"success": True, "path": path}
|
||||||
|
|
||||||
|
|
||||||
async def _save(thread_id: str, content: str) -> str:
|
async def _save(thread_id: str, content: str, path: str) -> None:
|
||||||
sandbox_path = await _write_to_sandbox(thread_id, content)
|
await save_plan_content(
|
||||||
await save_plan_content(thread_id, markdown=content, status=PLAN_STATUS_READY)
|
thread_id, markdown=content, status=PLAN_STATUS_READY, plan_file_path=path
|
||||||
return sandbox_path
|
)
|
||||||
|
|
||||||
|
|
||||||
async def _write_to_sandbox(thread_id: str, content: str) -> str:
|
async def _read_plan_file(thread_id: str, path: str) -> str:
|
||||||
"""Write ``plan.md`` into the thread's sandbox. Best-effort: a missing sandbox
|
backend = await get_sandbox_backend(thread_id)
|
||||||
must not block publishing the plan to the review page."""
|
result = await backend.aread(path, offset=0, limit=_MAX_PLAN_LINES)
|
||||||
try:
|
error = _value(result, "error")
|
||||||
from ..utils.sandbox_state import get_sandbox_backend
|
if error:
|
||||||
|
raise ValueError(error)
|
||||||
|
file_data = _value(result, "file_data")
|
||||||
|
if file_data is None:
|
||||||
|
raise ValueError("plan file could not be read")
|
||||||
|
encoding = _value(file_data, "encoding")
|
||||||
|
if encoding is not None and encoding != "utf-8":
|
||||||
|
raise ValueError("plan file must be UTF-8 text")
|
||||||
|
content = _value(file_data, "content")
|
||||||
|
if not isinstance(content, str):
|
||||||
|
raise ValueError("plan file content was not text")
|
||||||
|
if content.count("\n") + 1 >= _MAX_PLAN_LINES:
|
||||||
|
raise ValueError("plan file is too large")
|
||||||
|
return content
|
||||||
|
|
||||||
backend = await get_sandbox_backend(thread_id)
|
|
||||||
await backend.awrite(PLAN_FILE_PATH, content)
|
def _value(value: Any, key: str) -> Any:
|
||||||
return PLAN_FILE_PATH
|
if isinstance(value, Mapping):
|
||||||
except Exception:
|
return value.get(key)
|
||||||
logger.warning("Could not write plan.md to sandbox for %s", thread_id, exc_info=True)
|
return getattr(value, key, None)
|
||||||
return PLAN_FILE_PATH
|
|
||||||
|
|
||||||
|
def _is_markdown_path(path: str) -> bool:
|
||||||
|
if "\x00" in path or not path.startswith(f"{PLAN_FILE_DIRECTORY}/"):
|
||||||
|
return False
|
||||||
|
filename = path.removeprefix(f"{PLAN_FILE_DIRECTORY}/")
|
||||||
|
if not filename or "/" in filename:
|
||||||
|
return False
|
||||||
|
return filename.lower().endswith(_MARKDOWN_EXTENSIONS)
|
||||||
|
|
|
||||||
|
|
@ -231,7 +231,7 @@ def _save_plan_step(_messages: list[BaseMessage]) -> AIMessage:
|
||||||
tool_calls=[
|
tool_calls=[
|
||||||
{
|
{
|
||||||
"name": "save_plan",
|
"name": "save_plan",
|
||||||
"args": {"plan_markdown": PLAN_MARKDOWN},
|
"args": {"plan_file_path": PLAN_FILE_PATH},
|
||||||
"id": "call-save-plan",
|
"id": "call-save-plan",
|
||||||
}
|
}
|
||||||
],
|
],
|
||||||
|
|
|
||||||
|
|
@ -24,8 +24,6 @@ def test_plan_mode_prompt_absent_by_default() -> None:
|
||||||
def test_plan_mode_excluded_tools_cover_mutating_tools() -> None:
|
def test_plan_mode_excluded_tools_cover_mutating_tools() -> None:
|
||||||
excluded = server.PLAN_MODE_EXCLUDED_TOOLS
|
excluded = server.PLAN_MODE_EXCLUDED_TOOLS
|
||||||
for tool in (
|
for tool in (
|
||||||
"write_file",
|
|
||||||
"edit_file",
|
|
||||||
"task",
|
"task",
|
||||||
"open_pull_request",
|
"open_pull_request",
|
||||||
"request_pr_review",
|
"request_pr_review",
|
||||||
|
|
@ -38,6 +36,10 @@ def test_plan_mode_excluded_tools_cover_mutating_tools() -> None:
|
||||||
# Read-only tools must stay available.
|
# Read-only tools must stay available.
|
||||||
assert "read_file" not in excluded
|
assert "read_file" not in excluded
|
||||||
assert "execute" not in excluded
|
assert "execute" not in excluded
|
||||||
|
# File edit tools stay available so the agent can author the plan file under
|
||||||
|
# /workspace/plans/ (restricted to that path via prompt guidance).
|
||||||
|
assert "write_file" not in excluded
|
||||||
|
assert "edit_file" not in excluded
|
||||||
|
|
||||||
|
|
||||||
class _FakeThreadsClient:
|
class _FakeThreadsClient:
|
||||||
|
|
|
||||||
|
|
@ -99,12 +99,12 @@ async def test_save_plan_requires_run_context() -> None:
|
||||||
from agent.tools.save_plan import save_plan
|
from agent.tools.save_plan import save_plan
|
||||||
|
|
||||||
# No LangGraph run context → no thread_id → graceful error, not a crash.
|
# No LangGraph run context → no thread_id → graceful error, not a crash.
|
||||||
result = await save_plan("## Plan")
|
result = await save_plan("/workspace/plans/2026-07-08-test-plan.md")
|
||||||
assert result["success"] is False
|
assert result["success"] is False
|
||||||
assert "thread_id" in result["error"]
|
assert "thread_id" in result["error"]
|
||||||
|
|
||||||
|
|
||||||
async def test_save_plan_rejects_empty_markdown() -> None:
|
async def test_save_plan_rejects_empty_path() -> None:
|
||||||
from agent.tools.save_plan import save_plan
|
from agent.tools.save_plan import save_plan
|
||||||
|
|
||||||
result = await save_plan(" ")
|
result = await save_plan(" ")
|
||||||
|
|
@ -112,6 +112,70 @@ async def test_save_plan_rejects_empty_markdown() -> None:
|
||||||
assert "empty" in result["error"]
|
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:
|
def test_plan_routes_registered() -> None:
|
||||||
from agent.webapp import app
|
from agent.webapp import app
|
||||||
|
|
||||||
|
|
@ -145,12 +209,27 @@ def test_plan_status_constants() -> None:
|
||||||
assert plan_store.PLAN_STATUS_REVISING == "revising"
|
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:
|
def test_http_request_excluded_in_plan_mode() -> None:
|
||||||
from agent.server import PLAN_MODE_EXCLUDED_TOOLS
|
from agent.server import PLAN_MODE_EXCLUDED_TOOLS
|
||||||
|
|
||||||
assert "http_request" in 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:
|
class _FakeReq:
|
||||||
def __init__(self, tools: list[Any], state: dict[str, Any]) -> None:
|
def __init__(self, tools: list[Any], state: dict[str, Any]) -> None:
|
||||||
self.tools = tools
|
self.tools = tools
|
||||||
|
|
@ -239,3 +318,31 @@ async def test_approve_plan_posts_slack_approval_notice(monkeypatch: pytest.Monk
|
||||||
"thread_ts": "1700000000.0001",
|
"thread_ts": "1700000000.0001",
|
||||||
"text": "Plan approved with 1 comments by Alice\nbeginning implementation",
|
"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
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue