diff --git a/frontend/pull-sheet.js b/frontend/pull-sheet.js index 9adc047..4be1f6d 100644 --- a/frontend/pull-sheet.js +++ b/frontend/pull-sheet.js @@ -139,7 +139,11 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT headers: { Accept: 'application/json', 'Content-Type': 'application/json' }, body: JSON.stringify({ expected_head_sha: expectedHeadSha }), }).then(result => { - if (!result?.merged) throw new Error('Pull request merge was not confirmed.'); + if (!result?.merged) { + throw new Error(result?.confirmation_pending && result?.error + ? result.error + : 'Pull request merge was not confirmed.'); + } return result; }).finally(() => { mergeRequest = null; }); return mergeRequest; diff --git a/src/gitea_proxy.py b/src/gitea_proxy.py index 54206c8..ff0a1ab 100644 --- a/src/gitea_proxy.py +++ b/src/gitea_proxy.py @@ -1470,6 +1470,21 @@ async def pull_completion_review(repository: str, number: int) -> dict: } +async def is_pull_merged_at_head( + repository: str, number: int, expected_head_sha: str +) -> bool: + pull = await fetch(f"repos/{repository}/pulls/{number}") + if not isinstance(pull, dict): + return False + head_value = pull.get("head") + head = head_value if isinstance(head_value, dict) else {} + return ( + pull.get("merged") is True + and pull.get("state") == "closed" + and head.get("sha") == expected_head_sha + ) + + async def merge_assigned_pull( repository: str, number: int, expected_head_sha: str ) -> dict: diff --git a/src/main.py b/src/main.py index 618b471..16acabd 100644 --- a/src/main.py +++ b/src/main.py @@ -2068,9 +2068,26 @@ async def merge_assigned_pull( status_code=409, ) except Exception: + try: + merged = await asyncio.wait_for( + gitea_proxy.is_pull_merged_at_head( + repository, number, submission.expected_head_sha + ), + timeout=ISSUE_ACTION_TIMEOUT_SECONDS, + ) + except Exception: + merged = False + if merged: + return {"number": number, "merged": True, "state": "closed"} return JSONResponse( - {"error": "The pull request could not be merged. It remains in My Work; please retry."}, - status_code=503, + { + "number": number, + "merged": False, + "state": "unknown", + "confirmation_pending": True, + "error": "Merge confirmation is pending. Check its status before retrying.", + }, + status_code=202, headers={"Retry-After": "1"}, ) diff --git a/tests/test_my_work.py b/tests/test_my_work.py index 6910fbb..a895dff 100644 --- a/tests/test_my_work.py +++ b/tests/test_my_work.py @@ -2528,6 +2528,31 @@ Promise.all([first, duplicate]).then(async comments => {{ assert len(output["comments"]) == 2 and len(output["merges"]) == 2 +def test_pull_sheet_surfaces_pending_merge_confirmation_guidance(): + script = f""" +const createPullSheet = require({json.dumps(str(PULL_SHEET))}); +const controller = createPullSheet({{ + storage: null, + fetchJson: () => Promise.resolve({{ + number: 7, + merged: false, + state: 'unknown', + confirmation_pending: true, + error: 'Merge confirmation is pending. Check its status before retrying.', + }}), +}}); +controller.merge({{repository:'stackchain/api', number:7}}, 'abc123') + .then(() => process.stdout.write(JSON.stringify({{resolved:true}}))) + .catch(error => process.stdout.write(JSON.stringify({{resolved:false, message:error.message}}))); +""" + result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True) + + assert json.loads(result.stdout) == { + "resolved": False, + "message": "Merge confirmation is pending. Check its status before retrying.", + } + + def test_pull_sheet_enables_merge_only_for_current_safe_state(): script = f""" const createPullSheet = require({json.dumps(str(PULL_SHEET))}); diff --git a/tests/test_pull_api.py b/tests/test_pull_api.py index 95cdb37..ffa233e 100644 --- a/tests/test_pull_api.py +++ b/tests/test_pull_api.py @@ -1,3 +1,5 @@ +import asyncio + import httpx import pytest @@ -247,6 +249,108 @@ async def test_assigned_pull_merge_requires_current_eligible_head(monkeypatch): assert calls == [("stackchain/api", 7, "abc123")] +@pytest.mark.anyio +async def test_assigned_pull_merge_reconciles_acceptance_before_timeout(monkeypatch): + calls = [] + merged = False + + async def assigned(repository, number): + return True + + async def merge(repository, number, expected_head_sha): + nonlocal merged + calls.append(("merge", repository, number, expected_head_sha)) + merged = True + await asyncio.sleep(0.05) + return {"number": number, "merged": True, "state": "closed"} + + async def confirm(repository, number, expected_head_sha): + calls.append(("confirm", repository, number, expected_head_sha)) + return merged + + monkeypatch.setattr(main, "ISSUE_ACTION_TIMEOUT_SECONDS", 0.01) + monkeypatch.setattr(main.gitea_proxy, "is_assigned_pull", assigned) + monkeypatch.setattr(main.gitea_proxy, "merge_assigned_pull", merge) + monkeypatch.setattr(main.gitea_proxy, "is_pull_merged_at_head", confirm, raising=False) + transport = httpx.ASGITransport(app=main.app) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: + response = await client.post( + "/api/v1/repos/stackchain/api/pulls/7/merge", + json={"expected_head_sha": "abc123"}, + ) + + assert response.status_code == 200 + assert response.json() == {"number": 7, "merged": True, "state": "closed"} + assert calls == [ + ("merge", "stackchain/api", 7, "abc123"), + ("confirm", "stackchain/api", 7, "abc123"), + ] + + +@pytest.mark.anyio +async def test_gitea_merge_confirmation_requires_merged_expected_head(): + requests = [] + + async def handler(request): + requests.append((request.method, request.url.path)) + return httpx.Response(200, json={ + "number": 7, + "state": "closed", + "merged": True, + "head": {"sha": "abc123"}, + }) + + gitea_proxy.start_client(transport=httpx.MockTransport(handler)) + try: + confirmed = await gitea_proxy.is_pull_merged_at_head( + "stackchain/api", 7, "abc123" + ) + finally: + await gitea_proxy.stop_client() + + assert confirmed is True + assert requests == [("GET", "/api/v1/repos/stackchain/api/pulls/7")] + + +@pytest.mark.anyio +async def test_assigned_pull_merge_reports_unresolved_confirmation_without_false_state(monkeypatch): + calls = [] + + async def assigned(repository, number): + return True + + async def merge(repository, number, expected_head_sha): + calls.append(("merge", repository, number, expected_head_sha)) + raise httpx.ReadTimeout("upstream response was lost") + + async def confirm(repository, number, expected_head_sha): + calls.append(("confirm", repository, number, expected_head_sha)) + return False + + monkeypatch.setattr(main.gitea_proxy, "is_assigned_pull", assigned) + monkeypatch.setattr(main.gitea_proxy, "merge_assigned_pull", merge) + monkeypatch.setattr(main.gitea_proxy, "is_pull_merged_at_head", confirm) + transport = httpx.ASGITransport(app=main.app) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: + response = await client.post( + "/api/v1/repos/stackchain/api/pulls/7/merge", + json={"expected_head_sha": "abc123"}, + ) + + assert response.status_code == 202 + assert response.json() == { + "number": 7, + "merged": False, + "state": "unknown", + "confirmation_pending": True, + "error": "Merge confirmation is pending. Check its status before retrying.", + } + assert calls == [ + ("merge", "stackchain/api", 7, "abc123"), + ("confirm", "stackchain/api", 7, "abc123"), + ] + + @pytest.mark.anyio async def test_assigned_pull_merge_returns_conflict_without_mutating_stale_head(monkeypatch): async def assigned(repository, number):