mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-04 18:22:10 +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>
387 lines
13 KiB
TypeScript
387 lines
13 KiB
TypeScript
import { type KeyboardEvent, useCallback, useEffect, useState } from "react"
|
|
import { useNavigate } from "@tanstack/react-router"
|
|
|
|
import type { PlanComment, PlanData } from "@/lib/plan"
|
|
import {
|
|
addPlanComment,
|
|
approvePlan,
|
|
deletePlanComment,
|
|
getPlanComments,
|
|
rejectPlan,
|
|
updatePlan,
|
|
} from "@/lib/plan"
|
|
import { Button } from "@/components/ui/button"
|
|
import { Markdown } from "@/components/agents/ported"
|
|
import { useResolvedTheme } from "@/lib/theme"
|
|
|
|
const POLL_MS = 4000
|
|
|
|
// Copy text to the clipboard across browsers: prefer the async Clipboard API
|
|
// (needs a secure context), and fall back to a hidden-textarea + execCommand
|
|
// for older Safari/Firefox and non-HTTPS origins. Returns whether it copied.
|
|
async function copyToClipboard(text: string): Promise<boolean> {
|
|
// The DOM types mark navigator.clipboard required, but it's absent in older
|
|
// browsers and non-secure origins — treat it as optional.
|
|
const nav = navigator as { clipboard?: Clipboard }
|
|
try {
|
|
if (window.isSecureContext && nav.clipboard) {
|
|
await nav.clipboard.writeText(text)
|
|
return true
|
|
}
|
|
} catch {
|
|
/* fall through to the legacy path */
|
|
}
|
|
try {
|
|
const textarea = document.createElement("textarea")
|
|
textarea.value = text
|
|
textarea.setAttribute("readonly", "")
|
|
textarea.style.position = "fixed"
|
|
textarea.style.top = "-9999px"
|
|
document.body.appendChild(textarea)
|
|
textarea.select()
|
|
textarea.setSelectionRange(0, text.length)
|
|
const ok = document.execCommand("copy")
|
|
document.body.removeChild(textarea)
|
|
return ok
|
|
} catch {
|
|
return false
|
|
}
|
|
}
|
|
|
|
export function PlanReview({ plan }: { plan: PlanData }) {
|
|
const navigate = useNavigate()
|
|
const resolvedTheme = useResolvedTheme()
|
|
const [comments, setComments] = useState<Array<PlanComment>>([])
|
|
const [draft, setDraft] = useState("")
|
|
const [posting, setPosting] = useState(false)
|
|
const [decision, setDecision] = useState<string | null>(null)
|
|
const [busy, setBusy] = useState<"approve" | "reject" | null>(null)
|
|
const [error, setError] = useState<string | null>(null)
|
|
const [copied, setCopied] = useState(false)
|
|
// Locally track the displayed markdown so a manual edit shows immediately; the
|
|
// route's query stops polling once content exists, so the prop won't refetch.
|
|
const [markdown, setMarkdown] = useState(plan.markdown)
|
|
const [editing, setEditing] = useState(false)
|
|
const [editDraft, setEditDraft] = useState(plan.markdown)
|
|
const [saving, setSaving] = useState(false)
|
|
|
|
// Reflect external plan updates (e.g. an agent revision) while not editing.
|
|
useEffect(() => {
|
|
if (!editing) setMarkdown(plan.markdown)
|
|
}, [plan.markdown, editing])
|
|
|
|
const isShared = plan.status === "shared"
|
|
const canEdit =
|
|
plan.isOwner &&
|
|
!isShared &&
|
|
plan.status !== "approved" &&
|
|
plan.status !== "cancelled"
|
|
|
|
const startEditing = useCallback(() => {
|
|
setEditDraft(markdown)
|
|
setEditing(true)
|
|
setError(null)
|
|
}, [markdown])
|
|
|
|
const cancelEditing = useCallback(() => {
|
|
setEditing(false)
|
|
setError(null)
|
|
}, [])
|
|
|
|
const saveEdit = useCallback(async () => {
|
|
const next = editDraft.trim()
|
|
if (!next) {
|
|
setError("The plan cannot be empty.")
|
|
return
|
|
}
|
|
setSaving(true)
|
|
setError(null)
|
|
try {
|
|
const result = await updatePlan(plan.threadId, next)
|
|
setMarkdown(result.markdown)
|
|
setEditing(false)
|
|
} catch (e) {
|
|
setError((e as Error).message)
|
|
} finally {
|
|
setSaving(false)
|
|
}
|
|
}, [editDraft, plan.threadId])
|
|
|
|
// Poll so reviewers see each other's comments without a realtime transport.
|
|
useEffect(() => {
|
|
let cancelled = false
|
|
const load = async () => {
|
|
try {
|
|
const next = await getPlanComments(plan.threadId)
|
|
if (!cancelled) setComments(next)
|
|
} catch {
|
|
/* transient; next tick retries */
|
|
}
|
|
}
|
|
if (isShared) return
|
|
void load()
|
|
const timer = setInterval(load, POLL_MS)
|
|
return () => {
|
|
cancelled = true
|
|
clearInterval(timer)
|
|
}
|
|
}, [isShared, plan.threadId])
|
|
|
|
const submitComment = useCallback(async () => {
|
|
const body = draft.trim()
|
|
if (!body) return
|
|
setPosting(true)
|
|
setError(null)
|
|
try {
|
|
const created = await addPlanComment(plan.threadId, body)
|
|
setComments((prev) => [...prev, created])
|
|
setDraft("")
|
|
} catch (e) {
|
|
setError((e as Error).message)
|
|
} finally {
|
|
setPosting(false)
|
|
}
|
|
}, [draft, plan.threadId])
|
|
|
|
const handleCommentKeyDown = useCallback(
|
|
(event: KeyboardEvent<HTMLTextAreaElement>) => {
|
|
if (event.key !== "Enter" || (!event.metaKey && !event.ctrlKey)) return
|
|
event.preventDefault()
|
|
if (posting || !draft.trim()) return
|
|
void submitComment()
|
|
},
|
|
[draft, posting, submitComment]
|
|
)
|
|
|
|
const removeComment = useCallback(
|
|
async (id: string) => {
|
|
try {
|
|
await deletePlanComment(plan.threadId, id)
|
|
setComments((prev) => prev.filter((c) => c.id !== id))
|
|
} catch (e) {
|
|
setError((e as Error).message)
|
|
}
|
|
},
|
|
[plan.threadId]
|
|
)
|
|
|
|
const decide = useCallback(
|
|
async (kind: "approve" | "reject") => {
|
|
setBusy(kind)
|
|
setError(null)
|
|
try {
|
|
if (kind === "approve") {
|
|
await approvePlan(plan.threadId)
|
|
await navigate({
|
|
to: "/agents/$threadId",
|
|
params: { threadId: plan.threadId },
|
|
})
|
|
return
|
|
}
|
|
await rejectPlan(plan.threadId)
|
|
setDecision("Changes requested — the agent is revising the plan.")
|
|
} catch (e) {
|
|
setError((e as Error).message)
|
|
} finally {
|
|
setBusy(null)
|
|
}
|
|
},
|
|
[navigate, plan.threadId]
|
|
)
|
|
|
|
const copyPlan = useCallback(async () => {
|
|
setError(null)
|
|
if (await copyToClipboard(markdown)) {
|
|
setCopied(true)
|
|
window.setTimeout(() => setCopied(false), 1500)
|
|
} else {
|
|
setError("Couldn't copy the plan to the clipboard.")
|
|
}
|
|
}, [markdown])
|
|
|
|
return (
|
|
<div
|
|
data-testid="plan-review"
|
|
className="flex min-h-0 flex-1 flex-col bg-[var(--ui-bg)] text-[var(--ui-text)]"
|
|
>
|
|
<div className="flex flex-col gap-3 border-b border-[var(--ui-border)] px-4 py-3 md:flex-row md:items-center md:justify-between md:gap-4 md:px-6">
|
|
<div className="min-w-0">
|
|
<h1 className="text-base font-semibold text-[var(--ui-text)]">
|
|
{isShared ? "Shared response" : "Implementation plan"}
|
|
</h1>
|
|
<p className="text-xs text-[var(--ui-text-dim)]">
|
|
{isShared ? "Viewing" : "Reviewing"} as {plan.user.name}
|
|
{plan.isOwner ? " (owner)" : ""} · status:{" "}
|
|
<span data-testid="plan-status">{plan.status}</span>
|
|
</p>
|
|
</div>
|
|
<div className="flex min-w-0 flex-wrap items-center gap-2 md:shrink-0 md:justify-end">
|
|
{decision && (
|
|
<span
|
|
data-testid="plan-decision"
|
|
className="w-full text-xs text-[var(--ui-text-dim)] md:w-auto"
|
|
>
|
|
{decision}
|
|
</span>
|
|
)}
|
|
<Button
|
|
data-testid="copy-plan"
|
|
variant="secondary"
|
|
disabled={!markdown.trim()}
|
|
onClick={() => void copyPlan()}
|
|
>
|
|
{copied ? "Copied!" : "Copy markdown"}
|
|
</Button>
|
|
{canEdit && (
|
|
<Button
|
|
data-testid="edit-plan"
|
|
variant="secondary"
|
|
disabled={busy !== null || decision !== null}
|
|
onClick={startEditing}
|
|
>
|
|
Edit
|
|
</Button>
|
|
)}
|
|
{!isShared && plan.isOwner && (
|
|
<Button
|
|
data-testid="approve-plan"
|
|
disabled={busy !== null || decision !== null}
|
|
onClick={() => void decide("approve")}
|
|
>
|
|
Approve
|
|
</Button>
|
|
)}
|
|
{!isShared && (
|
|
<Button
|
|
data-testid="reject-plan"
|
|
variant="secondary"
|
|
// Requesting changes feeds the comments to the agent, so it's
|
|
// meaningless with none — disable until at least one is left.
|
|
disabled={busy !== null || decision !== null || comments.length === 0}
|
|
title={
|
|
comments.length === 0
|
|
? "Leave a comment first to request changes"
|
|
: undefined
|
|
}
|
|
onClick={() => void decide("reject")}
|
|
>
|
|
Request changes
|
|
</Button>
|
|
)}
|
|
</div>
|
|
</div>
|
|
|
|
<div className="flex min-h-0 flex-1 flex-col overflow-y-auto md:flex-row md:overflow-hidden">
|
|
<div
|
|
className="min-w-0 px-4 py-4 md:min-h-0 md:flex-1 md:overflow-auto md:px-6"
|
|
data-testid="plan-document"
|
|
data-color-scheme={resolvedTheme}
|
|
>
|
|
{editing ? (
|
|
<div className="flex h-full flex-col gap-3">
|
|
<textarea
|
|
data-testid="plan-edit-textarea"
|
|
value={editDraft}
|
|
onChange={(e) => setEditDraft(e.target.value)}
|
|
className="min-h-0 flex-1 resize-none rounded-md border border-[var(--ui-border)] bg-[var(--ui-bg)] p-3 font-mono text-sm text-[var(--ui-text)] outline-none focus:border-[var(--ui-accent)]"
|
|
placeholder="Write the plan in Markdown…"
|
|
/>
|
|
<div className="flex shrink-0 justify-end gap-2">
|
|
<Button
|
|
data-testid="plan-edit-cancel"
|
|
variant="secondary"
|
|
disabled={saving}
|
|
onClick={cancelEditing}
|
|
>
|
|
Cancel
|
|
</Button>
|
|
<Button
|
|
data-testid="plan-edit-save"
|
|
disabled={saving || !editDraft.trim()}
|
|
onClick={() => void saveEdit()}
|
|
>
|
|
{saving ? "Saving…" : "Save"}
|
|
</Button>
|
|
</div>
|
|
</div>
|
|
) : markdown.trim() ? (
|
|
<Markdown content={markdown} />
|
|
) : (
|
|
<p className="text-sm text-[var(--ui-text-dim)]">
|
|
The plan hasn't been written yet.
|
|
</p>
|
|
)}
|
|
</div>
|
|
|
|
{!isShared && (<aside className="flex shrink-0 flex-col border-t border-[var(--ui-border)] md:w-80 md:border-t-0 md:border-l">
|
|
<div className="border-b border-[var(--ui-border)] px-4 py-3">
|
|
<h2 className="text-sm font-semibold text-[var(--ui-text)]">
|
|
Comments
|
|
</h2>
|
|
</div>
|
|
<div
|
|
className="max-h-80 space-y-3 overflow-auto px-4 py-3 md:max-h-none md:min-h-0 md:flex-1"
|
|
data-testid="plan-comments"
|
|
>
|
|
{comments.length === 0 ? (
|
|
<p className="text-xs text-[var(--ui-text-dim)]">
|
|
No comments yet.
|
|
</p>
|
|
) : (
|
|
comments.map((c) => (
|
|
<div
|
|
key={c.id}
|
|
data-testid="plan-comment"
|
|
className="rounded-md border border-[var(--ui-border)] bg-[var(--ui-panel)] px-3 py-2"
|
|
>
|
|
<div className="flex items-center justify-between gap-2">
|
|
<span className="text-xs font-medium text-[var(--ui-text)]">
|
|
{c.author}
|
|
</span>
|
|
<button
|
|
type="button"
|
|
data-testid="comment-delete"
|
|
className="text-xs text-[var(--ui-text-dim)] hover:text-[var(--ui-text)]"
|
|
onClick={() => void removeComment(c.id)}
|
|
>
|
|
Delete
|
|
</button>
|
|
</div>
|
|
<p className="mt-1 text-sm whitespace-pre-wrap text-[var(--ui-text)]">
|
|
{c.body}
|
|
</p>
|
|
</div>
|
|
))
|
|
)}
|
|
</div>
|
|
<div className="border-t border-[var(--ui-border)] p-3">
|
|
{error && (
|
|
<p className="mb-2 text-xs text-[color:var(--ui-danger)]">
|
|
{error}
|
|
</p>
|
|
)}
|
|
<textarea
|
|
data-testid="comment-input"
|
|
value={draft}
|
|
onChange={(e) => setDraft(e.target.value)}
|
|
onKeyDown={handleCommentKeyDown}
|
|
placeholder="Leave a comment on the plan"
|
|
rows={3}
|
|
className="w-full resize-none rounded-md border border-[var(--ui-border)] bg-[var(--ui-bg)] px-2 py-1.5 text-sm text-[var(--ui-text)] outline-none focus:border-[var(--ui-accent)]"
|
|
/>
|
|
<div className="mt-2 flex justify-end">
|
|
<Button
|
|
data-testid="comment-submit"
|
|
size="sm"
|
|
disabled={posting || !draft.trim()}
|
|
onClick={() => void submitComment()}
|
|
>
|
|
Comment
|
|
</Button>
|
|
</div>
|
|
</div>
|
|
</aside>)}
|
|
</div>
|
|
</div>
|
|
)
|
|
}
|