From bd97678a5ea31757327a7b2f3f401ffa685cb9c8 Mon Sep 17 00:00:00 2001 From: "langsmith-forge[bot]" <270983758+langsmith-forge[bot]@users.noreply.github.com> Date: Fri, 1 May 2026 22:05:22 +0000 Subject: [PATCH] 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 Co-authored-by: Johannes du Plessis --- agent/middleware/open_pr.py | 4 ++++ agent/prompt.py | 2 ++ agent/tools/commit_and_open_pr.py | 21 ++++++++++------- agent/utils/github.py | 12 ++++++---- tests/test_github_security.py | 19 +++++++++++++++ tests/test_open_pr_middleware.py | 39 +++++++++++++++++++++++++++++++ 6 files changed, 84 insertions(+), 13 deletions(-) diff --git a/agent/middleware/open_pr.py b/agent/middleware/open_pr.py index 19443b80..45d911ae 100644 --- a/agent/middleware/open_pr.py +++ b/agent/middleware/open_pr.py @@ -91,6 +91,10 @@ async def open_pr_if_needed( return None 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): logger.info("Skipping PR safety net after permanent push failure") return None diff --git a/agent/prompt.py b/agent/prompt.py index 5dae7f70..70194104 100644 --- a/agent/prompt.py +++ b/agent/prompt.py @@ -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: 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.** 4. **Notify the source** immediately after `commit_and_open_pr` succeeds. Include a brief summary and the PR link: diff --git a/agent/tools/commit_and_open_pr.py b/agent/tools/commit_and_open_pr.py index 1b87c4df..c893d16c 100644 --- a/agent/tools/commit_and_open_pr.py +++ b/agent/tools/commit_and_open_pr.py @@ -181,15 +181,19 @@ def commit_and_open_pr( if result.exit_code != 0: return { "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, + "fatal": True, + } + else: + ok, git_err = git_checkout_branch(sandbox_backend, repo_dir, target_branch) + if not ok: + return { + "success": False, + "error": f"Failed to checkout branch {target_branch}: {git_err}. Do not retry this tool — the git environment needs manual inspection.", + "pr_url": None, + "fatal": True, } - elif not git_checkout_branch(sandbox_backend, repo_dir, target_branch): - return { - "success": False, - "error": f"Failed to checkout branch {target_branch}", - "pr_url": None, - } git_config_user( sandbox_backend, @@ -266,9 +270,10 @@ def commit_and_open_pr( if not pr_url: return { "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_existing": False, + "fatal": True, } return { diff --git a/agent/utils/github.py b/agent/utils/github.py index 18285656..eae34dde 100644 --- a/agent/utils/github.py +++ b/agent/utils/github.py @@ -63,17 +63,19 @@ def git_current_branch(sandbox_backend: SandboxBackendProtocol, repo_dir: str) - def git_checkout_branch( sandbox_backend: SandboxBackendProtocol, repo_dir: str, branch: str -) -> bool: - """Checkout branch, creating it if needed.""" +) -> tuple[bool, str]: + """Checkout branch, creating it if needed. Returns (success, error_output).""" safe_branch = shlex.quote(branch) checkout_result = _run_git(sandbox_backend, repo_dir, f"git checkout -B {safe_branch}") if checkout_result.exit_code == 0: - return True + return True, "" fallback_create = _run_git(sandbox_backend, repo_dir, f"git checkout -b {safe_branch}") if fallback_create.exit_code == 0: - return True + return True, "" 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( diff --git a/tests/test_github_security.py b/tests/test_github_security.py index f4585efa..dfc3d7a2 100644 --- a/tests/test_github_security.py +++ b/tests/test_github_security.py @@ -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) 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 diff --git a/tests/test_open_pr_middleware.py b/tests/test_open_pr_middleware.py index d92e5e57..8bfe3a4f 100644 --- a/tests/test_open_pr_middleware.py +++ b/tests/test_open_pr_middleware.py @@ -153,6 +153,45 @@ class TestOpenPrIfNeededMiddleware: 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 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."""