Merge b0fa57fa48 into 421290d066

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,
     *,

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(

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

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: