From ae04b72b4177a21cd31c3fe232f9e71b048813fe Mon Sep 17 00:00:00 2001 From: Ramon Nogueira Date: Mon, 29 Jun 2026 16:02:55 -0400 Subject: [PATCH] feat: publish plans from sandbox files (#1635) * feat: publish plans from sandbox files Co-authored-by: open-swe[bot] * fix: avoid fixed plan filenames Co-authored-by: open-swe[bot] * fix: virtualize local sandbox file paths Co-authored-by: open-swe[bot] * fix: preserve plan_file_path across set_plan_status set_plan_status was rewriting the content record with only markdown and status, dropping plan_file_path. After a reject, the owner's dashboard edit would mirror to a different file than the agent's original, and the next save_plan could republish the stale file. Preserve plan_file_path when updating status. --------- Co-authored-by: Ramon Nogueira <270434257+ramon-langchain@users.noreply.github.com> Co-authored-by: open-swe[bot] --- agent/dashboard/plan_api.py | 20 +++-- agent/dashboard/plan_store.py | 57 ++++++++++--- agent/integrations/local.py | 1 + agent/middleware/plan_mode.py | 15 ++-- agent/prompt.py | 10 +-- agent/server.py | 21 ++--- agent/tools/enter_plan_mode.py | 20 ++--- agent/tools/save_plan.py | 99 +++++++++++++++++------ tests/e2e/fake_llm.py | 22 ++++- tests/test_local_integration.py | 5 +- tests/test_plan_mode.py | 6 +- tests/test_plan_review.py | 138 ++++++++++++++++++++++++++++++-- 12 files changed, 327 insertions(+), 87 deletions(-) diff --git a/agent/dashboard/plan_api.py b/agent/dashboard/plan_api.py index 1dcbb34a..7e101d89 100644 --- a/agent/dashboard/plan_api.py +++ b/agent/dashboard/plan_api.py @@ -33,6 +33,7 @@ from .plan_store import ( delete_plan_comment, get_plan_content, list_plan_comments, + plan_file_path_for_thread, save_plan_content, set_plan_status, write_plan_to_sandbox, @@ -103,7 +104,7 @@ async def update_plan( """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 file) 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")): @@ -115,10 +116,18 @@ async def update_plan( 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 + plan_file_path = content.get("plan_file_path") + plan_file_path = ( + plan_file_path if isinstance(plan_file_path, str) else plan_file_path_for_thread(thread_id) ) - await write_plan_to_sandbox(thread_id, markdown) + await save_plan_content( + thread_id, + markdown=markdown, + status=PLAN_STATUS_READY, + clear_comments=False, + plan_file_path=plan_file_path, + ) + await write_plan_to_sandbox(thread_id, markdown, plan_file_path=plan_file_path) return {"status": PLAN_STATUS_READY, "markdown": markdown} @@ -210,7 +219,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/dashboard/plan_store.py b/agent/dashboard/plan_store.py index d12198a6..9ceef9c6 100644 --- a/agent/dashboard/plan_store.py +++ b/agent/dashboard/plan_store.py @@ -1,8 +1,7 @@ """Persistence for the plan-review feature. The plan lives in two places: - - the agent's sandbox, as a real ``plan.md`` file (written by the ``save_plan`` - tool — the source artifact the agent produces and can re-read), and + - the agent's sandbox, as a real Markdown file the agent creates and edits, and - the LangGraph store, as the published snapshot the dashboard renders. Reviewers leave whole-document comments, stored one item per comment under @@ -13,6 +12,7 @@ store operations (no CRDT/WebSocket). from __future__ import annotations import logging +import re import uuid from datetime import UTC, datetime from typing import Any @@ -24,8 +24,8 @@ logger = logging.getLogger(__name__) PLAN_CONTENT_NAMESPACE = ["plan", "content"] PLAN_COMMENTS_NAMESPACE = ["plan", "comments"] -# The plan is mirrored into the sandbox as a real file the agent can re-read. -PLAN_FILE_PATH = "plan.md" +# Plans are mirrored into the sandbox outside cloned repositories. +PLAN_FILE_DIRECTORY = "/workspace/plans" # Plan lifecycle, stored on both the content record and the thread metadata. PLAN_STATUS_PLANNING = "planning" @@ -35,6 +35,12 @@ PLAN_STATUS_APPROVED = "approved" PLAN_STATUS_CANCELLED = "cancelled" +def plan_file_path_for_thread(thread_id: str) -> str: + date = datetime.now(UTC).strftime("%Y-%m-%d") + slug = re.sub(r"[^a-zA-Z0-9-]+", "-", thread_id).strip("-").lower()[:48] + return f"{PLAN_FILE_DIRECTORY}/{date}-{slug or 'plan'}.md" + + def _client() -> Any: return get_client() @@ -46,12 +52,22 @@ def _item_value(item: Any) -> dict[str, Any] | None: return value if isinstance(value, dict) else None +async def _stored_plan_file_path(client: Any, thread_id: str) -> str | None: + try: + value = _item_value(await client.store.get_item(PLAN_CONTENT_NAMESPACE, thread_id)) or {} + except Exception: + return None + path = value.get("plan_file_path") + return path if isinstance(path, str) and path else None + + async def save_plan_content( thread_id: str, *, markdown: str, status: str = PLAN_STATUS_READY, clear_comments: bool = True, + plan_file_path: str | None = None, ) -> None: """Publish the plan markdown + status for the dashboard to render. @@ -60,10 +76,15 @@ async def save_plan_content( and be fed back to the agent on the next approve/reject. A manual owner edit passes ``clear_comments=False`` so reviewer feedback survives the edit.""" client = _client() + if plan_file_path is None: + plan_file_path = await _stored_plan_file_path(client, thread_id) + record = {"markdown": markdown, "status": status} + if plan_file_path: + record["plan_file_path"] = plan_file_path await client.store.put_item( PLAN_CONTENT_NAMESPACE, thread_id, - {"markdown": markdown, "status": status}, + record, ) if clear_comments: try: @@ -74,18 +95,24 @@ async def save_plan_content( await _merge_thread_metadata(thread_id, {"plan_status": status, "plan_mode": True}) -async def write_plan_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.""" +async def write_plan_to_sandbox( + thread_id: str, content: str, *, plan_file_path: str | None = None +) -> str: + """Mirror the dashboard plan edit into the thread's sandbox. + + Best-effort: a missing sandbox must not block publishing the plan to the + review page. + """ + path = plan_file_path or plan_file_path_for_thread(thread_id) try: from ..utils.sandbox_state import get_sandbox_backend backend = await get_sandbox_backend(thread_id) - await backend.awrite(PLAN_FILE_PATH, content) - return PLAN_FILE_PATH + await backend.awrite(path, content) + return path except Exception: - logger.warning("Could not write plan.md to sandbox for %s", thread_id, exc_info=True) - return PLAN_FILE_PATH + logger.warning("Could not write plan file to sandbox for %s", thread_id, exc_info=True) + return path async def get_plan_content( @@ -110,10 +137,14 @@ async def set_plan_status(thread_id: str, status: str, *, plan_mode: bool | None """Update the plan lifecycle status on both the content record and metadata.""" existing = await get_plan_content(thread_id) or {} client = _client() + record: dict[str, Any] = {"markdown": existing.get("markdown", ""), "status": status} + plan_file_path = existing.get("plan_file_path") + if isinstance(plan_file_path, str) and plan_file_path: + record["plan_file_path"] = plan_file_path await client.store.put_item( PLAN_CONTENT_NAMESPACE, thread_id, - {"markdown": existing.get("markdown", ""), "status": status}, + record, ) metadata: dict[str, Any] = {"plan_status": status} if plan_mode is not None: diff --git a/agent/integrations/local.py b/agent/integrations/local.py index 7cff4509..91c2c194 100644 --- a/agent/integrations/local.py +++ b/agent/integrations/local.py @@ -24,5 +24,6 @@ def create_local_sandbox(sandbox_id: str | None = None): return LocalShellBackend( root_dir=root_dir, + virtual_mode=True, inherit_env=True, ) diff --git a/agent/middleware/plan_mode.py b/agent/middleware/plan_mode.py index 342fcd40..90f925fc 100644 --- a/agent/middleware/plan_mode.py +++ b/agent/middleware/plan_mode.py @@ -1,11 +1,12 @@ """Plan-mode tool gating. -Hides the mutating tools whenever plan mode is active — either when the run -starts in plan mode (the per-thread ``plan_mode`` carried in configurable, e.g. -a reject re-dispatch) OR after the model calls ``enter_plan_mode`` mid-run, which -sets ``plan_mode`` in the run state. Installed unconditionally so self-activation -actually restricts the *next* model turn (the tool list is recomputed on every -model call), rather than only affecting a future run. +Hides tools that mutate external systems whenever plan mode is active — either +when the run starts in plan mode (the per-thread ``plan_mode`` carried in +configurable, e.g. a reject re-dispatch) OR after the model calls +``enter_plan_mode`` mid-run, which sets ``plan_mode`` in the run state. Installed +unconditionally so self-activation actually restricts the *next* model turn (the +tool list is recomputed on every model call), rather than only affecting a future +run. """ from __future__ import annotations @@ -37,7 +38,7 @@ def _tool_name(tool: BaseTool | dict[str, Any] | Any) -> str | None: class PlanModeMiddleware(AgentMiddleware): - """Strip mutating tools from each model request while plan mode is active.""" + """Strip disallowed tools from each model request while plan mode is active.""" state_schema = PlanModeState diff --git a/agent/prompt.py b/agent/prompt.py index 56b0bba4..4b4b65cd 100644 --- a/agent/prompt.py +++ b/agent/prompt.py @@ -99,7 +99,7 @@ PLAN_MODE_GUIDANCE_SECTION = """--- ### Plan Mode -If a task would genuinely benefit from a structured plan before any code — complex, many files, or multiple valid approaches — call the `enter_plan_mode` tool. This is NOT triggered by the word "plan" in the request; use judgment. Once in plan mode, stay read-only, research the code, save your plan with `save_plan` (it writes `plan.md` and publishes a review page), and share the plan-review link with the user, who approves before you implement. +If a task would genuinely benefit from a structured plan before any code — complex, many files, or multiple valid approaches — call the `enter_plan_mode` tool. This is NOT triggered by the word "plan" in the request; use judgment. Once in plan mode, stay read-only for the target repo, research the code, create/edit your plan as a dated Markdown file under `/workspace/plans/` (for example, `/workspace/plans/YYYY-MM-DD-short-task-slug.md`), publish it with `save_plan`, and share the plan-review link with the user, who approves before you implement. Plan-review link for this conversation: {plan_review_url}""" @@ -109,15 +109,15 @@ PLAN_MODE_SECTION = """--- **Plan mode is enabled for this run. This supersedes any instruction telling you to edit code, commit, push, or open a pull request.** -You are in a read-only research-and-planning phase. Your single deliverable is a clear, reviewable implementation plan saved with `save_plan` — NOT code changes. Share the plan-review link below with the user right after entering plan mode and again when the plan is ready. +You are in a read-only research-and-planning phase for the target repo. Your single deliverable is a clear, reviewable implementation plan saved as a Markdown file outside any repo and published with `save_plan` — NOT code changes. Share the plan-review link below with the user right after entering plan mode and again when the plan is ready. **Plan-review link:** {plan_url} -**You MUST NOT** edit/create/delete files, run state-changing `execute` commands (no `git commit`/`push`/`checkout -b`, installs, code generators, or file-rewriting formatters), commit, push, open/update a PR, call `request_pr_review`, or mutate Linear/external systems. The `task` subagent is disabled here (subagents wouldn't inherit these restrictions) — research directly. +**You MUST NOT** edit/create/delete files inside the target repo, run state-changing `execute` commands except creating `/workspace/plans` (no `git commit`/`push`/`checkout -b`, installs, code generators, or file-rewriting formatters), commit, push, open/update a PR, call `request_pr_review`, or mutate Linear/external systems. The `task` subagent is disabled here (subagents wouldn't inherit these restrictions) — research directly. -**You MAY (read-only):** clone and read the repo (`read_file`, `ls`, `glob`, `grep`, read-only `execute` like `git clone`/`status`/`log`/`diff`, `cat`, `rg`), research with `web_search`/`fetch_url`, and ask clarifying questions via `slack_thread_reply` / `linear_comment`. +**You MAY:** clone and read the repo (`read_file`, `ls`, `glob`, `grep`, read-only `execute` like `git clone`/`status`/`log`/`diff`, `cat`, `rg`), research with `web_search`/`fetch_url`, ask clarifying questions via `slack_thread_reply` / `linear_comment`, use `execute` only if needed to create `/workspace/plans`, and use `write_file` / `edit_file` only to create or revise the plan file outside any repo under `/workspace/plans/`. -**Workflow:** explore the relevant code enough to choose a sound approach, clarify ambiguity, then save ONE concise recommended plan with `save_plan` (pass the full Markdown as `plan_markdown`) using this structure. Keep it high level: focus on desired behavior, architecture boundaries, product decisions, tradeoffs, rollout/migration concerns, and verification. Avoid file/function-level details and exhaustive file lists unless a specific implementation detail is unusually tricky, risky, or controversial. Aim for about one page or less unless the task truly requires more. +**Workflow:** explore the relevant code enough to choose a sound approach, clarify ambiguity, choose a dated, descriptive plan path like `/workspace/plans/YYYY-MM-DD-short-task-slug.md`, create it with ONE recommended plan, refine it with normal file-editing tools if needed, then publish it with `save_plan` by passing that exact `plan_file_path`. Keep it high level: focus on desired behavior, architecture boundaries, product decisions, tradeoffs, rollout/migration concerns, and verification. Avoid file/function-level details and exhaustive file lists unless a specific implementation detail is unusually tricky, risky, or controversial. Aim for about one page or less unless the task truly requires more. Use this structure: ``` ## Plan: diff --git a/agent/server.py b/agent/server.py index 02f25c9b..944a2b6f 100644 --- a/agent/server.py +++ b/agent/server.py @@ -529,18 +529,19 @@ DEFAULT_RECURSION_LIMIT = 9_999 # signal via notify_step_limit_reached rather than dying silently. MODEL_CALL_RECURSION_LIMIT = 5_000 -# Mutating tools hidden from the model while plan mode is active so it can only -# research and propose a plan. `execute` stays available; plan-mode shell -# discipline (no mutating commands) is instructed via the system prompt rather -# than enforced. `http_request` is excluded because it can POST/PUT/PATCH/DELETE -# to external services — read-only web research goes through `web_search` / -# `fetch_url`. `task` is excluded because the general-purpose subagent is built -# with its own filesystem/PR/Linear tools and does not inherit this exclusion, so -# delegating to it would bypass the read-only intent. +# Mutating external tools hidden from the model while plan mode is active so it +# can only research and propose a plan. File edit tools stay available so the +# agent can draft and revise a plan under `/workspace/plans/`; prompt guidance +# restricts them to that plan file outside cloned repositories. `execute` stays available; +# plan-mode shell discipline (no mutating commands) is instructed via the system +# prompt rather than enforced. `http_request` is excluded because it can +# POST/PUT/PATCH/DELETE to external services — read-only web research goes +# through `web_search` / `fetch_url`. `task` is excluded because the +# general-purpose subagent is built with its own filesystem/PR/Linear tools and +# does not inherit this exclusion, so delegating to it would bypass the read-only +# 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 a1d97e89..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 a " - "concise, high-level 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 a concise, - high-level 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 d346d35b..e8fde295 100644 --- a/agent/tools/save_plan.py +++ b/agent/tools/save_plan.py @@ -1,35 +1,37 @@ -"""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, - write_plan_to_sandbox, -) +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__) +_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. - 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. +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. 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. Keep it concise and high level, focusing on @@ -37,14 +39,21 @@ async def save_plan(plan_markdown: str) -> dict[str, Any]: 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() @@ -56,14 +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_plan_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 _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 + + +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 3f491bdd..18971b41 100644 --- a/tests/e2e/fake_llm.py +++ b/tests/e2e/fake_llm.py @@ -136,6 +136,8 @@ def _step_reply(messages: list[BaseMessage]) -> AIMessage: # --- plan-mode flow -------------------------------------------------------- +PLAN_FILE_PATH = "/workspace/plans/2026-06-29-greet-helper.md" + PLAN_MARKDOWN = """## Plan: Add greet() helper ### Overview @@ -185,11 +187,28 @@ def _step_plan_research(_messages: list[BaseMessage]) -> AIMessage: ) +def _step_write_plan(_messages: list[BaseMessage]) -> AIMessage: + return AIMessage( + content="Writing the plan file for review.", + tool_calls=[ + { + "name": "write_file", + "args": {"file_path": PLAN_FILE_PATH, "content": PLAN_MARKDOWN}, + "id": "call-write-plan", + } + ], + ) + + def _step_save_plan(_messages: list[BaseMessage]) -> AIMessage: return AIMessage( content="Saving the plan for review.", tool_calls=[ - {"name": "save_plan", "args": {"plan_markdown": PLAN_MARKDOWN}, "id": "call-save-plan"} + { + "name": "save_plan", + "args": {"plan_file_path": PLAN_FILE_PATH}, + "id": "call-save-plan", + } ], ) @@ -220,6 +239,7 @@ def build_plan_script() -> list[Any]: _step_enter_plan, _step_plan_link, _step_plan_research, + _step_write_plan, _step_save_plan, _step_plan_complete, _step_plan_end, diff --git a/tests/test_local_integration.py b/tests/test_local_integration.py index b1ebba48..bb5c8eb9 100644 --- a/tests/test_local_integration.py +++ b/tests/test_local_integration.py @@ -2,8 +2,9 @@ import agent.integrations.local as local_mod class _StubLocalShellBackend: - def __init__(self, *, root_dir, inherit_env): + def __init__(self, *, root_dir, virtual_mode, inherit_env): self.root_dir = root_dir + self.virtual_mode = virtual_mode self.inherit_env = inherit_env @@ -16,6 +17,7 @@ def test_create_local_sandbox_creates_missing_root_dir(monkeypatch, tmp_path): assert root.is_dir() assert backend.root_dir == str(root) + assert backend.virtual_mode is True assert backend.inherit_env is True @@ -27,3 +29,4 @@ def test_create_local_sandbox_defaults_to_cwd(monkeypatch, tmp_path): backend = local_mod.create_local_sandbox() assert backend.root_dir == str(tmp_path) + assert backend.virtual_mode is True diff --git a/tests/test_plan_mode.py b/tests/test_plan_mode.py index cc3ea0ae..8a6e2d68 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", @@ -34,8 +32,10 @@ def test_plan_mode_excluded_tools_cover_mutating_tools() -> None: "linear_delete_issue", ): assert tool in excluded - # Read-only tools must stay available. + # Read-only tools and plan-file editing tools must stay available. assert "read_file" not in excluded + assert "write_file" not in excluded + assert "edit_file" not in excluded assert "execute" not in excluded diff --git a/tests/test_plan_review.py b/tests/test_plan_review.py index b308100e..daba0195 100644 --- a/tests/test_plan_review.py +++ b/tests/test_plan_review.py @@ -108,12 +108,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-06-29-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(" ") @@ -121,6 +121,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-06-29-test-plan.md") + + assert result == {"success": True, "path": "/workspace/plans/2026-06-29-test-plan.md"} + assert reads == [ + ("/workspace/plans/2026-06-29-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-06-29-test-plan.md", + } + + def test_plan_routes_registered() -> None: from agent.webapp import app @@ -150,12 +214,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 @@ -192,6 +271,34 @@ def test_plan_mode_middleware_self_activation_via_state() -> None: # --- manual plan editing ------------------------------------------------- +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 + + async def test_save_plan_content_clear_comments_flag(monkeypatch: pytest.MonkeyPatch) -> None: from agent.dashboard import plan_store @@ -236,13 +343,24 @@ def _patch_update_plan_deps( return content async def fake_save( - thread_id: str, *, markdown: str, status: str, clear_comments: bool = True + thread_id: str, + *, + markdown: str, + status: str, + clear_comments: bool = True, + plan_file_path: str | None = None, ) -> None: - saved.update(markdown=markdown, status=status, clear_comments=clear_comments) + saved.update( + markdown=markdown, + status=status, + clear_comments=clear_comments, + plan_file_path=plan_file_path, + ) - async def fake_write(thread_id: str, c: str) -> str: + async def fake_write(thread_id: str, c: str, *, plan_file_path: str | None = None) -> str: sandbox["content"] = c - return "plan.md" + sandbox["plan_file_path"] = plan_file_path + return plan_file_path or "/workspace/plans/fallback.md" monkeypatch.setattr(plan_api, "_thread_metadata", fake_meta) monkeypatch.setattr(plan_api, "_user_owns_thread", lambda *a, **k: owner) @@ -262,7 +380,11 @@ async def test_update_plan_owner_saves_and_mirrors_sandbox( monkeypatch, metadata={"plan_status": "ready"}, owner=True, - content={"markdown": "old", "status": "ready"}, + content={ + "markdown": "old", + "status": "ready", + "plan_file_path": "/workspace/plans/2026-06-29-existing.md", + }, saved=saved, sandbox=sandbox, ) @@ -273,7 +395,9 @@ async def test_update_plan_owner_saves_and_mirrors_sandbox( assert result == {"status": "ready", "markdown": "# New\n\ndo x"} assert saved["status"] == "ready" assert saved["clear_comments"] is False + assert saved["plan_file_path"] == "/workspace/plans/2026-06-29-existing.md" assert sandbox["content"] == "# New\n\ndo x" + assert sandbox["plan_file_path"] == "/workspace/plans/2026-06-29-existing.md" async def test_update_plan_rejects_non_owner(monkeypatch: pytest.MonkeyPatch) -> None: