diff --git a/agent/dashboard/plan_api.py b/agent/dashboard/plan_api.py index 491f819e..bde2a5dd 100644 --- a/agent/dashboard/plan_api.py +++ b/agent/dashboard/plan_api.py @@ -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) text = ( "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)'}" ) await _dispatch_followup(thread_id, metadata, text, plan_mode=True) diff --git a/agent/server.py b/agent/server.py index c16d36ab..8914e655 100644 --- a/agent/server.py +++ b/agent/server.py @@ -619,8 +619,6 @@ MODEL_CALL_RECURSION_LIMIT = 5_000 # intent. PLAN_MODE_EXCLUDED_TOOLS: frozenset[str] = frozenset( { - "write_file", - "edit_file", "task", "http_request", "open_pull_request", diff --git a/agent/tools/enter_plan_mode.py b/agent/tools/enter_plan_mode.py index 191e1ee7..938cd6f7 100644 --- a/agent/tools/enter_plan_mode.py +++ b/agent/tools/enter_plan_mode.py @@ -15,10 +15,11 @@ from ..dashboard.plan_store import PLAN_STATUS_PLANNING, set_plan_status logger = logging.getLogger(__name__) _ENTERED_MESSAGE = ( - "Plan mode is active. Stay read-only: research the codebase, then record your " - "implementation plan with the `save_plan` tool (it publishes the plan to the " - "review page) and share the plan-review link in the source channel. Do not edit " - "files, commit, push, or open a PR — wait for the user to approve the plan." + "Plan mode is active. Stay read-only for the target repo: research the codebase, " + "create or edit a dated, concise plan file under `/workspace/plans/`, then publish " + "it with the `save_plan` tool and share the plan-review link in the source channel. " + "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 judgment about whether planning is genuinely warranted. - Once activated, stay read-only: research the codebase, then record your plan - with the ``save_plan`` tool (it publishes the plan to the review page) and - share the plan-review link with the user. Do not edit files, commit, push, - or open a PR — the user reviews the plan and approves it before you - implement. + Once activated, stay read-only for the target repo: research the codebase, + create or edit a dated, concise Markdown plan outside any repo (for example, + ``/workspace/plans/YYYY-MM-DD-short-task-slug.md``), then publish it with + the ``save_plan`` tool and share the plan-review link with the user. Do not + 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() if thread_id: diff --git a/agent/tools/save_plan.py b/agent/tools/save_plan.py index 3016a0fc..e8fde295 100644 --- a/agent/tools/save_plan.py +++ b/agent/tools/save_plan.py @@ -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 -agent produces and can re-read) and publishes it to the plan-review page, where -the user and collaborators read it, comment inline, and approve or request -changes. Available in plan mode (it does not modify the repository under review). +Reads the Markdown plan file the agent created in the sandbox and publishes it to +the plan-review page, where the user and collaborators read it, comment inline, +and approve or request changes. Available in plan mode (it does not modify the +repository under review). """ from __future__ import annotations import logging +from collections.abc import Mapping from typing import Any 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__) -PLAN_FILE_PATH = "plan.md" +_MAX_PLAN_LINES = 20_000 +_MARKDOWN_EXTENSIONS = (".md", ".markdown") -async def save_plan(plan_markdown: str) -> dict[str, Any]: - """Write your implementation plan as a markdown file and publish it for review. +async def save_plan(plan_file_path: str) -> dict[str, Any]: + """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 - ``plan.md`` in the sandbox and published to the plan-review page linked in - the conversation, where the user (the owner) and any collaborators can read - it, leave inline comments, and then approve it or request changes. Call it - again to overwrite the plan with a revised version when addressing feedback. + Use this in plan mode once your plan is ready. First create a Markdown file + under ``/workspace/plans/`` using a dated, descriptive filename, then pass + that file path here. The file contents are published to the plan-review page + linked in the conversation, where the user (the owner) and any collaborators + 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 - fenced code blocks all render. Structure it clearly (overview, files to - change, ordered steps, risks). + fenced code blocks all render. Keep it concise and high level, focusing on + approach, decisions/tradeoffs, risks, and verification; avoid file/function + details unless they are unusually tricky or controversial. Args: - plan_markdown: The full plan, as a Markdown document. + plan_file_path: Path to the Markdown plan file in the sandbox. Returns: ``{success: True, path}`` on success, or ``{success: False, error}``. """ - content = plan_markdown.strip() - if not content: - return {"success": False, "error": "plan_markdown cannot be empty"} + if not isinstance(plan_file_path, str): + return {"success": False, "error": "plan_file_path must be a string"} + 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: 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"} 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 logger.exception("save_plan failed for thread %s", thread_id) return {"success": False, "error": f"failed to save plan: {exc}"} return {"success": True, "path": path} -async def _save(thread_id: str, content: str) -> str: - sandbox_path = await _write_to_sandbox(thread_id, content) - await save_plan_content(thread_id, markdown=content, status=PLAN_STATUS_READY) - return sandbox_path +async def _save(thread_id: str, content: str, path: str) -> None: + await save_plan_content( + thread_id, markdown=content, status=PLAN_STATUS_READY, plan_file_path=path + ) -async def _write_to_sandbox(thread_id: str, content: str) -> str: - """Write ``plan.md`` into the thread's sandbox. Best-effort: a missing sandbox - must not block publishing the plan to the review page.""" - try: - from ..utils.sandbox_state import get_sandbox_backend +async def _read_plan_file(thread_id: str, path: str) -> str: + backend = await get_sandbox_backend(thread_id) + result = await backend.aread(path, offset=0, limit=_MAX_PLAN_LINES) + error = _value(result, "error") + 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) - return PLAN_FILE_PATH - except Exception: - logger.warning("Could not write plan.md to sandbox for %s", thread_id, exc_info=True) - return PLAN_FILE_PATH + +def _value(value: Any, key: str) -> Any: + if isinstance(value, Mapping): + return value.get(key) + return getattr(value, key, None) + + +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) diff --git a/tests/e2e/fake_llm.py b/tests/e2e/fake_llm.py index 08dcd665..bf0d5356 100644 --- a/tests/e2e/fake_llm.py +++ b/tests/e2e/fake_llm.py @@ -231,7 +231,7 @@ def _save_plan_step(_messages: list[BaseMessage]) -> AIMessage: tool_calls=[ { "name": "save_plan", - "args": {"plan_markdown": PLAN_MARKDOWN}, + "args": {"plan_file_path": PLAN_FILE_PATH}, "id": "call-save-plan", } ], diff --git a/tests/test_plan_mode.py b/tests/test_plan_mode.py index cbf8ca59..32b3392b 100644 --- a/tests/test_plan_mode.py +++ b/tests/test_plan_mode.py @@ -24,8 +24,6 @@ def test_plan_mode_prompt_absent_by_default() -> None: def test_plan_mode_excluded_tools_cover_mutating_tools() -> None: excluded = server.PLAN_MODE_EXCLUDED_TOOLS for tool in ( - "write_file", - "edit_file", "task", "open_pull_request", "request_pr_review", @@ -38,6 +36,10 @@ def test_plan_mode_excluded_tools_cover_mutating_tools() -> None: # Read-only tools must stay available. assert "read_file" 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: diff --git a/tests/test_plan_review.py b/tests/test_plan_review.py index 7d0e5348..74804d38 100644 --- a/tests/test_plan_review.py +++ b/tests/test_plan_review.py @@ -99,12 +99,12 @@ 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("## Plan") + 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_markdown() -> None: +async def test_save_plan_rejects_empty_path() -> None: from agent.tools.save_plan import save_plan result = await save_plan(" ") @@ -112,6 +112,70 @@ async def test_save_plan_rejects_empty_markdown() -> None: 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 @@ -145,12 +209,27 @@ def test_plan_status_constants() -> None: 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 @@ -239,3 +318,31 @@ async def test_approve_plan_posts_slack_approval_notice(monkeypatch: pytest.Monk "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