fix: reconcile ambiguous merge outcomes (#277)
This commit is contained in:
parent
e0543b2007
commit
e58808f2d4
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
21
src/main.py
21
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"},
|
||||
)
|
||||
|
||||
|
|
|
|||
|
|
@ -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))});
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user