mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-04 10:12:10 +00:00
fix: prevent futile retry loop when commit_and_open_pr fails (#1210)
* fix: prevent futile retry loop when commit_and_open_pr fails with git/API errors
- Root cause: when git checkout or GitHub PR API fails, the tool returned a generic {"success": false} error with no signal that retrying is futile, causing the agent to loop 9-13+ times until hitting the 1000-step recursion limit
- Change: (1) git_checkout_branch now returns (bool, str) so the actual git error output is surfaced in the tool response; (2) checkout and PR creation failures now include "fatal": true and an explicit "Do not retry" message; (3) prompt.py COMMIT_PR_SECTION adds an explicit instruction to stop on fatal errors
- Verified: 109 unit tests pass, no regressions
* fix: skip PR safety net on fatal commit failures
* style(open_pr): ruff-format fatal retry skip condition
---------
Co-authored-by: LangSmith Forge <forge-agent@langsmith.ai>
Co-authored-by: Johannes du Plessis <johannes@langchain.dev>
This commit is contained in:
parent
48965a82a6
commit
bd97678a5e
6 changed files with 84 additions and 13 deletions
|
|
@ -91,6 +91,10 @@ async def open_pr_if_needed(
|
||||||
return None
|
return None
|
||||||
|
|
||||||
error = pr_payload.get("error")
|
error = pr_payload.get("error")
|
||||||
|
if pr_payload.get("fatal") is True or (isinstance(error, str) and "Do not retry" in error):
|
||||||
|
logger.info("Skipping PR safety net after fatal commit_and_open_pr failure")
|
||||||
|
return None
|
||||||
|
|
||||||
if isinstance(error, str) and is_permanent_github_push_failure(error):
|
if isinstance(error, str) and is_permanent_github_push_failure(error):
|
||||||
logger.info("Skipping PR safety net after permanent push failure")
|
logger.info("Skipping PR safety net after permanent push failure")
|
||||||
return None
|
return None
|
||||||
|
|
|
||||||
|
|
@ -306,6 +306,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: 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 `"fatal": true` or an error message containing "Do not retry", stop immediately — do NOT call `commit_and_open_pr` again. These are infrastructure failures that cannot be fixed by retrying the same tool. Report the failure and end the task.**
|
||||||
|
|
||||||
**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.**
|
**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:
|
4. **Notify the source** immediately after `commit_and_open_pr` succeeds. Include a brief summary and the PR link:
|
||||||
|
|
|
||||||
|
|
@ -181,14 +181,18 @@ def commit_and_open_pr(
|
||||||
if result.exit_code != 0:
|
if result.exit_code != 0:
|
||||||
return {
|
return {
|
||||||
"success": False,
|
"success": False,
|
||||||
"error": f"Failed to checkout branch {target_branch}",
|
"error": f"Failed to checkout branch {target_branch}: {result.output.strip()}. Do not retry this tool — the git environment needs manual inspection.",
|
||||||
"pr_url": None,
|
"pr_url": None,
|
||||||
|
"fatal": True,
|
||||||
}
|
}
|
||||||
elif not git_checkout_branch(sandbox_backend, repo_dir, target_branch):
|
else:
|
||||||
|
ok, git_err = git_checkout_branch(sandbox_backend, repo_dir, target_branch)
|
||||||
|
if not ok:
|
||||||
return {
|
return {
|
||||||
"success": False,
|
"success": False,
|
||||||
"error": f"Failed to checkout branch {target_branch}",
|
"error": f"Failed to checkout branch {target_branch}: {git_err}. Do not retry this tool — the git environment needs manual inspection.",
|
||||||
"pr_url": None,
|
"pr_url": None,
|
||||||
|
"fatal": True,
|
||||||
}
|
}
|
||||||
|
|
||||||
git_config_user(
|
git_config_user(
|
||||||
|
|
@ -266,9 +270,10 @@ def commit_and_open_pr(
|
||||||
if not pr_url:
|
if not pr_url:
|
||||||
return {
|
return {
|
||||||
"success": False,
|
"success": False,
|
||||||
"error": "Failed to create GitHub PR",
|
"error": "Failed to create GitHub PR. Do not retry this tool — if the push succeeded, the PR may need to be opened manually.",
|
||||||
"pr_url": None,
|
"pr_url": None,
|
||||||
"pr_existing": False,
|
"pr_existing": False,
|
||||||
|
"fatal": True,
|
||||||
}
|
}
|
||||||
|
|
||||||
return {
|
return {
|
||||||
|
|
|
||||||
|
|
@ -63,17 +63,19 @@ def git_current_branch(sandbox_backend: SandboxBackendProtocol, repo_dir: str) -
|
||||||
|
|
||||||
def git_checkout_branch(
|
def git_checkout_branch(
|
||||||
sandbox_backend: SandboxBackendProtocol, repo_dir: str, branch: str
|
sandbox_backend: SandboxBackendProtocol, repo_dir: str, branch: str
|
||||||
) -> bool:
|
) -> tuple[bool, str]:
|
||||||
"""Checkout branch, creating it if needed."""
|
"""Checkout branch, creating it if needed. Returns (success, error_output)."""
|
||||||
safe_branch = shlex.quote(branch)
|
safe_branch = shlex.quote(branch)
|
||||||
checkout_result = _run_git(sandbox_backend, repo_dir, f"git checkout -B {safe_branch}")
|
checkout_result = _run_git(sandbox_backend, repo_dir, f"git checkout -B {safe_branch}")
|
||||||
if checkout_result.exit_code == 0:
|
if checkout_result.exit_code == 0:
|
||||||
return True
|
return True, ""
|
||||||
fallback_create = _run_git(sandbox_backend, repo_dir, f"git checkout -b {safe_branch}")
|
fallback_create = _run_git(sandbox_backend, repo_dir, f"git checkout -b {safe_branch}")
|
||||||
if fallback_create.exit_code == 0:
|
if fallback_create.exit_code == 0:
|
||||||
return True
|
return True, ""
|
||||||
fallback = _run_git(sandbox_backend, repo_dir, f"git checkout {safe_branch}")
|
fallback = _run_git(sandbox_backend, repo_dir, f"git checkout {safe_branch}")
|
||||||
return fallback.exit_code == 0
|
if fallback.exit_code == 0:
|
||||||
|
return True, ""
|
||||||
|
return False, fallback.output.strip() or checkout_result.output.strip()
|
||||||
|
|
||||||
|
|
||||||
def git_checkout_existing_branch(
|
def git_checkout_existing_branch(
|
||||||
|
|
|
||||||
|
|
@ -27,3 +27,22 @@ def test_git_checkout_existing_branch_quotes_repo_dir_and_branch() -> None:
|
||||||
github.git_checkout_existing_branch(sandbox, repo_dir, branch)
|
github.git_checkout_existing_branch(sandbox, repo_dir, branch)
|
||||||
|
|
||||||
assert sandbox.commands == [f"cd {shlex.quote(repo_dir)} && git checkout {shlex.quote(branch)}"]
|
assert sandbox.commands == [f"cd {shlex.quote(repo_dir)} && git checkout {shlex.quote(branch)}"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_git_checkout_branch_returns_true_on_success() -> None:
|
||||||
|
sandbox = FakeSandboxBackend()
|
||||||
|
ok, err = github.git_checkout_branch(sandbox, "/tmp/repo", "my-branch")
|
||||||
|
assert ok is True
|
||||||
|
assert err == ""
|
||||||
|
|
||||||
|
|
||||||
|
def test_git_checkout_branch_returns_false_with_error_output_on_failure() -> None:
|
||||||
|
class FailingSandbox(FakeSandboxBackend):
|
||||||
|
def execute(self, command: str) -> SimpleNamespace:
|
||||||
|
self.commands.append(command)
|
||||||
|
return SimpleNamespace(exit_code=1, output="error: pathspec did not match")
|
||||||
|
|
||||||
|
sandbox = FailingSandbox()
|
||||||
|
ok, err = github.git_checkout_branch(sandbox, "/tmp/repo", "my-branch")
|
||||||
|
assert ok is False
|
||||||
|
assert "pathspec did not match" in err
|
||||||
|
|
|
||||||
|
|
@ -153,6 +153,45 @@ class TestOpenPrIfNeededMiddleware:
|
||||||
|
|
||||||
mock_sandbox.assert_not_called()
|
mock_sandbox.assert_not_called()
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_skips_when_commit_and_open_pr_failed_fatally(self) -> None:
|
||||||
|
payload = {
|
||||||
|
"success": False,
|
||||||
|
"error": (
|
||||||
|
"Failed to create GitHub PR. Do not retry this tool — if the push succeeded, "
|
||||||
|
"the PR may need to be opened manually."
|
||||||
|
),
|
||||||
|
"pr_url": None,
|
||||||
|
"fatal": True,
|
||||||
|
}
|
||||||
|
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-fatal",
|
||||||
|
"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
|
@pytest.mark.asyncio
|
||||||
async def test_proceeds_when_commit_and_open_pr_failed_git_push(self) -> None:
|
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."""
|
"""When success=False due to git push failure, safety net should attempt PR creation."""
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue