mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-09-30 09:13:14 +00:00
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 (#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>
226 lines
7.9 KiB
Python
226 lines
7.9 KiB
Python
"""Persistence for the plan-review feature.
|
|
|
|
The plan lives in two places:
|
|
- 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
|
|
``["plan", "comments", thread_id]`` so listing and deletion are simple plain
|
|
store operations (no CRDT/WebSocket).
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import logging
|
|
import re
|
|
import uuid
|
|
from datetime import UTC, datetime
|
|
from typing import Any
|
|
|
|
from langgraph_sdk import get_client
|
|
|
|
logger = logging.getLogger(__name__)
|
|
|
|
PLAN_CONTENT_NAMESPACE = ["plan", "content"]
|
|
PLAN_COMMENTS_NAMESPACE = ["plan", "comments"]
|
|
|
|
# Plans are mirrored into the sandbox outside cloned repositories.
|
|
PLAN_FILE_DIRECTORY = "/workspace/plans"
|
|
|
|
# Plan/share lifecycle, stored on both the content record and the thread metadata.
|
|
PLAN_STATUS_PLANNING = "planning"
|
|
PLAN_STATUS_READY = "ready"
|
|
PLAN_STATUS_SHARED = "shared"
|
|
PLAN_STATUS_REVISING = "revising"
|
|
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()
|
|
|
|
|
|
def _item_value(item: Any) -> dict[str, Any] | None:
|
|
if item is None:
|
|
return None
|
|
value = item.get("value") if isinstance(item, dict) else getattr(item, "value", 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,
|
|
plan_mode: bool | None = True,
|
|
) -> None:
|
|
"""Publish markdown + status for the dashboard to render.
|
|
|
|
A republished (revised) plan supersedes the prior revision, so comments left
|
|
on it are cleared — otherwise stale feedback would resurface on the new plan
|
|
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,
|
|
record,
|
|
)
|
|
if clear_comments:
|
|
try:
|
|
await clear_plan_comments(thread_id)
|
|
except Exception:
|
|
# Best-effort: a failed cleanup must not block publishing the new plan.
|
|
pass
|
|
metadata: dict[str, Any] = {"plan_status": status}
|
|
if plan_mode is not None:
|
|
metadata["plan_mode"] = plan_mode
|
|
await _merge_thread_metadata(thread_id, metadata)
|
|
|
|
|
|
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(path, content)
|
|
return path
|
|
except Exception:
|
|
logger.warning("Could not write plan file to sandbox for %s", thread_id, exc_info=True)
|
|
return path
|
|
|
|
|
|
async def get_plan_content(
|
|
thread_id: str, *, raise_on_error: bool = False
|
|
) -> dict[str, Any] | None:
|
|
"""The published plan record, or ``None`` when none exists.
|
|
|
|
With ``raise_on_error=True`` a store failure propagates instead of resolving
|
|
to ``None``. Approve uses this so a transient failure aborts the decision
|
|
rather than dispatching the agent without the (possibly edited) plan."""
|
|
client = _client()
|
|
try:
|
|
item = await client.store.get_item(PLAN_CONTENT_NAMESPACE, thread_id)
|
|
except Exception:
|
|
if raise_on_error:
|
|
raise
|
|
return None
|
|
return _item_value(item)
|
|
|
|
|
|
async def set_plan_status(thread_id: str, status: str, *, plan_mode: bool | None = None) -> None:
|
|
"""Update the plan lifecycle status on both the content record and metadata."""
|
|
existing = await get_plan_content(thread_id) or {}
|
|
entering_plan_after_share = (
|
|
existing.get("status") == PLAN_STATUS_SHARED and status == PLAN_STATUS_PLANNING
|
|
)
|
|
client = _client()
|
|
record: dict[str, Any] = {
|
|
"markdown": "" if entering_plan_after_share else existing.get("markdown", ""),
|
|
"status": status,
|
|
}
|
|
plan_file_path = existing.get("plan_file_path")
|
|
if not entering_plan_after_share and 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,
|
|
record,
|
|
)
|
|
metadata: dict[str, Any] = {"plan_status": status}
|
|
if plan_mode is not None:
|
|
metadata["plan_mode"] = plan_mode
|
|
await _merge_thread_metadata(thread_id, metadata)
|
|
|
|
|
|
def _comments_namespace(thread_id: str) -> list[str]:
|
|
return [*PLAN_COMMENTS_NAMESPACE, thread_id]
|
|
|
|
|
|
async def list_plan_comments(
|
|
thread_id: str, *, raise_on_error: bool = False
|
|
) -> list[dict[str, Any]]:
|
|
"""All comments on a plan, oldest first.
|
|
|
|
With ``raise_on_error=True`` a store/search failure propagates instead of
|
|
resolving to ``[]``. Approve/reject use this so a transient failure surfaces
|
|
(the decision endpoint errors) rather than silently feeding the agent an
|
|
empty comment set and dropping the reviewer's feedback."""
|
|
client = _client()
|
|
try:
|
|
items = await client.store.search_items(_comments_namespace(thread_id), limit=1000)
|
|
except Exception:
|
|
if raise_on_error:
|
|
raise
|
|
return []
|
|
raw = items.get("items", []) if isinstance(items, dict) else getattr(items, "items", [])
|
|
comments = [v for v in (_item_value(item) for item in raw) if v]
|
|
comments.sort(key=lambda c: str(c.get("created_at", "")))
|
|
return comments
|
|
|
|
|
|
async def clear_plan_comments(thread_id: str) -> None:
|
|
"""Delete every comment on a thread (called when a revised plan is published)."""
|
|
for comment in await list_plan_comments(thread_id):
|
|
comment_id = comment.get("id")
|
|
if isinstance(comment_id, str) and comment_id:
|
|
await delete_plan_comment(thread_id, comment_id)
|
|
|
|
|
|
async def add_plan_comment(
|
|
thread_id: str, *, author: str, author_login: str, body: str
|
|
) -> dict[str, Any]:
|
|
"""Append a whole-document comment; returns the stored comment."""
|
|
comment = {
|
|
"id": uuid.uuid4().hex,
|
|
"author": author,
|
|
"author_login": author_login,
|
|
"body": body,
|
|
"created_at": datetime.now(UTC).isoformat(),
|
|
}
|
|
await _client().store.put_item(_comments_namespace(thread_id), comment["id"], comment)
|
|
return comment
|
|
|
|
|
|
async def delete_plan_comment(thread_id: str, comment_id: str) -> None:
|
|
await _client().store.delete_item(_comments_namespace(thread_id), comment_id)
|
|
|
|
|
|
async def _merge_thread_metadata(thread_id: str, metadata: dict[str, Any]) -> None:
|
|
client = _client()
|
|
try:
|
|
await client.threads.update(thread_id=thread_id, metadata=metadata)
|
|
except Exception:
|
|
# The thread always exists by the time a plan is saved (the run created
|
|
# it); a transient update failure must not crash the agent mid-run.
|
|
pass
|