open-swe/agent/dashboard/plan_api.py
seahaven-openswe[bot] f87847baa4
Some checks are pending
CI / Lint (push) Waiting to run
CI / Format check (push) Waiting to run
CI / Unit tests (push) Waiting to run
CI / Playwright E2E (push) Waiting to run
CI / Docker build smoke (push) Waiting to run
CI / Triage ledger up to date (push) Waiting to run
CI / ui bun.lock in sync (push) Waiting to run
feat: port plan-review & workflow-approval UX (#159)
* feat: port plan-review & workflow-approval UX (#135)

Port six upstream commits onto dev:

- c03a6be7 (already ported): keep plan guidance high-level
- 546042a4: add workflow approval UI with diff preview, approval URLs,
  web review links, and polling for approval status during active runs
- 216cf181: remove workflow token elevation; approved pushes pass
  through directly without proxy token rewriting
- 3dbc0282: preserve plan redirects after login by accepting relative
  same-origin redirect_to values and rejecting blocked paths
- bb104d93: submit plan comments with cmd+enter
- 90cb6caa: terse Slack replies, shared content via save_plan outside
  plan mode (PLAN_STATUS_SHARED), reject shared-content mutations

Refs: #135

* fix: restore login page render and clear CI lint/format

The plan-review port removed the authRedirectUrl import from login.tsx
but left its call site, crashing the login page at runtime (blank page,
no 'Sign in to open-swe'). Pass the relative path straight to loginUrl,
matching the plan route and the backend relative-redirect handling.

Also drop an unused os import in the guard test and reformat
workflow_push_guard.py to satisfy ruff.

* fix: carry workflows:write on the standing proxy token

Complete the half-ported upstream 216cf181 cascade. The port dropped
_run_with_workflow_token from the guard but missed the paired github_app
change, so an approved .github/workflows push ran with the base token
(no workflows:write) and GitHub 403'd it.

Add workflows:write to BASE_RUNTIME_PROXY_TOKEN_PERMISSIONS and delete the
now-orphaned WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS constant; update the
github_app and proxy_auth tests to match. The HITL approval gate in
workflow_push_guard.py is unchanged — this only lets the standing token
push once a human approves.

* fix: restore transient workflow-token elevation (revert standing workflows:write)

The standing GitHub-App proxy token (BASE_RUNTIME_PROXY_TOKEN_PERMISSIONS) is
ALWAYS-ON, so carrying workflows:write on it made the fork's HITL workflow-push
guard the sole control over unapproved workflow pushes. The guard's git-push
parser has gaps (obfuscated-expansion push, `gh api` REST contents PUT,
fully-qualified cross-branch refspecs); with a permanently workflows-scoped
token those gaps become live unapproved-workflow-push exploits (1 critical, 2
high — security review BLOCK on #159).

Restore dev's transient-elevation model:
- Drop workflows:write from BASE_RUNTIME_PROXY_TOKEN_PERMISSIONS; re-add the
  WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS constant (base + workflows:write).
- Re-introduce _run_with_workflow_token in the guard: it mints the
  workflows-scoped token via refresh_proxy_token around the approved,
  guard-normalized fixed_command, then downscopes to RUNTIME then BASE in a
  finally. Route the approval branch through it.
- Restore the dev token/elevation tests.

The standing token no longer carries workflows:write, so the three parser
bypasses hit GitHub 403 again; an approved push still succeeds because the
elevation grants workflows:write only around the normalized command. Keeps all
of #159's diff-preview / approval-URL / Slack-card guard additions.

* fix: reject protocol-relative path from sanitizeAuthRedirect (open redirect)

sanitizeAuthRedirect returned parsed.pathname+search+hash, which `new URL` can
resolve to a protocol-relative `//host` (e.g. input `/..//evil.com` normalizes
same-origin, passing the origin check, but yields a path starting with `//`).
ClientRedirect / login.tsx feed that path to window.location.replace, so it
navigates cross-origin — an open redirect. Reject any resolved path that is not
a single-leading-slash path (`^/[^/]`), falling back to the default. Adds
coverage for `/..//evil.com`, `/.//evil.com`, and `//evil.com`.

* fix: log SECURITY error when workflow-token downscope fails

The elevate->push->downscope finally block was silent on failure. If both
refresh_proxy_token calls fail, the sandbox retains workflows:write for the
rest of the run with no signal. Log a SECURITY error on the partial and full
downscope-failure paths so the retention is observable.

Addresses the GPT-4.1 cross-family review of the token-scope remediation.

---------

Co-authored-by: amoussa1229 <166072409+amoussa1229@users.noreply.github.com>
Co-authored-by: Adam Moussa <adam@seahavenind.com>
2026-07-09 16:03:13 -04:00

338 lines
13 KiB
Python

"""REST API for the plan-review page: read the plan, comment, approve, or request
changes — all plain HTTP, no CRDT/WebSocket.
Reviewers leave whole-document comments via this API; they're stored server-side
and listed for everyone who can read the thread. On approve/reject the comments
are read back here, formatted, and handed to the agent as the instruction for the
follow-up run. The agent never sees comments during review — only this aggregated
feedback at the decision point.
Permissions: any authenticated org member can read a surfaced thread, comment, and
request changes (reject); only the thread owner can approve. A comment can be
deleted by its author or the thread owner.
"""
from __future__ import annotations
import logging
from typing import Any
from fastapi import APIRouter, Depends, HTTPException
from langgraph_sdk import get_client
from pydantic import BaseModel
from ..dispatch import dispatch_agent_run
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,
PLAN_STATUS_SHARED,
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,
_thread_is_readable,
_thread_source,
_user_owns_thread,
)
logger = logging.getLogger(__name__)
plan_router = APIRouter(
prefix="/dashboard/api/plan",
tags=["plan"],
dependencies=[Depends(require_same_origin_for_mutations)],
)
_SESSION_DEP = Depends(require_session)
class CommentBody(BaseModel):
body: str
class PlanUpdate(BaseModel):
markdown: str
async def _thread_metadata(thread_id: str) -> dict[str, Any]:
client = get_client()
try:
thread = await client.threads.get(thread_id)
except Exception as exc: # noqa: BLE001
raise HTTPException(404, "thread not found") from exc
metadata = (
thread.get("metadata") if isinstance(thread, dict) else getattr(thread, "metadata", None)
)
return metadata if isinstance(metadata, dict) else {}
@plan_router.get("/{thread_id}")
async def get_plan(thread_id: str, session: dict[str, Any] = _SESSION_DEP) -> dict[str, Any]:
metadata = await _thread_metadata(thread_id)
if not _thread_is_readable(metadata):
raise HTTPException(404, "thread not found")
login = session["sub"]
email = session.get("email")
content = await get_plan_content(thread_id) or {}
return {
"threadId": thread_id,
"status": content.get("status") or metadata.get("plan_status") or "planning",
"markdown": content.get("markdown", ""),
"isOwner": _user_owns_thread(metadata, login, email),
"user": {
"id": login,
"login": login,
"email": email,
"name": session.get("name") or login,
},
}
@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 {}
_reject_shared_content(content)
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
) -> dict[str, Any]:
metadata = await _thread_metadata(thread_id)
if not _thread_is_readable(metadata):
raise HTTPException(404, "thread not found")
return {"comments": await list_plan_comments(thread_id)}
@plan_router.post("/{thread_id}/comments")
async def post_plan_comment(
thread_id: str, body: CommentBody, session: dict[str, Any] = _SESSION_DEP
) -> dict[str, Any]:
metadata = await _thread_metadata(thread_id)
if not _thread_is_readable(metadata):
raise HTTPException(404, "thread not found")
_reject_shared_content(await get_plan_content(thread_id) or {})
text = body.body.strip()
if not text:
raise HTTPException(422, "comment body cannot be empty")
login = session["sub"]
return await add_plan_comment(
thread_id, author=session.get("name") or login, author_login=login, body=text
)
@plan_router.delete("/{thread_id}/comments/{comment_id}")
async def remove_plan_comment(
thread_id: str, comment_id: str, session: dict[str, Any] = _SESSION_DEP
) -> dict[str, Any]:
metadata = await _thread_metadata(thread_id)
if not _thread_is_readable(metadata):
raise HTTPException(404, "thread not found")
_reject_shared_content(await get_plan_content(thread_id) or {})
comments = await list_plan_comments(thread_id)
target = next((c for c in comments if c.get("id") == comment_id), None)
if target is None:
raise HTTPException(404, "comment not found")
login = session["sub"]
is_owner = _user_owns_thread(metadata, login, session.get("email"))
if target.get("author_login") != login and not is_owner:
raise HTTPException(403, "only the author or the plan owner can delete a comment")
await delete_plan_comment(thread_id, comment_id)
return {"ok": True}
@plan_router.post("/{thread_id}/approve")
async def approve_plan(thread_id: str, session: dict[str, Any] = _SESSION_DEP) -> dict[str, Any]:
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 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) — read it
# strictly so a transient failure can't silently drop the edit.
content = await get_plan_content(thread_id, raise_on_error=True) or {}
_reject_shared_content(content)
status = content.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 = str(content.get("markdown", "")).strip()
comments = await list_plan_comments(thread_id, raise_on_error=True)
feedback = _format_comments(comments)
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=_substantive_comment_count(comments),
actor=_approval_actor_name(session),
)
return {"status": PLAN_STATUS_APPROVED}
@plan_router.post("/{thread_id}/reject")
async def reject_plan(thread_id: str, session: dict[str, Any] = _SESSION_DEP) -> dict[str, Any]:
metadata = await _thread_metadata(thread_id)
if not _thread_is_readable(metadata):
raise HTTPException(404, "thread not found")
content = await get_plan_content(thread_id, raise_on_error=True) or {}
_reject_shared_content(content)
feedback = _format_comments(await list_plan_comments(thread_id, raise_on_error=True))
await set_plan_status(thread_id, PLAN_STATUS_REVISING, plan_mode=True)
text = (
"The plan needs changes before implementation. Address this reviewer "
"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)
return {"status": PLAN_STATUS_REVISING}
def _reject_shared_content(content: dict[str, Any]) -> None:
if content.get("status") == PLAN_STATUS_SHARED:
raise HTTPException(409, "shared content is not an implementation plan")
def _format_comments(comments: list[dict[str, Any]]) -> str:
lines: list[str] = []
index = 1
for comment in comments:
body = str(comment.get("body", "")).strip()
if not body:
continue
author = str(comment.get("author") or "reviewer").strip()
lines.append(f"{index}. {author}: {body}")
index += 1
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"
def _slack_thread_from_metadata(metadata: dict[str, Any]) -> tuple[str, str] | None:
source_context = metadata.get("source_context")
if not isinstance(source_context, dict):
return None
slack_thread = source_context.get("slack_thread")
if not isinstance(slack_thread, dict):
return None
channel_id = slack_thread.get("channel_id")
thread_ts = slack_thread.get("thread_ts")
if isinstance(channel_id, str) and channel_id and isinstance(thread_ts, str) and thread_ts:
return channel_id, thread_ts
return None
def _plan_approved_slack_text(comment_count: int, actor: str) -> str:
return f"Plan approved with {comment_count} comments by {actor}\nbeginning implementation"
async def _maybe_post_plan_approved_to_slack(
metadata: dict[str, Any], *, comment_count: int, actor: str
) -> None:
slack_thread = _slack_thread_from_metadata(metadata)
if slack_thread is None:
return
channel_id, thread_ts = slack_thread
text = _plan_approved_slack_text(comment_count, actor)
try:
posted = await post_slack_thread_reply(channel_id, thread_ts, text)
except Exception: # noqa: BLE001
logger.warning("Failed to post plan-approval notice to Slack", exc_info=True)
return
if not posted:
logger.warning("Plan-approval Slack notice was not posted (channel=%s)", channel_id)
async def _dispatch_followup(
thread_id: str, metadata: dict[str, Any], text: str, *, plan_mode: bool
) -> None:
"""Continue the existing thread with a new instruction run.
Runs on the same LangGraph thread, so the agent resumes from the checkpoint
with the full planning history plus this instruction. The configurable is
rebuilt from the thread's stored owner/repo/Slack context so the agent can
push, open a PR, and reply in the original channel.
"""
configurable: dict[str, Any] = {
"thread_id": thread_id,
"source": _thread_source(metadata) or "slack",
}
email = metadata.get("triggering_user_email")
if isinstance(email, str) and email:
configurable["user_email"] = email
login = metadata.get("github_login")
if isinstance(login, str) and login:
configurable["github_login"] = login
repo = _repo_config_from_metadata(metadata)
if repo:
configurable["repo"] = repo
source_context = metadata.get("source_context")
if isinstance(source_context, dict):
slack_thread = source_context.get("slack_thread")
if isinstance(slack_thread, dict):
configurable["slack_thread"] = slack_thread
# Carry the decision to the follow-up run: approve continues out of plan
# mode (implement), reject stays in plan mode (revise the plan).
configurable["plan_mode"] = plan_mode
await dispatch_agent_run(
thread_id,
text,
configurable,
source=configurable["source"],
)