mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-09-30 20:53:15 +00:00
fix: stop agent retrying commit_and_open_pr on 403 permission denied (#1123)
* fix: stop agent retrying commit_and_open_pr on 403 permission denied Detect 403/permission-denied push failures in commit_and_open_pr and return a PERMANENT_FAILURE message so the LLM stops retrying. Also add prompt-level guidance to the COMMIT_PR_SECTION reinforcing this. Add unit tests covering both the 403 and non-403 push failure paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: stop safety net retrying permanent push failures Skip the after-agent PR fallback when commit_and_open_pr reports a permanent GitHub push authorization failure, while preserving fallback behavior for recoverable failures. --------- Co-authored-by: Claude Agent <agent@anthropic.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Johannes du Plessis <johannes@langchain.dev>
This commit is contained in:
parent
4060933ce4
commit
c61d8b0376
6 changed files with 199 additions and 3 deletions
|
|
@ -36,6 +36,7 @@ from ..utils.github import (
|
|||
git_has_uncommitted_changes,
|
||||
git_has_unpushed_commits,
|
||||
git_push,
|
||||
is_permanent_github_push_failure,
|
||||
)
|
||||
from ..utils.github_app import get_github_app_installation_token
|
||||
from ..utils.github_token import get_github_token
|
||||
|
|
@ -87,7 +88,11 @@ async def open_pr_if_needed(
|
|||
return None
|
||||
|
||||
if pr_payload.get("success"):
|
||||
# Tool already handled commit/push/PR creation
|
||||
return None
|
||||
|
||||
error = pr_payload.get("error")
|
||||
if isinstance(error, str) and is_permanent_github_push_failure(error):
|
||||
logger.info("Skipping PR safety net after permanent push failure")
|
||||
return None
|
||||
|
||||
pr_title = pr_payload.get("title", "feat: Open SWE PR")
|
||||
|
|
|
|||
|
|
@ -305,6 +305,8 @@ When you have completed your implementation, follow these steps in order:
|
|||
|
||||
**IMPORTANT: Never claim a PR was created or updated unless `commit_and_open_pr` returned `success` and a PR link. If it returns "No changes detected" or any error, report that instead.**
|
||||
|
||||
**IMPORTANT: If `commit_and_open_pr` returns an error containing "403", "Permission denied", or "PERMANENT_FAILURE", this is a permanent authorization failure — the token does not have write access to the repository. Do NOT retry. Report the error to the user immediately and stop.**
|
||||
|
||||
4. **Notify the source** immediately after `commit_and_open_pr` succeeds. Include a brief summary and the PR link:
|
||||
- Linear-triggered: use `linear_comment` with an `@mention` of the user who triggered the task
|
||||
- Slack-triggered: use `slack_thread_reply`
|
||||
|
|
|
|||
|
|
@ -24,6 +24,7 @@ from ..utils.github import (
|
|||
git_has_uncommitted_changes,
|
||||
git_has_unpushed_commits,
|
||||
git_push,
|
||||
is_permanent_github_push_failure,
|
||||
)
|
||||
from ..utils.github_app import get_github_app_installation_token
|
||||
from ..utils.github_token import get_github_token
|
||||
|
|
@ -200,9 +201,20 @@ def commit_and_open_pr(
|
|||
|
||||
push_result = git_push(sandbox_backend, repo_dir, target_branch)
|
||||
if push_result.exit_code != 0:
|
||||
push_output = push_result.output.strip()
|
||||
if is_permanent_github_push_failure(push_output):
|
||||
return {
|
||||
"success": False,
|
||||
"error": (
|
||||
f"PERMANENT_FAILURE: do not retry. Git push was rejected with a 403 "
|
||||
f"permission denied error — the token does not have write access to this "
|
||||
f"repository. Report this to the user and stop. Details: {push_output}"
|
||||
),
|
||||
"pr_url": None,
|
||||
}
|
||||
return {
|
||||
"success": False,
|
||||
"error": f"Git push failed: {push_result.output.strip()}",
|
||||
"error": f"Git push failed: {push_output}",
|
||||
"pr_url": None,
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -15,6 +15,17 @@ HTTP_CREATED = 201
|
|||
HTTP_UNPROCESSABLE_ENTITY = 422
|
||||
|
||||
|
||||
def is_permanent_github_push_failure(output: str) -> bool:
|
||||
"""Return whether git push output indicates a permanent auth failure."""
|
||||
normalized_output = output.lower()
|
||||
return (
|
||||
"permanent_failure" in normalized_output
|
||||
or "403" in normalized_output
|
||||
or "permission" in normalized_output
|
||||
or "denied" in normalized_output
|
||||
)
|
||||
|
||||
|
||||
def _run_git(
|
||||
sandbox_backend: SandboxBackendProtocol, repo_dir: str, command: str
|
||||
) -> ExecuteResponse:
|
||||
|
|
|
|||
128
tests/test_commit_and_open_pr_403.py
Normal file
128
tests/test_commit_and_open_pr_403.py
Normal file
|
|
@ -0,0 +1,128 @@
|
|||
"""Tests that commit_and_open_pr returns a PERMANENT_FAILURE message on 403 push errors."""
|
||||
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
from deepagents.backends.protocol import ExecuteResponse
|
||||
|
||||
|
||||
def _make_push_result(exit_code: int, output: str) -> ExecuteResponse:
|
||||
return ExecuteResponse(output=output, exit_code=exit_code, truncated=False)
|
||||
|
||||
|
||||
def _make_config(thread_id: str = "test-thread") -> dict:
|
||||
return {
|
||||
"configurable": {
|
||||
"thread_id": thread_id,
|
||||
"repo": {"owner": "langchain-ai", "name": "docs"},
|
||||
},
|
||||
"metadata": {},
|
||||
}
|
||||
|
||||
|
||||
@patch("agent.tools.commit_and_open_pr.get_config")
|
||||
@patch("agent.tools.commit_and_open_pr.get_sandbox_backend_sync")
|
||||
@patch("agent.tools.commit_and_open_pr.resolve_repo_dir", return_value="/repo/docs")
|
||||
@patch("agent.tools.commit_and_open_pr.git_has_uncommitted_changes", return_value=False)
|
||||
@patch("agent.tools.commit_and_open_pr.git_fetch_origin")
|
||||
@patch("agent.tools.commit_and_open_pr.git_has_unpushed_commits", return_value=True)
|
||||
@patch("agent.tools.commit_and_open_pr.git_current_branch", return_value="open-swe/test-thread")
|
||||
@patch("agent.tools.commit_and_open_pr.git_checkout_branch", return_value=True)
|
||||
@patch("agent.tools.commit_and_open_pr.git_config_user")
|
||||
@patch("agent.tools.commit_and_open_pr.git_add_all")
|
||||
@patch("agent.tools.commit_and_open_pr.get_github_token", return_value="ghp_token")
|
||||
@patch(
|
||||
"agent.tools.commit_and_open_pr.get_github_app_installation_token",
|
||||
new_callable=AsyncMock,
|
||||
return_value="ghs_token",
|
||||
)
|
||||
@patch("agent.tools.commit_and_open_pr.git_push")
|
||||
def test_403_push_returns_permanent_failure(
|
||||
mock_git_push,
|
||||
mock_get_installation_token,
|
||||
mock_get_token,
|
||||
mock_git_add_all,
|
||||
mock_git_config_user,
|
||||
mock_git_checkout_branch,
|
||||
mock_git_current_branch,
|
||||
mock_git_has_unpushed,
|
||||
mock_git_fetch_origin,
|
||||
mock_git_has_uncommitted,
|
||||
mock_resolve_repo_dir,
|
||||
mock_get_sandbox,
|
||||
mock_get_config,
|
||||
) -> None:
|
||||
from agent.tools.commit_and_open_pr import commit_and_open_pr
|
||||
|
||||
mock_get_config.return_value = _make_config()
|
||||
mock_get_sandbox.return_value = MagicMock()
|
||||
mock_git_push.return_value = _make_push_result(
|
||||
exit_code=128,
|
||||
output=(
|
||||
"remote: Permission to langchain-ai/docs.git denied to hinthornw.\n"
|
||||
"fatal: unable to access 'https://github.com/langchain-ai/docs/': "
|
||||
"The requested URL returned error: 403"
|
||||
),
|
||||
)
|
||||
|
||||
result = commit_and_open_pr(
|
||||
title="fix: something", body="## Description\nfoo\n\n## Test Plan\n- [ ] check"
|
||||
)
|
||||
|
||||
assert result["success"] is False
|
||||
assert result["pr_url"] is None
|
||||
error = result["error"]
|
||||
assert "PERMANENT_FAILURE" in error
|
||||
assert "do not retry" in error
|
||||
assert "403" in error
|
||||
|
||||
|
||||
@patch("agent.tools.commit_and_open_pr.get_config")
|
||||
@patch("agent.tools.commit_and_open_pr.get_sandbox_backend_sync")
|
||||
@patch("agent.tools.commit_and_open_pr.resolve_repo_dir", return_value="/repo/docs")
|
||||
@patch("agent.tools.commit_and_open_pr.git_has_uncommitted_changes", return_value=False)
|
||||
@patch("agent.tools.commit_and_open_pr.git_fetch_origin")
|
||||
@patch("agent.tools.commit_and_open_pr.git_has_unpushed_commits", return_value=True)
|
||||
@patch("agent.tools.commit_and_open_pr.git_current_branch", return_value="open-swe/test-thread")
|
||||
@patch("agent.tools.commit_and_open_pr.git_checkout_branch", return_value=True)
|
||||
@patch("agent.tools.commit_and_open_pr.git_config_user")
|
||||
@patch("agent.tools.commit_and_open_pr.git_add_all")
|
||||
@patch("agent.tools.commit_and_open_pr.get_github_token", return_value="ghp_token")
|
||||
@patch(
|
||||
"agent.tools.commit_and_open_pr.get_github_app_installation_token",
|
||||
new_callable=AsyncMock,
|
||||
return_value="ghs_token",
|
||||
)
|
||||
@patch("agent.tools.commit_and_open_pr.git_push")
|
||||
def test_non_403_push_failure_returns_regular_error(
|
||||
mock_git_push,
|
||||
mock_get_installation_token,
|
||||
mock_get_token,
|
||||
mock_git_add_all,
|
||||
mock_git_config_user,
|
||||
mock_git_checkout_branch,
|
||||
mock_git_current_branch,
|
||||
mock_git_has_unpushed,
|
||||
mock_git_fetch_origin,
|
||||
mock_git_has_uncommitted,
|
||||
mock_resolve_repo_dir,
|
||||
mock_get_sandbox,
|
||||
mock_get_config,
|
||||
) -> None:
|
||||
from agent.tools.commit_and_open_pr import commit_and_open_pr
|
||||
|
||||
mock_get_config.return_value = _make_config()
|
||||
mock_get_sandbox.return_value = MagicMock()
|
||||
mock_git_push.return_value = _make_push_result(
|
||||
exit_code=1,
|
||||
output="error: failed to push some refs to 'origin'",
|
||||
)
|
||||
|
||||
result = commit_and_open_pr(
|
||||
title="fix: something", body="## Description\nfoo\n\n## Test Plan\n- [ ] check"
|
||||
)
|
||||
|
||||
assert result["success"] is False
|
||||
assert result["pr_url"] is None
|
||||
error = result["error"]
|
||||
assert "PERMANENT_FAILURE" not in error
|
||||
assert error.startswith("Git push failed:")
|
||||
|
|
@ -115,6 +115,44 @@ class TestOpenPrIfNeededMiddleware:
|
|||
|
||||
assert result is None
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_skips_when_commit_and_open_pr_failed_permanently(self) -> None:
|
||||
payload = {
|
||||
"success": False,
|
||||
"error": (
|
||||
"PERMANENT_FAILURE: do not retry. Git push was rejected with a 403 "
|
||||
"permission denied error."
|
||||
),
|
||||
"pr_url": None,
|
||||
}
|
||||
state = self._make_state(
|
||||
[
|
||||
ToolMessage(
|
||||
content=json.dumps(payload),
|
||||
tool_call_id="1",
|
||||
name="commit_and_open_pr",
|
||||
)
|
||||
]
|
||||
)
|
||||
|
||||
with (
|
||||
patch(
|
||||
"agent.middleware.open_pr.get_config",
|
||||
return_value={
|
||||
"configurable": {
|
||||
"thread_id": "thread-permanent",
|
||||
"repo": {"owner": "org", "name": "repo"},
|
||||
}
|
||||
},
|
||||
),
|
||||
patch(
|
||||
"agent.middleware.open_pr.get_sandbox_backend", new_callable=AsyncMock
|
||||
) as mock_sandbox,
|
||||
):
|
||||
await open_pr_if_needed.aafter_agent(state, self._make_runtime())
|
||||
|
||||
mock_sandbox.assert_not_called()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_proceeds_when_commit_and_open_pr_failed_git_push(self) -> None:
|
||||
"""When success=False due to git push failure, safety net should attempt PR creation."""
|
||||
|
|
@ -244,7 +282,7 @@ class TestOpenPrIfNeededMiddleware:
|
|||
"""
|
||||
payload = {
|
||||
"success": False,
|
||||
"error": "Git push failed: remote rejected (permission denied)",
|
||||
"error": "Git push failed: remote contains work",
|
||||
"pr_url": None,
|
||||
}
|
||||
state = self._make_state(
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue