diff --git a/INSTALLATION.md b/INSTALLATION.md index 419c663f..b601e107 100644 --- a/INSTALLATION.md +++ b/INSTALLATION.md @@ -78,6 +78,7 @@ Write this down. You'll use it in the callback URL below and again in step 4 whe - Issues: Read & write - Checks: Read & write — reports an "Open SWE Review" check run on PRs while an auto-review runs, and reads third-party CI conclusions for the auto-fix flow (it watches failing checks on agent-authored PRs and pushes fixes). Without it, check-run creation fails (logged, best-effort) but reviews still work, and CI auto-fix is disabled. - Commit statuses: Read-only — only needed if you enable the `Status` event below; the CI auto-fix flow reads the legacy combined commit-status API for integrations that report via statuses instead of check runs. Without it, status-based CI is silently ignored (logged as "Failed to read combined status"). + - Workflows: Read & write — required to let Open SWE push branches containing GitHub Actions workflow changes after explicit human approval. Runtime sandbox tokens are still minted without this permission by default and are elevated only around an approved workflow push. - Metadata: Read-only - **Organization permissions** (required only if you plan to set `ALLOWED_GITHUB_ORGS` — see step 5 / Security): - Members: Read-only — used to verify org membership for the dashboard-login gate via `GET /orgs/{org}/memberships/{username}`. Without this permission that call returns 403, the check fails closed, and **every** dashboard login is rejected. diff --git a/agent/dashboard/workflow_approval.py b/agent/dashboard/workflow_approval.py new file mode 100644 index 00000000..7652f08a --- /dev/null +++ b/agent/dashboard/workflow_approval.py @@ -0,0 +1,119 @@ +"""Workflow-file push approval state.""" + +from __future__ import annotations + +from collections.abc import Mapping +from datetime import UTC, datetime +from typing import Any + +from langgraph_sdk import get_client + +WORKFLOW_PUSH_APPROVALS_KEY = "workflow_push_approvals" +WORKFLOW_APPROVAL_PENDING = "pending" +WORKFLOW_APPROVAL_APPROVED = "approved" +WORKFLOW_APPROVAL_REJECTED = "rejected" +_MAX_APPROVAL_RECORDS = 20 + + +def _now() -> str: + return datetime.now(UTC).isoformat() + + +def _approvals_from_metadata(metadata: Mapping[str, Any] | None) -> dict[str, dict[str, Any]]: + raw = metadata.get(WORKFLOW_PUSH_APPROVALS_KEY) if metadata else None + if not isinstance(raw, dict): + return {} + approvals: dict[str, dict[str, Any]] = {} + for fingerprint, value in raw.items(): + if isinstance(fingerprint, str) and fingerprint and isinstance(value, dict): + record = dict(value) + record.setdefault("fingerprint", fingerprint) + approvals[fingerprint] = record + return approvals + + +async def get_workflow_push_approvals(thread_id: str) -> dict[str, dict[str, Any]]: + client = get_client() + thread = await client.threads.get(thread_id) + metadata = thread.get("metadata") if isinstance(thread, dict) else None + return _approvals_from_metadata(metadata if isinstance(metadata, dict) else None) + + +async def workflow_push_approved(thread_id: str, fingerprint: str) -> bool: + approvals = await get_workflow_push_approvals(thread_id) + return approvals.get(fingerprint, {}).get("status") == WORKFLOW_APPROVAL_APPROVED + + +async def ensure_workflow_push_pending( + thread_id: str, + *, + fingerprint: str, + repo: str, + branch: str, + base_sha: str, + head_sha: str, + files: list[str], +) -> tuple[dict[str, Any], bool]: + """Store a pending approval unless a terminal record already exists.""" + approvals = await get_workflow_push_approvals(thread_id) + existing = approvals.get(fingerprint) + if existing and existing.get("status") in { + WORKFLOW_APPROVAL_PENDING, + WORKFLOW_APPROVAL_APPROVED, + WORKFLOW_APPROVAL_REJECTED, + }: + return existing, False + + record = { + "fingerprint": fingerprint, + "status": WORKFLOW_APPROVAL_PENDING, + "repo": repo, + "branch": branch, + "base_sha": base_sha, + "head_sha": head_sha, + "files": files, + "requested_at": _now(), + "notified": False, + } + approvals[fingerprint] = record + await _save_approvals(thread_id, approvals) + return record, True + + +async def mark_workflow_push_notified(thread_id: str, fingerprint: str) -> None: + approvals = await get_workflow_push_approvals(thread_id) + record = approvals.get(fingerprint) + if not record: + return + record["notified"] = True + record["notified_at"] = _now() + approvals[fingerprint] = record + await _save_approvals(thread_id, approvals) + + +async def decide_workflow_push_approval( + thread_id: str, + fingerprint: str, + *, + approved: bool, + actor: str, +) -> dict[str, Any] | None: + approvals = await get_workflow_push_approvals(thread_id) + record = approvals.get(fingerprint) + if not record: + return None + record["status"] = WORKFLOW_APPROVAL_APPROVED if approved else WORKFLOW_APPROVAL_REJECTED + record["decided_at"] = _now() + record["decided_by"] = actor + approvals[fingerprint] = record + await _save_approvals(thread_id, approvals) + return record + + +async def _save_approvals(thread_id: str, approvals: dict[str, dict[str, Any]]) -> None: + ordered = sorted(approvals.values(), key=lambda r: str(r.get("requested_at", ""))) + trimmed = ordered[-_MAX_APPROVAL_RECORDS:] + await get_client().threads.update( + thread_id=thread_id, + metadata={WORKFLOW_PUSH_APPROVALS_KEY: {str(r["fingerprint"]): r for r in trimmed}}, + ) diff --git a/agent/dashboard/workflow_approval_api.py b/agent/dashboard/workflow_approval_api.py new file mode 100644 index 00000000..7e30fa11 --- /dev/null +++ b/agent/dashboard/workflow_approval_api.py @@ -0,0 +1,55 @@ +"""REST API for approving workflow-file pushes.""" + +from __future__ import annotations + +from typing import Any + +from fastapi import APIRouter, Depends, HTTPException + +from .oauth import require_same_origin_for_mutations, require_session +from .plan_api import _dispatch_followup, _thread_metadata +from .thread_api import _user_owns_thread +from .workflow_approval import decide_workflow_push_approval + +workflow_approval_router = APIRouter( + prefix="/dashboard/api/workflow-approval", + tags=["workflow-approval"], + dependencies=[Depends(require_same_origin_for_mutations)], +) +_SESSION_DEP = Depends(require_session) + + +@workflow_approval_router.post("/{thread_id}/{fingerprint}/approve") +async def approve_workflow_push( + thread_id: str, fingerprint: 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 thread owner can approve workflow pushes") + record = await decide_workflow_push_approval( + thread_id, fingerprint, approved=True, actor=session["sub"] + ) + if record is None: + raise HTTPException(404, "workflow push approval not found") + await _dispatch_followup( + thread_id, + metadata, + "The workflow-file push approval was approved. Retry the blocked git push now; do not alter workflow files before pushing.", + plan_mode=False, + ) + return {"status": "approved", "fingerprint": fingerprint} + + +@workflow_approval_router.post("/{thread_id}/{fingerprint}/reject") +async def reject_workflow_push( + thread_id: str, fingerprint: 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 thread owner can reject workflow pushes") + record = await decide_workflow_push_approval( + thread_id, fingerprint, approved=False, actor=session["sub"] + ) + if record is None: + raise HTTPException(404, "workflow push approval not found") + return {"status": "rejected", "fingerprint": fingerprint} diff --git a/agent/middleware/__init__.py b/agent/middleware/__init__.py index cb1c3af2..6fdebb32 100644 --- a/agent/middleware/__init__.py +++ b/agent/middleware/__init__.py @@ -12,6 +12,7 @@ from .sanitize_tool_inputs import SanitizeToolInputsMiddleware from .settle_review_check import settle_review_check_on_exit from .tool_artifact import ToolArtifactMiddleware from .tool_error_handler import ToolErrorMiddleware +from .workflow_push_guard import WorkflowPushGuardMiddleware __all__ = [ "ExcludeToolsMiddleware", @@ -22,6 +23,7 @@ __all__ = [ "SanitizeToolInputsMiddleware", "ToolArtifactMiddleware", "ToolErrorMiddleware", + "WorkflowPushGuardMiddleware", "SandboxCircuitBreakerMiddleware", "SlackAssistantStatusMiddleware", "check_message_queue_before_model", diff --git a/agent/middleware/workflow_push_guard.py b/agent/middleware/workflow_push_guard.py new file mode 100644 index 00000000..0366105e --- /dev/null +++ b/agent/middleware/workflow_push_guard.py @@ -0,0 +1,544 @@ +"""Gate workflow-file pushes on human approval.""" + +from __future__ import annotations + +import asyncio +import hashlib +import json +import logging +import re +import shlex +import threading +from collections.abc import Awaitable, Callable, Mapping +from dataclasses import dataclass +from typing import Any + +from langchain.agents.middleware.types import AgentMiddleware, AgentState +from langchain_core.messages import ToolMessage +from langgraph.config import get_config +from langgraph.prebuilt.tool_node import ToolCallRequest +from langgraph.types import Command + +from ..dashboard.workflow_approval import ( + ensure_workflow_push_pending, + mark_workflow_push_notified, + workflow_push_approved, +) +from ..tools.slack_thread_reply import build_workflow_approval_blocks +from ..utils.github_app import ( + RUNTIME_PROXY_TOKEN_PERMISSIONS, + WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS, +) +from ..utils.github_proxy import refresh_proxy_token +from ..utils.sandbox_state import SANDBOX_BACKENDS +from ..utils.slack import post_slack_thread_reply_with_ts + +logger = logging.getLogger(__name__) + +_WORKFLOW_PREFIX = ".github/workflows/" +_SHELL_OPERATORS = {";", "|", "||", "&"} +_REF_NAME = re.compile(r"^[A-Za-z0-9._/@+-]+$") +_GIT_OBJECT_ID = re.compile(r"^[0-9a-fA-F]{40,64}$") +_UNSAFE_RAW_COMMAND = re.compile(r"[;|`$<>\n\r]") + + +@dataclass(frozen=True) +class ParsedGitPush: + repo_dir: str | None + remote: str + local_ref: str + remote_ref: str + set_upstream: bool = False + + +@dataclass(frozen=True) +class WorkflowPushChange: + fingerprint: str + repo: str + branch: str + base_sha: str + head_sha: str + files: list[str] + remote: str + local_ref: str + remote_ref: str + fixed_command: str + + +@dataclass(frozen=True) +class GitInspectResult: + output: str + ok: bool + + +def _tool_name(request: ToolCallRequest) -> str | None: + tool_call = getattr(request, "tool_call", None) + if isinstance(tool_call, Mapping): + name = tool_call.get("name") + return name if isinstance(name, str) else None + return None + + +def _tool_args(request: ToolCallRequest) -> dict[str, Any]: + tool_call = getattr(request, "tool_call", None) + args = tool_call.get("args") if isinstance(tool_call, Mapping) else None + return dict(args) if isinstance(args, Mapping) else {} + + +def _tool_call_id(request: ToolCallRequest) -> str | None: + tool_call = getattr(request, "tool_call", None) + if isinstance(tool_call, Mapping): + value = tool_call.get("id") + return value if isinstance(value, str) else None + return None + + +def _config(request: ToolCallRequest) -> Mapping[str, Any]: + runtime_config = getattr(getattr(request, "runtime", None), "config", None) + if isinstance(runtime_config, Mapping): + return runtime_config + try: + config = get_config() + except Exception: + return {} + return config if isinstance(config, Mapping) else {} + + +def _configurable(request: ToolCallRequest) -> Mapping[str, Any]: + config = _config(request) + configurable = config.get("configurable") + return configurable if isinstance(configurable, Mapping) else {} + + +def _thread_id(request: ToolCallRequest) -> str | None: + thread_id = _configurable(request).get("thread_id") + return thread_id if isinstance(thread_id, str) and thread_id else None + + +def _backend(thread_id: str | None) -> Any | None: + return SANDBOX_BACKENDS.get(thread_id) if thread_id else None + + +def _response_output(response: Any) -> str: + output = getattr(response, "output", None) + if isinstance(output, str): + return output + if isinstance(response, Mapping): + value = response.get("output") + if isinstance(value, str): + return value + return str(response or "") + + +def _response_ok(response: Any) -> bool: + exit_code = getattr(response, "exit_code", None) + if isinstance(exit_code, int): + return exit_code == 0 + if isinstance(response, Mapping): + value = response.get("exit_code") + if isinstance(value, int): + return value == 0 + return True + + +def _parse_git_push(command: str) -> ParsedGitPush | None: + stripped = command.strip() + if _UNSAFE_RAW_COMMAND.search(stripped) or "&" in stripped.replace("&&", ""): + return None + try: + tokens = shlex.split(stripped) + except ValueError: + return None + if not tokens: + return None + + if len(tokens) >= 4 and tokens[0] == "cd" and tokens[2] == "&&": + if any(token in _SHELL_OPERATORS or token == "&&" for token in tokens[3:]): + return None + return _parse_git_tokens(tokens[3:], repo_dir=tokens[1]) + + if any(token in _SHELL_OPERATORS or token == "&&" for token in tokens): + return None + return _parse_git_tokens(tokens, repo_dir=None) + + +def _parse_git_tokens(tokens: list[str], *, repo_dir: str | None) -> ParsedGitPush | None: + if not tokens or tokens[0] != "git": + return None + i = 1 + while i < len(tokens) and tokens[i] != "push": + if tokens[i] == "-C" and i + 1 < len(tokens): + repo_dir = tokens[i + 1] + i += 2 + continue + return None + if i >= len(tokens) or tokens[i] != "push": + return None + return _parse_push_args(tokens[i + 1 :], repo_dir=repo_dir) + + +def _parse_push_args(tokens: list[str], *, repo_dir: str | None) -> ParsedGitPush | None: + set_upstream = False + while tokens and tokens[0] in {"-u", "--set-upstream"}: + set_upstream = True + tokens = tokens[1:] + if len(tokens) != 2 or tokens[0] != "origin": + return None + parsed = _parse_refspec(tokens[1]) + if parsed is None: + return None + local_ref, remote_ref = parsed + return ParsedGitPush( + repo_dir=repo_dir, + remote="origin", + local_ref=local_ref, + remote_ref=remote_ref, + set_upstream=set_upstream, + ) + + +def _parse_refspec(refspec: str) -> tuple[str, str] | None: + if refspec.startswith("-") or ".." in refspec: + return None + if ":" in refspec: + parts = refspec.split(":") + if len(parts) != 2 or not parts[0] or not parts[1]: + return None + local_ref, remote_ref = parts + else: + local_ref = remote_ref = refspec + if not _safe_ref(local_ref, allow_head=True) or not _safe_ref(remote_ref, allow_head=False): + return None + return local_ref, remote_ref + + +def _safe_ref(ref: str, *, allow_head: bool) -> bool: + if allow_head and ref == "HEAD": + return True + if ref == "HEAD" or not _REF_NAME.fullmatch(ref): + return False + return not any(part in {"", ".", ".."} for part in ref.split("/")) + + +def _git_command(repo_dir: str | None, args: str) -> str: + if repo_dir: + return f"git -C {shlex.quote(repo_dir)} {args}" + return f"git {args}" + + +def _run_git(backend: Any, repo_dir: str | None, args: str) -> GitInspectResult: + try: + response = backend.execute(_git_command(repo_dir, args), timeout=30) + except Exception: + logger.debug("workflow push inspection failed for git %s", args, exc_info=True) + return GitInspectResult("", False) + return GitInspectResult(_response_output(response).strip(), _response_ok(response)) + + +def _first_line(text: str) -> str: + for line in text.splitlines(): + stripped = line.strip() + if stripped: + return stripped + return "" + + +def _normalize_remote(remote: str) -> str: + value = remote.strip() + if value.endswith(".git"): + value = value[:-4] + value = re.sub(r"^https://[^/@]+@github\.com/", "https://github.com/", value) + value = re.sub(r"^git@github\.com:", "https://github.com/", value) + return value + + +def _fingerprint(payload: Mapping[str, Any]) -> str: + encoded = json.dumps(payload, sort_keys=True, separators=(",", ":")).encode("utf-8") + return hashlib.sha256(encoded).hexdigest() + + +def _run_coroutine_sync(coro: Awaitable[ToolMessage | Command]) -> ToolMessage | Command: + try: + asyncio.get_running_loop() + except RuntimeError: + return asyncio.run(coro) + + result: dict[str, ToolMessage | Command | BaseException] = {} + + def target() -> None: + try: + result["value"] = asyncio.run(coro) + except BaseException as exc: # noqa: BLE001 + result["value"] = exc + + thread = threading.Thread(target=target) + thread.start() + thread.join() + value = result["value"] + if isinstance(value, BaseException): + raise value + return value + + +def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPushChange | None: + root_result = _run_git(backend, parsed.repo_dir, "rev-parse --show-toplevel") + if not root_result.ok: + return None + root = _first_line(root_result.output) + if not root: + return None + + branch = _run_git(backend, root, "rev-parse --abbrev-ref HEAD") + branch_name = _first_line(branch.output) if branch.ok else "" + if not branch_name or branch_name == "HEAD" or parsed.remote_ref != branch_name: + return None + if parsed.local_ref not in {"HEAD", branch_name}: + return None + + target_sha = _run_git(backend, root, f"rev-parse {shlex.quote(parsed.local_ref)}") + head = _first_line(target_sha.output) if target_sha.ok else "" + if not head or not _GIT_OBJECT_ID.fullmatch(head): + return None + + remote_branch = f"refs/remotes/{parsed.remote}/{parsed.remote_ref}" + remote_branch_exists = _run_git( + backend, root, f"rev-parse --verify {shlex.quote(remote_branch)}" + ) + if remote_branch_exists.ok and _first_line(remote_branch_exists.output): + base_ref = remote_branch + range_expr = f"{shlex.quote(base_ref)}..{shlex.quote(head)}" + base_sha = _first_line(_run_git(backend, root, f"rev-parse {shlex.quote(base_ref)}").output) + else: + origin_head = _run_git(backend, root, "symbolic-ref --short refs/remotes/origin/HEAD") + base_ref = _first_line(origin_head.output) if origin_head.ok else "origin/main" + range_expr = f"{shlex.quote(base_ref)}...{shlex.quote(head)}" + base_sha = _first_line( + _run_git( + backend, root, f"merge-base {shlex.quote(head)} {shlex.quote(base_ref)}" + ).output + ) + + names = _run_git( + backend, + root, + f"diff --name-only --diff-filter=ACMRTD {range_expr} -- .github/workflows", + ) + if not names.ok: + return None + files = sorted( + line.strip() + for line in names.output.splitlines() + if line.strip().startswith(_WORKFLOW_PREFIX) + ) + if not files: + return None + + diff = _run_git(backend, root, f"diff --binary --full-index {range_expr} -- .github/workflows") + if not diff.ok or not diff.output: + return None + + remote = _run_git(backend, root, "config --get remote.origin.url") + repo = _normalize_remote(_first_line(remote.output)) if remote.ok else "" + fixed_refspec = f"{head}:refs/heads/{parsed.remote_ref}" + fixed_args = ["push"] + if parsed.set_upstream: + fixed_args.append("--set-upstream") + fixed_args.extend([parsed.remote, fixed_refspec]) + fixed_command = _git_command(root, " ".join(shlex.quote(arg) for arg in fixed_args)) + payload = { + "repo": repo, + "branch": branch_name, + "base_sha": base_sha, + "head_sha": head, + "files": files, + "diff": diff.output, + "remote": parsed.remote, + "local_ref": parsed.local_ref, + "remote_ref": parsed.remote_ref, + "fixed_refspec": fixed_refspec, + } + return WorkflowPushChange( + fingerprint=_fingerprint(payload), + repo=repo, + branch=branch_name, + base_sha=base_sha, + head_sha=head, + files=files, + remote=parsed.remote, + local_ref=parsed.local_ref, + remote_ref=parsed.remote_ref, + fixed_command=fixed_command, + ) + + +def _blocked_message(change: WorkflowPushChange, *, already_rejected: bool = False) -> ToolMessage: + status = "rejected" if already_rejected else "approval_required" + content = { + "status": "error", + "error_type": "WorkflowPushApprovalRequired", + "error": ( + "This git push includes GitHub workflow file changes and requires human " + "approval before Open SWE can push it. Retry the same standalone git push " + "after the thread owner approves the workflow diff." + ), + "workflow_approval_status": status, + "fingerprint": change.fingerprint, + "files": change.files, + "repo": change.repo, + "branch": change.branch, + } + return ToolMessage(content=json.dumps(content), tool_call_id="", status="error") + + +def _tool_message_for_request(message: ToolMessage, request: ToolCallRequest) -> ToolMessage: + message.tool_call_id = _tool_call_id(request) + return message + + +def _override_execute_command(request: ToolCallRequest, command: str) -> ToolCallRequest: + tool_call = getattr(request, "tool_call", None) + if not isinstance(tool_call, Mapping): + return request + args = dict(_tool_args(request)) + args["command"] = command + return request.override(tool_call={**dict(tool_call), "args": args}) + + +def _approval_slack_message(change: WorkflowPushChange) -> str: + files = "\n".join(f"• `{path}`" for path in change.files[:10]) + if len(change.files) > 10: + files += f"\n• …and {len(change.files) - 10} more" + repo = change.repo or "the repository" + branch = change.branch or "the current branch" + return ( + "*Workflow file approval required*\n" + f"Open SWE is trying to push changes to GitHub workflow files in `{repo}` on `{branch}`.\n\n" + f"*Files:*\n{files}\n\n" + f"*Fingerprint:* `{change.fingerprint}`\n\n" + "Approve only if this exact workflow diff is expected. If the workflow files change, " + "a new fingerprint will be required." + ) + + +async def _post_slack_approval_if_needed( + request: ToolCallRequest, change: WorkflowPushChange, record: Mapping[str, Any] +) -> None: + if record.get("notified") is True: + return + configurable = _configurable(request) + slack_thread = configurable.get("slack_thread") + if not isinstance(slack_thread, Mapping): + return + channel_id = slack_thread.get("channel_id") + thread_ts = slack_thread.get("thread_ts") + if not isinstance(channel_id, str) or not isinstance(thread_ts, str): + return + message = _approval_slack_message(change) + message_ts, error = await post_slack_thread_reply_with_ts( + channel_id, + thread_ts, + message, + blocks=build_workflow_approval_blocks(message, change.fingerprint), + ) + if message_ts and not error: + thread_id = _thread_id(request) + if thread_id: + await mark_workflow_push_notified(thread_id, change.fingerprint) + + +async def _approval_state(request: ToolCallRequest, change: WorkflowPushChange) -> str: + thread_id = _thread_id(request) + if not thread_id: + return "missing_thread" + try: + if await workflow_push_approved(thread_id, change.fingerprint): + return "approved" + record, _created = await ensure_workflow_push_pending( + thread_id, + fingerprint=change.fingerprint, + repo=change.repo, + branch=change.branch, + base_sha=change.base_sha, + head_sha=change.head_sha, + files=change.files, + ) + await _post_slack_approval_if_needed(request, change, record) + return str(record.get("status") or "pending") + except Exception: + logger.exception("Failed to read or write workflow push approval state") + return "approval_error" + + +async def _run_with_workflow_token( + thread_id: str, + run: Callable[[], Awaitable[ToolMessage | Command]], +) -> ToolMessage | Command: + elevated = await refresh_proxy_token( + thread_id, permissions=WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS + ) + try: + return await run() + finally: + if elevated: + await refresh_proxy_token(thread_id, permissions=RUNTIME_PROXY_TOKEN_PERMISSIONS) + + +class WorkflowPushGuardMiddleware(AgentMiddleware): + """Require approval before pushing `.github/workflows` changes.""" + + state_schema = AgentState + + def _change_for_request(self, request: ToolCallRequest) -> WorkflowPushChange | None: + if _tool_name(request) != "execute": + return None + command = _tool_args(request).get("command") + if not isinstance(command, str): + return None + parsed = _parse_git_push(command) + if parsed is None: + return None + backend = _backend(_thread_id(request)) + if backend is None: + return None + return _workflow_change_for_push(backend, parsed) + + async def _handle_change_async( + self, + request: ToolCallRequest, + handler: Callable[[ToolCallRequest], Awaitable[ToolMessage | Command]], + change: WorkflowPushChange, + ) -> ToolMessage | Command: + thread_id = _thread_id(request) + state = await _approval_state(request, change) + if state == "approved" and thread_id: + safe_request = _override_execute_command(request, change.fixed_command) + return await _run_with_workflow_token(thread_id, lambda: handler(safe_request)) + return _tool_message_for_request( + _blocked_message(change, already_rejected=state == "rejected"), request + ) + + def wrap_tool_call( + self, + request: ToolCallRequest, + handler: Callable[[ToolCallRequest], ToolMessage | Command], + ) -> ToolMessage | Command: + change = self._change_for_request(request) + if change is None: + return handler(request) + + async def run_handler() -> ToolMessage | Command: + return handler(request) + + return _run_coroutine_sync( + self._handle_change_async(request, lambda _request: run_handler(), change) + ) + + async def awrap_tool_call( + self, + request: ToolCallRequest, + handler: Callable[[ToolCallRequest], Awaitable[ToolMessage | Command]], + ) -> ToolMessage | Command: + change = self._change_for_request(request) + if change is None: + return await handler(request) + return await self._handle_change_async(request, handler, change) diff --git a/agent/prompt.py b/agent/prompt.py index 45ee49d0..e748f7a0 100644 --- a/agent/prompt.py +++ b/agent/prompt.py @@ -291,7 +291,7 @@ CODING_STANDARDS_SECTION = """--- - Only install trusted, well-maintained packages. Ensure package manifest files (e.g. pyproject.toml, package.json) are updated to include any new dependency. Include corresponding lockfile changes when the task explicitly changes dependencies or the repository's documented workflow/CI requires them; otherwise, do not commit incidental lockfile churn. - If a command fails (test, build, lint, etc.) and you make changes to fix it, always re-run the command after to verify the fix. - You are NEVER allowed to create backup files. All changes are tracked by git. -- GitHub workflow files (`.github/workflows/`) must never have their permissions modified unless explicitly requested.""" +- GitHub workflow files (`.github/workflows/`) may be changed only when explicitly requested. Any push that includes workflow-file changes requires human approval for the exact workflow diff fingerprint before it can proceed.""" CORE_BEHAVIOR_SECTION = """--- diff --git a/agent/server.py b/agent/server.py index 42e58661..2a63e2c3 100644 --- a/agent/server.py +++ b/agent/server.py @@ -63,6 +63,7 @@ from .middleware import ( SlackAssistantStatusMiddleware, ToolArtifactMiddleware, ToolErrorMiddleware, + WorkflowPushGuardMiddleware, check_message_queue_before_model, notify_step_limit_reached, refresh_github_proxy_before_model, @@ -95,6 +96,8 @@ from .utils.authorship import ( ) from .utils.dashboard_links import dashboard_plan_url, dashboard_thread_url from .utils.github_app import ( + RUNTIME_PROXY_TOKEN_PERMISSIONS, + PermissionMap, get_github_app_installation_token_with_expiry, ) from .utils.github_proxy import record_proxy_token_expiry @@ -180,16 +183,16 @@ async def _start_langsmith_sandbox_if_needed(sandbox_backend: SandboxBackendProt await asyncio.to_thread(sandbox.start) -async def _resolve_proxy_token(github_proxy_token: str | None) -> tuple[str | None, str | None]: - """Resolve the proxy token and its expiry. - - An explicitly supplied token has no known expiry; otherwise we mint a fresh - GitHub App installation token and keep its ``expires_at`` so the proxy can - be refreshed before the (hard 1h) expiry. - """ +async def _resolve_proxy_token( + github_proxy_token: str | None, + *, + permissions: PermissionMap | None = None, +) -> tuple[str | None, str | None]: + """Resolve the proxy token and its expiry.""" if github_proxy_token: return github_proxy_token, None - return await get_github_app_installation_token_with_expiry() + effective_permissions = permissions or RUNTIME_PROXY_TOKEN_PERMISSIONS + return await get_github_app_installation_token_with_expiry(permissions=effective_permissions) async def _resolve_snapshot_id_for_repo(repo: dict[str, str] | None) -> str | None: @@ -227,7 +230,12 @@ async def _create_sandbox_with_proxy( raise ValueError(msg) await _start_langsmith_sandbox_if_needed(sandbox_backend) await asyncio.to_thread(_configure_github_proxy, sandbox_backend.id, token) - record_proxy_token_expiry(thread_id, expires_at, repositories=github_proxy_repositories) + record_proxy_token_expiry( + thread_id, + expires_at, + repositories=github_proxy_repositories, + permissions=None if github_proxy_token else RUNTIME_PROXY_TOKEN_PERMISSIONS, + ) return sandbox_backend @@ -254,7 +262,12 @@ async def _refresh_github_proxy( current_backend = unwrap_sandbox_backend(sandbox_backend) await _start_langsmith_sandbox_if_needed(current_backend) await asyncio.to_thread(_configure_github_proxy, current_backend.id, token) - record_proxy_token_expiry(thread_id, expires_at, repositories=github_proxy_repositories) + record_proxy_token_expiry( + thread_id, + expires_at, + repositories=github_proxy_repositories, + permissions=None if github_proxy_token else RUNTIME_PROXY_TOKEN_PERMISSIONS, + ) async def _refresh_github_proxy_or_recreate( @@ -833,6 +846,7 @@ async def get_agent(config: RunnableConfig) -> Pregel: ModelCallLimitMiddleware(run_limit=MODEL_CALL_RECURSION_LIMIT, exit_behavior="end"), ToolErrorMiddleware(), ToolArtifactMiddleware(), + WorkflowPushGuardMiddleware(), refresh_github_proxy_before_model, check_message_queue_before_model, SlackAssistantStatusMiddleware(), diff --git a/agent/tools/slack_thread_reply.py b/agent/tools/slack_thread_reply.py index 6bd5cb7d..a56ac304 100644 --- a/agent/tools/slack_thread_reply.py +++ b/agent/tools/slack_thread_reply.py @@ -133,6 +133,43 @@ def _build_plan_approval_blocks(message: str) -> list[dict[str, Any]]: ] +def build_workflow_approval_blocks(message: str, fingerprint: str) -> list[dict[str, Any]]: + return [ + {"type": "section", "text": {"type": "mrkdwn", "text": message}}, + { + "type": "actions", + "elements": [ + { + "type": "button", + "text": {"type": "plain_text", "text": "Approve workflow push", "emoji": True}, + "style": "primary", + "value": json.dumps( + { + "type": "workflow_push_approval", + "action": "approve", + "fingerprint": fingerprint, + } + ), + "action_id": "open_swe_option_select", + }, + { + "type": "button", + "text": {"type": "plain_text", "text": "Reject", "emoji": True}, + "style": "danger", + "value": json.dumps( + { + "type": "workflow_push_approval", + "action": "reject", + "fingerprint": fingerprint, + } + ), + "action_id": "open_swe_option_select", + }, + ], + }, + ] + + def _slack_reply_failure_hint(slack_error: str | None) -> str: if slack_error == "msg_too_long": return "Slack rejected the message as too long; retry with a shorter message." diff --git a/agent/utils/github_app.py b/agent/utils/github_app.py index f69041bd..f511a04a 100644 --- a/agent/utils/github_app.py +++ b/agent/utils/github_app.py @@ -5,7 +5,7 @@ from __future__ import annotations import logging import os import time -from collections.abc import Sequence +from collections.abc import Mapping, Sequence from datetime import UTC, datetime, timedelta from typing import Any @@ -24,17 +24,41 @@ GITHUB_APP_INSTALLATION_ID = os.environ.get("GITHUB_APP_INSTALLATION_ID", "") # 5-minute refresh window (``github_proxy.PROXY_TOKEN_REFRESH_WINDOW``) so a # near-expiry proxy refresh still mints a genuinely fresh token. _TOKEN_CACHE_MARGIN = timedelta(minutes=10) +RUNTIME_PROXY_TOKEN_PERMISSIONS: dict[str, str] = { + "contents": "write", + "pull_requests": "write", + "issues": "write", + "checks": "write", +} +WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS: dict[str, str] = { + **RUNTIME_PROXY_TOKEN_PERMISSIONS, + "workflows": "write", +} + +PermissionMap = Mapping[str, str] +PermissionKey = tuple[tuple[str, str], ...] +ScopeKey = tuple[tuple[int, ...], tuple[str, ...], PermissionKey] + # scope key -> (token, expires_at, good_until). In-process only; never persisted. -_TOKEN_CACHE: dict[tuple[tuple[int, ...], tuple[str, ...]], tuple[str, str | None, datetime]] = {} +_TOKEN_CACHE: dict[ScopeKey, tuple[str, str | None, datetime]] = {} + + +def normalize_permissions(permissions: PermissionMap | None) -> PermissionKey: + """Return a stable, hashable permission scope key.""" + if not permissions: + return () + return tuple(sorted((str(k), str(v)) for k, v in permissions.items() if str(k) and str(v))) def _scope_key( - repository_ids: Sequence[int] | None, repositories: Sequence[str] | None -) -> tuple[tuple[int, ...], tuple[str, ...]]: - """Cache key segregating repo-scoped tokens from installation-wide ones.""" + repository_ids: Sequence[int] | None, + repositories: Sequence[str] | None, + permissions: PermissionMap | None = None, +) -> ScopeKey: + """Cache key segregating repo and permission-scoped tokens.""" ids = tuple(sorted(int(i) for i in repository_ids)) if repository_ids else () names = tuple(sorted(str(r) for r in repositories)) if repositories else () - return ids, names + return ids, names, normalize_permissions(permissions) def _parse_expiry(expires_at: Any) -> datetime | None: @@ -53,9 +77,7 @@ def _parse_expiry(expires_at: Any) -> datetime | None: return parsed if parsed.tzinfo else parsed.replace(tzinfo=UTC) -def _cached_token( - key: tuple[tuple[int, ...], tuple[str, ...]], *, now: datetime -) -> tuple[str, str | None] | None: +def _cached_token(key: ScopeKey, *, now: datetime) -> tuple[str, str | None] | None: cached = _TOKEN_CACHE.get(key) if cached is None: return None @@ -87,11 +109,13 @@ async def get_github_app_installation_token( *, repository_ids: Sequence[int] | None = None, repositories: Sequence[str] | None = None, + permissions: PermissionMap | None = None, ) -> str | None: """Exchange the GitHub App JWT for an installation access token.""" token, _ = await get_github_app_installation_token_with_expiry( repository_ids=repository_ids, repositories=repositories, + permissions=permissions, ) return token @@ -100,13 +124,14 @@ async def get_github_app_installation_token_with_expiry( *, repository_ids: Sequence[int] | None = None, repositories: Sequence[str] | None = None, + permissions: PermissionMap | None = None, ) -> tuple[str | None, str | None]: """Exchange the GitHub App JWT for an installation access token and its expiry.""" if not GITHUB_APP_ID or not GITHUB_APP_PRIVATE_KEY or not GITHUB_APP_INSTALLATION_ID: logger.debug("GitHub App env vars not fully configured, skipping app token") return None, None - key = _scope_key(repository_ids, repositories) + key = _scope_key(repository_ids, repositories, permissions) now = datetime.now(UTC) cached = _cached_token(key, now=now) if cached is not None: @@ -117,6 +142,9 @@ async def get_github_app_installation_token_with_expiry( body["repository_ids"] = list(repository_ids) elif repositories: body["repositories"] = list(repositories) + permission_key = normalize_permissions(permissions) + if permission_key: + body["permissions"] = dict(permission_key) try: app_jwt = _generate_app_jwt() diff --git a/agent/utils/github_proxy.py b/agent/utils/github_proxy.py index af288e5f..c92762a7 100644 --- a/agent/utils/github_proxy.py +++ b/agent/utils/github_proxy.py @@ -16,7 +16,12 @@ from collections.abc import Sequence from datetime import UTC, datetime, timedelta from typing import Any -from .github_app import get_github_app_installation_token_with_expiry +from .github_app import ( + PermissionKey, + PermissionMap, + get_github_app_installation_token_with_expiry, + normalize_permissions, +) from .sandbox_state import SANDBOX_BACKENDS, unwrap_sandbox_backend logger = logging.getLogger(__name__) @@ -26,8 +31,11 @@ PROXY_TOKEN_REFRESH_WINDOW = timedelta(minutes=5) # Used only when the token's own expiry is unknown: refresh after this age. PROXY_TOKEN_FALLBACK_TTL = timedelta(minutes=50) -# thread_id -> (token_expires_at | None, recorded_at, repositories scope | None) -_PROXY_TOKEN_EXPIRY: dict[str, tuple[datetime | None, datetime, tuple[str, ...] | None]] = {} +# thread_id -> (token_expires_at | None, recorded_at, repositories scope | None, permission scope) +_PROXY_TOKEN_EXPIRY: dict[ + str, tuple[datetime | None, datetime, tuple[str, ...] | None, PermissionKey] +] = {} +ProxyTokenRecord = tuple[datetime | None, datetime, tuple[str, ...] | None, PermissionKey] def _parse_expiry(expires_at: Any) -> datetime | None: @@ -60,17 +68,22 @@ def record_proxy_token_expiry( expires_at: Any, *, repositories: Sequence[str] | None = None, + permissions: PermissionMap | None = None, ) -> None: """Record when ``thread_id``'s proxy token expires and the repo scope it was minted with. - ``repositories`` preserves the original token scope (reviewer runs mint a - repo-scoped installation token) so a later refresh doesn't broaden it to an - installation-wide token. + ``repositories`` and ``permissions`` preserve the original token scope so a + later refresh doesn't broaden it to an installation-wide or more privileged token. """ if not thread_id: return scope = tuple(repositories) if repositories else None - _PROXY_TOKEN_EXPIRY[thread_id] = (_parse_expiry(expires_at), datetime.now(UTC), scope) + _PROXY_TOKEN_EXPIRY[thread_id] = ( + _parse_expiry(expires_at), + datetime.now(UTC), + scope, + normalize_permissions(permissions), + ) def clear_proxy_token_expiry(thread_id: str | None) -> None: @@ -78,6 +91,13 @@ def clear_proxy_token_expiry(thread_id: str | None) -> None: _PROXY_TOKEN_EXPIRY.pop(thread_id, None) +def _unpack_proxy_token_record(record: tuple[Any, ...]) -> ProxyTokenRecord: + expires_at, recorded_at, repositories, *rest = record + permissions = rest[0] if rest else () + permission_key = permissions if isinstance(permissions, tuple) else normalize_permissions(None) + return expires_at, recorded_at, repositories, permission_key + + def proxy_token_needs_refresh(thread_id: str | None, *, now: datetime | None = None) -> bool: """Whether the recorded proxy token is at/near expiry and should be refreshed.""" if not thread_id: @@ -85,45 +105,65 @@ def proxy_token_needs_refresh(thread_id: str | None, *, now: datetime | None = N record = _PROXY_TOKEN_EXPIRY.get(thread_id) if record is None: return False - expires_at, recorded_at, _scope = record + expires_at, recorded_at, _scope, _permissions = _unpack_proxy_token_record(record) current = (now or datetime.now(UTC)).astimezone(UTC) if expires_at is not None: return (expires_at - current) <= PROXY_TOKEN_REFRESH_WINDOW return (current - recorded_at) >= PROXY_TOKEN_FALLBACK_TTL +async def refresh_proxy_token( + thread_id: str | None, + *, + repositories: Sequence[str] | None = None, + permissions: PermissionMap | None = None, +) -> bool: + """Re-configure a LangSmith sandbox proxy with a freshly minted token.""" + if os.getenv("SANDBOX_TYPE", "langsmith") != "langsmith" or not thread_id: + return False + + sandbox_backend = SANDBOX_BACKENDS.get(thread_id) + if sandbox_backend is None: + return False + + _expires, _recorded, recorded_repositories, recorded_permissions = _unpack_proxy_token_record( + _PROXY_TOKEN_EXPIRY.get(thread_id, (None, None, None, ())) + ) + effective_repositories = tuple(repositories) if repositories else recorded_repositories + permission_key = normalize_permissions(permissions) or recorded_permissions + token_kwargs: dict[str, Any] = {} + if effective_repositories: + token_kwargs["repositories"] = list(effective_repositories) + if permission_key: + token_kwargs["permissions"] = dict(permission_key) + token, expires_at = await get_github_app_installation_token_with_expiry(**token_kwargs) + if not token: + logger.warning("Proxy token refresh for thread %s failed: no installation token", thread_id) + return False + + from ..integrations.langsmith import _configure_github_proxy + + current_backend = unwrap_sandbox_backend(sandbox_backend) + await asyncio.to_thread(_configure_github_proxy, current_backend.id, token) + record_proxy_token_expiry( + thread_id, + expires_at, + repositories=effective_repositories, + permissions=dict(permission_key) if permission_key else None, + ) + logger.info("Refreshed GitHub proxy token for thread %s", thread_id) + return True + + async def maybe_refresh_proxy_token(thread_id: str | None, *, now: datetime | None = None) -> bool: """Re-configure the sandbox proxy with a fresh token when near expiry. Returns True when a refresh was performed. Only applies to LangSmith sandboxes; other providers don't use the proxy. """ - if os.getenv("SANDBOX_TYPE", "langsmith") != "langsmith": - return False if not thread_id or not proxy_token_needs_refresh(thread_id, now=now): return False - - sandbox_backend = SANDBOX_BACKENDS.get(thread_id) - if sandbox_backend is None: - return False - - # Preserve the original token scope: reviewer runs mint a repo-scoped token, - # so refreshing must not broaden it to an installation-wide token. - _expires, _recorded, repositories = _PROXY_TOKEN_EXPIRY.get(thread_id, (None, None, None)) - token, expires_at = await get_github_app_installation_token_with_expiry( - repositories=list(repositories) if repositories else None - ) - if not token: - logger.warning( - "Proxy token for thread %s is near expiry but no installation token is available", - thread_id, - ) - return False - - from ..integrations.langsmith import _configure_github_proxy - - current_backend = unwrap_sandbox_backend(sandbox_backend) - await asyncio.to_thread(_configure_github_proxy, current_backend.id, token) - record_proxy_token_expiry(thread_id, expires_at, repositories=repositories) - logger.info("Refreshed GitHub proxy token for thread %s before expiry", thread_id) - return True + refreshed = await refresh_proxy_token(thread_id) + if refreshed: + logger.info("Refreshed GitHub proxy token for thread %s before expiry", thread_id) + return refreshed diff --git a/agent/webapp.py b/agent/webapp.py index c7909c6a..8d42b00d 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -44,6 +44,7 @@ from .dashboard.user_mappings import ( from .dashboard.user_mappings import ( refresh_cache as refresh_user_mapping_cache, ) +from .dashboard.workflow_approval import decide_workflow_push_approval from .reviewer_findings import ( REVIEWER_THREAD_KIND, Finding, @@ -161,8 +162,10 @@ if DASHBOARD_ALLOWED_ORIGINS: app.include_router(dashboard_router) from .dashboard.plan_api import plan_router # noqa: E402 +from .dashboard.workflow_approval_api import workflow_approval_router # noqa: E402 app.include_router(plan_router) +app.include_router(workflow_approval_router) LINEAR_WEBHOOK_SECRET = os.environ.get("LINEAR_WEBHOOK_SECRET", "") GITHUB_WEBHOOK_SECRET = os.environ.get("GITHUB_WEBHOOK_SECRET", "") @@ -1666,6 +1669,74 @@ async def slack_interactivity( action_value = json.loads(str(action.get("value") or "{}")) except json.JSONDecodeError: return {"status": "ignored", "reason": "Invalid action value"} + if action_value.get("type") == "workflow_push_approval": + workflow_action = str(action_value.get("action") or "").strip() + fingerprint = str(action_value.get("fingerprint") or "").strip() + channel = payload.get("channel") if isinstance(payload.get("channel"), dict) else {} + message = payload.get("message") if isinstance(payload.get("message"), dict) else {} + container = payload.get("container") if isinstance(payload.get("container"), dict) else {} + user = payload.get("user") if isinstance(payload.get("user"), dict) else {} + channel_id = str(channel.get("id") or container.get("channel_id") or "") + thread_ts = str( + message.get("thread_ts") or message.get("ts") or container.get("thread_ts") or "" + ) + user_id = str(user.get("id") or "") + if not channel_id or not thread_ts or not fingerprint: + return {"status": "ignored", "reason": "Missing workflow approval context"} + + thread_id = generate_thread_id_from_slack_thread(channel_id, thread_ts) + if not await _slack_user_is_thread_owner(thread_id, user_id): + await post_slack_thread_reply( + channel_id=channel_id, + thread_ts=thread_ts, + text="Only the person who requested this run can approve workflow file pushes.", + ) + return {"status": "ignored", "reason": "approver is not the thread owner"} + + if workflow_action not in {"approve", "reject"}: + return {"status": "ignored", "reason": "Unknown workflow approval action"} + approved = workflow_action == "approve" + record = await decide_workflow_push_approval( + thread_id, fingerprint, approved=approved, actor=user_id + ) + if record is None: + await post_slack_thread_reply( + channel_id=channel_id, + thread_ts=thread_ts, + text="I couldn't find that workflow approval request. Trigger the push again to create a fresh approval.", + ) + return {"status": "ignored", "reason": "workflow approval not found"} + if not approved: + await post_slack_thread_reply( + channel_id=channel_id, + thread_ts=thread_ts, + text=f"Workflow push rejected for fingerprint `{fingerprint}`. No workflow files will be pushed.", + ) + return {"status": "accepted", "message": "Workflow push rejected"} + + await post_slack_thread_reply( + channel_id=channel_id, + thread_ts=thread_ts, + text=f"Workflow push approved for fingerprint `{fingerprint}`. Open SWE will retry the blocked push.", + ) + repo_config = await get_slack_repo_config(channel_id, thread_ts, slack_user_id=user_id) + background_tasks.add_task( + process_slack_mention, + { + "channel_id": channel_id, + "thread_ts": thread_ts, + "event_ts": str(message.get("ts") or ""), + "user_id": user_id, + "text": ( + "The workflow-file push approval was approved. Retry the blocked " + "git push now; do not alter workflow files before pushing." + ), + "bot_user_id": SLACK_BOT_USER_ID, + }, + repo_config, + ) + return {"status": "accepted", "message": "Workflow push approved, retry queued"} + if action_value.get("type") == "plan_approval": plan_action = str(action_value.get("action") or "").strip() channel = payload.get("channel") if isinstance(payload.get("channel"), dict) else {} diff --git a/tests/test_github_app.py b/tests/test_github_app.py index 38c37ef8..eb7c8795 100644 --- a/tests/test_github_app.py +++ b/tests/test_github_app.py @@ -141,6 +141,48 @@ async def test_installation_token_can_be_scoped_to_repository_ids( assert _FakeAsyncClient.last_post["json"] == {"repository_ids": [123]} +@pytest.mark.asyncio +async def test_installation_token_includes_permissions(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(github_app, "GITHUB_APP_ID", "1") + monkeypatch.setattr(github_app, "GITHUB_APP_PRIVATE_KEY", "key") + monkeypatch.setattr(github_app, "GITHUB_APP_INSTALLATION_ID", "2") + monkeypatch.setattr(github_app, "_generate_app_jwt", lambda: "jwt") + monkeypatch.setattr(github_app.httpx, "AsyncClient", _FakeAsyncClient) + + await github_app.get_github_app_installation_token_with_expiry( + repositories=["open-swe"], permissions={"workflows": "write", "contents": "write"} + ) + + assert _FakeAsyncClient.last_post is not None + assert _FakeAsyncClient.last_post["json"] == { + "repositories": ["open-swe"], + "permissions": {"contents": "write", "workflows": "write"}, + } + + +@pytest.mark.asyncio +async def test_cache_is_scoped_per_permission_set(monkeypatch: pytest.MonkeyPatch) -> None: + future = (datetime.now(UTC) + timedelta(hours=1)).isoformat() + + class Client(_CountingClient): + posts = 0 + expires_at = future + + _configure(monkeypatch, Client) + + await github_app.get_github_app_installation_token_with_expiry( + permissions={"contents": "write"} + ) + await github_app.get_github_app_installation_token_with_expiry( + permissions={"contents": "write", "workflows": "write"} + ) + await github_app.get_github_app_installation_token_with_expiry( + permissions={"contents": "write"} + ) + + assert Client.posts == 2 + + @pytest.mark.asyncio async def test_installation_token_omits_scope_for_full_installation( monkeypatch: pytest.MonkeyPatch, diff --git a/tests/test_github_proxy_refresh.py b/tests/test_github_proxy_refresh.py index 8d630b92..7be9a94f 100644 --- a/tests/test_github_proxy_refresh.py +++ b/tests/test_github_proxy_refresh.py @@ -105,8 +105,9 @@ class TestMaybeRefreshProxyToken: assert result is True mock_configure.assert_called_once_with("sb-1", "ghs_new") - expires_at, _recorded, _scope = github_proxy._PROXY_TOKEN_EXPIRY["thread-1"] + expires_at, _recorded, _scope, permissions = github_proxy._PROXY_TOKEN_EXPIRY["thread-1"] assert expires_at == datetime(2025, 1, 1, 13, 0, 0, tzinfo=UTC) + assert permissions == () @pytest.mark.asyncio async def test_preserves_repo_scope_on_refresh(self) -> None: @@ -128,8 +129,9 @@ class TestMaybeRefreshProxyToken: assert result is True token_mock.assert_awaited_once_with(repositories=["open-swe"]) - _expires, _recorded, scope = github_proxy._PROXY_TOKEN_EXPIRY["thread-1"] + _expires, _recorded, scope, permissions = github_proxy._PROXY_TOKEN_EXPIRY["thread-1"] assert scope == ("open-swe",) + assert permissions == () @pytest.mark.asyncio async def test_no_refresh_when_token_unavailable(self) -> None: diff --git a/tests/test_plan_review.py b/tests/test_plan_review.py index a6de3d1e..f200355b 100644 --- a/tests/test_plan_review.py +++ b/tests/test_plan_review.py @@ -122,6 +122,8 @@ def test_plan_routes_registered() -> None: assert "/dashboard/api/plan/{thread_id}/comments" in paths assert "/dashboard/api/plan/{thread_id}/comments/{comment_id}" in paths assert "/dashboard/api/plan/yjs/{thread_id}" not in paths + assert "/dashboard/api/workflow-approval/{thread_id}/{fingerprint}/approve" in paths + assert "/dashboard/api/workflow-approval/{thread_id}/{fingerprint}/reject" in paths def test_save_plan_exported_and_wired() -> None: diff --git a/tests/test_workflow_push_guard.py b/tests/test_workflow_push_guard.py new file mode 100644 index 00000000..b507fded --- /dev/null +++ b/tests/test_workflow_push_guard.py @@ -0,0 +1,247 @@ +from __future__ import annotations + +import json +from typing import Any + +import pytest +from langchain_core.messages import ToolMessage + +from agent.middleware import workflow_push_guard as guard + + +class _Response: + def __init__(self, output: str, exit_code: int = 0) -> None: + self.output = output + self.exit_code = exit_code + self.truncated = False + + +class _Backend: + id = "sandbox-id" + + def __init__(self, *, workflow_files: str = ".github/workflows/ci.yml") -> None: + self.workflow_files = workflow_files + self.commands: list[str] = [] + self.head = "a" * 40 + + def execute(self, command: str, *, timeout: int | None = None) -> _Response: + self.commands.append(command) + if "rev-parse --show-toplevel" in command: + return _Response("/repo\n") + if "rev-parse --verify refs/remotes/origin/feature" in command: + return _Response("", 1) + if "symbolic-ref --short refs/remotes/origin/HEAD" in command: + return _Response("origin/main\n") + if f"merge-base {self.head} origin/main" in command: + return _Response("base-sha\n") + if "diff --name-only" in command: + return _Response(f"{self.workflow_files}\n" if self.workflow_files else "") + if "diff --binary --full-index" in command: + return _Response("diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml\n") + if "config --get remote.origin.url" in command: + return _Response("git@github.com:langchain-ai/open-swe.git\n") + if "rev-parse --abbrev-ref HEAD" in command: + return _Response("feature\n") + if "rev-parse HEAD" in command or "rev-parse feature" in command: + return _Response(f"{self.head}\n") + return _Response("") + + +class _Runtime: + config = { + "configurable": { + "thread_id": "thread-1", + "slack_thread": {"channel_id": "C123", "thread_ts": "1700000000.000100"}, + } + } + + +class _Request: + runtime = _Runtime() + + def __init__(self, command: str = "git -C /repo push origin feature") -> None: + self.tool_call = { + "name": "execute", + "args": {"command": command}, + "id": "call-1", + } + + def override(self, **kwargs: Any) -> _Request: + next_request = _Request() + next_request.tool_call = kwargs.get("tool_call", self.tool_call) + return next_request + + +@pytest.fixture(autouse=True) +def _clear_backend_cache() -> Any: + guard.SANDBOX_BACKENDS.clear() + yield + guard.SANDBOX_BACKENDS.clear() + + +def test_parse_git_push_supports_git_c_and_cd() -> None: + assert guard._parse_git_push("git -C /repo push origin feature") == guard.ParsedGitPush( + repo_dir="/repo", remote="origin", local_ref="feature", remote_ref="feature" + ) + assert guard._parse_git_push( + "cd /repo && git push -u origin HEAD:feature" + ) == guard.ParsedGitPush( + repo_dir="/repo", + remote="origin", + local_ref="HEAD", + remote_ref="feature", + set_upstream=True, + ) + assert guard._parse_git_push("git status && git push") is None + assert guard._parse_git_push("git push origin feature; git push origin evil:feature") is None + + +def test_workflow_change_for_push_fingerprints_workflow_diff() -> None: + backend = _Backend() + change = guard._workflow_change_for_push( + backend, + guard.ParsedGitPush( + repo_dir="/repo", remote="origin", local_ref="feature", remote_ref="feature" + ), + ) + + assert change is not None + assert change.repo == "https://github.com/langchain-ai/open-swe" + assert change.branch == "feature" + assert change.files == [".github/workflows/ci.yml"] + assert ( + change.fixed_command + == "git -C /repo push origin aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:refs/heads/feature" + ) + assert len(change.fingerprint) == 64 + + +def test_workflow_change_for_push_ignores_non_workflow_push() -> None: + backend = _Backend(workflow_files="") + + assert ( + guard._workflow_change_for_push( + backend, + guard.ParsedGitPush( + repo_dir="/repo", remote="origin", local_ref="feature", remote_ref="feature" + ), + ) + is None + ) + + +def test_workflow_change_for_push_rejects_non_current_refspec() -> None: + backend = _Backend() + + assert ( + guard._workflow_change_for_push( + backend, + guard.ParsedGitPush( + repo_dir="/repo", remote="origin", local_ref="evil", remote_ref="feature" + ), + ) + is None + ) + + +async def test_unapproved_workflow_push_blocks_and_posts_slack( + monkeypatch: pytest.MonkeyPatch, +) -> None: + guard.SANDBOX_BACKENDS["thread-1"] = _Backend() + posted: dict[str, Any] = {} + + async def fake_approved(thread_id: str, fingerprint: str) -> bool: + return False + + async def fake_pending(thread_id: str, **kwargs: Any) -> tuple[dict[str, Any], bool]: + return {"fingerprint": kwargs["fingerprint"], "status": "pending", "notified": False}, True + + async def fake_post( + channel_id: str, thread_ts: str, message: str, **kwargs: Any + ) -> tuple[str, None]: + posted.update( + channel_id=channel_id, thread_ts=thread_ts, message=message, blocks=kwargs["blocks"] + ) + return "1700000000.000200", None + + async def fake_notified(thread_id: str, fingerprint: str) -> None: + posted["notified"] = fingerprint + + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "ensure_workflow_push_pending", fake_pending) + monkeypatch.setattr(guard, "post_slack_thread_reply_with_ts", fake_post) + monkeypatch.setattr(guard, "mark_workflow_push_notified", fake_notified) + + called = False + + async def handler(_request: Any) -> ToolMessage: + nonlocal called + called = True + return ToolMessage(content="pushed", tool_call_id="call-1") + + result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + + assert called is False + assert isinstance(result, ToolMessage) + assert result.status == "error" + payload = json.loads(str(result.content)) + assert payload["workflow_approval_status"] == "approval_required" + assert payload["files"] == [".github/workflows/ci.yml"] + assert posted["channel_id"] == "C123" + assert posted["blocks"][1]["elements"][0]["value"] + + +async def test_approved_workflow_push_elevates_and_restores( + monkeypatch: pytest.MonkeyPatch, +) -> None: + guard.SANDBOX_BACKENDS["thread-1"] = _Backend() + refreshed: list[dict[str, str]] = [] + + async def fake_approved(thread_id: str, fingerprint: str) -> bool: + return True + + async def fake_refresh(thread_id: str | None, *, permissions: dict[str, str]) -> bool: + refreshed.append(dict(permissions)) + return True + + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "refresh_proxy_token", fake_refresh) + + pushed_command = "" + + async def handler(request: Any) -> ToolMessage: + nonlocal pushed_command + pushed_command = request.tool_call["args"]["command"] + return ToolMessage(content="pushed", tool_call_id="call-1") + + result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + + assert isinstance(result, ToolMessage) + assert result.content == "pushed" + assert ( + pushed_command + == "git -C /repo push origin aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:refs/heads/feature" + ) + assert refreshed[0]["workflows"] == "write" + assert "workflows" not in refreshed[1] + + +async def test_non_workflow_push_runs_without_approval(monkeypatch: pytest.MonkeyPatch) -> None: + guard.SANDBOX_BACKENDS["thread-1"] = _Backend(workflow_files="") + called = False + + async def fail_approval(*args: Any, **kwargs: Any) -> bool: + raise AssertionError("approval should not be checked") + + monkeypatch.setattr(guard, "workflow_push_approved", fail_approval) + + async def handler(_request: Any) -> ToolMessage: + nonlocal called + called = True + return ToolMessage(content="pushed", tool_call_id="call-1") + + result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + + assert called is True + assert isinstance(result, ToolMessage) + assert result.content == "pushed"