[F062] work_session.merge_pr: idempotency + active-status guard

merge_pr unconditionally set pr_status=merged, pr_merged_at, merged_by,
status=COMPLETED on whatever session it loaded — the only session-terminal
transition in WorkSessionService lacking both the active-status guard
(complete/abandon) and the terminal-idempotency guard (close). Two failure
modes: (1) a retried merge after a successful-but-unconfirmed GitHub merge
overwrote merged_by/pr_merged_at with the retry's actor/timestamp, corrupting
the merge audit trail; (2) merge_pr on an ABANDONED session resurrected it to
COMPLETED, undoing the single-active abandonment. Mirrors close()'s guard:
if status != ACTIVE, return the session unchanged. Both git.py callers await
merge_pr and discard the return, so the no-op is safe. TDD: 3 tests
(happy-path + both modes).
This commit is contained in:
Renn F
2026-06-28 17:02:04 +02:00
parent f826285651
commit ccf895dd67
2 changed files with 97 additions and 0 deletions
+86
View File
@@ -2,10 +2,12 @@
from __future__ import annotations
from datetime import UTC, datetime
from unittest.mock import AsyncMock, MagicMock
from uuid import uuid4
import pytest
from roboco.models.work_session import WorkSessionStatus
from roboco.services.work_session import WorkSessionService
@@ -73,3 +75,87 @@ async def test_has_unpushed_commits_false_when_session_missing() -> None:
svc = _service()
_bind(svc, "get", AsyncMock(return_value=None))
assert await svc.has_unpushed_commits(uuid4()) is False
# ---------------------------------------------------------------------------
# F062 — merge_pr must be idempotent + active-guarded like close()/complete()
# ---------------------------------------------------------------------------
def _merge_service() -> tuple[WorkSessionService, MagicMock]:
"""A service whose session flush is observable. Returns ``(svc, session)`` so
the tests can assert that a guarded no-op never reached ``flush``."""
session = MagicMock()
session.flush = AsyncMock()
svc = WorkSessionService(session)
return svc, session
@pytest.mark.asyncio
async def test_merge_pr_completes_active_session() -> None:
"""Happy path: an ACTIVE session is recorded as merged + COMPLETED."""
original_merger = uuid4()
ws = MagicMock(
status=WorkSessionStatus.ACTIVE, pr_number=42, branch_name="feature/x"
)
svc, session = _merge_service()
_bind(svc, "get", AsyncMock(return_value=ws))
result = await svc.merge_pr(uuid4(), original_merger)
assert result is ws
assert ws.pr_status == "merged"
assert ws.merged_by == original_merger
assert ws.pr_merged_at is not None
assert ws.status == WorkSessionStatus.COMPLETED
session.flush.assert_awaited_once()
@pytest.mark.asyncio
async def test_merge_pr_idempotent_on_already_completed_preserves_audit_trail() -> None:
"""F062 (mode 1): a retried merge after a successful-but-unconfirmed GitHub
merge must NOT overwrite the original ``merged_by`` / ``pr_merged_at`` — the
merge audit trail is preserved. Mirrors close()'s idempotency guard."""
original_merger = uuid4()
original_ts = datetime(2026, 6, 1, 12, 0, tzinfo=UTC)
retry_merger = uuid4()
ws = MagicMock(
status=WorkSessionStatus.COMPLETED,
pr_number=42,
merged_by=original_merger,
pr_merged_at=original_ts,
pr_status="merged",
)
svc, session = _merge_service()
_bind(svc, "get", AsyncMock(return_value=ws))
result = await svc.merge_pr(uuid4(), retry_merger)
assert result is ws
# Audit trail untouched — retry did not overwrite the original merger/ts.
assert ws.merged_by == original_merger
assert ws.pr_merged_at == original_ts
assert ws.status == WorkSessionStatus.COMPLETED
# No mutation happened, so flush must not have run.
session.flush.assert_not_awaited()
@pytest.mark.asyncio
async def test_merge_pr_does_not_resurrect_abandoned_session() -> None:
"""F062 (mode 2): merge_pr on an ABANDONED session must NOT flip it to
COMPLETED — that would silently undo the single-active invariant's
abandonment and make discarded work look like a successful merge."""
ws = MagicMock(status=WorkSessionStatus.ABANDONED, pr_number=42, merged_by=None)
svc, session = _merge_service()
_bind(svc, "get", AsyncMock(return_value=ws))
result = await svc.merge_pr(uuid4(), uuid4())
assert result is ws
# Not resurrected — stays abandoned, no merger recorded.
assert ws.status == WorkSessionStatus.ABANDONED
assert ws.merged_by is None
session.flush.assert_not_awaited()