Merge b0fa57fa48 into 421290d066

Using git show per changed file failed when a workflow file was deleted,
because the blob no longer exists at the pushed head. That caused the guard to
return None and skip approval for deletions. Hash the already-computed diff
output instead; it captures adds, mods, deletes, and renames and cannot fail on
a missing blob.

Also adds a test that a deleted workflow file still requires approval.

Refs: 98

agent/middleware/workflow_push_guard.py

@@ -6,6 +6,7 @@ import asyncio
 import hashlib
 import json
 import logging
+import os
 import re
 import shlex
 import threading
@@ -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,14 +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))
-    identity_payload = {
+    content_payload = {
         "repo": repo,
         "branch": branch_name,
-        "base_sha": base_sha,
         "files": files,
+        "content_hash": content_hash,
     }
     return WorkflowPushChange(
-        fingerprint=_fingerprint(identity_payload),
+        fingerprint=_fingerprint(content_payload),
         repo=repo,
         branch=branch_name,
         base_sha=base_sha,
@@ -422,9 +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"
-        "Approval covers the workflow files listed above on this branch, including future "
-        "rebases or amends of the same change. If the set of workflow files or the branch "
-        "changes, 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."
     )
 
 
@@ -493,22 +496,44 @@ async def _approval_state(request: ToolCallRequest, change: WorkflowPushChange)
 
 async def _run_with_workflow_token(
     thread_id: str,
+    request: ToolCallRequest,
     run: Callable[[], Awaitable[ToolMessage | Command]],
 ) -> ToolMessage | Command:
+    sandbox_type = os.getenv("SANDBOX_TYPE", "langsmith")
+    if sandbox_type != "langsmith":
+        return await run()
+
     elevated = await refresh_proxy_token(
         thread_id, permissions=WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS
     )
+    if not elevated:
+        logger.error(
+            "Workflow push approved for thread %s, but proxy token elevation to workflows:write "
+            "failed; the sandbox cannot push workflow files without an elevated token.",
+            thread_id,
+        )
+        error_message = ToolMessage(
+            content=json.dumps(
+                {
+                    "status": "error",
+                    "error_type": "WorkflowPushElevationFailed",
+                    "error": (
+                        "Workflow push approved, but the sandbox could not obtain a "
+                        "workflows-scoped token. Please retry the push or check the "
+                        "GitHub proxy / token minting configuration."
+                    ),
+                }
+            ),
+            tool_call_id="",
+            status="error",
+        )
+        return _tool_message_for_request(error_message, request)
     try:
         return await run()
     finally:
-        if elevated:
-            restored = await refresh_proxy_token(
-                thread_id, permissions=RUNTIME_PROXY_TOKEN_PERMISSIONS
-            )
-            if not restored:
-                await refresh_proxy_token(
-                    thread_id, permissions=BASE_RUNTIME_PROXY_TOKEN_PERMISSIONS
-                )
+        restored = await refresh_proxy_token(thread_id, permissions=RUNTIME_PROXY_TOKEN_PERMISSIONS)
+        if not restored:
+            await refresh_proxy_token(thread_id, permissions=BASE_RUNTIME_PROXY_TOKEN_PERMISSIONS)
 
 
 class WorkflowPushGuardMiddleware(AgentMiddleware):
@@ -540,7 +565,7 @@ class WorkflowPushGuardMiddleware(AgentMiddleware):
         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 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,

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 before it can proceed. Approval is keyed to the repo, branch, and workflow files being pushed, so rebases or amends of the same workflow change do not require a fresh approval; changing the branch or the set of workflow files does require a new approval. 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

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import json
+import os
 from typing import Any
 
 import pytest
@@ -19,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
 
@@ -29,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:
@@ -37,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("")
@@ -332,10 +350,11 @@ async def test_stale_workflow_approval_is_loud_and_blocks(
     assert posted["channel_id"] == "C123"
 
 
-async def test_rebased_workflow_push_uses_identity_fingerprint(
+async def test_rebased_workflow_push_keeps_fingerprint_for_same_diff(
     monkeypatch: pytest.MonkeyPatch,
 ) -> None:
-    backend = _Backend()
+    # 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
 
@@ -370,9 +389,135 @@ async def test_rebased_workflow_push_uses_identity_fingerprint(
     assert payload["fingerprint"]
     assert payload["files"] == [".github/workflows/ci.yml"]
 
-    # A second push with a different head but same branch/files should produce the same fingerprint.
+    # 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:
+    guard.SANDBOX_BACKENDS["thread-1"] = _Backend()
+
+    async def fake_approved(thread_id: str, fingerprint: str) -> bool:
+        return True
+
+    refresh_calls: list[dict[str, str]] = []
+
+    async def fake_refresh(*args: Any, **kwargs: Any) -> bool:
+        refresh_calls.append(dict(kwargs.get("permissions", {})))
+        return False
+
+    monkeypatch.setattr(guard, "workflow_push_approved", fake_approved)
+    monkeypatch.setattr(guard, "refresh_proxy_token", fake_refresh)
+
+    called = False
+    request = _Request()
+
+    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.tool_call_id == "call-1"
+    assert result.status == "error"
+    payload = json.loads(str(result.content))
+    assert payload["status"] == "error"
+    assert payload["error_type"] == "WorkflowPushElevationFailed"
+    assert "workflows-scoped token" in payload["error"]
+    assert len(refresh_calls) == 1
+    assert refresh_calls[0].get("workflows") == "write"
+
+
+async def test_approved_workflow_push_runs_on_non_langsmith_providers(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    guard.SANDBOX_BACKENDS["thread-1"] = _Backend()
+
+    async def fake_approved(thread_id: str, fingerprint: str) -> bool:
+        return True
+
+    refresh_calls: list[dict[str, str]] = []
+
+    async def fake_refresh(*args: Any, **kwargs: Any) -> bool:
+        refresh_calls.append(dict(kwargs.get("permissions", {})))
+        return False
+
+    monkeypatch.setattr(guard, "workflow_push_approved", fake_approved)
+    monkeypatch.setattr(guard, "refresh_proxy_token", fake_refresh)
+    monkeypatch.setattr(guard, "os", os)
+
+    called = False
+
+    async def handler(request: Any) -> ToolMessage:
+        nonlocal called
+        called = True
+        return ToolMessage(content="pushed", tool_call_id=request.tool_call["id"])
+
+    with monkeypatch.context() as mp:
+        mp.setenv("SANDBOX_TYPE", "local")
+        result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler)
+
+    assert called is True
+    assert isinstance(result, ToolMessage)
+    assert result.content == "pushed"
+    assert refresh_calls == []