diff --git a/agent/middleware/workflow_push_guard.py b/agent/middleware/workflow_push_guard.py index 12e784b4..40abfe50 100644 --- a/agent/middleware/workflow_push_guard.py +++ b/agent/middleware/workflow_push_guard.py @@ -340,6 +340,14 @@ def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPu if not diff.ok or not diff.output: return None + content_parts: list[str] = [] + for path in files: + blob = _run_git(backend, root, f"show {shlex.quote(head)}:{shlex.quote(path)}") + if not blob.ok: + return None + content_parts.append(blob.output) + content_hash = _fingerprint({"contents": content_parts}) + 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}" @@ -348,14 +356,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, @@ -423,9 +431,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." ) diff --git a/agent/prompt.py b/agent/prompt.py index f65c52ce..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 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 diff --git a/tests/test_workflow_push_guard.py b/tests/test_workflow_push_guard.py index a8d8e378..c1c44211 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("") @@ -333,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 @@ -371,13 +389,23 @@ 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_approved_workflow_push_aborts_when_elevation_fails( monkeypatch: pytest.MonkeyPatch,