diff --git a/agent/dashboard/workflow_approval.py b/agent/dashboard/workflow_approval.py index 7652f08a..eaae96d7 100644 --- a/agent/dashboard/workflow_approval.py +++ b/agent/dashboard/workflow_approval.py @@ -44,6 +44,28 @@ async def workflow_push_approved(thread_id: str, fingerprint: str) -> bool: return approvals.get(fingerprint, {}).get("status") == WORKFLOW_APPROVAL_APPROVED +async def find_workflow_push_approval( + thread_id: str, + *, + repo: str, + branch: str, + files: list[str], +) -> dict[str, Any] | None: + """Return the most recent approved record matching identity-level keys, if any.""" + approvals = await get_workflow_push_approvals(thread_id) + identity = (repo, branch, tuple(sorted(files))) + matches = [ + r + for r in approvals.values() + if r.get("status") == WORKFLOW_APPROVAL_APPROVED + and (r.get("repo"), r.get("branch"), tuple(sorted(r.get("files", [])))) == identity + ] + if not matches: + return None + matches.sort(key=lambda r: str(r.get("decided_at", "")), reverse=True) + return matches[0] + + async def ensure_workflow_push_pending( thread_id: str, *, diff --git a/agent/middleware/workflow_push_guard.py b/agent/middleware/workflow_push_guard.py index 293bd385..67b5069f 100644 --- a/agent/middleware/workflow_push_guard.py +++ b/agent/middleware/workflow_push_guard.py @@ -22,6 +22,7 @@ from langgraph.types import Command from ..dashboard.workflow_approval import ( ensure_workflow_push_pending, + find_workflow_push_approval, mark_workflow_push_notified, workflow_push_approved, ) @@ -339,6 +340,8 @@ def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPu if not diff.ok or not diff.output: return None + content_hash = _fingerprint({"diff": diff.output}) + 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}" @@ -347,20 +350,14 @@ def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPu 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 = { + content_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, + "content_hash": content_hash, } return WorkflowPushChange( - fingerprint=_fingerprint(payload), + fingerprint=_fingerprint(content_payload), repo=repo, branch=branch_name, base_sha=base_sha, @@ -373,16 +370,27 @@ def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPu ) -def _blocked_message(change: WorkflowPushChange, *, already_rejected: bool = False) -> ToolMessage: +def _blocked_message( + change: WorkflowPushChange, *, already_rejected: bool = False, stale: bool = False +) -> ToolMessage: status = "rejected" if already_rejected else "approval_required" - content = { - "status": "error", - "error_type": "WorkflowPushApprovalRequired", - "error": ( + if stale: + error = ( + "This git push includes GitHub workflow file changes. A previous approval " + "exists for the same branch and workflow files, but the workflow diff has " + "changed since that approval (for example, a rebase or amend). The thread " + "owner must re-approve the new fingerprint before Open SWE can push it." + ) + else: + 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." - ), + ) + content = { + "status": "error", + "error_type": "WorkflowPushApprovalRequired", + "error": error, "workflow_approval_status": status, "fingerprint": change.fingerprint, "files": change.files, @@ -417,8 +425,9 @@ def _approval_slack_message(change: WorkflowPushChange) -> str: 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." + "Approval covers the exact workflow-file content diff listed above on this branch, " + "including future rebases or amends that replay the same diff. If the set of workflow " + "files, the branch, or the workflow-file content changes, a new fingerprint will be required." ) @@ -455,6 +464,20 @@ async def _approval_state(request: ToolCallRequest, change: WorkflowPushChange) try: if await workflow_push_approved(thread_id, change.fingerprint): return "approved" + + # If the exact identity fingerprint is not approved, check whether a prior + # approval covers the same (repo, branch, files) identity. If so, the diff + # changed underneath the prior approval (rebase/amend/edit), so we surface a + # loud re-approval message rather than a fresh silent pending record. + prior = await find_workflow_push_approval( + thread_id, + repo=change.repo, + branch=change.branch, + files=change.files, + ) + if prior is not None: + return "stale_approval" + record, _created = await ensure_workflow_push_pending( thread_id, fingerprint=change.fingerprint, @@ -543,8 +566,24 @@ class WorkflowPushGuardMiddleware(AgentMiddleware): if state == "approved" and thread_id: safe_request = _override_execute_command(request, change.fixed_command) return await _run_with_workflow_token(thread_id, request, lambda: handler(safe_request)) + if state == "stale_approval": + 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 _tool_message_for_request( - _blocked_message(change, already_rejected=state == "rejected"), request + _blocked_message( + change, + already_rejected=state == "rejected", + stale=state == "stale_approval", + ), + request, ) def wrap_tool_call( diff --git a/agent/prompt.py b/agent/prompt.py index 549c6902..fab505d4 100644 --- a/agent/prompt.py +++ b/agent/prompt.py @@ -317,7 +317,7 @@ Steps, in order: **IMPORTANT: If `git push` or `gh` returns "403", "Permission denied", or another permanent authorization failure, do not retry. Report the error to the user immediately and stop.** -**IMPORTANT: Workflow files (`.github/workflows/`) may be changed only when explicitly requested. Any push that includes workflow-file changes requires human approval of the exact workflow diff fingerprint before it can proceed — do not attempt to bypass it.** +**IMPORTANT: Workflow files (`.github/workflows/`) may be changed only when explicitly requested. Any push that includes workflow-file changes requires human approval before it can proceed. Approval is keyed to the repo, branch, workflow files, and the exact workflow-file content diff, so rebases or amends that replay the same workflow diff do not require a fresh approval; changing the branch, the set of workflow files, or the workflow-file content does require a new approval. Do not attempt to bypass it.** 4. **Notify the source** immediately after pushing and, when applicable, PR creation/update succeeds. Include a brief summary plus the PR link or branch URL: - Linear-triggered: use `linear_comment` with an `@mention` of the user who triggered the task diff --git a/tests/test_workflow_push_guard.py b/tests/test_workflow_push_guard.py index 3e7ec01e..63362ec8 100644 --- a/tests/test_workflow_push_guard.py +++ b/tests/test_workflow_push_guard.py @@ -20,8 +20,16 @@ class _Response: class _Backend: id = "sandbox-id" - def __init__(self, *, workflow_files: str = ".github/workflows/ci.yml") -> None: + def __init__( + self, + *, + workflow_files: str = ".github/workflows/ci.yml", + remote_branch: str | None = None, + diff_output: str = "diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml\n", + ) -> None: self.workflow_files = workflow_files + self.remote_branch = remote_branch + self.diff_output = diff_output self.commands: list[str] = [] self.head = "a" * 40 @@ -30,7 +38,9 @@ class _Backend: 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 self.remote_branch is None: + return _Response("", 1) + return _Response(f"{self.remote_branch}\n") 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: @@ -38,11 +48,18 @@ class _Backend: 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") + return _Response(self.diff_output) + if command.startswith("git -C /repo show "): + content = "workflow content\n" + if "+new content" in self.diff_output: + content += "new content\n" + return _Response(content) 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 refs/remotes/origin/feature" in command: + return _Response(f"{self.remote_branch}\n") if "rev-parse HEAD" in command or "rev-parse feature" in command: return _Response(f"{self.head}\n") return _Response("") @@ -154,6 +171,9 @@ async def test_unapproved_workflow_push_blocks_and_posts_slack( async def fake_approved(thread_id: str, fingerprint: str) -> bool: return False + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return None + async def fake_pending(thread_id: str, **kwargs: Any) -> tuple[dict[str, Any], bool]: return {"fingerprint": kwargs["fingerprint"], "status": "pending", "notified": False}, True @@ -169,6 +189,7 @@ async def test_unapproved_workflow_push_blocks_and_posts_slack( posted["notified"] = fingerprint monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) 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) @@ -205,7 +226,11 @@ async def test_approved_workflow_push_elevates_and_restores( refreshed.append(dict(permissions)) return True + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return None + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) monkeypatch.setattr(guard, "refresh_proxy_token", fake_refresh) pushed_command = "" @@ -241,7 +266,11 @@ async def test_workflow_push_restoration_falls_back_when_actions_read_unavailabl refreshed.append(dict(permissions)) return "actions" not in permissions + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return None + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) monkeypatch.setattr(guard, "refresh_proxy_token", fake_refresh) async def handler(_request: Any) -> ToolMessage: @@ -276,6 +305,150 @@ async def test_non_workflow_push_runs_without_approval(monkeypatch: pytest.Monke assert result.content == "pushed" +async def test_stale_workflow_approval_is_loud_and_blocks( + 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_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return {"fingerprint": "old-fp", "status": "approved", "decided_at": "2024-01-01T00:00:00"} + + 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.000300", 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, "find_workflow_push_approval", fake_find_approval) + 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) + + async def handler(_request: Any) -> ToolMessage: + return ToolMessage(content="pushed", tool_call_id="call-1") + + result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + + assert isinstance(result, ToolMessage) + assert result.status == "error" + payload = json.loads(str(result.content)) + assert payload["workflow_approval_status"] == "approval_required" + assert "changed since that approval" in payload["error"] + assert posted["channel_id"] == "C123" + + +async def test_rebased_workflow_push_keeps_fingerprint_for_same_diff( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # Remote branch exists, so base_sha is derived from the remote tip and changes on rebase. + backend = _Backend(remote_branch="r" * 40) + backend.head = "b" * 40 + guard.SANDBOX_BACKENDS["thread-1"] = backend + + async def fake_approved(thread_id: str, fingerprint: str) -> bool: + return False + + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return None + + 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(*args: Any, **kwargs: Any) -> tuple[str, None]: + return "1700000000.000300", None + + async def fake_notified(*args: Any, **kwargs: Any) -> None: + return None + + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) + 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) + + async def handler(_request: Any) -> ToolMessage: + return ToolMessage(content="pushed", tool_call_id="call-1") + + result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + assert isinstance(result, ToolMessage) + assert result.status == "error" + payload = json.loads(str(result.content)) + assert payload["fingerprint"] + assert payload["files"] == [".github/workflows/ci.yml"] + + # Rebase onto a new base with the same workflow diff -> fingerprint stays stable. + backend.remote_branch = "s" * 40 + backend.head = "c" * 40 + result2 = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + assert isinstance(result2, ToolMessage) + payload2 = json.loads(str(result2.content)) + assert payload2["fingerprint"] == payload["fingerprint"] + + # A content change to the same workflow file produces a different fingerprint. + backend.diff_output = ( + "diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml\n+new content\n" + ) + result3 = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + assert isinstance(result3, ToolMessage) + payload3 = json.loads(str(result3.content)) + assert payload3["fingerprint"] != payload["fingerprint"] + + +async def test_deleted_workflow_file_requires_approval( + monkeypatch: pytest.MonkeyPatch, +) -> None: + backend = _Backend( + workflow_files=".github/workflows/ci.yml", + diff_output="diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml\ndeleted file\n", + ) + guard.SANDBOX_BACKENDS["thread-1"] = backend + + async def fake_approved(thread_id: str, fingerprint: str) -> bool: + return False + + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return None + + 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(*args: Any, **kwargs: Any) -> tuple[str, None]: + return "1700000000.000300", None + + async def fake_notified(*args: Any, **kwargs: Any) -> None: + return None + + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) + 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) + + async def handler(_request: Any) -> ToolMessage: + return ToolMessage(content="pushed", tool_call_id="call-1") + + result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + + 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"] + + async def test_approved_workflow_push_aborts_when_elevation_fails( monkeypatch: pytest.MonkeyPatch, ) -> None: