From 7d8c5a150a1f5ebd4af8d9277316108f64c6e39e Mon Sep 17 00:00:00 2001 From: amoussa1229 <166072409+amoussa1229@users.noreply.github.com> Date: Mon, 13 Jul 2026 18:37:04 +0000 Subject: [PATCH] fix: restructure PR creation to POST-first with diagnose-on-failure Move preflight checks from an authoritative gate (before POST) to a diagnostic run after POST failure. This avoids false-positive failures when a just-pushed head branch is momentarily invisible to GitHub ref endpoints, and eliminates 2-3 extra serial API round-trips on the happy path. Also drop unused _PR_CREATED_FALSE indirection and add a docstring to pr_creation_guard acknowledging the fail-open detection design. --- agent/middleware/pr_creation_guard.py | 12 +++++++ agent/tools/open_pull_request.py | 39 ++++++++++++++-------- tests/test_open_pull_request.py | 48 +++++++++++++++++++++++++-- 3 files changed, 84 insertions(+), 15 deletions(-) diff --git a/agent/middleware/pr_creation_guard.py b/agent/middleware/pr_creation_guard.py index 84fbd69b..3420cfae 100644 --- a/agent/middleware/pr_creation_guard.py +++ b/agent/middleware/pr_creation_guard.py @@ -179,6 +179,18 @@ def _contains_direct_pull_create(tokens: list[str]) -> bool: def is_pr_creation_fallback_command(command: str) -> bool: + """Return True when *command* is a known PR-creation shell fallback. + + Detection is literal-token matching — it catches the primary vectors + (``gh pr create``, ``gh api repos/.../pulls -X POST``, ``curl`` to + ``/pulls``) but is intentionally fail-open: shell aliases, ``gh`` + aliases (``gh prc`` set via ``gh alias set``), flags between ``pr`` and + ``create`` (``gh pr --repo x create``), and non-curl HTTP clients + (``python -c …``, ``wget``, etc.) will not be blocked. This is + acceptable because the threat model is an honest agent papering over an + ``open_pull_request`` failure, not an adversary trying to bypass the + guardrail. + """ tokens = _shell_tokens(command) return ( _contains_gh_pr_create(tokens) diff --git a/agent/tools/open_pull_request.py b/agent/tools/open_pull_request.py index 36c6424b..1389bcb3 100644 --- a/agent/tools/open_pull_request.py +++ b/agent/tools/open_pull_request.py @@ -25,7 +25,6 @@ _REFERENCES_HEADING = "## References" _ACCESS_FAILURE_CODE = "github_app_access_missing_or_repo_not_found" _BRANCH_FAILURE_CODE = "github_pr_branch_not_visible" _PREFLIGHT_FAILURE_CODE = "github_pr_preflight_failed" -_PR_CREATED_FALSE = False async def _resolve_pr_author_token() -> tuple[str | None, str]: @@ -140,7 +139,7 @@ def _failure_payload( "token_kind": token_kind, "http_status": http_status, "branch_pushed": branch_pushed, - "pr_created": _PR_CREATED_FALSE, + "pr_created": False, "failed_step": failed_step, "likely_cause": likely_cause, "suggested_action": suggested_action, @@ -281,6 +280,12 @@ async def _preflight_pr_access( head: str, base: str, ) -> dict[str, Any] | None: + """Diagnose *why* a PR creation POST failed — not an authoritative gate. + + This runs only after the POST returns a non-201/non-422 status so it + doesn't add latency on the happy path and avoids false positives from + read-after-write inconsistency on just-pushed head branches. + """ repo_resp = await _github_get(client, token, f"/repos/{owner}/{repo}") if repo_resp.status_code in {403, 404}: return _access_failure_payload( @@ -637,17 +642,6 @@ async def _open_pull_request( ) async with httpx.AsyncClient(timeout=30.0) as client: - preflight_failure = await _preflight_pr_access( - client=client, - token=token, - token_kind=kind, - owner=owner, - repo=repo, - head=head, - base=base, - ) - if preflight_failure is not None: - return preflight_failure body = await _maybe_append_references(client, token, owner, repo, body) payload = {"title": title, "head": head, "base": base, "body": body, "draft": draft} resp = await client.post( @@ -699,6 +693,25 @@ async def _open_pull_request( "token_kind": kind, } + # POST failed — run preflight diagnostics to understand why, so the + # agent gets an actionable diagnosis instead of a raw HTTP status. + # Preflight runs *after* the POST so a just-pushed branch that is + # momentarily invisible to GitHub's ref endpoints does not cause a + # false-positive preflight failure on what would have been a successful + # PR creation. + diagnostic = await _preflight_pr_access( + client=client, + token=token, + token_kind=kind, + owner=owner, + repo=repo, + head=head, + base=base, + ) + if diagnostic is not None: + return diagnostic + + # Preflight found nothing — surface the raw POST failure. if resp.status_code == 404: return _access_failure_payload( owner=owner, diff --git a/tests/test_open_pull_request.py b/tests/test_open_pull_request.py index 14b22ad1..e30a88fd 100644 --- a/tests/test_open_pull_request.py +++ b/tests/test_open_pull_request.py @@ -322,10 +322,13 @@ def test_404_create_returns_actionable_access_diagnostic( def test_preflight_head_branch_404_reports_branch_not_pushed( monkeypatch: pytest.MonkeyPatch, ) -> None: + """Preflight runs as a diagnostic after the POST fails — the POST is attempted + first so a just-pushed branch that momentarily 404s from GitHub's ref + endpoints doesn't cause a false-positive preflight failure.""" _set_config(monkeypatch, {"source": "slack", "github_login": "johannes117"}) _stub_token(monkeypatch) client = _RoutingClient( - post=_FakeResponse(201, {"html_url": "u", "number": 1, "user": {}}), + post=_FakeResponse(422, {"message": "Validation failed"}), get_routes={ "/repos/langchain-ai/open-swe/branches/main": _FakeResponse(200, {"name": "main"}), "/repos/langchain-ai/open-swe/branches/open-swe%2Ffeature": _FakeResponse( @@ -343,7 +346,7 @@ def test_preflight_head_branch_404_reports_branch_not_pushed( assert result["branch_pushed"] is False assert result["head_branch_visible"] is False assert result["failed_step"] == "preflight_head_branch" - assert client.post_calls == [] + assert len(client.post_calls) == 1 async def _coro(value: Any) -> Any: @@ -617,3 +620,44 @@ def test_derive_pr_state_draft() -> None: def test_derive_pr_state_open() -> None: assert opr.derive_pr_state(state="open", merged=False, draft=False) == "open" + + +def test_happy_path_skips_preflight_branch_gets(monkeypatch: pytest.MonkeyPatch) -> None: + """POST-first: when the PR is created successfully no preflight GETs + are made to /branches/... endpoints — only reference / plan checks.""" + _set_config(monkeypatch, {"source": "slack", "github_login": "johannes117"}) + monkeypatch.setattr(opr, "_resolve_pr_author_token", lambda: _coro(("tok", "user"))) + + client = _RoutingClient( + post=_FakeResponse(201, {"html_url": "u", "number": 1, "user": {}}), + get_routes={"/repos/langchain-ai/open-swe": _FakeResponse(200, {"private": False})}, + ) + _install_client(monkeypatch, client) + + result = _open() + + assert result["success"] is True + branch_gets = [c for c in client.get_calls if "/branches/" in c["url"]] + assert branch_gets == [] + + +def test_post_failure_diagnoses_via_preflight(monkeypatch: pytest.MonkeyPatch) -> None: + """When the POST fails with a non-201/non-422 status, the preflight runs + as a diagnostic and its result is returned (not the raw POST failure).""" + _set_config(monkeypatch, {"source": "slack", "github_login": "johannes117"}) + _stub_token(monkeypatch) + + client = _RoutingClient( + post=_FakeResponse(403, {"message": "Resource not accessible"}), + get_routes={ + "/repos/langchain-ai/open-swe": _FakeResponse(403, {"message": "Not Found"}), + }, + ) + _install_client(monkeypatch, client) + + result = _open() + + assert result["success"] is False + assert result["code"] == "github_app_access_missing_or_repo_not_found" + assert result["failed_step"] == "preflight_repo" + assert result["repo_visible"] is False