Make pull merges durably auditable before mutation #1029
59
src/main.py
59
src/main.py
|
|
@ -5918,31 +5918,60 @@ async def merge_assigned_pull(
|
||||||
target=f"{repository}#{number}",
|
target=f"{repository}#{number}",
|
||||||
)
|
)
|
||||||
|
|
||||||
async def merge_pull():
|
try:
|
||||||
if not await gitea_proxy.is_assigned_pull(repository, number):
|
assigned = await asyncio.wait_for(
|
||||||
raise HTTPException(status_code=404, detail="Assigned pull request not found")
|
gitea_proxy.is_assigned_pull(repository, number),
|
||||||
return await gitea_proxy.merge_assigned_pull(
|
timeout=ISSUE_ACTION_TIMEOUT_SECONDS,
|
||||||
repository, number, submission.expected_head_sha
|
)
|
||||||
|
except Exception:
|
||||||
|
return JSONResponse(
|
||||||
|
{"error": "The pull request assignment could not be verified. Nothing was merged."},
|
||||||
|
status_code=503,
|
||||||
|
headers={"Retry-After": "1"},
|
||||||
|
)
|
||||||
|
if not assigned:
|
||||||
|
raise HTTPException(status_code=404, detail="Assigned pull request not found")
|
||||||
|
|
||||||
|
journal = _security_event_store()
|
||||||
|
try:
|
||||||
|
operation_id = await asyncio.to_thread(
|
||||||
|
journal.reserve,
|
||||||
|
"pull_merged",
|
||||||
|
target=f"{repository}#{number}",
|
||||||
|
)
|
||||||
|
except SecurityEventStoreError:
|
||||||
|
return JSONResponse(
|
||||||
|
{"error": "Security activity is temporarily unavailable. Nothing was merged."},
|
||||||
|
status_code=503,
|
||||||
|
headers={"Retry-After": "1"},
|
||||||
)
|
)
|
||||||
|
|
||||||
try:
|
try:
|
||||||
result = await asyncio.wait_for(
|
result = await asyncio.wait_for(
|
||||||
merge_pull(), timeout=ISSUE_ACTION_TIMEOUT_SECONDS
|
gitea_proxy.merge_assigned_pull(
|
||||||
)
|
repository, number, submission.expected_head_sha
|
||||||
await asyncio.to_thread(
|
),
|
||||||
_security_event_store().record,
|
timeout=ISSUE_ACTION_TIMEOUT_SECONDS,
|
||||||
"pull_merged",
|
|
||||||
target=f"{repository}#{number}",
|
|
||||||
)
|
)
|
||||||
|
try:
|
||||||
|
await asyncio.to_thread(journal.finalize, operation_id)
|
||||||
|
except SecurityEventStoreError:
|
||||||
|
pass
|
||||||
return result
|
return result
|
||||||
except HTTPException:
|
|
||||||
raise
|
|
||||||
except gitea_proxy.StalePullError:
|
except gitea_proxy.StalePullError:
|
||||||
|
try:
|
||||||
|
await asyncio.to_thread(journal.discard, operation_id)
|
||||||
|
except SecurityEventStoreError:
|
||||||
|
pass
|
||||||
return JSONResponse(
|
return JSONResponse(
|
||||||
{"error": "New commits were pushed. Refresh before merging."},
|
{"error": "New commits were pushed. Refresh before merging."},
|
||||||
status_code=409,
|
status_code=409,
|
||||||
)
|
)
|
||||||
except gitea_proxy.PullNotMergeableError:
|
except gitea_proxy.PullNotMergeableError:
|
||||||
|
try:
|
||||||
|
await asyncio.to_thread(journal.discard, operation_id)
|
||||||
|
except SecurityEventStoreError:
|
||||||
|
pass
|
||||||
return JSONResponse(
|
return JSONResponse(
|
||||||
{"error": "This pull request is not currently safe to merge. Refresh its status."},
|
{"error": "This pull request is not currently safe to merge. Refresh its status."},
|
||||||
status_code=409,
|
status_code=409,
|
||||||
|
|
@ -5958,6 +5987,10 @@ async def merge_assigned_pull(
|
||||||
except Exception:
|
except Exception:
|
||||||
merged = False
|
merged = False
|
||||||
if merged:
|
if merged:
|
||||||
|
try:
|
||||||
|
await asyncio.to_thread(journal.finalize, operation_id)
|
||||||
|
except SecurityEventStoreError:
|
||||||
|
pass
|
||||||
return {"number": number, "merged": True, "state": "closed"}
|
return {"number": number, "merged": True, "state": "closed"}
|
||||||
return JSONResponse(
|
return JSONResponse(
|
||||||
{
|
{
|
||||||
|
|
|
||||||
|
|
@ -4,6 +4,7 @@ import httpx
|
||||||
import pytest
|
import pytest
|
||||||
|
|
||||||
from src import gitea_proxy, main
|
from src import gitea_proxy, main
|
||||||
|
from src.security_event_store import SecurityEventStoreError
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.anyio
|
@pytest.mark.anyio
|
||||||
|
|
@ -312,11 +313,98 @@ async def test_assigned_pull_merge_requires_current_eligible_head(monkeypatch):
|
||||||
assert calls == [("stackchain/api", 7, "abc123")]
|
assert calls == [("stackchain/api", 7, "abc123")]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.anyio
|
||||||
|
async def test_assigned_pull_merge_fails_before_gitea_when_audit_reservation_fails(
|
||||||
|
monkeypatch,
|
||||||
|
):
|
||||||
|
merge_calls = []
|
||||||
|
|
||||||
|
class UnavailableJournal:
|
||||||
|
def reserve(self, *_args, **_kwargs):
|
||||||
|
raise SecurityEventStoreError("unavailable")
|
||||||
|
|
||||||
|
async def assigned(*_args):
|
||||||
|
return True
|
||||||
|
|
||||||
|
async def merge(*args):
|
||||||
|
merge_calls.append(args)
|
||||||
|
return {"number": 7, "merged": True, "state": "closed"}
|
||||||
|
|
||||||
|
monkeypatch.setattr(main, "_security_event_store", lambda: UnavailableJournal())
|
||||||
|
monkeypatch.setattr(main.gitea_proxy, "is_assigned_pull", assigned)
|
||||||
|
monkeypatch.setattr(main.gitea_proxy, "merge_assigned_pull", merge)
|
||||||
|
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 == 503
|
||||||
|
assert response.json() == {
|
||||||
|
"error": "Security activity is temporarily unavailable. Nothing was merged."
|
||||||
|
}
|
||||||
|
assert merge_calls == []
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.anyio
|
||||||
|
async def test_assigned_pull_merge_reports_success_and_retains_pending_audit_when_finalize_fails(
|
||||||
|
monkeypatch,
|
||||||
|
):
|
||||||
|
lifecycle = []
|
||||||
|
|
||||||
|
class InterruptedJournal:
|
||||||
|
def reserve(self, kind, *, target):
|
||||||
|
lifecycle.append(("reserve", kind, target))
|
||||||
|
return "merge-operation"
|
||||||
|
|
||||||
|
def finalize(self, operation_id):
|
||||||
|
lifecycle.append(("finalize", operation_id))
|
||||||
|
raise SecurityEventStoreError("unavailable")
|
||||||
|
|
||||||
|
def discard(self, operation_id):
|
||||||
|
lifecycle.append(("discard", operation_id))
|
||||||
|
|
||||||
|
async def assigned(*_args):
|
||||||
|
return True
|
||||||
|
|
||||||
|
async def merge(*_args):
|
||||||
|
return {"number": 7, "merged": True, "state": "closed"}
|
||||||
|
|
||||||
|
monkeypatch.setattr(main, "_security_event_store", lambda: InterruptedJournal())
|
||||||
|
monkeypatch.setattr(main.gitea_proxy, "is_assigned_pull", assigned)
|
||||||
|
monkeypatch.setattr(main.gitea_proxy, "merge_assigned_pull", merge)
|
||||||
|
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 lifecycle == [
|
||||||
|
("reserve", "pull_merged", "stackchain/api#7"),
|
||||||
|
("finalize", "merge-operation"),
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.anyio
|
@pytest.mark.anyio
|
||||||
async def test_assigned_pull_merge_reconciles_acceptance_before_timeout(monkeypatch):
|
async def test_assigned_pull_merge_reconciles_acceptance_before_timeout(monkeypatch):
|
||||||
calls = []
|
calls = []
|
||||||
merged = False
|
merged = False
|
||||||
|
|
||||||
|
class LifecycleJournal:
|
||||||
|
def reserve(self, kind, *, target):
|
||||||
|
calls.append(("reserve", kind, target))
|
||||||
|
return "merge-operation"
|
||||||
|
|
||||||
|
def finalize(self, operation_id):
|
||||||
|
calls.append(("finalize", operation_id))
|
||||||
|
|
||||||
|
def discard(self, operation_id):
|
||||||
|
calls.append(("discard", operation_id))
|
||||||
|
|
||||||
async def assigned(repository, number):
|
async def assigned(repository, number):
|
||||||
return True
|
return True
|
||||||
|
|
||||||
|
|
@ -332,6 +420,7 @@ async def test_assigned_pull_merge_reconciles_acceptance_before_timeout(monkeypa
|
||||||
return merged
|
return merged
|
||||||
|
|
||||||
monkeypatch.setattr(main, "ISSUE_ACTION_TIMEOUT_SECONDS", 0.01)
|
monkeypatch.setattr(main, "ISSUE_ACTION_TIMEOUT_SECONDS", 0.01)
|
||||||
|
monkeypatch.setattr(main, "_security_event_store", lambda: LifecycleJournal())
|
||||||
monkeypatch.setattr(main.gitea_proxy, "is_assigned_pull", assigned)
|
monkeypatch.setattr(main.gitea_proxy, "is_assigned_pull", assigned)
|
||||||
monkeypatch.setattr(main.gitea_proxy, "merge_assigned_pull", merge)
|
monkeypatch.setattr(main.gitea_proxy, "merge_assigned_pull", merge)
|
||||||
monkeypatch.setattr(main.gitea_proxy, "is_pull_merged_at_head", confirm, raising=False)
|
monkeypatch.setattr(main.gitea_proxy, "is_pull_merged_at_head", confirm, raising=False)
|
||||||
|
|
@ -345,8 +434,10 @@ async def test_assigned_pull_merge_reconciles_acceptance_before_timeout(monkeypa
|
||||||
assert response.status_code == 200
|
assert response.status_code == 200
|
||||||
assert response.json() == {"number": 7, "merged": True, "state": "closed"}
|
assert response.json() == {"number": 7, "merged": True, "state": "closed"}
|
||||||
assert calls == [
|
assert calls == [
|
||||||
|
("reserve", "pull_merged", "stackchain/api#7"),
|
||||||
("merge", "stackchain/api", 7, "abc123"),
|
("merge", "stackchain/api", 7, "abc123"),
|
||||||
("confirm", "stackchain/api", 7, "abc123"),
|
("confirm", "stackchain/api", 7, "abc123"),
|
||||||
|
("finalize", "merge-operation"),
|
||||||
]
|
]
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -435,6 +526,47 @@ async def test_assigned_pull_merge_returns_conflict_without_mutating_stale_head(
|
||||||
assert "New commits" in response.json()["error"]
|
assert "New commits" in response.json()["error"]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.anyio
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"rejection",
|
||||||
|
[gitea_proxy.StalePullError("changed"), gitea_proxy.PullNotMergeableError("unsafe")],
|
||||||
|
)
|
||||||
|
async def test_assigned_pull_merge_discards_audit_reservation_after_definite_rejection(
|
||||||
|
monkeypatch, rejection
|
||||||
|
):
|
||||||
|
lifecycle = []
|
||||||
|
|
||||||
|
class LifecycleJournal:
|
||||||
|
def reserve(self, kind, *, target):
|
||||||
|
lifecycle.append(("reserve", kind, target))
|
||||||
|
return "merge-operation"
|
||||||
|
|
||||||
|
def discard(self, operation_id):
|
||||||
|
lifecycle.append(("discard", operation_id))
|
||||||
|
|
||||||
|
async def assigned(*_args):
|
||||||
|
return True
|
||||||
|
|
||||||
|
async def merge(*_args):
|
||||||
|
raise rejection
|
||||||
|
|
||||||
|
monkeypatch.setattr(main, "_security_event_store", lambda: LifecycleJournal())
|
||||||
|
monkeypatch.setattr(main.gitea_proxy, "is_assigned_pull", assigned)
|
||||||
|
monkeypatch.setattr(main.gitea_proxy, "merge_assigned_pull", merge)
|
||||||
|
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 == 409
|
||||||
|
assert lifecycle == [
|
||||||
|
("reserve", "pull_merged", "stackchain/api#7"),
|
||||||
|
("discard", "merge-operation"),
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.anyio
|
@pytest.mark.anyio
|
||||||
async def test_gitea_merge_rejects_failed_ci_without_upstream_mutation():
|
async def test_gitea_merge_rejects_failed_ci_without_upstream_mutation():
|
||||||
requests = []
|
requests = []
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue
Block a user